Don’t compute onbuild triggers for images that are stage names

pkg/skaffold/docker/context_test.go

@@ -29,10 +29,9 @@ func TestDockerContext(t *testing.T) {
 	tmpDir, cleanup := testutil.NewTempDir(t)
 	defer cleanup()
 
-	RetrieveImage = mockRetrieveImage
-	defer func() {
-		RetrieveImage = retrieveImage
-	}()
+	imageFetcher := fakeImageFetcher{}
+	RetrieveImage = imageFetcher.fetch
+	defer func() { RetrieveImage = retrieveImage }()
 
 	artifact := &v1alpha2.DockerArtifact{
 		DockerfilePath: "Dockerfile",

pkg/skaffold/docker/parse.go

@@ -102,10 +102,23 @@ func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*s
 
 	// Then process onbuilds, if present.
 	onbuildsImages := [][]string{}
+	stages := map[string]bool{}
 	for _, value := range res.AST.Children {
 		switch value.Value {
 		case command.From:
-			onbuilds, err := processBaseImage(value)
+			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)
 			}
@@ -287,18 +300,18 @@ func GetDependencies(workspace string, a *v1alpha2.DockerArtifact) ([]string, er
 	return dependencies, nil
 }
 
-func processBaseImage(value *parser.Node) ([]string, error) {
-	base := value.Next.Value
-	logrus.Debugf("Checking base image %s for ONBUILD triggers.", base)
-	if strings.ToLower(base) == "scratch" {
-		logrus.Debugf("SCRATCH base image found, skipping check: %s", base)
+func processBaseImage(baseImageName string) ([]string, error) {
+	if strings.ToLower(baseImageName) == "scratch" {
 		return nil, nil
 	}
-	img, err := RetrieveImage(base)
+
+	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, base)
+
+	logrus.Debugf("Found onbuild triggers %v in image %s", img.Config.OnBuild, baseImageName)
 	return img.Config.OnBuild, nil
 }
 

pkg/skaffold/docker/parse_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 	"github.com/google/go-containerregistry/pkg/v1"
 )
@@ -140,40 +141,31 @@ COPY $FOO .
 CMD $FOO
 `
 
-var fooArg = "server.go" // used for build args
+const fromStage = `
+FROM ubuntu:14.04 as base
+FROM base as dist
+`
 
-var ImageConfigs = map[string]*v1.ConfigFile{
-	"golang:onbuild": {
-		Config: v1.Config{
-			OnBuild: []string{
-				"COPY . /go/src/app",
-			},
-		},
-	},
-	"golang:1.9.2":           {Config: v1.Config{}},
-	"gcr.io/distroless/base": {Config: v1.Config{}},
-	"ubuntu:14.04":           {Config: v1.Config{}},
-	"nginx":                  {Config: v1.Config{}},
-	"busybox":                {Config: v1.Config{}},
-	"oneport": {
-		Config: v1.Config{
-			ExposedPorts: map[string]struct{}{
-				"8000": {},
-			},
-		}},
-	"severalports": {
-		Config: v1.Config{
-			ExposedPorts: map[string]struct{}{
-				"8000":     {},
-				"8001/tcp": {},
-			},
-		}},
+type fakeImageFetcher struct {
+	fetched []string
 }
 
-func mockRetrieveImage(image string) (*v1.ConfigFile, error) {
-	if cfg, ok := ImageConfigs[image]; ok {
-		return cfg, nil
+func (f *fakeImageFetcher) fetch(image string) (*v1.ConfigFile, error) {
+	f.fetched = append(f.fetched, image)
+
+	switch image {
+	case "ubuntu:14.04", "busybox", "nginx", "golang:1.9.2":
+		return &v1.ConfigFile{}, nil
+	case "golang:onbuild":
+		return &v1.ConfigFile{
+			Config: v1.Config{
+				OnBuild: []string{
+					"COPY . /go/src/app",
+				},
+			},
+		}, nil
 	}
+
 	return nil, fmt.Errorf("No image found for %s", image)
 }
 
@@ -186,6 +178,7 @@ func TestGetDependencies(t *testing.T) {
 		buildArgs   map[string]*string
 
 		expected  []string
+		fetched   []string
 		badReader bool
 		shouldErr bool
 	}{
@@ -194,30 +187,35 @@ func TestGetDependencies(t *testing.T) {
 			dockerfile:  copyServerGo,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "add dependency",
 			dockerfile:  addNginx,
 			workspace:   "docker",
 			expected:    []string{"Dockerfile", "nginx.conf"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "wildcards",
 			dockerfile:  wildcards,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go", "worker.go"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "wildcards matches none",
 			dockerfile:  wildcardsMatchesNone,
 			workspace:   ".",
+			fetched:     []string{"nginx"},
 			shouldErr:   true,
 		},
 		{
 			description: "one wilcard matches none",
 			dockerfile:  oneWilcardMatchesNone,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go", "worker.go"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "bad read",
@@ -229,117 +227,139 @@ func TestGetDependencies(t *testing.T) {
 			description: "no dependencies on remote files",
 			dockerfile:  remoteFileAdd,
 			expected:    []string{"Dockerfile"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "multistage dockerfile",
 			dockerfile:  multiStageDockerfile,
 			workspace:   "",
 			expected:    []string{"Dockerfile", "worker.go"},
+			fetched:     []string{"golang:1.9.2", "gcr.io/distroless/base"},
 		},
 		{
 			description: "copy twice",
 			dockerfile:  multiCopy,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "test.conf"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "env test",
 			dockerfile:  envTest,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "bar"},
+			fetched:     []string{"busybox"},
 		},
 		{
 			description: "multi file copy",
 			dockerfile:  multiFileCopy,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "file", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "dockerignore test",
 			dockerfile:  copyDirectory,
 			ignore:      "bar\ndocker/*",
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "file", "server.go", "test.conf", "worker.go"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "dockerignore dockerfile",
 			dockerfile:  copyServerGo,
 			ignore:      "Dockerfile",
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "dockerignore with non canonical workspace",
 			dockerfile:  contextDockerfile,
 			workspace:   "docker/../docker",
 			ignore:      "bar\ndocker/*",
 			expected:    []string{"Dockerfile", "nginx.conf"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "dockerignore with context in parent directory",
 			dockerfile:  copyDirectory,
 			workspace:   "docker/..",
 			ignore:      "bar\ndocker/*\n*.go",
 			expected:    []string{"Dockerfile", "file", "test.conf"},
+			fetched:     []string{"nginx"},
 		},
 		{
 			description: "onbuild test",
 			dockerfile:  onbuild,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "bar", filepath.Join("docker", "bar"), filepath.Join("docker", "nginx.conf"), "file", "server.go", "test.conf", "worker.go"},
+			fetched:     []string{"golang:onbuild"},
 		},
 		{
 			description: "onbuild error",
 			dockerfile:  onbuildError,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "file"},
+			fetched:     []string{"noimage:latest"},
 		},
 		{
 			description: "build args",
 			dockerfile:  copyServerGoBuildArg,
 			workspace:   ".",
-			buildArgs:   map[string]*string{"FOO": &fooArg},
+			buildArgs:   map[string]*string{"FOO": util.StringPtr("server.go")},
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "build args with curly braces",
 			dockerfile:  copyServerGoBuildArgCurlyBraces,
 			workspace:   ".",
-			buildArgs:   map[string]*string{"FOO": &fooArg},
+			buildArgs:   map[string]*string{"FOO": util.StringPtr("server.go")},
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "build args with extra whitespace",
 			dockerfile:  copyServerGoBuildArgExtraWhitespace,
 			workspace:   ".",
-			buildArgs:   map[string]*string{"FOO": &fooArg},
+			buildArgs:   map[string]*string{"FOO": util.StringPtr("server.go")},
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "build args with default value and buildArgs unset",
 			dockerfile:  copyServerGoBuildArgDefaultValue,
 			workspace:   ".",
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
 		},
 		{
 			description: "build args with default value and buildArgs set",
 			dockerfile:  copyServerGoBuildArgDefaultValue,
 			workspace:   ".",
-			buildArgs:   map[string]*string{"FOO": &fooArg},
+			buildArgs:   map[string]*string{"FOO": util.StringPtr("server.go")},
 			expected:    []string{"Dockerfile", "server.go"},
+			fetched:     []string{"ubuntu:14.04"},
+		},
+		{
+			description: "from base stage",
+			dockerfile:  fromStage,
+			workspace:   ".",
+			expected:    []string{"Dockerfile"},
+			fetched:     []string{"ubuntu:14.04"}, // Don't fetch `base`
 		},
 	}
 
-	RetrieveImage = mockRetrieveImage
-	defer func() {
-		RetrieveImage = retrieveImage
-	}()
-
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
 			tmpDir, cleanup := testutil.NewTempDir(t)
 			defer cleanup()
 
+			imageFetcher := fakeImageFetcher{}
+			RetrieveImage = imageFetcher.fetch
+			defer func() { RetrieveImage = retrieveImage }()
+
 			for _, file := range []string{"docker/nginx.conf", "docker/bar", "server.go", "test.conf", "worker.go", "bar", "file"} {
 				tmpDir.Write(file, "")
 			}
@@ -359,6 +379,7 @@ func TestGetDependencies(t *testing.T) {
 			})
 
 			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, deps)
+			testutil.CheckDeepEqual(t, test.fetched, imageFetcher.fetched)
 		})
 	}
 }