Merge pull request #760 from rumpl/compose-validation

Compose validation

Author: eunomie

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

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

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

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

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),

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

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")
-	}
-}

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
+}

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)
+	})
+
+}

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
+}