Bug 1842982: preserve ARG values that come before the first FROM

pkg/build/builder/common.go

@@ -392,6 +392,21 @@ func addBuildParameters(dir string, build *buildapiv1.Build, sourceInfo *git.Sou
 		return err
 	}
 
+	// Arguments set before the first stage get discarded when we split the
+	// tree up into stages in replaceImagesFromSource.  Other instructions
+	// are preserved.  Preserve the arguments here so that we can prepend
+	// them to the replacement Dockerfile that we generate.
+	var preambleArgs []byte
+	for _, child := range node.Children {
+		if child.Value == dockercmd.Arg {
+			preambleArgs = append(preambleArgs, []byte(child.Original+"\n")...)
+		} else {
+			if child.Value == dockercmd.From {
+				break
+			}
+		}
+	}
+
 	// Update base image if build strategy specifies the From field.
 	if build.Spec.Strategy.DockerStrategy != nil && build.Spec.Strategy.DockerStrategy.From != nil && build.Spec.Strategy.DockerStrategy.From.Kind == "DockerImage" {
 		// Reduce the name to a minimal canonical form for the daemon
@@ -425,19 +440,23 @@ func addBuildParameters(dir string, build *buildapiv1.Build, sourceInfo *git.Sou
 		return err
 	}
 
-	if err := replaceImagesFromSource(node, build.Spec.Source.Images); err != nil {
+	var buildArgs []corev1.EnvVar
+	if build.Spec.Strategy.DockerStrategy != nil {
+		buildArgs = build.Spec.Strategy.DockerStrategy.BuildArgs
+	}
+	if err := replaceImagesFromSource(node, build.Spec.Source.Images, buildArgs); err != nil {
 		return err
 	}
 
-	out := dockerfile.Write(node)
+	out := append(preambleArgs, dockerfile.Write(node)...)
 	log.V(4).Infof("Replacing dockerfile\n%s\nwith:\n%s", string(in), string(out))
 	return overwriteFile(dockerfilePath, out)
 }
 
 // replaceImagesFromSource updates a single or multi-stage Dockerfile with any replacement
 // image sources ('FROM <name>' and 'COPY --from=<name>'). It operates on exact string matches
 // and performs no interpretation of names from the Dockerfile.
-func replaceImagesFromSource(node *parser.Node, imageSources []buildapiv1.ImageSource) error {
+func replaceImagesFromSource(node *parser.Node, imageSources []buildapiv1.ImageSource, buildArgs []corev1.EnvVar) error {
 	replacements := make(map[string]string)
 	for _, image := range imageSources {
 		if image.From.Kind != "DockerImage" || len(image.From.Name) == 0 {
@@ -448,7 +467,11 @@ func replaceImagesFromSource(node *parser.Node, imageSources []buildapiv1.ImageS
 		}
 	}
 	names := make(map[string]string)
-	stages, err := imagebuilder.NewStages(node, imagebuilder.NewBuilder(make(map[string]string)))
+	buildArgMap := make(map[string]string)
+	for _, ba := range buildArgs {
+		buildArgMap[ba.Name] = ba.Value
+	}
+	stages, err := imagebuilder.NewStages(node, imagebuilder.NewBuilder(buildArgMap))
 	if err != nil {
 		return err
 	}
@@ -478,7 +501,7 @@ func replaceImagesFromSource(node *parser.Node, imageSources []buildapiv1.ImageS
 }
 
 // findReferencedImages returns all qualified images referenced by the Dockerfile, or returns an error.
-func findReferencedImages(dockerfilePath string) ([]string, error) {
+func findReferencedImages(dockerfilePath string, buildArgs []corev1.EnvVar) ([]string, error) {
 	if len(dockerfilePath) == 0 {
 		return nil, nil
 	}
@@ -488,7 +511,11 @@ func findReferencedImages(dockerfilePath string) ([]string, error) {
 	}
 	names := make(map[string]string)
 	images := sets.NewString()
-	stages, err := imagebuilder.NewStages(node, imagebuilder.NewBuilder(make(map[string]string)))
+	buildArgMap := make(map[string]string)
+	for _, ba := range buildArgs {
+		buildArgMap[ba.Name] = ba.Value
+	}
+	stages, err := imagebuilder.NewStages(node, imagebuilder.NewBuilder(buildArgMap))
 	if err != nil {
 		return nil, err
 	}

pkg/build/builder/common_test.go

@@ -205,6 +205,30 @@ func Test_addBuildParameters(t *testing.T) {
 				`),
 			want: want{
 				Out: heredoc.Doc(`
+				ARG GOLANG_CONTAINER=golang:latest
+				FROM $GOLANG_CONTAINER
+				RUN echo "hello world"
+				`),
+			},
+		},
+		{
+			// won't actually build: only ARG is allowed before the
+			// first FROM. preserving and then re-prepending ARG
+			// instructions effectively reorders them before any
+			// non-ARG instructions that also come before the first
+			// FROM.
+			original: heredoc.Doc(`
+				ARG GOLANG_CONTAINER=golang:latest
+				LABEL this=error
+				ARG GOLANG_CONTAINER2=golang:1.11
+				FROM $GOLANG_CONTAINER
+				RUN echo "hello world"
+				`),
+			want: want{
+				Out: heredoc.Doc(`
+				ARG GOLANG_CONTAINER=golang:latest
+				ARG GOLANG_CONTAINER2=golang:1.11
+				LABEL this=error
 				FROM $GOLANG_CONTAINER
 				RUN echo "hello world"
 				`),
@@ -628,7 +652,7 @@ func Test_findReferencedImages(t *testing.T) {
 			if _, err := dockerfile.Parse(strings.NewReader(test.original)); err != nil {
 				t.Fatal(err)
 			}
-			images, err := findReferencedImages(f.Name())
+			images, err := findReferencedImages(f.Name(), nil)
 			got := want{
 				Images: images,
 				Err:    err != nil,

pkg/build/builder/docker.go

@@ -79,7 +79,7 @@ func (d *DockerBuilder) Build() error {
 	buildTag := randomBuildTag(d.build.Namespace, d.build.Name)
 	dockerfilePath := getDockerfilePath(buildDir, d.build)
 
-	imageNames, err := findReferencedImages(dockerfilePath)
+	imageNames, err := findReferencedImages(dockerfilePath, d.build.Spec.Strategy.DockerStrategy.BuildArgs)
 	if err != nil {
 		return err
 	}
@@ -95,7 +95,8 @@ func (d *DockerBuilder) Build() error {
 		_, err = d.dockerClient.InspectImage(imageName)
 		if err != nil {
 			if err != docker.ErrNoSuchImage {
-				return err
+				log.V(4).Infof("\nError inspecting image \"%s\": %v, continuing", imageName, err)
+				continue
 			}
 			imageExists = false
 		}