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

config-linux: Specify host mount namespace for namespace paths #275

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

wking
Copy link
Contributor

@wking wking commented Dec 18, 2015

Avoid trouble with situations like:

# mount --bind /mnt/test /mnt/test
# mount --make-rprivate /mnt/test
# touch /mnt/test/mnt /mnt/test/user
# mount --bind /proc/123/ns/mnt /mnt/test/mnt
# mount --bind /proc/123/ns/user /mnt/test/user
# nsenter --mount=/proc/123/ns/mnt --user /proc/123/ns/user sh

which uses the required private mount for binding mount
namespace references. We want to avoid:

  1. Runtime opens /mnt/test/mnt as fd 3.
  2. Runtime joins the mount namespace referenced by fd 3.
  3. Runtime fails to open /mnt/test/user, because /mnt/test is not
    visible in the current mount namespace.

and instead get runtime authors to setup flows like:

  1. Runtime opens /mnt/test/mnt as fd 3.
  2. Runtime opens /mnt/test/user as fd 4.
  3. Runtime joins the mount namespace referenced by fd 3.
  4. Runtime joins the user namespace referenced by fd 4.

This also applies to new namespace creation. We want to avoid:

  1. Runtime clones a container process with a new mount namespace.
  2. Container process fails to open /mnt/test/user, because /mnt/test
    is not visible in the current mount namespace.

in favor of something like:

  1. Runtime opens /mnt/test/user as fd 3.
  2. Runtime clones a container process with a new mount namespace.
  3. Host process closes unneeded fd 3.
  4. Container process joins the user namespace referenced by fd 3.

@@ -17,7 +17,7 @@ The following parameters can be specified to setup namespaces:
* **`uts`** the container will be able to have its own hostname and domain name
* **`user`** the container will be able to remap user and group IDs from the host to local users and groups within the container

* **`path`** *(string, optional)* - path to namespace file
* **`path`** *(string, optional)* - path to namespace file in the host mount namespace
Copy link
Member

Choose a reason for hiding this comment

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

😞

It is the namespace of the runtime (not host) as you can nest things so this is not exactly correct to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Fri, Dec 18, 2015 at 11:02:55AM -0800, Michael Crosby wrote:

-* path (string, optional) - path to namespace file
+* path (string, optional) - path to namespace file in the host mount namespace

😞

It is the namespace of the runtime (not host) as you can nest things
so this is not exactly correct to say.

If we prefer “runtime” to “host” for that distinction, I'm fine
changing that. Ideally we'd land a glossary like #107 so we had a
place to define these terms. For me:

  • host namespace: a namespace you are in when you invoke the runtime
  • host process: the runtime process invoked by the user
  • container process: the process created by a clone call in the host
    process which will eventually execute the user-configured process.

Both the host and container processes are running runtime code
(although the container process eventually transitions to
user-configured code), so I find “runtime process”, “runtime
namespace”, etc. to be imprecise.

@mrunalp used the “host” phrasing in some of the earlier #231 commits
[1,2], but again, if “host” → “runtime” gets me to a consensus naming
scheme, then I'll reroll this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good the way it is. The path to a namespace file is unambiguous :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Fri, Dec 18, 2015 at 11:28:29AM -0800, Mrunal Patel wrote:

-* path (string, optional) - path to namespace file
+* path (string, optional) - path to namespace file in the host mount namespace

I think it is good the way it is. The path to a namespace file is
unambiguous :)

Is that “we can close this PR” or “we don't need the ‘host’ →
‘runtime’ change 1”?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that we can close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

this seems more like a validation concern to me. i'm not sure what you're technically attempting to achieve here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Fri, Dec 18, 2015 at 01:48:35PM -0800, Vincent Batts wrote:

-* path (string, optional) - path to namespace file
+* path (string, optional) - path to namespace file in the host mount namespace

this seems more like a validation concern to me.

The spec is a meeting point beween runtime authors, bundle
authors, runtime callers, and validation authors (who are likely both
bundle authors and runtime callers). I don't see how you could have
an ambiguity that only impacted one subset.

i'm not sure what you're technically attempting to achieve here.

I'm just trying to make sure folks don't misinterpret the namespace
path as “in the current mount namespace just before the setns call”,
which is what I did in ccons (and which you want for the alternative
namespace approach in 1). If we all agree that the interpretation
should be “paths are resolved in the mount namespace from which the
runtime is launched”, then landing wording to that effect seems easy
to do, and will either:

a. help reduce ambiguity and resulting conflicts, or
b. not take up too much space

depending on your view of how ambiguous the current wording is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "runtime namespace" is the most compact way of expressing this. We can expand that meaning in a glossary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Tue, Dec 22, 2015 at 10:32:46PM -0800, Brandon Philips wrote:

-* path (string, optional) - path to namespace file
+* path (string, optional) - path to namespace file in the host mount namespace

I think "runtime namespace" is the most compact way of expressing
this. We can expand that meaning in a glossary.

Ok, that makes two votes in favor of “runtime” phrasing 1 after I'd
written up my reasons for preferring host in detail 2. I'll rebase
this PR to use that language and add it to the glossary once something
like #107 lands and we have a glossary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Wed, Dec 23, 2015 at 09:33:02AM -0800, W. Trevor King wrote:

Ok, that makes two votes in favor of “runtime” phrasing 1 after
I'd written up my reasons for preferring host in detail 2. I'll
rebase this PR to use that language and add it to the glossary once
something like #107 lands and we have a glossary.

Done with f1146ed4484bdd.

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 30, 2015
Avoid trouble with situations like:

  # mount --bind /mnt/test /mnt/test
  # mount --make-rprivate /mnt/test
  # touch /mnt/test/mnt /mnt/test/user
  # mount --bind /proc/123/ns/mnt /mnt/test/mnt
  # mount --bind /proc/123/ns/user /mnt/test/user
  # nsenter --mount=/proc/123/ns/mnt --user /proc/123/ns/user sh

which uses the required private mount for binding mount namespace
references [1,2,3].  We want to avoid:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime joins the mount namespace referenced by fd 3.
3. Runtime fails to open /mnt/test/user, because /mnt/test is not
   visible in the current mount namespace.

and instead get runtime authors to setup flows like:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime opens /mnt/test/user as fd 4.
3. Runtime joins the mount namespace referenced by fd 3.
4. Runtime joins the user namespace referenced by fd 4.

This also applies to new namespace creation.  We want to avoid:

1. Runtime clones a container process with a new mount namespace.
2c. Container process fails to open /mnt/test/user, because /mnt/test
    is not visible in the current mount namespace.

in favor of something like:

1. Runtime opens /mnt/test/user as fd 3.
2. Runtime clones a container process with a new mount namespace.
3h. Host process closes unneeded fd 3.
3c. Container process joins the user namespace referenced by fd 3.

I also define runtime and container namespaces, so we have consistent
terminology.  I prefer:

* host namespace: a namespace you are in when you invoke the runtime
* host process: the runtime process invoked by the user
* container process: the process created by a clone call in the host
  process which will eventually execute the user-configured process.

Both the host and container processes are running runtime code
(although the container process eventually transitions to
user-configured code), so I find "runtime process", "runtime
namespace", etc. to be imprecise.  However, the maintainer consensus
is for "runtime namespace" [4,5], so that's what we're going with
here.

[1]: http://karelzak.blogspot.com/2015/04/persistent-namespaces.html
[2]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4ce5d2b1a8fde84c0eebe70652cf28b9beda6b4e
[3]: http://mid.gmane.org/[email protected]
[4]: opencontainers#275 (comment)
[5]: opencontainers#275 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@mrunalp
Copy link
Contributor

mrunalp commented Mar 16, 2016

Needs a rebase

Avoid trouble with situations like:

  # mount --bind /mnt/test /mnt/test
  # mount --make-rprivate /mnt/test
  # touch /mnt/test/mnt /mnt/test/user
  # mount --bind /proc/123/ns/mnt /mnt/test/mnt
  # mount --bind /proc/123/ns/user /mnt/test/user
  # nsenter --mount=/proc/123/ns/mnt --user /proc/123/ns/user sh

which uses the required private mount for binding mount namespace
references [1,2,3].  We want to avoid:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime joins the mount namespace referenced by fd 3.
3. Runtime fails to open /mnt/test/user, because /mnt/test is not
   visible in the current mount namespace.

and instead get runtime authors to setup flows like:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime opens /mnt/test/user as fd 4.
3. Runtime joins the mount namespace referenced by fd 3.
4. Runtime joins the user namespace referenced by fd 4.

This also applies to new namespace creation.  We want to avoid:

1. Runtime clones a container process with a new mount namespace.
2c. Container process fails to open /mnt/test/user, because /mnt/test
    is not visible in the current mount namespace.

in favor of something like:

1. Runtime opens /mnt/test/user as fd 3.
2. Runtime clones a container process with a new mount namespace.
3h. Host process closes unneeded fd 3.
3c. Container process joins the user namespace referenced by fd 3.

I also define runtime and container namespaces, so we have consistent
terminology.  I prefer:

* host namespace: a namespace you are in when you invoke the runtime
* host process: the runtime process invoked by the user
* container process: the process created by a clone call in the host
  process which will eventually execute the user-configured process.

Both the host and container processes are running runtime code
(although the container process eventually transitions to
user-configured code), so I find "runtime process", "runtime
namespace", etc. to be imprecise.  However, the maintainer consensus
is for "runtime namespace" [4,5], so that's what we're going with
here.

[1]: http://karelzak.blogspot.com/2015/04/persistent-namespaces.html
[2]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4ce5d2b1a8fde84c0eebe70652cf28b9beda6b4e
[3]: http://mid.gmane.org/[email protected]
[4]: opencontainers#275 (comment)
[5]: opencontainers#275 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@wking wking changed the title runtime-config-linux: Specify host mount namespace for namespace paths config-linux: Specify host mount namespace for namespace paths Mar 16, 2016
@wking
Copy link
Contributor Author

wking commented Mar 16, 2016

On Wed, Mar 16, 2016 at 10:39:42AM -0700, Mrunal Patel wrote:

Needs a rebase

Rebased with 4484bdd5dad125, with Git resolving any conflicts for
me.

@crosbymichael
Copy link
Member

LGTM

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Mar 16, 2016

LGTM

mrunalp pushed a commit that referenced this pull request Mar 16, 2016
config-linux: Specify host mount namespace for namespace paths
@mrunalp mrunalp merged commit a7a90d9 into opencontainers:master Mar 16, 2016
@wking wking deleted the namespace-host-paths branch March 30, 2016 04:18
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Apr 30, 2016
The language from 15dee2e (runtime: Add prestart/poststop hooks,
2015-08-03, opencontainers#34) landed well before we had glossary entries for the
runtime and container namespaces (from 5dad125, config-linux: Specify
host mount namespace for namespace paths, 2015-12-18, opencontainers#275).  Now that
we do have language to cover that concept, it's better to explicitly
say that hooks run in the runtime namespace instead of leaving it to
the reader to extrapolate from the filesystem requirement.

With the new namespace wording, the "host's filesystem" wording is
somewhat redundant.  I've left it in though, because I think it helps
to have a more gradual transition from hook paths to namespaces.

Signed-off-by: W. Trevor King <[email protected]>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
Avoid trouble with situations like:

  # mount --bind /mnt/test /mnt/test
  # mount --make-rprivate /mnt/test
  # touch /mnt/test/mnt /mnt/test/user
  # mount --bind /proc/123/ns/mnt /mnt/test/mnt
  # mount --bind /proc/123/ns/user /mnt/test/user
  # nsenter --mount=/proc/123/ns/mnt --user /proc/123/ns/user sh

which uses the required private mount for binding mount namespace
references [1,2,3].  We want to avoid:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime joins the mount namespace referenced by fd 3.
3. Runtime fails to open /mnt/test/user, because /mnt/test is not
   visible in the current mount namespace.

and instead get runtime authors to setup flows like:

1. Runtime opens /mnt/test/mnt as fd 3.
2. Runtime opens /mnt/test/user as fd 4.
3. Runtime joins the mount namespace referenced by fd 3.
4. Runtime joins the user namespace referenced by fd 4.

This also applies to new namespace creation.  We want to avoid:

1. Runtime clones a container process with a new mount namespace.
2c. Container process fails to open /mnt/test/user, because /mnt/test
    is not visible in the current mount namespace.

in favor of something like:

1. Runtime opens /mnt/test/user as fd 3.
2. Runtime clones a container process with a new mount namespace.
3h. Host process closes unneeded fd 3.
3c. Container process joins the user namespace referenced by fd 3.

I also define runtime and container namespaces, so we have consistent
terminology.  I prefer:

* host namespace: a namespace you are in when you invoke the runtime
* host process: the runtime process invoked by the user
* container process: the process created by a clone call in the host
  process which will eventually execute the user-configured process.

Both the host and container processes are running runtime code
(although the container process eventually transitions to
user-configured code), so I find "runtime process", "runtime
namespace", etc. to be imprecise.  However, the maintainer consensus
is for "runtime namespace" [4,5], so that's what we're going with
here.

[1]: http://karelzak.blogspot.com/2015/04/persistent-namespaces.html
[2]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4ce5d2b1a8fde84c0eebe70652cf28b9beda6b4e
[3]: http://mid.gmane.org/[email protected]
[4]: opencontainers#275 (comment)
[5]: opencontainers#275 (comment)

Signed-off-by: W. Trevor King <[email protected]>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
The language from 15dee2e (runtime: Add prestart/poststop hooks,
2015-08-03, opencontainers#34) landed well before we had glossary entries for the
runtime and container namespaces (from 5dad125, config-linux: Specify
host mount namespace for namespace paths, 2015-12-18, opencontainers#275).  Now that
we do have language to cover that concept, it's better to explicitly
say that hooks run in the runtime namespace instead of leaving it to
the reader to extrapolate from the filesystem requirement.

With the new namespace wording, the "host's filesystem" wording is
somewhat redundant.  I've left it in though, because I think it helps
to have a more gradual transition from hook paths to namespaces.

Signed-off-by: W. Trevor King <[email protected]>
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.

5 participants