mount: Allow relative mount destinations on Linux #1225
Conversation
rata
requested review from
AkihiroSuda,
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
kolyshkin,
mrunalp,
thaJeztah,
tianon,
vbatts and
utam0k
as code owners
config.md
Outdated
| @@ -69,6 +69,7 @@ For Solaris, the mount entry corresponds to the 'fs' resource in the [zonecfg(1M | |||
|
|
|||
| * **`destination`** (string, REQUIRED) Destination of mount point: path inside container. | |||
| This value MUST be an absolute path. | |||
| * Linux: It MAY be a relative path too, in which case it MUST be relative to "/". | |||
There was a problem hiding this comment.
Why is this specific to Linux?
There was a problem hiding this comment.
Also, relative path should be discouraged/deprecated anyway
There was a problem hiding this comment.
Just because I know the problems with the ecosystem exist on linux and I don't know how to do it on Windows (shall we say relative to C:? Or to D:?)
There was a problem hiding this comment.
for windows (haven't looked yet at our implementations), but at least I know that Windows containers by default only have a single drive (so only C:\) and any path should be on the C:\ drive)
There was a problem hiding this comment.
I've updated. I kept it limited to Linux as that is the only platform I know it can cause issues. If others find out later that other platforms are also affected, we can relax them later too :)
There was a problem hiding this comment.
I've also mentioned relative paths are deprecated. Let me know what you think!
config.md
Outdated
| This value MUST be an absolute path. | ||
| * Linux: It MAY be a relative path too, in which case it MUST be relative to "/". |
There was a problem hiding this comment.
Perhaps the combination of MUST and MAY here is a bit ambiguous.
As we're differentiating per platform here, perhaps we should
- make each of these a "complete" requirement 🤔
- For Linux, I would still consider it to be a
SHOULD(strong recommendation), as that's what we'd preferred it to have been (were it not for backward compatibility). - If we only define "per platform", I guess we need to add a "For other platforms .... " which would be a
MUST(unless we know of implementations on other platforms that use relative paths)
Also wondering if it would be worth mentioning why we allow (MAY) have relative paths on Linux; maybe it doesn't hurt to explain the reason (i.e. "we tried, but had to continue it for backward compatibility"). Not sure if we've had similar cases before, and if there's wording we could use for such cases.
There was a problem hiding this comment.
Makes sense, will update it tomorrow. Thanks! :)
There was a problem hiding this comment.
@thaJeztah Does it looks good now? I was not decided between this version I pushed and another one saying just "This value MUST be an absolute path, except when the platform listed below say otherwise", and then just Linux says otherwise.
Let me know what you think :)
We tried to make runc enforce abs dest path several times, and always had to revert it due to some tools not yet doing it. The last occurrence is this one: opencontainers/runc#3944 (comment) I don't see any reason to force abs dst paths on Linux, as far as I know there is no security bug nor anything. Let's just relax the spec wording, matching all the runtimes behavior when the paths is relative, and be done with it. Signed-off-by: Rodrigo Campos <[email protected]>
rata
force-pushed
the
rata/mount-rel-path
branch
from
efd5a59
to
6ffddf6
Compare
|
@thaJeztah @AkihiroSuda pushed with fixes, PTAL |
|
@tianon no objections! 👋 ❤️ |
We changed it in PR: opencontainers/runtime-spec#1225 But we missed to remove this comment. Signed-off-by: Rodrigo Campos <[email protected]>
We changed it in PR: opencontainers/runtime-spec#1225 But we missed to remove this comment. Signed-off-by: Rodrigo Campos <[email protected]>
We changed it in PR: opencontainers/runtime-spec#1225 But we missed to remove this comment. Signed-off-by: Rodrigo Campos <[email protected]>
We tried to make runc enforce abs dest path several times, and always had to revert it due to breakin users. The last occurrence is this one, in which we detected a few places not up to date and decided to revert the "throw an error" behavior:
opencontainers/runc#3944 (comment)
I don't see any reason to force abs dst paths on Linux, as far as I know there is no security bug nor anything. Let's just relax the spec wording, matching all the runtimes behavior when the paths is relative, and be done with it.
cc @thaJeztah