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
Modify composefile loading behavior towards relative paths #966
Conversation
This sounds like a huge breaking change, as paths have always been relative to the compose file 😅. I think in the Docker for Desktop situation, the idea was to make the "remote" machine "feel" as if it's running locally (which is why the whole osxfs filesystem was added). For bind-mounts ( |
@thaJeztah, On Kubernetes, it is a bit more problematic: from a windows client, relative paths generate complete garbage (not the valid /c/local/path, as it does not go trough our moby api proxy). |
(and I am not sure if there is any kind of validation when a pod starts with a mount from a non-existing path, which is far less than ideal) |
One thing that we could do, is to warn the user when a source path seems to be relative (and advise that old behavior can be reproduced with the additional flag) |
-1 on a breaking change. If you want absolute paths, use absolute paths. The real problem here seems to be:
Let's fix that problem instead of breaking everyone. |
Could you open an issue with more detail about where the path is turned into garbage, so that we can discuss solutions? |
I think moby/moby#33375 was originally intended for this as well |
@dnephin @thaJeztah @vdemeester here are 2 scenarios currently completely broken:
One thing I imagined was to get OS information from the node we are connected to and change the path expansion to take this into account, but it would not work: on hybrid clusters you can have Linux masters with Windows workers (or the contrary). I also thought about examining the service image to detect target OS, but with multi-arch images there is no determinist way to detect target OS. If we want to avoid the breaking change, we can also keep the broken behavior by default and add a |
I just added a commit that does just that (default behavior does not change, fix is opt-in). |
Thanks for adding more detail about the problem. I'm still not a big fan of the flag. I think we can handle this better by examining the path. The problem seems to be just the
Do you know where this is happening? In
I think we can fix this. Instead of using just if !path.IsAbs(filePath) && !isWindowsDrivePath(filePath)) {
filePAth = absPath(...)
}
func isWindowsDrivePath(p string) bool {
return len(p) >= 3 && unicode.IsLetter(p[0]) && p[1:#] == ":\"
} Shouldn't that handle both cases? |
About the windows case, actually this one is not broken. but In any case, I am not a big fan of finding ways to understand both filesystem path semantics at the same time... There are so many corner cases that we might not anticipate (for example, UNC paths on Windows where you can express |
Ya, it's not great, but unfortunately this is something we already need to do for parsing volume specs, so we have this problem already. I think we should solve the problem the same way instead of adding confusing flags. There are definitely a couple edge cases, but I don't think there are so many that it can't be solved. My example code from above can easily handle the UNC paths as well with another condition.
Where does that translation happen? |
./some/relative/path is relative both in |
The windows client should be able to understand unix paths. I think the problem is that when we added the We should use |
The problem with I suggest that we create our own cross-platform (without platform-specific code) filepath package with the following features:
This way we would have a comprehensive that we could trust to have the same consistent behavior on all platforms we support. |
74d6650
to
912ac46
Compare
@dnephin @thaJeztah I've put together a cross platform paths library that is capable to handle both windows and unix style file paths, convert them, combine them, and do extra stuff like detecting best possible os-match for a given path. It supports the various weird Windows paths (like the win32 filesystem namespace, and device namespaces used for using non-shell-supported chars in file paths, or referencing named pipes), and this fix the original issue for this PR |
Can you make sure that repository has a license before we merge? |
@thaJeztah done! |
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.
LGTM 🐯
But needs to be 2 commits (one for the vendor, one for the change)
cc @dnephin
path: `relative\data`, | ||
workingDir: `c:\working\dir`, | ||
expected: `c:\working\dir\relative\data`, | ||
}, |
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.
These tests look great, but we're missing test coverage for the crosspath.HomeRooted case, arent' we?
There is gotestyourself/env.Patch
for patching HOME
which should make it easier to test
For what is worth with this PR two tests now fail when running the unit tests on Windows in Powershell:
(ellipses added for clarity) |
I'm coming a bit late to the party but my understanding is that what is needed is a library for path handling which behaves as
|
@mat007 there are some corner cases where we have to combine windows absolute paths with linux relative paths or the contrary (like, handling ~/data or ./data from a windows machine). So we need a lib that can:
|
I think that's definitely something that's missing in the go library (perhaps also the reason why I was looking at this library - see my review comments). Basically
Combined with the above, that would become; fmt.Println(pathutil.DetectFromPath(`C:\Users\foo`))
# windows
fmt.Println(pathutil.DetectFromPath(`\\hostname\path`))
# windows
fmt.Println(pathutil.DetectFromPath(`\\?\C:\Windows\foo`))
# windows
fmt.Println(pathutil.DetectFromPath(`/usr/share`))
# unix
fmt.Println(pathutil.DetectFromPath(`~/.docker`))
# unix
fmt.Println(pathutil.DetectFromPath(`/usr/some\ path\ with\ spaces`))
# unix And feed the result to
Append relative to absolute paths is what
Which would be a combination of |
Stumbled upon this issue; we should check if this PR resolves that as well; moby/moby#33746 |
@thaJeztah That is exactly the issue it fixes, yes :) |
46df454
to
1700392
Compare
@thaJeztah Rebased and added Raw() function and having String() normalize the paths |
@thaJeztah are you satisfied with the state of the code, or should I change something ? |
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
…n of absolute path in cross-plat Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
1700392
to
68b1890
Compare
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.
One last question on the new HomeDir
field of ConfigDetails
.
@@ -362,11 +362,11 @@ func formatInvalidKeyError(keyPrefix string, key interface{}) error { | |||
|
|||
// LoadServices produces a ServiceConfig map from a compose file Dict | |||
// the servicesDict is not validated if directly used. Use Load() to enable validation | |||
func LoadServices(servicesDict map[string]interface{}, workingDir string, lookupEnv template.Mapping) ([]types.ServiceConfig, error) { | |||
func LoadServices(servicesDict map[string]interface{}, workingDir, homeDir string, lookupEnv template.Mapping) ([]types.ServiceConfig, error) { |
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.
Looking at that function, we should probably create a LoadServicesFromConfigDetails
(which would take configDetails
as argument instead of those 3) or something.
That said LoadServices
is exported so… if we break it once, let's try to break it in a future-proof way
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.
Should I add this function alongside the existing one or rename LoadServices
to LoadServicesFromConfigDetails
(and modify its parameters) ?
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.
Hum actually, looking back, I'm not sure anymore 😅
@@ -58,6 +58,7 @@ type ConfigFile struct { | |||
type ConfigDetails struct { | |||
Version string | |||
WorkingDir string | |||
HomeDir string |
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.
Why do we need to pop-up HomeDir
in the ConfigDetails
? (also, it's a bit unrelated to the change as we could do the homedir.Get
at the same point of the code where we did the lookupEnv("HOME")
, right ?
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.
The home path resolution on Windows was broken (it did only do a lookupEnv("HOME")), and as we already have WorkingDir in config details, it made sense to also put HomeDir there (or remove both of them for good) for consistency towards ambient path.
Having them both in ConfigDetails makes it easier to test combinations of windows and linux base environments though.
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.
Yeah, I'm not a huge fan of that but… ok fair enough 😛
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.
LGTM 🐫
Would be nice that this can be fixed |
edit:wrong issue |
ignore my comment; commented on the wrong issue |
@thaJeztah is it possible that this can be implemented? |
@thaJeztah, @vdemeester feels like I should revive this PR, WDYT? |
Is there any workaround for this? |
hello, any news on this ? |
@simonferquel @thaJeztah @vdemeester assuming that this was already approved and only waiting for rebase I actually created rebase one to #1871 PTAL |
hi, |
Closing this one, as there was no consensus. There's a ticket in moby/moby that's somewhat related to this, on which I left some comments; moby/moby#43483 (comment) |
Currently, relative sources in bind mounts in a compose file, are expanded to absolute paths from the client perspective. The only case where it is correct is when the clients points to a single-node swarm/kubernetes cluster running on the same machine.
So now, by default, paths are not expanded locally, and we let the underlying stack backend fail if it requires absolute paths. For local dev purposes, docker stack deploy now supports an
--expand-bind-sources
flag restoring the old behavior.