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

Add missing interface to set init processes Umask #706

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Jul 13, 2020

Umask is a field specified in the runtime spec, but we don't
have a method to set it in runtime-tools. Some users might
want to modify the default Umask of a container.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 13, 2020

@mrunalp PTAL

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 13, 2020

@q384566678 @wking @cyphar PTAL

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 13, 2020

@haircommander PTAL

@zhouhao3
Copy link

I think we need to add corresponding options in the generate function to call this function.

@rhatdan rhatdan force-pushed the umask branch 2 times, most recently from eadda1f to 829dfc5 Compare July 13, 2020 10:20
@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 13, 2020

@q384566678 Added.

@haircommander
Copy link

LGTM, thanks @rhatdan

Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouhao3
Copy link

zhouhao3 commented Jul 14, 2020

LGTM

Approved with PullApprove

Umask is a field specified in the runtime spec, but we don't
have a method to set it in runtime-tools.  Some users might
want to modify the default Umask of a container.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 14, 2020

@philips
Copy link
Contributor

philips commented Jul 14, 2020

I am no longer a maintainer FYI 👍

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 14, 2020

@philips Your name is still listed, sorry for bothering you.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 14, 2020

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 07406c5 into opencontainers:master Jul 14, 2020
@philips
Copy link
Contributor

philips commented Jul 14, 2020

@rhatdan where is it listed? I think i am gone from maintainers.

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 14, 2020

@cyphar
Copy link
Member

cyphar commented Jul 15, 2020

Yeah the @opencontainers/runtime-tools-maintainers list needs to be updated. /cc @caniszczyk?

Comment on lines +448 to +451
func (g *Generator) SetProcessUmask(umask uint32) {
g.initConfigProcess()
g.Config.Process.User.Umask = umask
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed to be an *uint32 with opencontainers/runtime-spec@d3f079a, which breaks compatibility between the tools and the spec of both latest master branches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just found the same thing :)

Copy link
Contributor

Choose a reason for hiding this comment

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

So, there is a PR to fix it (#708) but apparently it requires a new release of runtime-specs, for which there is an issue opened (opencontainers/runtime-spec#1052) but it's not yet done so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants