Skip to content

Introduce ability to reject/override relative bind mount paths #10

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.

Sign up for GitHub

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

Closed
ndeloof wants to merge 1 commit into from
Closed

Introduce ability to reject/override relative bind mount paths #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 49 additions & 32 deletions loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package loader

import (
"fmt"
"os"
"path"
"path/filepath"
"reflect"
Expand Down Expand Up @@ -34,6 +35,38 @@ type Options struct {
Interpolate *interp.Options
// Discard 'env_file' entries after resolving to 'environment' section
discardEnvFiles bool
// CheckVolumes check binds for supported path and transform them if needed
CheckVolumes BindPathResolverFn
}

// BindPathResolverFn is used to check/override bind-mount volume sources
type BindPathResolverFn func(string) (string, error)

// RejectRelativeBindPath check bind path for volume is absolute
func RejectRelativeBindPath(filePath string) (string, error) {
Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ndeloof ndeloof Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@ndeloof ndeloof Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> https://github.com/docker/compose-spec/blob/master/spec.md#short-syntax-3

Copy link
Collaborator Author

@ndeloof ndeloof Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative implementaion is to introduce ServiceVolumeConfig.IsAbsolute() as a helper function for client to check volumes.

Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation should be handled as much as possible by the server, as the server has the "source of truth" what it supports

Copy link
Collaborator Author

@ndeloof ndeloof Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() and isAbs() makes sense at that point.

if !path.IsAbs(filePath) && !isAbs(filePath) {
Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in a mixed platform environment (windows client - linux host and vice-versa). See docker/cli#1990

Copy link
Collaborator Author

@ndeloof ndeloof Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get it. This code is the one extracted from cli/compose/ so it includes the fix frome docker/cli#1990

Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂ Ignore me, I looked at the first path.IsAbs only

return "", fmt.Errorf("volume bind mount MUST be an absolute path")
}
return filePath, nil
}

// ResolveRelativeBindPath build a BindPathResolverFn to expand relative paths based on local environment
func ResolveRelativeBindPath(workingDir string) BindPathResolverFn {
return func(filePath string) (string, error) {
if strings.HasPrefix(filePath, "~") {
home, err := os.UserHomeDir()
if err != nil {
return "", fmt.Errorf("cannot resolve user home dir to expand '~'")
}
return strings.Replace(filePath, "~", home, 1), nil
}

if !path.IsAbs(filePath) && !isAbs(filePath) {
filePath = absPath(workingDir, filePath)
}

return filePath, nil
}
}

// WithDiscardEnvFiles sets the Options to discard the `env_file` section after resolving to
Expand Down Expand Up @@ -72,6 +105,7 @@ func Load(configDetails types.ConfigDetails, options ...func(*Options)) (*types.
LookupValue: configDetails.LookupEnv,
TypeCastMapping: interpolateTypeCastMapping,
},
CheckVolumes: RejectRelativeBindPath,
}

for _, op := range options {
Expand Down Expand Up @@ -108,7 +142,7 @@ func Load(configDetails types.ConfigDetails, options ...func(*Options)) (*types.
}
}

cfg, err := loadSections(configDict, configDetails)
cfg, err := loadSections(configDict, configDetails, opts)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -137,7 +171,7 @@ func validateForbidden(configDict map[string]interface{}) error {
return nil
}

func loadSections(config map[string]interface{}, configDetails types.ConfigDetails) (*types.Config, error) {
func loadSections(config map[string]interface{}, configDetails types.ConfigDetails, opts *Options) (*types.Config, error) {
var err error
cfg := types.Config{
Version: schema.Version(config),
Expand All @@ -150,7 +184,7 @@ func loadSections(config map[string]interface{}, configDetails types.ConfigDetai
{
key: "services",
fnc: func(config map[string]interface{}) error {
cfg.Services, err = LoadServices(config, configDetails.WorkingDir, configDetails.LookupEnv)
cfg.Services, err = LoadServices(config, configDetails.WorkingDir, configDetails.LookupEnv, opts)
return err
},
},
Expand Down Expand Up @@ -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) {
Copy link
Member

@thaJeztah thaJeztah Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

var services []types.ServiceConfig

for name, serviceDef := range servicesDict {
serviceConfig, err := LoadService(name, serviceDef.(map[string]interface{}), workingDir, lookupEnv)
serviceConfig, err := LoadService(name, serviceDef.(map[string]interface{}), workingDir, lookupEnv, opts)
if err != nil {
return nil, err
}
Expand All @@ -409,7 +443,7 @@ func LoadServices(servicesDict map[string]interface{}, workingDir string, lookup

// LoadService produces a single ServiceConfig from a compose file Dict
// the serviceDict is not validated if directly used. Use Load() to enable validation
func LoadService(name string, serviceDict map[string]interface{}, workingDir string, lookupEnv template.Mapping) (*types.ServiceConfig, error) {
func LoadService(name string, serviceDict map[string]interface{}, workingDir string, lookupEnv template.Mapping, opts *Options) (*types.ServiceConfig, error) {
serviceConfig := &types.ServiceConfig{}
if err := Transform(serviceDict, serviceConfig); err != nil {
return nil, err
Expand All @@ -420,7 +454,7 @@ func LoadService(name string, serviceDict map[string]interface{}, workingDir str
return nil, err
}

if err := resolveVolumePaths(serviceConfig.Volumes, workingDir, lookupEnv); err != nil {
if err := resolveVolumePaths(serviceConfig.Volumes, opts.CheckVolumes); err != nil {
return nil, err
}

Expand Down Expand Up @@ -484,7 +518,7 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, l
return nil
}

func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string, lookupEnv template.Mapping) error {
func resolveVolumePaths(volumes []types.ServiceVolumeConfig, resolveFn BindPathResolverFn) error {
for i, volume := range volumes {
if volume.Type != "bind" {
continue
Expand All @@ -494,35 +528,18 @@ func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string,
return errors.New(`invalid mount config for type "bind": field Source must not be empty`)
}

filePath := expandUser(volume.Source, lookupEnv)
// Check if source is an absolute path (either Unix or Windows), to
// handle a Windows client with a Unix daemon or vice-versa.
//
// Note that this is not required for Docker for Windows when specifying
// a local Windows path, because Docker for Windows translates the Windows
// path into a valid path within the VM.
if !path.IsAbs(filePath) && !isAbs(filePath) {
filePath = absPath(workingDir, filePath)
if resolveFn != nil {
filePath, err := resolveFn(volume.Source)
if err != nil {
return err
}
volume.Source = filePath
volumes[i] = volume
}
volume.Source = filePath
volumes[i] = volume
}
return nil
}

// TODO: make this more robust
func expandUser(path string, lookupEnv template.Mapping) string {
if strings.HasPrefix(path, "~") {
home, ok := lookupEnv("HOME")
if !ok {
logrus.Warn("cannot expand '~', because the environment lacks HOME")
return path
}
return strings.Replace(path, "~", home, 1)
}
return path
}

func transformUlimits(data interface{}) (interface{}, error) {
switch value := data.(type) {
case int:
Expand Down
12 changes: 11 additions & 1 deletion loader/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,17 @@ func loadYAMLWithEnv(yaml string, env map[string]string) (*types.Config, error)
return nil, err
}

return Load(buildConfigDetails(dict, env))
if home, ok := env["HOME"]; ok {
// test overrides $HOME
os.Setenv("HOME", home)
}
wd, err := os.Getwd()
if err != nil {
return nil, err
}
return Load(buildConfigDetails(dict, env), func(options *Options) {
options.CheckVolumes = ResolveRelativeBindPath(wd)
})
}

var sampleYAML = `
Expand Down