clarify user ns mappings and time ns offset configurations #1237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: lfbzhm <[email protected]>
lifubang
requested review from
AkihiroSuda,
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
kolyshkin,
mrunalp,
thaJeztah,
tianon,
vbatts and
utam0k
as code owners
rata
reviewed
Dec 6, 2023
| @@ -37,7 +37,7 @@ The following parameters can be specified to set up namespaces: | |||
| * **`time`** the container will be able to have its own clocks. | |||
| * **`path`** *(string, OPTIONAL)* - namespace file. | |||
| This value MUST be an absolute path in the [runtime mount namespace](glossary.md#runtime-namespace). | |||
| The runtime MUST place the container process in the namespace associated with that `path`. | |||
| The runtime MUST let the container process join in the namespace associated with that `path`. | |||
There was a problem hiding this comment.
The old wording was better IMHO, "let the container process join" is not an accurate description.
| @@ -80,6 +80,9 @@ If a `namespaces` field contains duplicated namespaces with same `type`, the run | |||
|
|
|||
| ## <a name="configLinuxUserNamespaceMappings" />User namespace mappings | |||
|
|
|||
| If the runtime should create an new user namespace for the container, `uidMappings` and `gidMappings` should be provided, otherwise, these two fields should not be specified, | |||
| and it will be ignored by the runtime. | |||
There was a problem hiding this comment.
Suggested change
| and it will be ignored by the runtime. | |
| and they MUST be ignored by the runtime. |
| @@ -113,6 +116,9 @@ Note that the number of mapping entries MAY be limited by the [kernel][user-name | |||
|
|
|||
| ## <a name="configLinuxTimeOffset" />Offset for Time Namespace | |||
|
|
|||
| If the runtime should create an new time namespace for the container, `timeOffsets` should be provided, otherwise, it should not be specified, | |||
| and it will be ignored by the runtime. | |||
cyphar
requested changes
Dec 7, 2023
Comment on lines
+83
to
+84
| If the runtime should create an new user namespace for the container, `uidMappings` and `gidMappings` should be provided, otherwise, these two fields should not be specified, | ||
| and it will be ignored by the runtime. |
There was a problem hiding this comment.
In the runtime-spec, we don't hard-wrap lines. Each line is meant to be a complete sentence (to make diffs less messy).
I think the text here is also a little confusing. Maybe something like this would better explain things:
Suggested change
| If the runtime should create an new user namespace for the container, `uidMappings` and `gidMappings` should be provided, otherwise, these two fields should not be specified, | |
| and it will be ignored by the runtime. | |
| If the runtime is creating a new user namespace for this container (in other words, the [`user` namespace entry in `namespaces`](#configLinuxNamespaces) does not have a `path` specified), `uidMappings` and `gidMappings` MUST be provided. | |
| However, if the container will join an existing user namespace, `uidMappings` and `gidMappings` SHOULD NOT be specified (user namespaces cannot have their mappings changed after being configured). | |
| If specified, the specified `uidMappings` and `gidMappings` SHOULD be the same mappings as the existing user namespace, and MUST be ignored by the runtime. | |
| Runtimes MAY [generate an error](runtime.md#errors) if the specified `uidMappings` and `gidMappings` do not match the existing user namespace. | |
| > **NOTE**: runc 1.1 and earlier, as well as some other spec-compliant runtimes, incorrectly required users to specify `uidMappings` and `gidMappings` even if the container was joining an existing namespace. The provided mappings were completely ignored until runc 1.2. |
Comment on lines
+119
to
+120
| If the runtime should create an new time namespace for the container, `timeOffsets` should be provided, otherwise, it should not be specified, | ||
| and it will be ignored by the runtime. |
There was a problem hiding this comment.
Suggested change
| If the runtime should create an new time namespace for the container, `timeOffsets` should be provided, otherwise, it should not be specified, | |
| and it will be ignored by the runtime. | |
| If the container will join an existing time namespace, `timeOffsets` SHOULD NOT be specified (time namespaces cannot have their mappings changed after being used), and MUST be ignored by the runtime. | |
| Runtimes MAY [generate an error](runtime.md#errors) if the `timeOffsets` is specified and the container is joining an existing time namespace. |
We can be a bit more strict here because there's no released version of runc with timens support yet.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Nowadays, when we let the container to join an existing
userortimenamespace, we should not provideuidMappings,gidMappings, andtimeOffsetsconfigurations.Let's add some description to clarify user ns mappings and time ns offset configurations.
background: