Merge pull request #1028 from dgageot/build-args

Fix issues with build args replacement

pkg/skaffold/docker/parse.go

@@ -27,6 +27,7 @@ import (
 	"sync"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha3"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/docker/docker/builder/dockerignore"
 	"github.com/docker/docker/pkg/fileutils"
 	"github.com/google/go-containerregistry/pkg/v1"
@@ -62,6 +63,33 @@ func ValidateDockerfile(path string) bool {
 	return true
 }
 
+func expandBuildArgs(nodes []*parser.Node, buildArgs map[string]*string) {
+	var key, value string
+
+	for _, node := range nodes {
+		switch node.Value {
+		case command.Arg:
+			// build arg's key
+			keyValue := strings.Split(node.Next.Value, "=")
+			key = keyValue[0]
+
+			// build arg's value
+			if buildArgs[key] != nil {
+				value = *buildArgs[key]
+			} else if len(keyValue) > 1 {
+				value = keyValue[1]
+			}
+		default:
+			if key != "" {
+				// replace $key with value
+				for curr := node; curr != nil; curr = curr.Next {
+					curr.Value = util.Expand(curr.Value, key, value)
+				}
+			}
+		}
+	}
+}
+
 func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*string) ([]string, error) {
 	f, err := os.Open(absDockerfilePath)
 	if err != nil {
@@ -74,52 +102,7 @@ func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*s
 		return nil, errors.Wrap(err, "parsing dockerfile")
 	}
 
-	var copied [][]string
-	envs := map[string]string{}
-
-	// First process all build args, and replace if necessary.
-	for i, value := range res.AST.Children {
-		switch value.Value {
-		case command.Arg:
-			var val, defaultValue, arg string
-			argSetting := strings.Fields(value.Original)[1]
-
-			argValues := strings.Split(argSetting, "=")
-
-			if len(argValues) > 1 {
-				arg, defaultValue = argValues[0], argValues[1]
-			} else {
-				arg = argValues[0]
-			}
-
-			valuePtr := buildArgs[arg]
-			if valuePtr == nil && defaultValue == "" {
-				logrus.Warnf("arg %s referenced in dockerfile but not provided with default or in build args", arg)
-			} else {
-				if valuePtr == nil {
-					val = defaultValue
-				} else {
-					val = *valuePtr
-				}
-				if val == "" {
-					logrus.Warnf("empty build arg provided in skaffold config: %s", arg)
-					break
-				}
-				// we have a non-empty arg: replace it in all subsequent nodes
-				for j := i; j < len(res.AST.Children); j++ {
-					currentNode := res.AST.Children[j]
-					for {
-						if currentNode == nil {
-							break
-						}
-						currentNode.Value = strings.Replace(currentNode.Value, "$"+arg, val, -1)
-						currentNode.Value = strings.Replace(currentNode.Value, "${"+arg+"}", val, -1)
-						currentNode = currentNode.Next
-					}
-				}
-			}
-		}
-	}
+	expandBuildArgs(res.AST.Children, buildArgs)
 
 	// Then process onbuilds, if present.
 	onbuildsImages := [][]string{}
@@ -147,6 +130,9 @@ func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*s
 		}
 	}
 
+	var copied [][]string
+	envs := map[string]string{}
+
 	var dispatchInstructions = func(r *parser.Result) {
 		for _, value := range r.AST.Children {
 			switch value.Value {

pkg/skaffold/docker/parse_test.go

@@ -70,7 +70,6 @@ RUN go build -o worker .
 FROM gcr.io/distroless/base
 WORKDIR /root/
 COPY --from=0 /go/src/github.com/r2d4/leeroy .
-CMD ["./worker"]
 `
 
 const envTest = `
@@ -84,7 +83,6 @@ const copyDirectory = `
 FROM nginx
 ADD . /etc/
 COPY ./file /etc/file
-CMD nginx
 `
 const multiFileCopy = `
 FROM ubuntu:14.04
@@ -100,7 +98,6 @@ const contextDockerfile = `
 FROM nginx
 ADD nginx.conf /etc/nginx
 COPY . /files
-CMD nginx
 `
 
 // This has an ONBUILD instruction of "COPY . /go/src/app"
@@ -117,28 +114,38 @@ const copyServerGoBuildArg = `
 FROM ubuntu:14.04
 ARG FOO
 COPY $FOO .
-CMD $FOO
+`
+
+const copyServerGoBuildArgSamePrefix = `
+FROM ubuntu:14.04
+ARG FOO=server.go
+ARG FOO2
+COPY $FOO2 .
 `
 
 const copyServerGoBuildArgCurlyBraces = `
 FROM ubuntu:14.04
 ARG FOO
 COPY ${FOO} .
-CMD ${FOO}
 `
 
 const copyServerGoBuildArgExtraWhitespace = `
 FROM ubuntu:14.04
 ARG  FOO
 COPY $FOO .
-CMD $FOO
 `
 
 const copyServerGoBuildArgDefaultValue = `
 FROM ubuntu:14.04
 ARG FOO=server.go
 COPY $FOO .
-CMD $FOO
+`
+
+const copyServerGoBuildArgRedefinedDefaultValue = `
+FROM ubuntu:14.04
+ARG FOO=server.go
+ARG FOO=worker.go
+COPY $FOO .
 `
 
 const fromStage = `
@@ -311,6 +318,14 @@ func TestGetDependencies(t *testing.T) {
 			expected:    []string{"Dockerfile", "server.go"},
 			fetched:     []string{"ubuntu:14.04"},
 		},
+		{
+			description: "build args with same prefix",
+			dockerfile:  copyServerGoBuildArgSamePrefix,
+			workspace:   ".",
+			buildArgs:   map[string]*string{"FOO2": util.StringPtr("worker.go")},
+			expected:    []string{"Dockerfile", "worker.go"},
+			fetched:     []string{"ubuntu:14.04"},
+		},
 		{
 			description: "build args with curly braces",
 			dockerfile:  copyServerGoBuildArgCurlyBraces,
@@ -328,17 +343,32 @@ func TestGetDependencies(t *testing.T) {
 			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
-			description: "build args with default value and buildArgs unset",
+			description: "build args with default value",
 			dockerfile:  copyServerGoBuildArgDefaultValue,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go"},
 			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
-			description: "build args with default value and buildArgs set",
+			description: "build args with redefined default value",
+			dockerfile:  copyServerGoBuildArgRedefinedDefaultValue,
+			workspace:   ".",
+			expected:    []string{"Dockerfile", "worker.go"},
+			fetched:     []string{"ubuntu:14.04"},
+		},
+		{
+			description: "override default build arg",
 			dockerfile:  copyServerGoBuildArgDefaultValue,
 			workspace:   ".",
-			buildArgs:   map[string]*string{"FOO": util.StringPtr("server.go")},
+			buildArgs:   map[string]*string{"FOO": util.StringPtr("worker.go")},
+			expected:    []string{"Dockerfile", "worker.go"},
+			fetched:     []string{"ubuntu:14.04"},
+		},
+		{
+			description: "ignore build arg and use default arg value",
+			dockerfile:  copyServerGoBuildArgDefaultValue,
+			workspace:   ".",
+			buildArgs:   map[string]*string{"FOO": nil},
 			expected:    []string{"Dockerfile", "server.go"},
 			fetched:     []string{"ubuntu:14.04"},
 		},

pkg/skaffold/util/util.go

@@ -23,6 +23,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -178,3 +179,26 @@ func RemoveFromSlice(s []string, target string) []string {
 	}
 	return s
 }
+
+// Expand replaces placeholders for a given key with a given value.
+// It supports the ${key} and the $key syntax.
+func Expand(text, key, value string) string {
+	text = strings.Replace(text, "${"+key+"}", value, -1)
+
+	indices := regexp.MustCompile(`\$`+key).FindAllStringIndex(text, -1)
+
+	for i := len(indices) - 1; i >= 0; i-- {
+		from := indices[i][0]
+		to := indices[i][1]
+
+		if to >= len(text) || !isAlphaNum(text[to]) {
+			text = text[0:from] + value + text[to:]
+		}
+	}
+
+	return text
+}
+
+func isAlphaNum(c uint8) bool {
+	return c == '_' || '0' <= c && c <= '9' || 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z'
+}

pkg/skaffold/util/util_test.go

@@ -114,3 +114,64 @@ func TestDefaultConfigFilenameAlternate(t *testing.T) {
 
 	testutil.CheckErrorAndDeepEqual(t, false, err, []byte("foo"), content)
 }
+
+func TestExpand(t *testing.T) {
+	var tests = []struct {
+		description string
+		text        string
+		key         string
+		value       string
+		expected    string
+	}{
+		{
+			description: "${key} syntax",
+			text:        "BEFORE[${key}]AFTER",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "BEFORE[VALUE]AFTER",
+		},
+		{
+			description: "$key syntax",
+			text:        "BEFORE[$key]AFTER",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "BEFORE[VALUE]AFTER",
+		},
+		{
+			description: "replace all",
+			text:        "BEFORE[$key][${key}][$key][${key}]AFTER",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "BEFORE[VALUE][VALUE][VALUE][VALUE]AFTER",
+		},
+		{
+			description: "ignore common prefix",
+			text:        "BEFORE[$key1][${key1}]AFTER",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "BEFORE[$key1][${key1}]AFTER",
+		},
+		{
+			description: "just the ${key} placeholder",
+			text:        "${key}",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "VALUE",
+		},
+		{
+			description: "just the $key placeholder",
+			text:        "$key",
+			key:         "key",
+			value:       "VALUE",
+			expected:    "VALUE",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			actual := Expand(test.text, test.key, test.value)
+
+			testutil.CheckDeepEqual(t, test.expected, actual)
+		})
+	}
+}