libcontainer: relax validation for absolute paths #3004
Conversation
|
@kolyshkin @AkihiroSuda PTAL. Let me know if this is acceptable for v1.0.0 (we can remove it in a patch release after that) |
thaJeztah
force-pushed
the
relax_validation
branch
from
dd349d9 to
82dc2ff
Compare
|
AFAIK, only Docker Swarm-mode was known to have depended on the old behavior, and that was fixed in Docker 20.10.7: moby/moby@afbb127 Is there any other project that depends on the old behavior? |
|
BuildKit also needed changes, and (I think) for the swarmkit case at least, it's possible that existing services will break after updating (as they would have to be recreated for the new paths to be set); I'm wondering if there would be other situations where this could happen. |
Do we have an issue ticket? |
|
A fix was merged on master moby/buildkit#2123, and back ported to the v0.8 branch (but no tagged release exists with that fix) |
libcontainer/specconv/spec_linux.go
Outdated
| // Relax validation for backward compatibility | ||
| oldDest := m.Destination | ||
| var err error | ||
| m.Destination, err = filepath.Abs(m.Destination) |
There was a problem hiding this comment.
can probably simplify this to filepath.Join("/", ...) (like on BuildKit); let me do so
thaJeztah
force-pushed
the
relax_validation
branch
from
82dc2ff to
c5a5189
Compare
thaJeztah
force-pushed
the
relax_validation
branch
3 times, most recently
from
dbcf967 to
21aaece
Compare
|
I like this approach. thanks @thaJeztah |
libcontainer/specconv/spec_linux.go
Outdated
| // Relax validation for backward compatibility | ||
| // TODO (runc 1.2.0): replace a warning below with | ||
| // return nil, fmt.Errorf("mount destination %s not absolute", m.Destination) | ||
| logrus.Warnf("mount destination %s not absolute (this won't be supported past runc 1.2.0)", m.Destination) |
There was a problem hiding this comment.
We are still not sure how our post-v1 version scheme will look like.
So "late 2021" might be better than "1.2.0"?
There was a problem hiding this comment.
Think we should make it more "generic"?
| logrus.Warnf("mount destination %s not absolute (this won't be supported past runc 1.2.0)", m.Destination) | |
| logrus.Warnf("mount destination %s not absolute (this will no longer be supported in a future release)", m.Destination) |
There was a problem hiding this comment.
Updated the description and TODO
thaJeztah
force-pushed
the
relax_validation
branch
from
21aaece to
5e163ae
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Commits 1f1e91b and 2192670 added validation for mountpoints to be an absolute path, to match the OCI specs. Unfortunately, the old behavior (accepting the path to be a relative path) has been around for a long time, and although "not according to the spec", various higher level runtimes rely on this behavior. While higher level runtime have been updated to address this requirement, there will be a transition period before all runtimes are updated to carry these fixes. This patch relaxes the validation, to generate a WARNING instead of failing, allowing runtimes to update (but allowing them to update runc to the current version, which includes security fixes). We can remove this exception in a future patch release. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
relax_validation
branch
from
5e163ae to
b31a934
Compare
Arf. My mistake; fixed! |
|
Thanks! I opened #3020 to track removal of this. |
Commits 1f1e91b and 2192670 (#2917) added validation for mountpoints to be an absolute path, to match the OCI specs.
Unfortunately, the old behavior (accepting the path to be a relative path) has been around for a long time, and although "not according to the spec", various higher level runtimes rely on this behavior.
While higher level runtime have been updated to address this requirement, there will be a transition period before all runtimes are updated to carry these fixes.
This patch relaxes the validation, to generate a WARNING instead of failing, allowing runtimes to update (but allowing them to update runc to the current version, which includes security fixes).
We can remove this exception in a future patch release.
relates to #2928, containerd/containerd#5547