-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtimetest: add validation of mount order #88
Conversation
This seems like it overlaps with the in-flight #81 (but that may be a
false-positive based on my quick skim); ping @liangchenye.
|
I just realised this implementation is way too crazy. We should just iterate through |
A little overlap :) @wking But luckily I'm working on mount existence validation and Aleksa is working on mount order. |
@liangchenye Sure, but I can't remember if |
@cyphar it uses |
Okay cool, we can use that then. Less work for me. :P |
#81 has been merged so this can be updated. Thanks! |
ping @cyphar need rebase and update. |
8eb8d60
to
165cccf
Compare
I haven't looked at the code in a few months, but I think I've updated this properly. PTAL. |
165cccf
to
428cc80
Compare
@@ -489,6 +605,7 @@ func validate(context *cli.Context) error { | |||
validateMaskedPaths, | |||
validateROPaths, | |||
validateOOMScoreAdj, | |||
validateMountOrder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mount is not Linux specified, I think we'd better put it into defaultValidations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Fixed. I forgot that I switched to pkg/mount
. :P
428cc80
to
06fff77
Compare
} | ||
|
||
cparts := filepath.SplitList(child) | ||
for i, part := range filepath.SplitList(parent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here may be a little problem.
I'm not sure what SplitList() here works for. Is a mount destination can be "pathA:pathB"?
But path in Windows generally looks like "C:\test". And mount path is not allowed nested in Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what SplitList() here works for. Is a mount destination can be "pathA:pathB"?
SplitList
splits a path into each component. So "A/B"
becomes []string{"A", "B"}
. This code is checking if one path is lexically the parent of another. While this isn't bulletproof there isn't a nice way to deal with other cases like symlinks (though I could try using ResolveSymlinks
).
But path in Windows generally looks like "C:\test". And mount path is not allowed nested in Windows.
I don't use Windows. If Windows doesn't support nested mounts, then this code is not useful for Windows (there is no meaningful concept of "order" when mounting things if you can't mask mounts with other mounts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what SplitList() here works for. Is a mount destination can be "pathA:pathB"?
SplitList
splits a path into each component. So"A/B"
becomes[]string{"A", "B"}
. This code is checking if one path is lexically the parent of another. While this isn't bulletproof there isn't a nice way to deal with other cases like symlinks (though I could try usingResolveSymlinks
).
I think SplitList is not used for that.
In go document's example, it works like "/a/b/c:/usr/bin"
becomes []string{"/a/b/c", "/usr/bin"}
.
And I tested, it really works like this.
Did I miss something?
But path in Windows generally looks like "C:\test". And mount path is not allowed nested in Windows.
I don't use Windows. If Windows doesn't support nested mounts, then this code is not useful for Windows.
In config.md of runtime-spec, "For the Windows operating system, one mount destination MUST NOT be nested within another mount. (Ex: c:\foo and c:\foo\bar)."
So, I think we should make exception for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SplitList is not used for that.
You're quite right. I've fixed it to use filepath.ToSlash
and then strings.Split
.
So, I think we should make exception for Windows.
Sure. I've added it using runtime.GOOS
-- is that fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine
d9dddea
to
a282e7c
Compare
In order to make sure that runtimes correctly implement the ordering of []spec.Mount, we need to check /proc/1/mountinfo (keeping in mind that Unix allows a user to mount over the top of an existing mountpoint -- thus masking it). We only run this test if there happen to be two mountpoints in the list where one is a parent of the other. We also don't run it on Windows. An extension of this might be to check all of the nested mounts specified in the spec. Signed-off-by: Aleksa Sarai <asarai@suse.de>
return fmt.Errorf("expected %q to be a mountpoint", A) | ||
} | ||
|
||
// B must be a mountpoint iff A was first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iff -> if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iff == if and only if. It's a mathematical statement which is like a two-way if-then statement. https://en.wikipedia.org/wiki/If_and_only_if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, got it.
But I think using general and easy understood expression may be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll write out the words "if and only if" if it'll make you feel better. The reason I wrote it like that is because the test is testing that precise statement, namely that B will only be a mountpoint if A was first (and that if A is first then B will be a mountpoint).
cparts := strings.Split(child, "/") | ||
for i, part := range strings.Split(parent, "/") { | ||
if cparts[i] != part { | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we use filepath.Rel() can reduce the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I remember that I didn't use it for /some/ reason but I can't remember what it was -- so we'll just switch it.
It seems currently we can just check one pair of parent and child mount points. @cyphar how do you think about it? |
We could do that. It's just probably going to be a bit tricky to track which orderings matter (if you have mountpoints for As I said in the actual patch, that is something we could do. It's just a bit tricky to actually test properly. I can probably take a look at it in a few weeks. Right now I'm swamped with |
@cyphar OK, got it. |
In order to make sure that runtimes correctly implement the ordering of
[]spec.Mount, we need to check /proc/1/mountinfo (keeping in mind that
Unix allows a user to mount over the top of an existing mountpoint --
thus masking it). We only run this test if there happen to be two
mountpoints in the list where one is a parent of the other.
An extension of this might be to check all of the nested mounts
specified in the spec.
Signed-off-by: Aleksa Sarai asarai@suse.de