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.

Sign up for GitHub

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
+7 −1
Open

clarify user ns mappings and time ns offset configurations #1237

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

The runtime MUST [generate an error](runtime.md#errors) if `path` is not associated with a namespace of type `type`.

If `path` is not specified, the runtime MUST create a new [container namespace](glossary.md#container-namespace) of type `type`.
Expand Down Expand Up @@ -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.

Comment on lines +83 to +84
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.


**`uidMappings`** (array of objects, OPTIONAL) describes the user namespace uid mappings from the host to the container.
**`gidMappings`** (array of objects, OPTIONAL) describes the user namespace gid mappings from the host to the container.

Expand Down Expand Up @@ -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 +119 to +120
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.


**`timeOffsets`** (object, OPTIONAL) sets the offset for Time Namespace. For more information
see the [time_namespaces][time_namespaces.7].

Expand Down