Forbid variable substitution on image field

Ulysses Souza · Ulysses Souza · commit 5e09f2aafa13 · 2019-02-05T11:28:06.000+01:00
Signed-off-by: Ulysses Souza &lt;ulysses.souza@docker.com&gt;
diff --git a/.gitignore b/.gitignore
@@ -2,12 +2,8 @@
 #  if you want to ignore files created by your editor/tools,
 #  please consider a global .gitignore https://help.github.com/articles/ignoring-files
 *.tar.gz
-*.dockerapp
 _build/
 bin/
-!examples/*/*.dockerapp
-!e2e/**/*.dockerapp
-!integrations/**/*.dockerapp
 .gradle
 coverage.html
 cover.out
diff --git a/e2e/commands_test.go b/e2e/commands_test.go
@@ -90,8 +90,8 @@ func TestInit(t *testing.T) {
 	composeData := `version: "3.2"
 services:
   nginx:
-    image: nginx:${NGINX_VERSION}
-    command: nginx $NGINX_ARGS
+    image: nginx:latest
+    command: nginx $NGINX_ARGS ${NGINX_DRY_RUN}
 `
 	meta := `# Version of the application
 version: 0.1.0
@@ -108,7 +108,7 @@ maintainers:
   - name: joe
     email: joe@joe.com
 `
-	envData := "# some comment\nNGINX_VERSION=latest"
+	envData := "# some comment\nNGINX_DRY_RUN=-t"
 	dir := fs.NewDir(t, "app_input",
 		fs.WithFile(internal.ComposeFileName, composeData),
 		fs.WithFile(".env", envData),
@@ -130,7 +130,7 @@ maintainers:
 		fs.WithMode(0755),
 		fs.WithFile(internal.MetadataFileName, meta, fs.WithMode(0644)), // too many variables, cheating
 		fs.WithFile(internal.ComposeFileName, composeData, fs.WithMode(0644)),
-		fs.WithFile(internal.ParametersFileName, "NGINX_ARGS: FILL ME\nNGINX_VERSION: latest\n", fs.WithMode(0644)),
+		fs.WithFile(internal.ParametersFileName, "NGINX_ARGS: FILL ME\nNGINX_DRY_RUN: -t\n", fs.WithMode(0644)),
 	)
 	assert.Assert(t, fs.Equal(dirName, manifest))
 
diff --git a/e2e/testdata/attachments.dockerapp/docker-compose.yml b/e2e/testdata/attachments.dockerapp/docker-compose.yml
@@ -1,7 +1,7 @@
 version: "3.7"
 services:
   front:
-    image: nginx:${myapp.nginx_version}
+    image: nginx:latest
     deploy:
       replicas: ${myapp.nginx_replicas}
   debug:
@@ -26,4 +26,4 @@ services:
     command: monitor --source ${app.name}-${app.version} $$dollar
     x-enabled: "! ${myapp.debug}"
   app-watcher:
-    image: $watcher.image
+    image: watcher:latest
diff --git a/e2e/testdata/attachments.dockerapp/parameters.yml b/e2e/testdata/attachments.dockerapp/parameters.yml
@@ -1,10 +1,7 @@
 privileged: false
 myapp:
   debug: false
-  nginx_version: latest
   nginx_replicas: 2
-watcher:
-  image: watcher:latest
 read_only: true
 stdin_open: true
 tty: true
diff --git a/e2e/testdata/envvariables-inspect.golden b/e2e/testdata/envvariables-inspect.golden
@@ -6,9 +6,8 @@ Service (1) Replicas Ports Image
 ----------- -------- ----- -----
 test        1              alpine:latest
 
-Parameters (4)       Value
---------------       -----
-myapp.alpine_version latest
-myapp.command1       cat
-myapp.command2       foo
-myapp.command3       bar
+Parameters (3) Value
+-------------- -----
+myapp.command1 cat
+myapp.command2 foo
+myapp.command3 bar
diff --git a/e2e/testdata/init-singlefile.dockerapp b/e2e/testdata/init-singlefile.dockerapp
@@ -19,10 +19,10 @@ maintainers:
 version: "3.2"
 services:
   nginx:
-    image: nginx:${NGINX_VERSION}
-    command: nginx $NGINX_ARGS
+    image: nginx:latest
+    command: nginx $NGINX_ARGS ${NGINX_DRY_RUN}
 
 ---
 # This section contains the default values for your application parameters.
 NGINX_ARGS: FILL ME
-NGINX_VERSION: latest
+NGINX_DRY_RUN: -t
diff --git a/e2e/testdata/render/envvariables/my.dockerapp/docker-compose.yml b/e2e/testdata/render/envvariables/my.dockerapp/docker-compose.yml
@@ -1,5 +1,5 @@
 version: "3.4"
 services:
   test:
-    image: alpine:${myapp.alpine_version}
+    image: alpine:latest
     command: bash -c "${myapp.command1} ${myapp.command2} ${myapp.command3}"
diff --git a/e2e/testdata/render/envvariables/my.dockerapp/parameters.yml b/e2e/testdata/render/envvariables/my.dockerapp/parameters.yml
@@ -1,5 +1,4 @@
 myapp:
-  alpine_version: latest
   command1: cat
   command2: foo
   command3: bar
diff --git a/e2e/testdata/simple/simple.dockerapp/docker-compose.yml b/e2e/testdata/simple/simple.dockerapp/docker-compose.yml
@@ -1,23 +1,23 @@
 version: "3.6"
 services:
   api:
-    image: python:${versions.python}
+    image: python:3.6
     networks:
       back:
       front:
         aliases:
           - corp.app.api.com
           - ${api_host}
   web:
-    image: nginx:${versions.nginx}
+    image: nginx:latest
     networks:
       - front
     volumes:
       - static:/opt/${static_subdir}
     ports:
       - ${web_port}:80
   db:
-    image: postgres:${versions.postgres}
+    image: postgres:9.3
     networks:
       - back
 networks:
diff --git a/e2e/testdata/simple/simple.dockerapp/parameters.yml b/e2e/testdata/simple/simple.dockerapp/parameters.yml
@@ -1,7 +1,3 @@
-versions:
-  python: '3.6'
-  postgres: '9.3'
-  nginx: latest
 web_port: '8082'
 api_host: coolapp.com
 static_subdir: data/static
diff --git a/examples/voting-app/voting-app.dockerapp/docker-compose.yml b/examples/voting-app/voting-app.dockerapp/docker-compose.yml
@@ -26,7 +26,7 @@ services:
         constraints: [node.role == manager]
 
   vote:
-    image: ${vote.image.name}:${vote.image.tag}
+    image: dockersamples/examplevotingapp_vote:latest
     ports:
       - ${vote.port}:80
     networks:
@@ -41,7 +41,7 @@ services:
         condition: on-failure
 
   result:
-    image: ${result.image.name}:${result.image.tag}
+    image: dockersamples/examplevotingapp_result:latest
     ports:
       - ${result.port}:80
     networks:
@@ -57,7 +57,7 @@ services:
         condition: on-failure
 
   worker:
-    image: ${worker.image.name}:${worker.image.tag}
+    image: dockersamples/examplevotingapp_worker:latest
     networks:
       - frontend
       - backend
@@ -74,7 +74,7 @@ services:
         constraints: [node.role == manager]
 
   visualizer:
-    image: ${visualizer.image.name}:${visualizer.image.tag}
+    image: dockersamples/visualizer:latest
     ports:
       - ${visualizer.port}:8080
     stop_grace_period: 1m30s
diff --git a/examples/voting-app/voting-app.dockerapp/parameters.yml b/examples/voting-app/voting-app.dockerapp/parameters.yml
@@ -1,29 +1,17 @@
 # Vote.
 vote:
-  image:
-    name: dockersamples/examplevotingapp_vote
-    tag: latest
   port: 8080
   replicas: 1
 
 # Result.
 result:
-  image:
-    name: dockersamples/examplevotingapp_result
-    tag: latest
   port: 8181
   replicas: 1
 
 # Visualizer.
 visualizer:
-  image:
-    name: dockersamples/visualizer
-    tag: latest
   port: 8282
 
 # Worker.
 worker:
-  image:
-    name: dockersamples/examplevotingapp_worker
-    tag: latest
   replicas: 1
diff --git a/examples/wordpress/wordpress.dockerapp/docker-compose.yml b/examples/wordpress/wordpress.dockerapp/docker-compose.yml
@@ -3,7 +3,7 @@ version: "3.6"
 services:
 
   mysql:
-    image: mysql:${mysql.image.version}
+    image: mysql:8
     environment:
       MYSQL_ROOT_PASSWORD: ${mysql.rootpass}
       MYSQL_DATABASE: ${mysql.database}
diff --git a/internal/compose/compose.go b/internal/compose/compose.go
@@ -1,14 +1,30 @@
 package compose
 
 import (
+	"fmt"
 	"regexp"
+	"strings"
 
 	"github.com/docker/cli/cli/compose/loader"
 	"github.com/docker/cli/cli/compose/template"
 	composetypes "github.com/docker/cli/cli/compose/types"
 	"github.com/pkg/errors"
 )
 
+const (
+	delimiter    = "\\$"
+	substitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?"
+)
+
+var (
+	patternString = fmt.Sprintf(
+		"%s(?i:(?P<escaped>%s)|(?P<named>%s)|{(?P<braced>%s)}|(?P<invalid>))",
+		delimiter, delimiter, substitution, substitution,
+	)
+	// ExtrapolationPattern is the variable regexp pattern used to interpolate or extract variables when rendering
+	ExtrapolationPattern = regexp.MustCompile(patternString)
+)
+
 // Load applies the specified function when loading a slice of compose data
 func Load(composes [][]byte, apply func(string) (string, error)) ([]composetypes.ConfigFile, error) {
 	configFiles := []composetypes.ConfigFile{}
@@ -23,9 +39,46 @@ func Load(composes [][]byte, apply func(string) (string, error)) ([]composetypes
 		}
 		configFiles = append(configFiles, composetypes.ConfigFile{Config: parsed})
 	}
+
+	if err := validateImageInConfigFiles(configFiles); err != nil {
+		return nil, err
+	}
+
 	return configFiles, nil
 }
 
+func validateImageInConfigFiles(configFiles []composetypes.ConfigFile) error {
+	var errors []string
+	for _, configFile := range configFiles {
+		services, ok := configFile.Config["services"].(map[string]interface{})
+		if !ok {
+			continue
+		}
+		for serviceName, serviceContent := range services {
+			serviceMap, ok := serviceContent.(map[string]interface{})
+			if !ok {
+				continue
+			}
+			imageName, ok := serviceMap["image"].(string)
+			if !ok {
+				continue
+			}
+
+			if ExtrapolationPattern.MatchString(imageName) {
+				errors = append(errors,
+					fmt.Sprintf("%s: variables are not allowed in the service's image field. Found: '%s'",
+						serviceName, imageName))
+			}
+		}
+	}
+
+	if len(errors) > 0 {
+		return fmt.Errorf("%s", strings.Join(errors, "\n"))
+	}
+
+	return nil
+}
+
 // ExtractVariables extracts the variables from the specified compose data
 // This is a small helper to docker/cli template.ExtractVariables function
 func ExtractVariables(data []byte, pattern *regexp.Regexp) (map[string]string, error) {
diff --git a/internal/compose/compose_test.go b/internal/compose/compose_test.go
@@ -0,0 +1,60 @@
+package compose
+
+import (
+	"testing"
+
+	"github.com/docker/app/internal/renderer"
+	"gotest.tools/assert"
+
+	composetypes "github.com/docker/cli/cli/compose/types"
+)
+
+const (
+	imageVarsValidationErrorMessage = "variables are not allowed in the service's image field. Found: "
+)
+
+func TestValidateNonDynamicImageField(t *testing.T) {
+	composeFile := []byte(`
+version: "3.6"
+services:
+    hello:
+        image: ${image}`)
+	_, err := runLoad(composeFile)
+	assert.ErrorContains(t, err, imageVarsValidationErrorMessage)
+}
+
+func TestValidateNonDynamicImageFieldNoBrackets(t *testing.T) {
+	composeFile := []byte(`
+version: "3.6"
+services:
+    hello:
+        image: $image`)
+	_, err := runLoad(composeFile)
+	assert.ErrorContains(t, err, imageVarsValidationErrorMessage)
+}
+
+func TestValidateNonDynamicImageFieldPartial(t *testing.T) {
+	composeFile := []byte(`
+version: "3.6"
+services:
+    hello:
+        image: prefix-${image}:v1`)
+	_, err := runLoad(composeFile)
+	assert.ErrorContains(t, err, imageVarsValidationErrorMessage)
+}
+
+func TestValidateNonDynamicImageFieldPartialNoBrackets(t *testing.T) {
+	composeFile := []byte(`
+version: "3.6"
+services:
+    hello:
+        image: prefix-$image:v1`)
+	_, err := runLoad(composeFile)
+	assert.ErrorContains(t, err, imageVarsValidationErrorMessage)
+}
+
+func runLoad(composeFile []byte) ([]composetypes.ConfigFile, error) {
+	return Load([][]byte{composeFile}, func(data string) (string, error) {
+		return renderer.Apply(data, map[string]interface{}{"image": "nginx"})
+	})
+}
diff --git a/internal/packager/init.go b/internal/packager/init.go
@@ -15,7 +15,6 @@ import (
 	"github.com/docker/app/internal/compose"
 	"github.com/docker/app/internal/yaml"
 	"github.com/docker/app/loader"
-	"github.com/docker/app/render"
 	"github.com/docker/app/types"
 	"github.com/docker/app/types/metadata"
 	composeloader "github.com/docker/cli/cli/compose/loader"
@@ -147,7 +146,7 @@ func initFromComposeFile(name string, composeFile string) error {
 			}
 		}
 	}
-	vars, err := compose.ExtractVariables(composeRaw, render.Pattern)
+	vars, err := compose.ExtractVariables(composeRaw, compose.ExtrapolationPattern)
 	if err != nil {
 		return errors.Wrap(err, "failed to parse compose file")
 	}
diff --git a/render/render.go b/render/render.go
diff --git a/render/render_test.go b/render/render_test.go
