Improve Dockerfile parsing

pkg/skaffold/docker/parse.go

@@ -39,6 +39,11 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+type from struct {
+	image string
+	as    string
+}
+
 // RetrieveImage is overridden for unit testing
 var RetrieveImage = retrieveImage
 
@@ -64,30 +69,116 @@ func ValidateDockerfile(path string) bool {
 }
 
 func expandBuildArgs(nodes []*parser.Node, buildArgs map[string]*string) {
-	var key, value string
+	for i, node := range nodes {
+		if node.Value != command.Arg {
+			continue
+		}
+
+		// build arg's key
+		keyValue := strings.Split(node.Next.Value, "=")
+		key := keyValue[0]
+
+		// build arg's value
+		var value string
+		if buildArgs[key] != nil {
+			value = *buildArgs[key]
+		} else if len(keyValue) > 1 {
+			value = keyValue[1]
+		}
+
+		for _, node := range nodes[i+1:] {
+			// Stop replacements if an arg is redefined with the same key
+			if node.Value == command.Arg && strings.Split(node.Next.Value, "=")[0] == key {
+				break
+			}
+
+			// replace $key with value
+			for curr := node; curr != nil; curr = curr.Next {
+				curr.Value = util.Expand(curr.Value, key, value)
+			}
+		}
+	}
+}
+
+func fromInstructions(nodes []*parser.Node) []from {
+	var list []from
 
+	for _, node := range nodes {
+		if node.Value == command.From {
+			list = append(list, fromInstruction(node))
+		}
+	}
+
+	return list
+}
+
+func fromInstruction(node *parser.Node) from {
+	var as string
+	if next := node.Next.Next; next != nil && strings.ToLower(next.Value) == "as" && next.Next != nil {
+		as = next.Next.Value
+	}
+
+	return from{
+		image: strings.ToLower(node.Next.Value),
+		as:    strings.ToLower(as),
+	}
+}
+
+func onbuildInstructions(nodes []*parser.Node) ([]*parser.Node, error) {
+	var instructions []string
+
+	stages := map[string]bool{}
+	for _, from := range fromInstructions(nodes) {
+		stages[from.as] = true
+
+		if from.image == "scratch" {
+			continue
+		}
+
+		if _, found := stages[from.image]; found {
+			continue
+		}
+
+		logrus.Debugf("Checking base image %s for ONBUILD triggers.", from.image)
+		img, err := RetrieveImage(from.image)
+		if err != nil {
+			logrus.Warnf("Error processing base image for ONBUILD triggers: %s. Dependencies may be incomplete.", err)
+			continue
+		}
+
+		logrus.Debugf("Found ONBUILD triggers %v in image %s", img.Config.OnBuild, from.image)
+		instructions = append(instructions, img.Config.OnBuild...)
+	}
+
+	obRes, err := parser.Parse(strings.NewReader(strings.Join(instructions, "\n")))
+	if err != nil {
+		return nil, errors.Wrap(err, "parsing ONBUILD instructions")
+	}
+
+	return obRes.AST.Children, nil
+}
+
+func copiedFiles(nodes []*parser.Node) ([][]string, error) {
+	var copied [][]string
+
+	envs := map[string]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]
+		case command.Add, command.Copy:
+			files, err := processCopy(node, envs)
+			if err != nil {
+				return nil, err
 			}
-		default:
-			if key != "" {
-				// replace $key with value
-				for curr := node; curr != nil; curr = curr.Next {
-					curr.Value = util.Expand(curr.Value, key, value)
-				}
+
+			if len(files) > 0 {
+				copied = append(copied, files)
 			}
+		case command.Env:
+			envs[node.Next.Value] = node.Next.Next.Value
 		}
 	}
+
+	return copied, nil
 }
 
 func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*string) ([]string, error) {
@@ -104,60 +195,20 @@ func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*s
 
 	expandBuildArgs(res.AST.Children, buildArgs)
 
-	// Then process onbuilds, if present.
-	onbuildsImages := [][]string{}
-	stages := map[string]bool{}
-	for _, value := range res.AST.Children {
-		switch value.Value {
-		case command.From:
-			imageName := value.Next.Value
-			if _, found := stages[imageName]; found {
-				continue
-			}
-
-			next := value.Next.Next
-			if next != nil && strings.ToLower(next.Value) == "as" {
-				if next.Next != nil {
-					stages[next.Next.Value] = true
-				}
-			}
-
-			onbuilds, err := processBaseImage(imageName)
-			if err != nil {
-				logrus.Warnf("Error processing base image for onbuild triggers: %s. Dependencies may be incomplete.", err)
-			}
-			onbuildsImages = append(onbuildsImages, onbuilds)
-		}
+	instructions, err := onbuildInstructions(res.AST.Children)
+	if err != nil {
+		return nil, errors.Wrap(err, "listing ONBUILD instructions")
 	}
 
-	var copied [][]string
-	envs := map[string]string{}
-
-	var dispatchInstructions = func(r *parser.Result) {
-		for _, value := range r.AST.Children {
-			switch value.Value {
-			case command.Add, command.Copy:
-				files, _ := processCopy(value, envs)
-				if len(files) > 0 {
-					copied = append(copied, files)
-				}
-			case command.Env:
-				envs[value.Next.Value] = value.Next.Next.Value
-			}
-		}
-	}
-	for _, image := range onbuildsImages {
-		for _, ob := range image {
-			obRes, err := parser.Parse(strings.NewReader(ob))
-			if err != nil {
-				return nil, err
-			}
-			dispatchInstructions(obRes)
-		}
+	copied, err := copiedFiles(append(instructions, res.AST.Children...))
+	if err != nil {
+		return nil, errors.Wrap(err, "listing copied files")
 	}
 
-	dispatchInstructions(res)
+	return expandPaths(workspace, copied)
+}
 
+func expandPaths(workspace string, copied [][]string) ([]string, error) {
 	expandedPaths := make(map[string]bool)
 	for _, files := range copied {
 		matchesOne := false
@@ -307,21 +358,6 @@ func GetDependencies(workspace string, a *latest.DockerArtifact) ([]string, erro
 	return dependencies, nil
 }
 
-func processBaseImage(baseImageName string) ([]string, error) {
-	if strings.ToLower(baseImageName) == "scratch" {
-		return nil, nil
-	}
-
-	logrus.Debugf("Checking base image %s for ONBUILD triggers.", baseImageName)
-	img, err := RetrieveImage(baseImageName)
-	if err != nil {
-		return nil, err
-	}
-
-	logrus.Debugf("Found onbuild triggers %v in image %s", img.Config.OnBuild, baseImageName)
-	return img.Config.OnBuild, nil
-}
-
 var imageCache sync.Map
 
 func retrieveImage(image string) (*v1.ConfigFile, error) {

pkg/skaffold/docker/parse_test.go

@@ -116,7 +116,7 @@ ARG FOO
 COPY $FOO .
 `
 
-const copyServerGoBuildArgSamePrefix = `
+const copyWorkerGoBuildArgSamePrefix = `
 FROM ubuntu:14.04
 ARG FOO=server.go
 ARG FOO2
@@ -141,16 +141,31 @@ ARG FOO=server.go
 COPY $FOO .
 `
 
-const copyServerGoBuildArgRedefinedDefaultValue = `
+const copyWorkerGoBuildArgRedefinedDefaultValue = `
 FROM ubuntu:14.04
 ARG FOO=server.go
 ARG FOO=worker.go
 COPY $FOO .
 `
 
+const copyServerGoBuildArgsAtTheTop = `
+FROM ubuntu:14.04
+ARG FOO=server.go
+ARG FOO2=ignored
+ARG FOO3=ignored
+COPY $FOO .
+`
+
 const fromStage = `
 FROM ubuntu:14.04 as base
 FROM base as dist
+FROM dist as prod
+`
+
+const fromStageIgnoreCase = `
+FROM ubuntu:14.04 as BASE
+FROM base as dist
+FROM DIST as prod
 `
 
 type fakeImageFetcher struct {
@@ -320,7 +335,7 @@ func TestGetDependencies(t *testing.T) {
 		},
 		{
 			description: "build args with same prefix",
-			dockerfile:  copyServerGoBuildArgSamePrefix,
+			dockerfile:  copyWorkerGoBuildArgSamePrefix,
 			workspace:   ".",
 			buildArgs:   map[string]*string{"FOO2": util.StringPtr("worker.go")},
 			expected:    []string{"Dockerfile", "worker.go"},
@@ -351,11 +366,18 @@ func TestGetDependencies(t *testing.T) {
 		},
 		{
 			description: "build args with redefined default value",
-			dockerfile:  copyServerGoBuildArgRedefinedDefaultValue,
+			dockerfile:  copyWorkerGoBuildArgRedefinedDefaultValue,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "worker.go"},
 			fetched:     []string{"ubuntu:14.04"},
 		},
+		{
+			description: "build args all defined a the top",
+			dockerfile:  copyServerGoBuildArgsAtTheTop,
+			workspace:   ".",
+			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
+		},
 		{
 			description: "override default build arg",
 			dockerfile:  copyServerGoBuildArgDefaultValue,
@@ -377,7 +399,14 @@ func TestGetDependencies(t *testing.T) {
 			dockerfile:  fromStage,
 			workspace:   ".",
 			expected:    []string{"Dockerfile"},
-			fetched:     []string{"ubuntu:14.04"}, // Don't fetch `base`
+			fetched:     []string{"ubuntu:14.04"},
+		},
+		{
+			description: "from base stage, ignoring case",
+			dockerfile:  fromStageIgnoreCase,
+			workspace:   ".",
+			expected:    []string{"Dockerfile"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 	}