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

clarify user ns mappings and time ns offset configurations #1237

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

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Dec 6, 2023

Nowadays, when we let the container to join an existing user or time namespace, we should not provide uidMappings, gidMappings, and timeOffsets configurations.
Let's add some description to clarify user ns mappings and time ns offset configurations.


background:

  1. (u|g)idMappings should not exist when joining an existing user ns runc#4122
  2. libcontainer: remove all mount logic from nsexec runc#3985 (comment)

@@ -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`.
Copy link
Member

Choose a reason for hiding this comment

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

why this wording change?

Copy link
Member

@cyphar cyphar Dec 7, 2023

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

idem

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants