From 4f5d9467a62f76f602ef194f62b83c6437da276d Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:39:48 +0100 Subject: [PATCH 1/9] Add external secrets rule Signed-off-by: Djordje Lukic --- internal/validator/rules/externalsecrets.go | 40 +++++++++++++ .../validator/rules/externalsecrets_test.go | 58 +++++++++++++++++++ internal/validator/rules/rule.go | 7 +++ 3 files changed, 105 insertions(+) create mode 100644 internal/validator/rules/externalsecrets.go create mode 100644 internal/validator/rules/externalsecrets_test.go create mode 100644 internal/validator/rules/rule.go diff --git a/internal/validator/rules/externalsecrets.go b/internal/validator/rules/externalsecrets.go new file mode 100644 index 000000000..66a9d24de --- /dev/null +++ b/internal/validator/rules/externalsecrets.go @@ -0,0 +1,40 @@ +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 v1, ok := secret.(map[string]interface{}); ok { + var hasExternal = false + for key := range v1 { + if key == "external" { + hasExternal = true + } + } + if !hasExternal { + errs = append(errs, errors.Errorf(`secret %q should 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..30fb49dc5 --- /dev/null +++ b/internal/validator/rules/externalsecrets_test.go @@ -0,0 +1,58 @@ +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" should 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) + + assert.ErrorContains(t, errs[0], `secret "my_secret" should be external`) + assert.ErrorContains(t, errs[1], `secret "my_other_secret" should be external`) + }) + +} 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 +} From e1d436459d4ad60674c220b5be5ef07087215eac Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:40:08 +0100 Subject: [PATCH 2/9] Add relative path rule Signed-off-by: Djordje Lukic --- internal/validator/rules/relativepath.go | 74 ++++++++++++++++ internal/validator/rules/relativepath_test.go | 88 +++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 internal/validator/rules/relativepath.go create mode 100644 internal/validator/rules/relativepath_test.go diff --git a/internal/validator/rules/relativepath.go b/internal/validator/rules/relativepath.go new file mode 100644 index 000000000..a80919fb4 --- /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("1 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) != 2 { + 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..1364aab9d --- /dev/null +++ b/internal/validator/rules/relativepath_test.go @@ -0,0 +1,88 @@ +package rules + +import ( + "fmt" + "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) + fmt.Println(errs) + 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"`) + }) +} From 08c99b7c72fea92f8b905addfb00876cfb26e824 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:40:44 +0100 Subject: [PATCH 3/9] Add the validator Signed-off-by: Djordje Lukic --- internal/validator/validator.go | 133 +++++++++++++++++++++++++++ internal/validator/validator_test.go | 59 ++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 internal/validator/validator.go create mode 100644 internal/validator/validator_test.go 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) +} From 5d2b8334f1efaa5e640c85fe6930a1a3ea0a0a38 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:41:15 +0100 Subject: [PATCH 4/9] Call the validator in packager init and extract Remove the no longer used check for the relative path Signed-off-by: Djordje Lukic --- internal/packager/extract.go | 10 +++++- internal/packager/init.go | 59 ++++++---------------------------- internal/packager/init_test.go | 36 --------------------- 3 files changed, 18 insertions(+), 87 deletions(-) diff --git a/internal/packager/extract.go b/internal/packager/extract.go index f52ca22de..ea30562ab 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,12 +65,19 @@ 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), types.WithSource(types.AppSourceSplit), ) - return loader.LoadFromDirectory(appname, appOpts...) + app, err := loader.LoadFromDirectory(appname, appOpts...) + return app, err } // not a dir: a tarball package, extract that in a temp dir app, err := loader.LoadFromTar(appname, ops...) 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") - } -} From fae5df6b7b189e48c26c65885d6978b48c7ca0e8 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:41:40 +0100 Subject: [PATCH 5/9] Add e2e test for the init with an invalid file Signed-off-by: Djordje Lukic --- e2e/commands_test.go | 12 ++++++++++++ e2e/testdata/init-invalid-output.golden | 4 ++++ e2e/testdata/invalid-compose/docker-compose.yml | 15 +++++++++++++++ 3 files changed, 31 insertions(+) create mode 100644 e2e/testdata/init-invalid-output.golden create mode 100644 e2e/testdata/invalid-compose/docker-compose.yml 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..7e8f02924 --- /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" should 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 From 0be3c167669da4096af3aabf5255ba081807c409 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 16:49:35 +0100 Subject: [PATCH 6/9] Remove useless check This check is done by the packager now Signed-off-by: Djordje Lukic --- internal/commands/build/compose.go | 9 --------- 1 file changed, 9 deletions(-) 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 From 25bc6b43685c9def87997f9a8bfd994132d53d5e Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 21 Nov 2019 17:54:35 +0100 Subject: [PATCH 7/9] Only test the number of errors The map implementation of go iterates a map random;y so we can't know which error has what message. It's ok to remove this here because we have an e2e test (the final errors are sorted alphabetically). Signed-off-by: Djordje Lukic --- internal/validator/rules/externalsecrets_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/validator/rules/externalsecrets_test.go b/internal/validator/rules/externalsecrets_test.go index 30fb49dc5..0e5f68911 100644 --- a/internal/validator/rules/externalsecrets_test.go +++ b/internal/validator/rules/externalsecrets_test.go @@ -50,9 +50,6 @@ func TestExternalSecrets(t *testing.T) { errs := s.Validate(input) assert.Equal(t, len(errs), 2) - - assert.ErrorContains(t, errs[0], `secret "my_secret" should be external`) - assert.ErrorContains(t, errs[1], `secret "my_other_secret" should be external`) }) } From cac8c325836906b362fef42b0338997bad86a594 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Sat, 23 Nov 2019 20:06:32 +0100 Subject: [PATCH 8/9] Change the external secret validation message A secret *must* be external, not *should* Signed-off-by: Djordje Lukic --- e2e/testdata/init-invalid-output.golden | 2 +- internal/validator/rules/externalsecrets.go | 6 +++--- internal/validator/rules/externalsecrets_test.go | 2 +- internal/validator/rules/relativepath.go | 2 +- internal/validator/rules/relativepath_test.go | 2 -- 5 files changed, 6 insertions(+), 8 deletions(-) diff --git a/e2e/testdata/init-invalid-output.golden b/e2e/testdata/init-invalid-output.golden index 7e8f02924..847fb5292 100644 --- a/e2e/testdata/init-invalid-output.golden +++ b/e2e/testdata/init-invalid-output.golden @@ -1,4 +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" should be external +* secret "my_secret" must be external diff --git a/internal/validator/rules/externalsecrets.go b/internal/validator/rules/externalsecrets.go index 66a9d24de..56bd45e52 100644 --- a/internal/validator/rules/externalsecrets.go +++ b/internal/validator/rules/externalsecrets.go @@ -23,15 +23,15 @@ func (s *externalSecretsValidator) Validate(cfgMap interface{}) []error { errs := []error{} if value, ok := cfgMap.(map[string]interface{}); ok { for secretName, secret := range value { - if v1, ok := secret.(map[string]interface{}); ok { + if secretMap, ok := secret.(map[string]interface{}); ok { var hasExternal = false - for key := range v1 { + for key := range secretMap { if key == "external" { hasExternal = true } } if !hasExternal { - errs = append(errs, errors.Errorf(`secret %q should be external`, secretName)) + errs = append(errs, errors.Errorf(`secret %q must be external`, secretName)) } } } diff --git a/internal/validator/rules/externalsecrets_test.go b/internal/validator/rules/externalsecrets_test.go index 0e5f68911..4ff5cf320 100644 --- a/internal/validator/rules/externalsecrets_test.go +++ b/internal/validator/rules/externalsecrets_test.go @@ -35,7 +35,7 @@ func TestExternalSecrets(t *testing.T) { errs := s.Validate(input) assert.Equal(t, len(errs), 1) - assert.ErrorContains(t, errs[0], `secret "my_secret" should be external`) + assert.ErrorContains(t, errs[0], `secret "my_secret" must be external`) }) t.Run("should return all errors", func(t *testing.T) { diff --git a/internal/validator/rules/relativepath.go b/internal/validator/rules/relativepath.go index a80919fb4..0fcdc8fe1 100644 --- a/internal/validator/rules/relativepath.go +++ b/internal/validator/rules/relativepath.go @@ -39,7 +39,7 @@ func (s *relativePathRule) Validate(value interface{}) []error { } _, volumeExists := s.volumes[src.(string)] if !filepath.IsAbs(src.(string)) && !volumeExists { - return []error{fmt.Errorf("1 can't use relative path as volume source (%q) in service %q", src, s.service)} + return []error{fmt.Errorf("can't use relative path as volume source (%q) in service %q", src, s.service)} } } diff --git a/internal/validator/rules/relativepath_test.go b/internal/validator/rules/relativepath_test.go index 1364aab9d..2d26cb78b 100644 --- a/internal/validator/rules/relativepath_test.go +++ b/internal/validator/rules/relativepath_test.go @@ -1,7 +1,6 @@ package rules import ( - "fmt" "testing" "gotest.tools/assert" @@ -62,7 +61,6 @@ func TestRelativePathRule(t *testing.T) { 1, } errs := s.Validate(input) - fmt.Println(errs) assert.Equal(t, len(errs), 1) assert.ErrorContains(t, errs[0], `invalid volume in service "test"`) From 582f377312605c262883e348837c82b1d8b9024d Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 28 Nov 2019 10:34:18 +0100 Subject: [PATCH 9/9] Fix invalid volume definition It can have more than one `:` in its definition. Signed-off-by: Djordje Lukic --- internal/packager/extract.go | 3 +-- internal/validator/rules/externalsecrets.go | 1 - internal/validator/rules/relativepath.go | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/packager/extract.go b/internal/packager/extract.go index ea30562ab..571ea8831 100644 --- a/internal/packager/extract.go +++ b/internal/packager/extract.go @@ -76,8 +76,7 @@ func Extract(name string, ops ...func(*types.App) error) (*types.App, error) { types.WithPath(appname), types.WithSource(types.AppSourceSplit), ) - app, err := loader.LoadFromDirectory(appname, appOpts...) - return app, err + return loader.LoadFromDirectory(appname, appOpts...) } // not a dir: a tarball package, extract that in a temp dir app, err := loader.LoadFromTar(appname, ops...) diff --git a/internal/validator/rules/externalsecrets.go b/internal/validator/rules/externalsecrets.go index 56bd45e52..c6122627b 100644 --- a/internal/validator/rules/externalsecrets.go +++ b/internal/validator/rules/externalsecrets.go @@ -12,7 +12,6 @@ func NewExternalSecretsRule() Rule { } func (s *externalSecretsValidator) Collect(parent string, key string, value interface{}) { - } func (s *externalSecretsValidator) Accept(parent string, key string) bool { diff --git a/internal/validator/rules/relativepath.go b/internal/validator/rules/relativepath.go index 0fcdc8fe1..c4d24ad3e 100644 --- a/internal/validator/rules/relativepath.go +++ b/internal/validator/rules/relativepath.go @@ -53,7 +53,7 @@ func (s *relativePathRule) Validate(value interface{}) []error { } parts := strings.Split(str, ":") - if len(parts) != 2 { + if len(parts) <= 1 { errs = append(errs, fmt.Errorf("invalid volume definition (%q) in service %q", str, s.service)) continue }