Clarify behavior around namespaces paths #158

mrunalp · 2015-09-08T16:43:44Z

See #155
Signed-off-by: Mrunal Patel mrunalp@gmail.com

wking · 2015-09-08T20:16:01Z

Instead of “ignore any other config when a namespace path is set”, I'd
rather just error-out if some incompatible values are set 1 and
support the join-and-tweak (or tweak-and-join) where possible 2.
Silently ignoring portions of the config seems like it would be a
source of confusion.

gao-feng · 2015-09-09T07:52:39Z

@wking Can not join-and-tweat be implemented by hook, Ignore configuation may confuse someone, how about warning or error-out.

laijs · 2015-09-09T10:36:12Z

The clarify is definitely necessary and important! Thanks!

I think we should also point out which config are the "other config related to that namespace" clearly.
And the implementation should output warning or error if it is hit.

Spec.Root
Spec.Mount
LinuxSpec.Linux.RootfsPropagation
RuntimeSpec.Mounts
LinuxRuntime.Devices
...

I think this list hasn't contains all

Any thought?

philips · 2015-09-09T16:45:21Z

runtime-config-linux.md

@@ -7,6 +7,7 @@ For more information, see [the man page](http://man7.org/linux/man-pages/man7/na
 Namespaces are specified in the spec as an array of entries.
 Each entry has a type field with possible values described below and an optional path element.
 If a path is specified, that particular file is used to join that type of namespace.
+Also, when a path is specified, a runtime MUST assume that the setup for that particular namespace has already been done and ignore any other config related to that namespace.


Hrm, if there is a join and a create in the runtime I would say it should error, not ignore.

+1 for validation and raising an error. We essentially need something like proto oneof to get around this issue.

Right. I was wondering if we should describe config and state in a richer language than go struct ( like protobuf ), which we can then serialize to/from json.

@vishh Yes, that is the requirement.
@philips Updated.

wking · 2015-09-09T18:22:19Z

On Wed, Sep 09, 2015 at 12:52:40AM -0700, Gao feng wrote:

@wking Can not join-and-tweat be implemented by hook…

Lots of existing config stuff can be implemented via hooks
(e.g. everything under ‘mounts’). But we have spec entries for it,
and there's nothing non-sensical about “Join the mnt namespace at
foo/bar and mount a tmpfs at /a/b/c”. If we thought the “mount a
tmpfs at /a/b/c” was common enough to be in the spec for container
creation, I see no reason to have it not be in the spec for
namespace joinin.

Ignore configuation may confuse someone, how about warning or
error-out.

Yeah, I prefer explicitly failing to silently ignoring non-sensical
content 1. I just don't think all “joining an existing namespace
and manipulating that namespace” specs are non-sensical. But in
today's call, the consensus seemed to be punting on cases like that
until someone tells us a convincing story about why they would be
useful. Until then, I'm fine erroring out whenever a config specifies
a namespace path and also modifications for that namespace.

wking · 2015-09-09T19:53:01Z

runtime-config-linux.md

@@ -7,6 +7,7 @@ For more information, see [the man page](http://man7.org/linux/man-pages/man7/na
 Namespaces are specified in the spec as an array of entries.
 Each entry has a type field with possible values described below and an optional path element.
 If a path is specified, that particular file is used to join that type of namespace.
+Also, when a path is specified, a runtime MUST assume that the setup for that particular namespace has already been done and error out if there is any other config related to that namespace specified.


“namespace specified” reads a little awkwardly to me. Maybe “if the config specifies anything else related to that namespace”?

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

gao-feng · 2015-09-10T03:37:40Z

@wking

Yeah, I prefer explicitly failing to silently ignoring non-sensical
content [1]. I just don't think all “joining an existing namespace
and manipulating that namespace” specs are non-sensical. But in
today's call, the consensus seemed to be punting on cases like that
until someone tells us a convincing story about why they would be
useful. Until then, I'm fine erroring out whenever a config specifies
a namespace path and also modifications for that namespace.

Thanks for your information,
Let's disallow this behavior to get rid of the possible mistake.

wking · 2015-09-10T03:45:29Z

02b456e looks good to me.

gao-feng · 2015-09-10T06:21:15Z

LGTM

vbatts · 2015-09-10T21:20:43Z

LGTM

crosbymichael · 2015-09-10T21:25:10Z

LGTM

Clarify behavior around namespaces paths

The UTS namespace is for hostnames and NIS domain names [1]. Without a new namespace, the hostname entry would clobber the host environment's hostname. Clobbering the host's hostname or a joined-namespace's hostname might be acceptable for folks who trust their bundles, but it's not allowed by the "error out if the config specifies anything else related to that namespace" language that landed in 02b456e (Clarify behavior around namespaces paths, 2015-09-08, opencontainers#158). [1]: http://man7.org/linux/man-pages/man7/namespaces.7.html Signed-off-by: W. Trevor King <wking@tremily.us>

This was initially based on OCI's limitation [1]. But I never felt that strongly about that limitation [2,3]. And join-and-tweak is occasionally useful (see namespaces.mount in examples/good/create-exec-delete-root/exec.json). We never enforced the nominal restriction, so this commit updates the docs to allow join-and-tweak. Users who try to tweak nonsensical things will get errors from the kernel rejecting the invalid tweaks. [1]: opencontainers/runtime-spec#158 [2]: opencontainers/runtime-spec#158 (comment) [3]: opencontainers/runtime-spec#158 (comment)

Since [1] we've required runtimes to error out if a configuration joins an existing namespace and adjusts it somehow (e.g. joining an existing UTC namespace and setting 'hostname', [2]). However, the wording from [1] (which survives untouched in the current master) only talked about "when a path is specified". I see two possible approaches for internal consistency: a. Lift the OCI restriction and allow join-and-tweak [3] where the kernel supports it. When we landed the current restriction, the main issues seemed to be "we don't have a clear use-case for join and tweak" [4] (although see [5]) and "this is a foot gun [6,7]" (I'd rather leave policy to higher-level config linters). b. Extend the OCI restriction to all cases where the runtime does not create a new namespace. Besides the already covered "namespace entry exists and includes 'path'", we'd also want to forbid configs that were missing the relevant namespace(s) entirely (in which case the container inherits the host namespace(s)). I'm partial to (a) in the long run, but (b) is less of a shift from the current spec and likely a better choice for a pending 1.0. This commit implements (b). It also makes it explicit that not listing a namespace type will cause the container to inherit the runtime namespace of that type. [1]: opencontainers#158 Subject: Clarify behavior around namespaces paths [2]: opencontainers#214 Subject: config: Require a new UTS namespace for config.json's hostname [3]: opencontainers#158 (comment) [4]: opencontainers#158 (comment) [5]: opencontainers#305 Subject: [Tracker] Live Container Updates [6]: opencontainers#158 (comment) [7]: opencontainers#537 (comment) Subject: [linux] Tweaking host namespaces? Signed-off-by: W. Trevor King <wking@tremily.us>

There are more background references for the Linux-namespaces no-tweaking rule in 01c2d55 (config-linux: Extend no-tweak requirement to runtime namespaces, 2016-08-24, opencontainers#538). But the old rule's: > ... error out if the config specifies anything else related to that > namespace. was overly broad. For example, it arguably blocked you from setting network interface priorities for interfaces belonging to an old network namespace even if you were setting those priorities in a new cgroup (because the interfaces and therefore priorities for them are related to the old network namespace). The new rule tries to apply the spirit of the old rule ("don't touch things that already exist") more generally so we have a consistent approach that clearly *does* allow you to configure a new cgroup without having to care about new/old namespaces. I'm personally fine with join-and-tweak, but the maintainer consensus is that it's too complicated to allow (at least for now) [1,2]. [1]: opencontainers#158 Subject: Clarify behavior around namespaces paths [2]: opencontainers#537 (comment) Subject: [linux] Tweaking host namespaces? Signed-off-by: W. Trevor King <wking@tremily.us>

This restriction originally landed via 02b456e (Clarify behavior around namespaces paths, 2015-09-08, opencontainers#158). The hostname case landed via 66a0543 (config: Require a new UTS namespace for config.json's hostname, 2015-10-05, opencontainers#214) citing the namespace restriction. The restriciton extended to runtime namespaces in 01c2d55 (config-linux: Extend no-tweak requirement to runtime namespaces, 2016-08-24, opencontainers#538). There was a proposal in-flight to get config-wide consistency around the no-tweaking concept [1]. In today's meeting, the maintainer consensus was to strike the no-tweaking restriction [2], which is what I've done here. The hostname entry still mentions the UTS namespace to provide a guard against accidental foot-gunning. There was no no-tweaking language for properties related to other namespaces (e.g. 'mounts'). Maybe the other namespaces have more obvious names. [1]: opencontainers#540 [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-01-11-22.04.log.html#l-117 Signed-off-by: W. Trevor King <wking@tremily.us>

This restriction originally landed via 02b456e (Clarify behavior around namespaces paths, 2015-09-08, opencontainers#158). The hostname case landed via 66a0543 (config: Require a new UTS namespace for config.json's hostname, 2015-10-05, opencontainers#214) citing the namespace restriction. The restriciton extended to runtime namespaces in 01c2d55 (config-linux: Extend no-tweak requirement to runtime namespaces, 2016-08-24, opencontainers#538). There was a proposal in-flight to get config-wide consistency around the no-tweaking concept [1]. In today's meeting, the maintainer consensus was to strike the no-tweaking restriction [2], which is what I've done here. I've removed the ROADMAP entry because this gives folks a way to adjust existing containers (launch a new container which joins and tweaks the original). The hostname entry still mentions the UTS namespace to provide a guard against accidental foot-gunning. There was no no-tweaking language for properties related to other namespaces (e.g. 'mounts'). Maybe the other namespaces have more obvious names. [1]: opencontainers#540 [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-01-11-22.04.log.html#l-117 Signed-off-by: W. Trevor King <wking@tremily.us>

mrunalp mentioned this pull request Sep 8, 2015

what happen if set mount path in linux namespaces and mounts in spec at the same time #155

Closed

wking mentioned this pull request Sep 8, 2015

Adding cgroups path to the Spec. #137

Merged

philips reviewed Sep 9, 2015
View reviewed changes

mrunalp force-pushed the ns_path branch from 2f45895 to ddb0c4b Compare September 9, 2015 19:44

wking reviewed Sep 9, 2015
View reviewed changes

vbatts force-pushed the master branch from fd0ff56 to 26b9be2 Compare September 9, 2015 20:51

Clarify behavior around namespaces paths.

02b456e

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp force-pushed the ns_path branch from ddb0c4b to 02b456e Compare September 9, 2015 22:26

crosbymichael added a commit that referenced this pull request Sep 10, 2015

Merge pull request #158 from mrunalp/ns_path

cbda521

Clarify behavior around namespaces paths

crosbymichael merged commit cbda521 into opencontainers:master Sep 10, 2015

gao-feng mentioned this pull request Sep 11, 2015

Allow to call Pre-start hooks for join-ns-spec #161

Closed

wking mentioned this pull request Oct 6, 2015

config: Require a new UTS namespace for config.json's hostname #214

Merged

wking mentioned this pull request May 27, 2016

*: add support for cgroup namespace #397

Merged

wking mentioned this pull request Jun 9, 2016

config-linux: clarify cgroup requirements #493

Merged

wking mentioned this pull request Jun 28, 2016

validate: add sysctl check opencontainers/runtime-tools#124

Merged

This was referenced Aug 24, 2016

[linux] Tweaking host namespaces? #537

Closed

add namespace check for uid/gid mappings opencontainers/runtime-tools#199

Merged

validate: add network host-specific check opencontainers/runtime-tools#200

Merged

wking mentioned this pull request Aug 26, 2016

config: Shift Linux-namespace no-tweaking rule to a generic rule #540

Closed

wking mentioned this pull request Jan 11, 2017

config-linux: Lift no-tweaking namespace restriction #649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify behavior around namespaces paths #158

Clarify behavior around namespaces paths #158

mrunalp commented Sep 8, 2015

wking commented Sep 8, 2015

gao-feng commented Sep 9, 2015

laijs commented Sep 9, 2015

philips Sep 9, 2015

vishh Sep 9, 2015

dqminh Sep 9, 2015

mrunalp Sep 9, 2015

wking commented Sep 9, 2015

wking Sep 9, 2015

gao-feng commented Sep 10, 2015

wking commented Sep 10, 2015 via email

gao-feng commented Sep 10, 2015

vbatts commented Sep 10, 2015

crosbymichael commented Sep 10, 2015

Clarify behavior around namespaces paths #158

Clarify behavior around namespaces paths #158

Conversation

mrunalp commented Sep 8, 2015

wking commented Sep 8, 2015

gao-feng commented Sep 9, 2015

laijs commented Sep 9, 2015

philips Sep 9, 2015

Choose a reason for hiding this comment

vishh Sep 9, 2015

Choose a reason for hiding this comment

dqminh Sep 9, 2015

Choose a reason for hiding this comment

mrunalp Sep 9, 2015

Choose a reason for hiding this comment

wking commented Sep 9, 2015

wking Sep 9, 2015

Choose a reason for hiding this comment

gao-feng commented Sep 10, 2015

wking commented Sep 10, 2015 via email

gao-feng commented Sep 10, 2015

vbatts commented Sep 10, 2015

crosbymichael commented Sep 10, 2015