filepath: Add a stand-alone package for explicit-OS path logic #445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM The |
wking
force-pushed
the
explicit-os-filepath
branch
from
8af7e86
to
df8d285
Compare
|
LGTM |
|
I think this cause a regression. Before this PR we can check whether windows mounts are nested, but now we can't any more. |
|
On Fri, Aug 25, 2017 at 09:36:45AM +0000, Ma Shimiao wrote:
I think this cause a regression. Before this PR we can check whether
windows mounts are nested, but now we can't any more.
I'm not sure master has working Windows nested checks. For example,
[1] is broken on Windows, where / is not the root path. And [2] is
broken on Windows unless you're compiled for Windows, because Go's
non-Windows filepath.Clean may mangle Windows paths. If you're
worried about the validating-Windows-configs-from-Windows use case
(but not the validating-Windows-configs-from-other-OSes use case), we
could, as a temporary measure, have my IsAncestor fall back to Go's
IsAncestor if os matches GOOS. Would that be sufficient for now?
[1]: https://github.com/opencontainers/runtime-tools/blob/3db644c31789a6793c11a1c975312c489befab3d/validate/validate.go#L846
[2]: https://github.com/opencontainers/runtime-tools/blob/3db644c31789a6793c11a1c975312c489befab3d/validate/validate.go#L857
|
|
I don't think tha's IsAncestor's problem, it's Abs()'s problem. I think we can check whether a windows path is absolute or not as https://github.com/opencontainers/runtime-tools/blob/master/validate/validate.go#L843 |
|
On Tue, Aug 29, 2017 at 11:30:44PM -0700, Ma Shimiao wrote:
I don't think tha's IsAncestor's problem, it's Abs()'s problem. I
think we can check whether a windows path is absolute or not as
https://github.com/opencontainers/runtime-tools/blob/master/validate/validate.go#L843
This link currently points at a rspec.UTSNamespace line, and no longer
points at anything filepath-like. Looking through master's history, I
think you made the comment after #448 landed but before #374 landed,
which would mean you ment to reference [1]:
func nestedValid(os, pathA, pathB string) (bool, error) {
if pathA == pathB {
return false, nil
}
…
In this PR, I'm replacing nestedValid with IsAncestor [2], and
IsAncestor isn't counting matching paths as ancestors [3]. That
matches the code you reference, where matching paths are not counted
as nested. So I don't see an Abs issue there, or even a
platform-specific issue there. Can you be more specific about the
issue you're thinking about?
We've also had previous discussion pointing out the ambiguity of the
spec's current “nested” condition [3,4] (although our interpretation
of that is not changing in this PR). I've also had two off-list
threads with @RobDolinMS on this point, but haven't followed up on
those either. @RobDolinMS, if it helps, the tails of those threads
are [5,6].
[1]: https://github.com/opencontainers/runtime-tools/blob/3db644c31789a6793c11a1c975312c489befab3d/validate/validate.go#L843
[2]: df8d285#diff-8351286f57eb81e245c9c99c07f3b34fL427
[3]: df8d285#diff-accfd27539832ca1c0a3950cf8403c80R12
[4]: #256 (comment)
[5]: Message-ID: <20161026050002.GB7420@odin.tremily.us>
Subject: Re: Helping close CLI (#513)
Date: Tue, 25 Oct 2016 22:00:02 -0700
[6]: Message-ID: <20161026213320.GA28698@odin.tremily.us>
Subject: Nested Mounts
Date: Wed, 26 Oct 2016 14:33:20 -0700
|
Ah, so maybe copy part of that into |
|
On 08/31/2017 09:45 AM, W. Trevor King wrote:
Ah, so maybe copy part of that into |IsAbs|, and then replace our current |pathValid| calls with |IsAbs|?
Yeah, currently pathValid mainly check whether the path is absolute for windows or linux.
And should add more tests of Clean() for windows path.
|
Go's path/filepath has lots of useful path operations, but there is a
compile-time decision to select only the logic that applies to your
$GOOS. That makes it hard to validate a Windows config from a Linux
host (or vice versa) because Go's builtin tools won't tell you whether
a path is absolute on the target platform (just whether the path is
absolute on *your* platform). This commit adds a new package to do
the same sorts of things but with an explicit OS argument.
In some cases, there's also an explicit workding directory argument.
For example, Go's Abs has [1]:
If the path is not absolute it will be joined with the current
working directory to turn it into an absolute path.
but that doesn't make sense for a cross-platform Abs call because
the real current working directory will be for the wrong platform.
Instead, cross-platform calls to Abs and similar should fake a working
directory as if they were being called from the other platform.
The Windows implementation is not very complete, with IsAbs definitely
missing a lot of stuff; Abs, Clean, and IsAncestor probably missing
stuff; and a lack of Windows-path tests. But the current tools are
broken for validating Windows configs anyway, so I've left completing
Windows support to future work. The regular expression I've used is
similar to what we used to use in pathValid, but has been adjusted
based on [2]:
The absolute path has the following format:
LocalDrive:\Path\FileName
I've assumed that c:\a is absolute as well, even though it doesn't
quite match the above pattern. And I no longer test for colons,
question marks, and other reserved characters [3], although it would
be easy to add checks for that as well if we wanted.
Besides adding the new package, I updated the config validation to use
the new package where appropriate. For example checks for absolute
hook paths (based on [4]) now appropriately account for the target
platform (although Abs has limited Windows support at the moment, as
mentioned above).
There are still a number of config validation checks that use Go's
stock filepath, because they're based around actual filesystem access
(e.g. reading config.json off the disk, asserting that root.path
exists on the disk, etc.). Some of those will need logic to convert
between path platforms (which I'm leaving to future work). For
example, if root.path is formed for another platform, then:
* If root.path is absolute (on the target platform), there's no way to
check whether root.path exists inside the bundle.
* If root.path is relative, we should be converting it from the target
platform to the host platform before joining it to our bundle path.
For example, with a Windows bundle in "/foo" on a Linux host where
root.path is "bar\baz", then runtime-tools should be checking for
the root directory in /foo/bar/baz, not in /foo/bar\baz. The
root.path example is somewhat guarded by the bundle requirement for
siblinghood [5], but I'd rather enforce siblinghood independently
from existence [6], since the spec has separate requirements for
both.
I'm not clear on why we were using pathValid for mount sources on
Windows. We've been doing that since 4dcd484 (validate: add mounts
whether nested check, 2016-10-25, opencontainers#256, [7]), but I see no
absolute-path requirement in the spec [8], which only forbids UNC
paths and mapped drives for source (which we don't yet have tooling to
detect). Perhaps the idea was just to check for paths which contained
reserved characters? In any case, I think source handling on Windows
should not involve IsAbs and should be addressed more thoroughly in
future work.
[1]: https://golang.org/pkg/path/filepath/#Abs
[2]: https://technet.microsoft.com/en-us/library/ee692607.aspx
[3]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#naming_conventions
[4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L375
[5]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/bundle.md#L17-L19
[6]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L43
[7]: https://github.com/opencontainers/runtime-tools/pull/256/files#diff-8351286f57eb81e245c9c99c07f3b34fR413
[8]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
explicit-os-filepath
branch
from
df8d285
to
cf64923
Compare
|
On Wed, Aug 30, 2017 at 07:01:22PM -0700, Ma Shimiao wrote:
Yeah, currently pathValid mainly check whether the path is absolute
for windows or linux.
I've dropped pathValid with df8d285 → cf64923, which also adds some
IsAbs tests (including for Windows) and adjusts the Windows abs-path
regexp based on some Windows docs. I've also dropped the [Windows
source check][1], which I don't see support for [in the spec][2]. The
update also rebases around #451.
And should add more tests of Clean() for windows path.
I'm happy to do that, but I don't have example Windows paths or
expected results to feed in. Can you list the examples you'd like to
see tested and the cleaned result?
[1]: https://github.com/opencontainers/runtime-tools/pull/256/files#diff-8351286f57eb81e245c9c99c07f3b34fR413
[2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
|
|
LGTM |
|
@liangchenye, still look good to you? |
|
I'll review the changes @wking . |
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed here and later, and based on the Gist I'd created for that comment.
Go's path/filepath has lots of useful path operations, but there is a compile-time decision to select only the logic that applies to your
$GOOS. That makes it hard to validate a Windows config from a Linux host (or vice versa) because Go's builtin tools won't tell you whether a path is absolute on the target platform (just whether the path is absolute on your platform). This commit adds a new package to do the same sorts of things but with an explicit OS argument.In some cases, there's also an explicit workding directory argument. For example, Go's
Abshas:but that doesn't make sense for a cross-platform
Abscall because the real current working directory will be for the wrong platform. Instead, cross-platform calls toAbsand similar should fake a working directory as if they were being called from the other platform.The Windows implementation is not very complete, with
IsAbsdefinitely missing a lot of stuff;Abs,Clean, andIsAncestorprobably missing stuff; and a lack of Windows-path tests. But the current tools are broken for validating Windows configs anyway, so I've left completing Windows support to future work.Besides adding the new package, I updated the config validation to use the new package where appropriate. For example, checks for absolute hook paths (based on this) now appropriately account for the target platform (although
Abshas limited Windows support at the moment, as mentioned above).There are still a number of config validation checks that use Go's stock filepath, because they're based around actual filesystem access (e.g. reading
config.jsonoff the disk, asserting thatroot.pathexists on the disk, etc.). Some of those will need logic to convert between path platforms (which I'm leaving to future work). For example, ifroot.pathis formed for another platform, then:root.pathis absolute (on the target platform), there's no way to check whether root.path exists inside the bundle.root.pathis relative, we should be converting it from the target platform to the host platform before joining it to our bundle path. For example, with a Windows bundle in/fooon a Linux host whereroot.pathisbar\baz, then runtime-tools should be checking for the root directory in/foo/bar/baz, not in/foo/bar\baz. Theroot.pathexample is somewhat guarded by the bundle requirement for siblinghood, but I'd rather enforce siblinghood independently from existence, since the spec has separate requirements for both.