Introduce ability to reject/override relative bind mount paths #10
Conversation
provide default implementation to reject relative paths provide implementation to support relative paths from local environment Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
|
||
| // RejectRelativeBindPath check bind path for volume is absolute | ||
| func RejectRelativeBindPath(filePath string) (string, error) { | ||
| if !path.IsAbs(filePath) && !isAbs(filePath) { |
There was a problem hiding this comment.
This won't work in a mixed platform environment (windows client - linux host and vice-versa). See docker/cli#1990
There was a problem hiding this comment.
Not sure I get it. This code is the one extracted from cli/compose/ so it includes the fix frome docker/cli#1990
There was a problem hiding this comment.
🤦♂ Ignore me, I looked at the first path.IsAbs only
| type BindPathResolverFn func(string) (string, error) | ||
|
|
||
| // RejectRelativeBindPath check bind path for volume is absolute | ||
| func RejectRelativeBindPath(filePath string) (string, error) { |
There was a problem hiding this comment.
I don't think this check should be done on the client side; while I'm aware of the problem that "local filesystem" != "deploy filesystem", compose files convert relative paths to absolute paths, relative to the location of the compose file on the client, so the server would never receive a relative path.
The daemon (server) could reject paths that do not exist (which should be the behavior when using the long-form syntax). The short-hand syntax (currently) auto-creates missing paths on the server side; this is something we tried to deprecate (produce an error on missing paths), but turned out to be a feature that people relied on, see moby/moby#21666
That said, we could consider having a set of "known" variables for substitution (e.g. $project-root, $compose-file-path) to make the conversion more explicit.
There was a problem hiding this comment.
By the compose spec, client is responsible to reject relative paths (like the docker CLI does), and can OPTIONALY implement relative path support when it detect a local engine.
So, either we make compose-go a stupid yaml parser and let the client manage path conversions by itsefl after loading succeeded, or we offer such flexibility during parsing. I selected the latter here to limit required refactoring + impact on docker/cli
There was a problem hiding this comment.
There was a problem hiding this comment.
Alternative implementaion is to introduce ServiceVolumeConfig.IsAbsolute() as a helper function for client to check volumes.
There was a problem hiding this comment.
Validation should be handled as much as possible by the server, as the server has the "source of truth" what it supports
There was a problem hiding this comment.
Yes, but doing so we can't validate the compose file is valid before sending it to engine and get some resources created (we miss ws-transaction support :P). But if you prefer this approach I can live with it
There was a problem hiding this comment.
Validation should be limited (at most) to "values being well-formed", but other than that, I don't think a client could make many assumptions.
- volume-name with valid characters
- relative path that we're able to expand (this is something we could, potentially require, because relative paths must be resolved locally)
This is already tricky:
- valid absolute path (mac or windows). Tricky, because we can't check "existence" of path as we don't know anything about the deployment environment. Paths are really "flexible" (almost "anything goes" on Linux at least, including zero-width characters https://twitter.com/0xdade/status/1215101768230563840), so only checking for
path.IsAbs()andisAbs()makes sense at that point.
| @@ -393,11 +427,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 string, lookupEnv template.Mapping, opts *Options) ([]types.ServiceConfig, error) { | |||
There was a problem hiding this comment.
(orthogonal to this PR) wondering if we should consider using functional arguments instead of an Options type. Functional argument can be more flexible/extensible without having to change the API
compose-go is not compliant with https://github.com/docker/compose-spec/blob/master/spec.md#short-syntax-3 as it accept relative paths for bind-mounts.
Proposed changes still make this possible (as demonstrated by test cases) but disabled by default and flexible for extensibility by Compose implementations