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 namespace check for uid/gid mappings #199

Merged

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao [email protected]

@@ -340,6 +336,14 @@ func checkLinux(spec rspec.Spec, rootfs string, hostCheck bool) (msgs []string)
}
}

if (len(spec.Linux.UIDMappings) > 0 || len(spec.Linux.GIDMappings) > 0) && !userExists {
msgs = append(msgs, "UID/GID mappings require User namespace exists")
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrasing used by existing similar warnings is:

{setting} requires a new {type} namespace to be specified as well

Copy link
Author

Choose a reason for hiding this comment

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

On 08/24/2016 12:50 AM, W. Trevor King wrote:

The phrasing used by existing similar warnings is:

{setting} requires a new {type} namespace to be specified as well

I think container joins into an existing user namespace also can work.
Doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Aug 23, 2016 at 05:58:31PM -0700, Ma Shimiao wrote:

  • if (len(spec.Linux.UIDMappings) > 0 || len(spec.Linux.GIDMappings) > 0) && !userExists {
  •   msgs = append(msgs, "UID/GID mappings require User namespace exists")
    

08/24/2016 12:50 AM, W. Trevor King:

The phrasing used by existing similar warnings is:

{setting} requires a new {type} namespace to be specified as well

I think container joins into an existing user namespace also can
work. Doesn't it?

The kernel has no problem with join-and-tweak, but the OCI spec does
not allow it at the moment [1,2]. Interestingly, the spec places no
such restriction on namespaces inherited from the host. I've filed
3 to figure out what the runtime-spec maintainers want to do about
that.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, got it.
Since spec does not allow now, I fixed error message as you suggested.

@Mashimiao Mashimiao force-pushed the add-namespace-check-for-mappings branch from b477536 to bf50b73 Compare August 24, 2016 07:31
@wking
Copy link
Contributor

wking commented Aug 24, 2016

bf50b73 looks good to me. It's taking the (b) approach from 1,
even though the current spec wording doesn't quite match that
interpretation. But it matches the ocitools existing namespace
validation logic, and we can revisit all of that if 1 ends up being
resolved in a different direction.

@mrunalp
Copy link
Contributor

mrunalp commented Aug 24, 2016

LGTM

@mrunalp mrunalp merged commit 1a3f3c6 into opencontainers:master Aug 24, 2016
wking pushed a commit to wking/ocitools-v2 that referenced this pull request Aug 24, 2016
Signed-off-by: Ma Shimiao <[email protected]>

Backported to v1.0.0.rc1 from bf50b73 opencontainers#199 (cherry-pick applied
cleanly).

Signed-off-by: W. Trevor King <[email protected]>
@Mashimiao Mashimiao deleted the add-namespace-check-for-mappings branch November 14, 2016 09:29
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.

3 participants