config: Require absolute mount destinations

[wking]

destination has been the path inside the container since #136. My personal preference is to have an explicit pivot root and allow paths relative to the current working directory, but that would be a big shift from the current OCI spec. The only way the current spec lets you turn off the root pivot is by not setting a mount namespace at all (and even then, it's not clear if that turns off the pivot). And the config's root entry is required (despite my attempts to have it made optional), so it's not really clear how containers that don't set a mount namespace are supposed to work (if they're supported at all).

You might be able to get away with something like:

When a mount namespace is not set, destination paths are relative to the runtime's initial working directory (or relative to the config.json, or whatever). When a mount namespace is set, destination paths are relative to the mount namespace's root.

but with mount-namespace-less containers already so unclear, it seems better to just require absolute destinations. If/when we get clearer support for explicit pivot-root calls or containers that inherit the host mount namespace (without re-joining it and losing their old working directory), we can consider lifting the absolute-path restriction.

[cyphar]

I think it should be taken as relative to root.path (without the absolute path requirement).

Also, regarding your point about explicit pivot_root, I don't see why explicit pivot_root makes sense. All pivot_root does is make your root (/) be set to root.path is set to. Since root.path is explicitly set, I don't see what you gain from exposing internals like pivot_root (which is actually not as simple as you might think in all cases -- exposing it to users is just asking for trouble).

[wking]

On Fri, Nov 04, 2016 at 12:32:43AM -0700, Aleksa Sarai wrote:

I think it should be taken as relative to root.path (without the
absolute path requirement).

This is the mount destination, which is a path inside the container
mount namespace [1](which may or may not be the same as the host
mount namespace. And presumably means “the path that will end up at
‘destination’ after the pivot”). root.path, on the other hand is in
the host mount namespace which never pivots. Joining paths that are
in different namespaces and different sides of the pivot does not seem
like a good idea to me. For example, if ‘root.path’ is ‘rootfs’ and
‘destination’ is ‘dev’, I don't think the user expects to see a
/rootfs/dev in the post-pivot container mount namespace.

But perhaps you're just pointing out that ‘dev’, relative to the
post-pivot container mount namespace root, will be ‘/dev’ as an
absolute-in-container path. And that in the pre-pivot would you'll
want to mount to {root.path}/dev to set that up. That makes sense to
me. But in that case, you can make all relative destinations absolute
by prepending / (or whatever the Windows equivalent is if you happen
to be on Windows). And I don't see the point in allowing the user to
leave off the leading slash if the in-container reference point is
always /.

Also, regarding your point about explicit pivot_root, I don't see
why explicit pivot_root makes sense. All pivot_root does is make
your root (/) be set to root.path is set to. Since root.path
is explicitly set, I don't see what you gain from exposing internals
like pivot_root (which is actually not as simple as you might
think in all cases -- exposing it to users is just asking for
trouble).

The utility is that it clarifies when (or if) the container process
changes the current working directory from wherever it was launched to
the post-pivot root. That means mounting paths (both sources and
targets) can be sanely relative to the container process's current
working directory.

[cyphar]

@wking

This is the mount destination, which is a path inside the container
mount namespace [1](which may or may not be the same as the host
mount namespace. And presumably means “the path that will end up at
‘destination’ after the pivot”). root.path, on the other hand is in
the host mount namespace which never pivots. Joining paths that are
in different namespaces and different sides of the pivot does not seem
like a good idea to me. For example, if ‘root.path’ is ‘rootfs’ and
‘destination’ is ‘dev’, I don't think the user expects to see a
/rootfs/dev in the post-pivot container mount namespace.

That's not what I meant. I mean that if you set the value to "dev" then the mountpoint will be rootfs/dev in the host mount namespace (which is /dev in the pivoted one). The fact is that you have to mount things before you pivot because source is in the host -- so you have to mount to some path that exists on the host. The host path for the mountpoint should be root.path + dest, because IMO it's the least surprising behaviour.

But overall denying it is also fine, I don't really mind that much (explicitly using / feels safer in any case).

The utility is that it clarifies when (or if) the container process
changes the current working directory from wherever it was launched to
the post-pivot root. That means mounting paths (both sources and
targets) can be sanely relative to the container process's current
working directory.

That utility only exists if you have semantics that are related to the current working directory (other than the one set in the config). I don't think there are any such semantics in the spec, so I'm not really sure what the practical utility of it is. I get that your mounting semantics in ccon work like that, but I don't think it's a good idea overall (and you'd be better served not making the mountpoints relative to pwd and instead make pivot_root implicit).

[wking]

On Fri, Nov 04, 2016 at 01:30:32AM -0700, Aleksa Sarai wrote:

I mean that if you set the value to "dev" then the mountpoint will
be rootfs/dev in the host mount namespace (which is /dev in the
pivoted one). The fact is that you have to mount things before you
pivot because source is in the host -- so you have to mount to
some path that exists on the host. The host path for the mountpoint
should be root.path + dest, because IMO it's the least surprising
behaviour.

Agreed, and this is what I was trying to get at in my “But perhaps…”
paragraph.

But overall denying it is also fine, I don't really mind that much
(explicitly using / feels safer in any case).

Yeah. Do you see any way that a relative POSIX-path destination
(e.g. ‘dev’) could ever have an absolute post-pivot container path
that wasn't “/ + {destination}” (e.g. ‘/dev’)? I don't.

That utility only exists if you have semantics that are related to
the current working directory…

Right.

I don't think there are any such semantics in the spec…

I don't either, but there are some “relative to the bundle directory
(or equivalently, to config.json” semantics in the spec. In ccon,
relative paths are always relative to the runtime/container process's
current working directory, which allows you to get off the disk and
pipe configs directly into the runtime.

[crosbymichael]

The 1 line change looks fine to me but I have no clue whats going on in these comments.

[cyphar]

@crosbymichael Don't worry about it.

[tianon]

LGTM

(I guess PullApprove needs me to be more explicit)

[hqhq]

LGTM

[hqhq]

Why @tianon 's LGTM doesn't count? 😟

[caniszczyk]

LGTM

[caniszczyk]

@hqhq not sure why @tianon's LGTM didn't count