Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting user/group ownership of template output #1531

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

deblasis
Copy link
Contributor

@deblasis deblasis commented Nov 3, 2021

Adds support for the uid and gid entries in the template stanza that would ultimately be passed to a os.Chown command.

It obviously works only on POSIX and Windows is handled with graceful noop with a warning if the user set uid or gid

Related issues #639, #1497, #1185 and also nomad/5020

Signed-off-by: Alessandro De Blasis alex@deblasis.net

Related issues hashicorp#639, hashicorp#1497, hashicorp#1185 and also nomad/5020

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
if the test user is not member of any group, we skip and log

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
@deblasis deblasis changed the title Allow setting user/group ownership of templete output Allow setting user/group ownership of template output Nov 3, 2021
@eikenb eikenb added enhancement hashicat-update-required Changes that need to be ported to hashicat labels Nov 4, 2021
@eikenb
Copy link
Contributor

eikenb commented Nov 4, 2021

Looks like a problem with the Windows build and the CI system doesn't like the Chown call, might be a mount time restriction on the /tmp directory. Maybe try using a local directory for it instead?

- fix: leftover unused package
- fix: removed troubleshooting t.Log("groups:"...)
- fix: now we matrix test setting the flags Uid and Gid to -1 => noop

Signed-off-by: Alessandro De Blasis <alex@deblasis.net>
@deblasis
Copy link
Contributor Author

deblasis commented Nov 5, 2021

Hi @eikenb, I covered more cases and changed things a bit. It should work now.
I didn't switch to a local directory because I see that /tmp is used extensively without issues and I think I found the bug elsewhere.
Let's see if Miss CI is happy about it 😄

@lukas-w
Copy link
Contributor

lukas-w commented Nov 5, 2021

I also did some work on this issue recently but didn't find the time to open a PR yet unfortunately (no tests yet). My implementation supports setting a file owner/group and permission bits (converts octal permissions to ACL entries) on Windows as well. See master...lukas-w:f-file-owner. Maybe we can join efforts on this?

@3nprob
Copy link

3nprob commented Jan 21, 2022

@eikenb Did you get to take another look at this after the CI issues were addressed? For being able to use separate uids for different tasks, this is crucial.

While it would be sweet to get Windows support as per @lukas-w , isn't *nix/mac independently worthwhile enough to push through?

@eikenb eikenb added this to the v0.28.0 milestone Jan 24, 2022
@eikenb
Copy link
Contributor

eikenb commented Jan 24, 2022

Thanks for the prompt @3nprob, I'll take another look at this when I start work on 0.28.0 (should be soon, next in my project queue).

@eikenb eikenb added hashicat-update-required Changes that need to be ported to hashicat and removed hashicat-update-required Changes that need to be ported to hashicat labels Jan 27, 2022
Copy link
Contributor

@eikenb eikenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. Thanks!

@lukas-w
Copy link
Contributor

lukas-w commented Jan 27, 2022

While it would be sweet to get Windows support as per @lukas-w , isn't *nix/mac independently worthwhile enough to push through?

Sure, It's great to see this merged! Another difference in the two implementations though is that mine supports specifying user and group entries as string so you can pass uid/gid as well as user/group names which I find more useful personally because it allows for more readable configurations and facilitates Windows support (even if it's not implemented now) as Windows doesn't use integer identifiers. I can prepare a PR for this soon if that's something that's desired as to avoid a future interface change in Nomad & consul-template.

@eikenb
Copy link
Contributor

eikenb commented Jan 28, 2022

Hey @lukas-w,

If you'd like to open a PR I'll be happy to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement hashicat-update-required Changes that need to be ported to hashicat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants