Consider throwing a warning on not absolute mounts destination paths

[rata]

Description

We upgraded from warning to an error when the destination path of a mount is not an absolute path. This was done on commmit: 881e92a

This is causing issues on containerd.

Describe the results you received and expected

When upgrading containerd to use a runc version that includes this commit, several unit and integration test break.

This is in containerd main, so I expect the same will happen in containerd 1.6 and 1.7 too.

To fix it, I had to do several changes and I'm not sure I haven't broke windows with that yet. See the changes in https://github.com/containerd/containerd/pull/8287/commits before "Update runc binary to support idmap mounts", all of them are needed.

I've added a topic in the next containerd community meeting to discuss this (see here), but I wouldn't be surprised if containerd maintainers will be happy if we delay doing this change and they can fix it in some release, let it soak before applying it to other supported versions. Maybe they don't think it is needed, I don't know :)

I'll keep this up to date, but wanted to know your thoughts on making the error a warning.

What version of runc are you using?

These tests were using commit: 867ee90

Although any runc version that includes the commit switching the warning to errors should fail.

Host OS information

No response

Host kernel information

No response

[AkihiroSuda]

👍

Do you plan to open a PR?

[rata]

@AkihiroSuda Yes, I'll join the containerd community meeting and if they think it is better to revert, I'll open a PR ASAP. Maybe they are ok backporting the changes to handle that, though, let's see :)

[rata]

We will try to do the change in containerd and see how that goes. If this PR is merged with no complaints, we hope that will be enough from containerd side to allow this behavior change in runc 1.2: containerd/containerd#8885

[rata]

@thaJeztah anything you want to check on the docker/buildkit side for this?

[rata]

@thaJeztah friendly ping?

[thaJeztah]

Oh! Sorry, I saw the ping fly by, but forgot to look.

We should at least make sure that containerd/containerd#8885 finds its way into the containerd release branches if it fixes these.

That said, existing installations of docker (and containerd) could potentially be updating runc separately, which means that updating could result in things no longer functioning, so while we should make sure that any consumer won't use relative paths going forward, I think it's still worth considering to make this a warning if we're able to work around the issue.

Let me ping @tonistiigi @jedevc for the BuildKit side of things

[rata]

@thaJeztah I will create the backport of that for containerd today :)

[thaJeztah]

• I also did some very quick / rudimentary testing with relative paths, and left a comment on Handle relative paths in VOLUME directives moby/moby#46093

From the above; does the containerd fix also handle cases where the image contains a relative volume definition, or only if the volume was specified as argument?

[rata]

@thaJeztah containerd handle cases where the image contains a relative volume definition (VOLUME directive).

If you see the PR in main, as that where we have an e2e test for that, it changed this file. This is to handle the image created by this Dockerfile:

VOLUME ["/test_dir", "C:/weird_test_dir", "/:colon_prefixed"]

The second path, on !Windows, is relative (see the big comment for more info).

I'm not sure about creating a container with an image argument. I've never used nerdctl yet.

But the fact that it wasn't solved in containerd and buildkit seems enough for me to revert it for this release. We can try again in one or more releases, when the fixed versions are widely used.

What do you think?

[thaJeztah]

The second path, on !Windows, is relative (see the big comment for more info).

Ah yes, Windows is fun; also note that containers don't have anything other than a C drive, which is why usually /some_path is the more portable approach (and works for both Windows and other platforms). I know we also have some utilities for cross-platform cases (cli on Windows and daemon on Linux and vice-versa), e.g. https://github.com/moby/moby/blob/4b19b2f4babd577a11bd0c1c018e7263e51d49fa/pkg/system/filesys.go#L9-L19

But the fact that it wasn't solved in containerd and buildkit seems enough for me to revert it for this release.

Yes, this is a tricky one indeed, and part of very old logic where things were sometimes a bit too relaxed, or at least "too relaxed in the wrong places", and where UX leaked into the implementation / internals.

We can try again in one or more releases, when the fixed versions are widely used.

My biggest concern is cases where these things may be persisted, such as container configs "on-disk", or (as mentioned) image-configs. Basically, situations where things may still fail, even on "latest" versions of binaries, but using some older image (or existing containers).

What do you think?

Yes, making it a hard error sounds (see above) risky, so perhaps we should still make it a warning.

Of course, we should fix cases where we discover them, if possible, but it may take time.

I'm also thinking; could somehow introduce a --strict-mode option (or the reverse: a --permissive-mode) that we can somehow use for this? Perhaps a "all-or-nothing" option may be too broad (perhaps it needs to be more granular if we have similar cases). Such an option could be used to either have an escape-hatch, or to start testing "strict" validation of specs.

The question would be "where" that option would live; I guess to some extent it could even be part of the generated OCI Spec, but then the question becomes: how does that relate to the Version field? Or should it just have a +strict suffix? https://github.com/opencontainers/runtime-spec/blob/e8c413418efcb816b963be9762ddc202e181ba88/specs-go/config.go#L8

[rata]

@thaJeztah cool, will switch the PR from draft to ready to review, then. Thanks!

I agree all or nothing seems more risky. But do we have to make this change at all? I mean, don't we want to change the OCI runtime-spec to allow for relative paths (without ambiguity, also mandate they are relative to "/" in the container, that is what we are doing for ages now it seems) and be done with it?

AFAIK, there is no security issue involved nor anything, just the desire to comply with the spec. We can change the spec to reflect the hard to change reality and that is all? :)