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 definite limit for username mappings #939

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mashimiao
Copy link

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

@vbatts
Copy link
Member

vbatts commented Apr 4, 2018

I suppose this is fine. Any assumptions on providing that mapping would be left to the runtime implementation, so the MUST here makes sense.
LGTM

Approved with PullApprove

Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

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

There's related discussion in #961, where I point out that I'm fine punting this to runtime errors. But if we want to require runtimes to validate this on their own (instead of letting the kernel validate it), that's ok with me too.

@@ -87,6 +87,7 @@ Each entry has the following structure:
* **`size`** *(uint32, REQUIRED)* - is the number of ids to be mapped.

The runtime SHOULD NOT modify the ownership of referenced filesystems to realize the mapping.
The runtime MUST generate an error when user namespace mappings is specified, but a new user namespace is not specified to create.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should “generate an error” be a link, like we generally use now:

$ git describe
v1.0.1-20-g4ad8e74
$ git grep 'generate an error]' config-linux.md
config-linux.md:    The runtime MUST [generate an error](runtime.md#errors) if `path` is not associated with a namespace of type `type`.
config-linux.md:If a `namespaces` field contains duplicated namespaces with same `type`, the runtime MUST [generate an error](runtime.md#errors).
config-linux.md:    If a [file][] already exists at `path` that does not match the requested device, the runtime MUST generate an error.
config-linux.md:Runtimes MAY consider certain `cgroupsPath` values to be invalid, and MUST generate an error if this is the case.
config-linux.md:    If no mounted `resctrl` pseudo-filesystem is available in the [runtime mount namespace](glossary.md#runtime-namespace), the runtime MUST [generate an error](runtime.md#errors).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also ok rewording this to be a config limitation, with something like:

Configurations that set an ID mapping MUST have entry in linux.namespaces where type is user and path is unset.

That way runtimes can punt to the kernel, but folks who want to pre-check their config with runtime-tools (or other) validation can catch instances of this.

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping: hrm. This is another conditional MUST, but it is a behavior I think is consistent with how runc and pals currently does. So rev'ing the spec version ought not be needed.
@opencontainers/runtime-spec-maintainers is this sufficient to add just this sentence? (i think it should be fine)

@h-vetinari h-vetinari mentioned this pull request Feb 3, 2020
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.

None yet

3 participants