diff --git a/e2e/commands_test.go b/e2e/commands_test.go index a4de01417..b5683d804 100644 --- a/e2e/commands_test.go +++ b/e2e/commands_test.go @@ -226,6 +226,18 @@ maintainers: golden.Assert(t, stdOut, "validate-output.golden") } +func TestInitWithInvalidCompose(t *testing.T) { + cmd, cleanup := dockerCli.createTestCmd() + defer cleanup() + composePath := filepath.Join("testdata", "invalid-compose", "docker-compose.yml") + + cmd.Command = dockerCli.Command("app", "init", "invalid", "--compose-file", composePath) + stdOut := icmd.RunCmd(cmd).Assert(t, icmd.Expected{ + ExitCode: 1, + }).Combined() + golden.Assert(t, stdOut, "init-invalid-output.golden") +} + func TestInspectApp(t *testing.T) { runWithDindSwarmAndRegistry(t, func(info dindSwarmAndRegistryInfo) { cmd := info.configuredCmd diff --git a/e2e/testdata/init-invalid-output.golden b/e2e/testdata/init-invalid-output.golden new file mode 100644 index 000000000..847fb5292 --- /dev/null +++ b/e2e/testdata/init-invalid-output.golden @@ -0,0 +1,4 @@ +Compose file validation failed: +* can't use relative path as volume source ("./src:/src") in service "api" +* can't use relative path as volume source ("./static:/opt/${static_subdir}") in service "web" +* secret "my_secret" must be external diff --git a/e2e/testdata/invalid-compose/docker-compose.yml b/e2e/testdata/invalid-compose/docker-compose.yml new file mode 100644 index 000000000..a4ef3d7fa --- /dev/null +++ b/e2e/testdata/invalid-compose/docker-compose.yml @@ -0,0 +1,15 @@ +version: "3.6" +services: + api: + image: python:3.6 + volumes: + - ./src:/src + web: + image: nginx + networks: + - front + volumes: + - ./static:/opt/${static_subdir} +secrets: + my_secret: + first: ./path/to/secret.txt diff --git a/internal/commands/build/compose.go b/internal/commands/build/compose.go index b521a0528..54da5af21 100644 --- a/internal/commands/build/compose.go +++ b/internal/commands/build/compose.go @@ -1,9 +1,7 @@ package build import ( - "fmt" "path" - "path/filepath" "strings" "github.com/docker/app/render" @@ -26,13 +24,6 @@ func parseCompose(app *types.App, contextPath string, options buildOptions) (map pulledServices := []compose.ServiceConfig{} opts := map[string]build.Options{} for _, service := range comp.Services { - // Sanity check - for _, vol := range service.Volumes { - if vol.Type == "bind" && !filepath.IsAbs(vol.Source) { - return nil, nil, fmt.Errorf("invalid service %q: can't use relative path as volume source", service.Name) - } - } - if service.Build.Context == "" { pulledServices = append(pulledServices, service) continue diff --git a/internal/packager/extract.go b/internal/packager/extract.go index f52ca22de..571ea8831 100644 --- a/internal/packager/extract.go +++ b/internal/packager/extract.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/docker/app/internal" + "github.com/docker/app/internal/validator" "github.com/docker/app/loader" "github.com/docker/app/types" "github.com/pkg/errors" @@ -64,6 +65,12 @@ func Extract(name string, ops ...func(*types.App) error) (*types.App, error) { return nil, errors.Wrapf(err, "cannot locate application %q in filesystem", name) } if s.IsDir() { + v := validator.NewValidatorWithDefaults() + err := v.Validate(filepath.Join(appname, internal.ComposeFileName)) + if err != nil { + return nil, err + } + // directory: already decompressed appOpts := append(ops, types.WithPath(appname), diff --git a/internal/packager/init.go b/internal/packager/init.go index b69de1cf3..fd38d1015 100644 --- a/internal/packager/init.go +++ b/internal/packager/init.go @@ -14,6 +14,7 @@ import ( "github.com/docker/app/internal" "github.com/docker/app/internal/compose" + "github.com/docker/app/internal/validator" "github.com/docker/app/internal/yaml" "github.com/docker/app/types" "github.com/docker/app/types/metadata" @@ -49,6 +50,11 @@ func Init(errWriter io.Writer, name string, composeFile string) (string, error) if composeFile == "" { err = initFromScratch(name) } else { + v := validator.NewValidatorWithDefaults() + err = v.Validate(composeFile) + if err != nil { + return "", err + } err = initFromComposeFile(errWriter, name, composeFile) } if err != nil { @@ -82,11 +88,11 @@ func checkComposeFileVersion(compose map[string]interface{}) error { func getEnvFiles(svcName string, envFileEntry interface{}) ([]string, error) { var envFiles []string - switch envFileEntry.(type) { + switch envFileEntry := envFileEntry.(type) { case string: - envFiles = append(envFiles, envFileEntry.(string)) + envFiles = append(envFiles, envFileEntry) case []interface{}: - for _, env := range envFileEntry.([]interface{}) { + for _, env := range envFileEntry { envFiles = append(envFiles, env.(string)) } default: @@ -125,50 +131,6 @@ func checkEnvFiles(errWriter io.Writer, appName string, cfgMap map[string]interf return nil } -func checkRelativePaths(cfgMap map[string]interface{}) error { - services := cfgMap["services"] - servicesMap, ok := services.(map[string]interface{}) - if !ok { - return fmt.Errorf("invalid Compose file") - } - for svcName, svc := range servicesMap { - svcContent, ok := svc.(map[string]interface{}) - if !ok { - return fmt.Errorf("invalid service %q", svcName) - } - v, ok := svcContent["volumes"] - if !ok { - continue - } - volumes, ok := v.([]interface{}) - if !ok { - return fmt.Errorf("invalid Compose file") - } - for _, volume := range volumes { - switch volume.(type) { - case string: - svol := volume.(string) - source := strings.TrimRight(svol, ":") - if !filepath.IsAbs(source) { - return fmt.Errorf("invalid service %q: can't use relative path as volume source", svcName) - } - case map[string]interface{}: - lvol := volume.(map[string]interface{}) - src, ok := lvol["source"] - if !ok { - return fmt.Errorf("invalid volume in service %q", svcName) - } - if !filepath.IsAbs(src.(string)) { - return fmt.Errorf("invalid service %q: can't use relative path as volume source", svcName) - } - default: - return fmt.Errorf("invalid Compose file") - } - } - } - return nil -} - func getParamsFromDefaultEnvFile(composeFile string, composeRaw []byte) (map[string]string, bool, error) { params := make(map[string]string) envs, err := opts.ParseEnvFile(filepath.Join(filepath.Dir(composeFile), ".env")) @@ -217,9 +179,6 @@ func initFromComposeFile(errWriter io.Writer, name string, composeFile string) e if err := checkEnvFiles(errWriter, name, cfgMap); err != nil { return err } - if err := checkRelativePaths(cfgMap); err != nil { - return err - } params, needsFilling, err := getParamsFromDefaultEnvFile(composeFile, composeRaw) if err != nil { return err diff --git a/internal/packager/init_test.go b/internal/packager/init_test.go index f341575df..b809b3fff 100644 --- a/internal/packager/init_test.go +++ b/internal/packager/init_test.go @@ -186,39 +186,3 @@ maintainers: ) assert.Assert(t, fs.Equal(tmpdir.Path(), manifest)) } - -func TestInitRelativeVolumePath(t *testing.T) { - for _, composeData := range []string{` -version: '3.7' -services: - nginx: - image: nginx - volumes: - - ./foo:/data -`, - ` -version: '3.7' -services: - nginx: - image: nginx - volumes: - - type: bind - source: ./foo - target: /data -`, - } { - inputDir := fs.NewDir(t, "app_input_", - fs.WithFile(internal.ComposeFileName, composeData), - ) - defer inputDir.Remove() - - appName := "my.dockerapp" - dir := fs.NewDir(t, "app_", - fs.WithDir(appName), - ) - defer dir.Remove() - - err := initFromComposeFile(nil, dir.Join(appName), inputDir.Join(internal.ComposeFileName)) - assert.ErrorContains(t, err, "can't use relative path") - } -} diff --git a/internal/validator/rules/externalsecrets.go b/internal/validator/rules/externalsecrets.go new file mode 100644 index 000000000..c6122627b --- /dev/null +++ b/internal/validator/rules/externalsecrets.go @@ -0,0 +1,39 @@ +package rules + +import ( + "github.com/pkg/errors" +) + +type externalSecretsValidator struct { +} + +func NewExternalSecretsRule() Rule { + return &externalSecretsValidator{} +} + +func (s *externalSecretsValidator) Collect(parent string, key string, value interface{}) { +} + +func (s *externalSecretsValidator) Accept(parent string, key string) bool { + return key == "secrets" +} + +func (s *externalSecretsValidator) Validate(cfgMap interface{}) []error { + errs := []error{} + if value, ok := cfgMap.(map[string]interface{}); ok { + for secretName, secret := range value { + if secretMap, ok := secret.(map[string]interface{}); ok { + var hasExternal = false + for key := range secretMap { + if key == "external" { + hasExternal = true + } + } + if !hasExternal { + errs = append(errs, errors.Errorf(`secret %q must be external`, secretName)) + } + } + } + } + return errs +} diff --git a/internal/validator/rules/externalsecrets_test.go b/internal/validator/rules/externalsecrets_test.go new file mode 100644 index 000000000..4ff5cf320 --- /dev/null +++ b/internal/validator/rules/externalsecrets_test.go @@ -0,0 +1,55 @@ +package rules + +import ( + "testing" + + "gotest.tools/assert" +) + +func TestExternalSecrets(t *testing.T) { + s := NewExternalSecretsRule() + + t.Run("should accept secrets", func(t *testing.T) { + // The secrets key is on the root path, that's why it doesn't + // have a parent + assert.Equal(t, s.Accept("", "secrets"), true) + }) + + t.Run("should return nil if all secrets are external", func(t *testing.T) { + input := map[string]interface{}{ + "my_secret": map[string]interface{}{ + "external": "true", + }, + } + + errs := s.Validate(input) + assert.Equal(t, len(errs), 0) + }) + + t.Run("should return error if no external secrets", func(t *testing.T) { + input := map[string]interface{}{ + "my_secret": map[string]interface{}{ + "file": "./my_secret.txt", + }, + } + + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + assert.ErrorContains(t, errs[0], `secret "my_secret" must be external`) + }) + + t.Run("should return all errors", func(t *testing.T) { + input := map[string]interface{}{ + "my_secret": map[string]interface{}{ + "file": "./my_secret.txt", + }, + "my_other_secret": map[string]interface{}{ + "file": "./my_secret.txt", + }, + } + + errs := s.Validate(input) + assert.Equal(t, len(errs), 2) + }) + +} diff --git a/internal/validator/rules/relativepath.go b/internal/validator/rules/relativepath.go new file mode 100644 index 000000000..c4d24ad3e --- /dev/null +++ b/internal/validator/rules/relativepath.go @@ -0,0 +1,74 @@ +package rules + +import ( + "fmt" + "path/filepath" + "regexp" + "strings" +) + +type relativePathRule struct { + volumes map[string]interface{} + service string +} + +func NewRelativePathRule() Rule { + return &relativePathRule{ + volumes: map[string]interface{}{}, + } +} + +func (s *relativePathRule) Collect(parent string, key string, value interface{}) { + if parent == "volumes" { + s.volumes[key] = value + } +} + +func (s *relativePathRule) Accept(parent string, key string) bool { + if parent == "services" { + s.service = key + } + return regexp.MustCompile("services.(.*).volumes").MatchString(parent + "." + key) +} + +func (s *relativePathRule) Validate(value interface{}) []error { + if m, ok := value.(map[string]interface{}); ok { + src, ok := m["source"] + if !ok { + return []error{fmt.Errorf("invalid volume in service %q", s.service)} + } + _, volumeExists := s.volumes[src.(string)] + if !filepath.IsAbs(src.(string)) && !volumeExists { + return []error{fmt.Errorf("can't use relative path as volume source (%q) in service %q", src, s.service)} + } + } + + if m, ok := value.([]interface{}); ok { + errs := []error{} + for _, p := range m { + str, ok := p.(string) + if !ok { + errs = append(errs, fmt.Errorf("invalid volume in service %q", s.service)) + continue + } + + parts := strings.Split(str, ":") + if len(parts) <= 1 { + errs = append(errs, fmt.Errorf("invalid volume definition (%q) in service %q", str, s.service)) + continue + } + + volumeName := parts[0] + _, volumeExists := s.volumes[volumeName] + if !filepath.IsAbs(volumeName) && !volumeExists { + errs = append(errs, fmt.Errorf("can't use relative path as volume source (%q) in service %q", str, s.service)) + continue + } + } + + if len(errs) > 0 { + return errs + } + } + return nil +} diff --git a/internal/validator/rules/relativepath_test.go b/internal/validator/rules/relativepath_test.go new file mode 100644 index 000000000..2d26cb78b --- /dev/null +++ b/internal/validator/rules/relativepath_test.go @@ -0,0 +1,86 @@ +package rules + +import ( + "testing" + + "gotest.tools/assert" +) + +func TestRelativePathRule(t *testing.T) { + s := NewRelativePathRule() + + t.Run("should accept only volume paths", func(t *testing.T) { + assert.Equal(t, s.Accept("services", "test"), false) + assert.Equal(t, s.Accept("services.test.volumes", "my_volume"), true) + assert.Equal(t, s.Accept("services.test", "volumes"), true) + }) + + t.Run("should validate named volume paths", func(t *testing.T) { + input := map[string]string{ + "toto": "tata", + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 0) + }) + + t.Run("should return error if short syntax volume path is relative", func(t *testing.T) { + input := []interface{}{ + "./foo:/data", + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + + assert.ErrorContains(t, errs[0], `can't use relative path as volume source ("./foo:/data") in service "test"`) + }) + + t.Run("should return error if the volume definition is invalid", func(t *testing.T) { + input := []interface{}{ + "foo", + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + + assert.ErrorContains(t, errs[0], `invalid volume definition ("foo") in service "test"`) + }) + + t.Run("should return all volume errors", func(t *testing.T) { + input := []interface{}{ + "./foo:/data1", + "./bar:/data2", + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 2) + + assert.ErrorContains(t, errs[0], `can't use relative path as volume source ("./foo:/data1") in service "test"`) + assert.ErrorContains(t, errs[1], `can't use relative path as volume source ("./bar:/data2") in service "test"`) + }) + + // When a volume is in short syntax, the list of volumes must be strings + t.Run("shoud return error if volume list is invalid", func(t *testing.T) { + input := []interface{}{ + 1, + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + + assert.ErrorContains(t, errs[0], `invalid volume in service "test"`) + }) + + t.Run("should return error if long syntax volume path is relative", func(t *testing.T) { + input := map[string]interface{}{ + "source": "./foo", + } + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + + assert.ErrorContains(t, errs[0], `can't use relative path as volume source ("./foo") in service "test"`) + }) + + t.Run("shoud return error if volume map is invalid", func(t *testing.T) { + input := map[string]interface{}{} + errs := s.Validate(input) + assert.Equal(t, len(errs), 1) + + assert.ErrorContains(t, errs[0], `invalid volume in service "test"`) + }) +} diff --git a/internal/validator/rules/rule.go b/internal/validator/rules/rule.go new file mode 100644 index 000000000..eade3e65f --- /dev/null +++ b/internal/validator/rules/rule.go @@ -0,0 +1,7 @@ +package rules + +type Rule interface { + Collect(path string, key string, value interface{}) + Accept(parent string, key string) bool + Validate(value interface{}) []error +} diff --git a/internal/validator/validator.go b/internal/validator/validator.go new file mode 100644 index 000000000..ac12c33d9 --- /dev/null +++ b/internal/validator/validator.go @@ -0,0 +1,133 @@ +package validator + +import ( + "io/ioutil" + "sort" + "strings" + + "github.com/docker/app/internal/validator/rules" + composeloader "github.com/docker/cli/cli/compose/loader" + "github.com/pkg/errors" +) + +type Validator struct { + Rules []rules.Rule + errors []error +} + +type ValidationError struct { + Errors []error +} + +type ValidationCallback func(string, string, interface{}) + +func (v ValidationError) Error() string { + parts := []string{} + for _, err := range v.Errors { + parts = append(parts, "* "+err.Error()) + } + + sort.Strings(parts) + parts = append([]string{"Compose file validation failed:"}, parts...) + + return strings.Join(parts, "\n") +} + +type Config func(*Validator) +type Opt func(c *Validator) error + +func NewValidator(opts ...Config) Validator { + validator := Validator{} + for _, opt := range opts { + opt(&validator) + } + return validator +} + +func WithRelativePathRule() Config { + return func(v *Validator) { + v.Rules = append(v.Rules, rules.NewRelativePathRule()) + } +} + +func WithExternalSecretsRule() Config { + return func(v *Validator) { + v.Rules = append(v.Rules, rules.NewExternalSecretsRule()) + } +} + +func NewValidatorWithDefaults() Validator { + return NewValidator( + WithRelativePathRule(), + WithExternalSecretsRule(), + ) +} + +// Validate validates the compose file, it returns an error +// if it can't parse the compose file or a ValidationError +// that contains all the validation errors (if any), nil otherwise +func (v *Validator) Validate(composeFile string) error { + composeRaw, err := ioutil.ReadFile(composeFile) + if err != nil { + return errors.Wrapf(err, "failed to read compose file %q", composeFile) + } + cfgMap, err := composeloader.ParseYAML(composeRaw) + if err != nil { + return errors.Wrap(err, "failed to parse compose file") + } + + // First phase, the rules collect all the dependent values they need + v.visitAll("", cfgMap, v.collect) + // Second phase, validate the compose file + v.visitAll("", cfgMap, v.validate) + + if len(v.errors) > 0 { + return ValidationError{ + Errors: v.errors, + } + } + return nil +} + +func (v *Validator) collect(parent string, key string, value interface{}) { + for _, rule := range v.Rules { + rule.Collect(parent, key, value) + } +} + +func (v *Validator) validate(parent string, key string, value interface{}) { + for _, rule := range v.Rules { + if rule.Accept(parent, key) { + verrs := rule.Validate(value) + if len(verrs) > 0 { + v.errors = append(v.errors, verrs...) + } + } + } +} + +func (v *Validator) visitAll(parent string, cfgMap interface{}, cb ValidationCallback) { + m, ok := cfgMap.(map[string]interface{}) + if !ok { + return + } + + for key, value := range m { + switch value := value.(type) { + case string: + continue + default: + cb(parent, key, value) + + path := parent + "." + key + if parent == "" { + path = key + } + + sub, ok := m[key].(map[string]interface{}) + if ok { + v.visitAll(path, sub, cb) + } + } + } +} diff --git a/internal/validator/validator_test.go b/internal/validator/validator_test.go new file mode 100644 index 000000000..7eded6d16 --- /dev/null +++ b/internal/validator/validator_test.go @@ -0,0 +1,59 @@ +package validator + +import ( + "testing" + + "github.com/docker/app/internal" + "gotest.tools/assert" + "gotest.tools/fs" +) + +type mockRule struct { + acceptCalled bool + validateCalled bool +} + +func (m *mockRule) Collect(path string, key string, value interface{}) { + +} + +func (m *mockRule) Accept(path string, key string) bool { + m.acceptCalled = true + return true +} + +func (m *mockRule) Validate(value interface{}) []error { + m.validateCalled = true + return nil +} + +func TestValidate(t *testing.T) { + composeData := ` +version: '3.7' +services: + nginx: + image: nginx + volumes: + - ./foo:/data +` + inputDir := fs.NewDir(t, "app_input_", + fs.WithFile(internal.ComposeFileName, composeData), + ) + defer inputDir.Remove() + + appName := "my.dockerapp" + dir := fs.NewDir(t, "app_", + fs.WithDir(appName), + ) + defer dir.Remove() + + r := &mockRule{} + v := NewValidator(func(v *Validator) { + v.Rules = append(v.Rules, r) + }) + + err := v.Validate(inputDir.Join(internal.ComposeFileName)) + assert.NilError(t, err) + assert.Equal(t, r.acceptCalled, true) + assert.Equal(t, r.validateCalled, true) +}