Fixes for run image extension (#1134)

* When pulling remote image data, fail if the remote image is not found

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* When validating dockerfiles, set extend to true if there are any instructions (vs more than one instruction)

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Update matching logic when considering if two image names are equivalent to ignore the digest portion of the reference if present (for the purpose of selecting data from run.toml to add to the lifecycle metadata label i.e., “run image for rebase”)

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Comments and cleanup

Don't print `%!s(<nil>)` if nil is provided to the "parse maybe" function

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* When exporting, continue to use run image identifier (which could be a digest reference or daemon image ID)
instead of falling back to image name when exporting to a daemon.

Previously, the digest reference was incorrect which caused the daemon not to find the image.
But when provided a correct digest reference the daemon can still find it.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Add Contains method to structs that hold run image information for export

When determining if a provided reference is found in existing metadata, remove its digest -
except when setting the new run image "image" in analyzed.toml,
because we should always respect what the extension author wrote.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* When finding the run image info for export, use the run image "image" (name)
in analyzed.toml as the search key, because the run image "reference" could be a daemon image ID
or include the digest, which isn't helpful when retrieving image names that are supposed to float.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Fix detector acceptance and add more logging

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Fix: use "image" instead of "reference" and also guard against image not found
when we are only updating the reference and target data in analyzed.toml

Signed-off-by: Natalie Arellano <narellano@vmware.com>

* Add comment

Signed-off-by: Natalie Arellano <narellano@vmware.com>

---------

Signed-off-by: Natalie Arellano <narellano@vmware.com>

acceptance/detector_test.go

@@ -396,7 +396,7 @@ fail: fail_detect_buildpack@some_version
 			var analyzed files.Analyzed
 			_, err = toml.DecodeFile(foundAnalyzedTOML, &analyzed)
 			h.AssertNil(t, err)
-			h.AssertEq(t, analyzed.RunImage.Reference, "some-run-image-from-extension")
+			h.AssertEq(t, analyzed.RunImage.Image, "some-run-image-from-extension")
 		})
 	})
 

acceptance/testdata/restorer/container/layers/some-extend-false-analyzed.toml.placeholder

@@ -1,3 +1,3 @@
 [run-image]
-  reference = "REPLACE"
+  reference = ""
   image = "REPLACE"

acceptance/testdata/restorer/container/layers/some-extend-true-analyzed.toml.placeholder

@@ -1,4 +1,4 @@
 [run-image]
-  reference = "REPLACE"
+  reference = ""
   extend = true
   image = "REPLACE"

analyzer.go

@@ -242,8 +242,14 @@ func (a *Analyzer) Analyze() (files.Analyzed, error) {
 	}
 
 	return files.Analyzed{
-		PreviousImage:  &files.ImageIdentifier{Reference: previousImageRef},
-		RunImage:       &files.RunImage{Reference: runImageRef, TargetMetadata: atm, Image: runImageName},
+		PreviousImage: &files.ImageIdentifier{
+			Reference: previousImageRef,
+		},
+		RunImage: &files.RunImage{
+			Reference:      runImageRef, // the image identifier, e.g. "s0m3d1g3st" (the image identifier) when exporting to a daemon, or "some.registry/some-repo@sha256:s0m3d1g3st" when exporting to a registry
+			TargetMetadata: atm,
+			Image:          runImageName, // the provided tag, e.g., "some.registry/some-repo:some-tag" if supported by the platform
+		},
 		LayersMetadata: appMeta,
 	}, nil
 }

buildpack/dockerfile.go

@@ -150,10 +150,8 @@ func ValidateRunDockerfile(dInfo *DockerfileInfo, logger log.Logger) error {
 		if stage.BaseName != baseImageArgRef {
 			newBase = stage.BaseName
 		}
-		for idx, command := range stage.Commands {
-			if idx > 0 {
-				extend = true
-			}
+		for _, command := range stage.Commands {
+			extend = true
 			found := false
 			for _, rc := range recommendedCommands {
 				if rc == strings.ToUpper(command.Name()) {

buildpack/dockerfile_test.go

@@ -194,16 +194,19 @@ COPY --from=0 /some-source.txt ./some-dest.txt
 
 		when("run", func() {
 			when("valid", func() {
-				it("succeeds", func() {
+				it("succeeds and sets extend to true in the result", func() {
 					for i, content := range validCases {
 						dockerfileName := fmt.Sprintf("Dockerfile%d", i)
 						dockerfilePath := filepath.Join(tmpDir, dockerfileName)
 						h.AssertNil(t, os.WriteFile(dockerfilePath, []byte(content), 0600))
-						err := buildpack.ValidateRunDockerfile(&buildpack.DockerfileInfo{Path: dockerfilePath}, logger)
+						dInfo := &buildpack.DockerfileInfo{Path: dockerfilePath}
+						err := buildpack.ValidateRunDockerfile(dInfo, logger)
 						if err != nil {
 							t.Fatalf("Error validating Dockerfile %d: %s", i, err)
 						}
 						h.AssertEq(t, len(logHandler.Entries), 0)
+						h.AssertEq(t, dInfo.Extend, true)
+						h.AssertEq(t, dInfo.WithBase, "")
 					}
 				})
 
@@ -218,22 +221,25 @@ FROM ${base_image}
 							h.AssertNil(t, os.WriteFile(dockerfilePath, []byte(preamble+tc.dockerfileContent), 0600))
 							logHandler = memory.New()
 							logger = &log.Logger{Handler: logHandler}
-							err := buildpack.ValidateRunDockerfile(&buildpack.DockerfileInfo{Path: dockerfilePath}, logger)
+							dInfo := &buildpack.DockerfileInfo{Path: dockerfilePath}
+							err := buildpack.ValidateRunDockerfile(dInfo, logger)
 							h.AssertNil(t, err)
 							assertLogEntry(t, logHandler, "run.Dockerfile "+tc.expectedWarning)
+							h.AssertEq(t, dInfo.Extend, true)
+							h.AssertEq(t, dInfo.WithBase, "")
 						}
 					})
 				})
 
 				when("switching the runtime base image", func() {
-					it("returns the new base image", func() {
+					it("sets the new base image in the result", func() {
 						dockerfilePath := filepath.Join(tmpDir, "run.Dockerfile")
 						h.AssertNil(t, os.WriteFile(dockerfilePath, []byte(`FROM some-base-image`), 0600))
 						dInfo := &buildpack.DockerfileInfo{Path: dockerfilePath}
 						err := buildpack.ValidateRunDockerfile(dInfo, logger)
 						h.AssertNil(t, err)
-						h.AssertEq(t, dInfo.WithBase, "some-base-image")
 						h.AssertEq(t, dInfo.Extend, false)
+						h.AssertEq(t, dInfo.WithBase, "some-base-image")
 					})
 				})
 			})

cmd/lifecycle/analyzer.go

@@ -131,6 +131,8 @@ func (a *analyzeCmd) Exec() error {
 	if err != nil {
 		return cmd.FailErrCode(err, a.CodeFor(platform.AnalyzeError), "analyze")
 	}
+	cmd.DefaultLogger.Debugf("Run image info in analyzed metadata is: ")
+	cmd.DefaultLogger.Debugf(encoding.ToJSONMaybe(analyzedMD.RunImage))
 	if err = encoding.WriteTOML(a.AnalyzedPath, analyzedMD); err != nil {
 		return cmd.FailErr(err, "write analyzed")
 	}

cmd/lifecycle/detector.go

@@ -182,6 +182,8 @@ func (d *detectCmd) writeDetectData(group buildpack.Group, plan files.Plan) erro
 
 // writeGenerateData re-outputs the analyzedMD that we read previously, but now we've added the RunImage, if a custom runImage was configured
 func (d *detectCmd) writeGenerateData(analyzedMD files.Analyzed) error {
+	cmd.DefaultLogger.Debugf("Run image info in analyzed metadata is: ")
+	cmd.DefaultLogger.Debugf(encoding.ToJSONMaybe(analyzedMD.RunImage))
 	if err := encoding.WriteTOML(d.AnalyzedPath, analyzedMD); err != nil {
 		return cmd.FailErr(err, "write analyzed metadata")
 	}

cmd/lifecycle/exporter.go

@@ -234,16 +234,6 @@ func (e *exportCmd) export(group buildpack.Group, cacheStore lifecycle.Cache, an
 }
 
 func (e *exportCmd) initDaemonAppImage(analyzedMD files.Analyzed) (imgutil.Image, string, error) {
-	if isDigestRef(e.RunImageRef) {
-		// If extensions were used to extend the runtime base image, the run image reference will contain a digest.
-		// The restorer uses a name reference to pull the image from the registry (because the extender needs a manifest),
-		// and writes a digest reference to analyzed.toml.
-		// For remote images, this works perfectly well.
-		// However for local images, the daemon can't find the image when the reference contains a digest,
-		// so we use image name from analyzed.toml which is the reference written by the extension.
-		e.RunImageRef = analyzedMD.RunImageImage()
-	}
-
 	var opts = []local.ImageOption{
 		local.FromBaseImage(e.RunImageRef),
 	}
@@ -295,14 +285,6 @@ func (e *exportCmd) initDaemonAppImage(analyzedMD files.Analyzed) (imgutil.Image
 	return appImage, runImageID.String(), nil
 }
 
-func isDigestRef(ref string) bool {
-	digest, err := name.NewDigest(ref)
-	if err != nil {
-		return false
-	}
-	return digest.DigestStr() != ""
-}
-
 func toContainerConfig(v1C *v1.Config) *container.Config {
 	return &container.Config{
 		ArgsEscaped:     v1C.ArgsEscaped,
@@ -364,6 +346,7 @@ func (e *exportCmd) initRemoteAppImage(analyzedMD files.Analyzed) (imgutil.Image
 			return nil, "", cmd.FailErr(err, "get extended image config")
 		}
 		if extendedConfig != nil {
+			cmd.DefaultLogger.Debugf("Using config from extensions...")
 			opts = append(opts, remote.WithConfig(extendedConfig))
 		}
 	}

cmd/lifecycle/restorer.go

@@ -11,7 +11,6 @@ import (
 	"github.com/buildpacks/imgutil/layout/sparse"
 	"github.com/buildpacks/imgutil/remote"
 	"github.com/google/go-containerregistry/pkg/authn"
-	"github.com/google/go-containerregistry/pkg/name"
 
 	"github.com/buildpacks/lifecycle"
 	"github.com/buildpacks/lifecycle/auth"
@@ -85,60 +84,43 @@ func (r *restoreCmd) Exec() error {
 	)
 	if analyzedMD, err = files.ReadAnalyzed(r.AnalyzedPath, cmd.DefaultLogger); err == nil {
 		if r.supportsBuildImageExtension() && r.BuildImageRef != "" {
-			cmd.DefaultLogger.Debugf("Pulling builder image metadata...")
-			buildImage, err := r.pullSparse(r.BuildImageRef)
+			cmd.DefaultLogger.Debugf("Pulling builder image metadata for %s...", r.BuildImageRef)
+			remoteBuildImage, err := r.pullSparse(r.BuildImageRef)
 			if err != nil {
-				return cmd.FailErr(err, "read builder image")
+				return cmd.FailErr(err, "pull builder image")
 			}
-			digestRef, err := digestReference(r.BuildImageRef, buildImage)
+			digestRef, err := remoteBuildImage.Identifier()
 			if err != nil {
 				return cmd.FailErr(err, "get digest reference for builder image")
 			}
-			analyzedMD.BuildImage = &files.ImageIdentifier{Reference: digestRef}
+			analyzedMD.BuildImage = &files.ImageIdentifier{Reference: digestRef.String()}
+			cmd.DefaultLogger.Debugf("Adding build image info to analyzed metadata: ")
+			cmd.DefaultLogger.Debugf(encoding.ToJSONMaybe(analyzedMD.BuildImage))
 		}
+		var (
+			remoteRunImage imgutil.Image
+		)
+		runImageName := analyzedMD.RunImageImage() // FIXME: if we have a digest reference available in `Reference` (e.g., in the non-daemon case) we should use it
 		if r.supportsRunImageExtension() && needsPulling(analyzedMD.RunImage) {
-			cmd.DefaultLogger.Debugf("Pulling run image metadata...")
-			runImageRef := analyzedMD.RunImageImage()
-			if runImageRef == "" {
-				runImageRef = analyzedMD.RunImage.Reference // older platforms don't populate Image
-			}
-			runImage, err := r.pullSparse(runImageRef)
-			if err != nil {
-				return cmd.FailErr(err, "read run image")
-			}
-			targetData, err := platform.GetTargetMetadata(runImage)
+			cmd.DefaultLogger.Debugf("Pulling run image metadata for %s...", runImageName)
+			remoteRunImage, err = r.pullSparse(runImageName)
 			if err != nil {
-				return cmd.FailErr(err, "read target data from run image")
+				return cmd.FailErr(err, "pull run image")
 			}
-			digestRef, err := digestReference(runImageRef, runImage)
-			if err != nil {
-				return cmd.FailErr(err, "get digest reference for builder image")
-			}
-			analyzedMD.RunImage = &files.RunImage{
-				Reference:      digestRef,
-				Image:          analyzedMD.RunImageImage(),
-				Extend:         true,
-				TargetMetadata: targetData,
+			// update analyzed metadata, even if we only needed to pull the image metadata, because
+			// the extender needs a digest reference in analyzed.toml,
+			// and daemon images will only have a daemon image ID
+			if err = updateAnalyzedMD(&analyzedMD, remoteRunImage); err != nil {
+				return cmd.FailErr(err, "update analyzed metadata")
 			}
 		} else if r.supportsTargetData() && needsUpdating(analyzedMD.RunImage) {
-			cmd.DefaultLogger.Debugf("Updating analyzed metadata...")
-			runImage, err := remote.NewImage(analyzedMD.RunImage.Reference, r.keychain)
-			if err != nil {
-				return cmd.FailErr(err, "read run image")
-			}
-			targetData, err := platform.GetTargetMetadata(runImage)
-			if err != nil {
-				return cmd.FailErr(err, "read target data from run image")
+			cmd.DefaultLogger.Debugf("Updating run image info in analyzed metadata...")
+			remoteRunImage, err = remote.NewImage(runImageName, r.keychain)
+			if err != nil || !remoteRunImage.Found() {
+				return cmd.FailErr(err, "pull run image")
 			}
-			digestRef, err := digestReference(analyzedMD.RunImage.Reference, runImage)
-			if err != nil {
-				return cmd.FailErr(err, "get digest reference for builder image")
-			}
-			analyzedMD.RunImage = &files.RunImage{
-				Reference:      digestRef,
-				Image:          analyzedMD.RunImageImage(),
-				Extend:         analyzedMD.RunImage.Extend,
-				TargetMetadata: targetData,
+			if err = updateAnalyzedMD(&analyzedMD, remoteRunImage); err != nil {
+				return cmd.FailErr(err, "update analyzed metadata")
 			}
 		}
 		if err = encoding.WriteTOML(r.AnalyzedPath, analyzedMD); err != nil {
@@ -169,20 +151,47 @@ func (r *restoreCmd) Exec() error {
 	return r.restore(appMeta, group, cacheStore)
 }
 
+func updateAnalyzedMD(analyzedMD *files.Analyzed, remoteRunImage imgutil.Image) error {
+	digestRef, err := remoteRunImage.Identifier()
+	if err != nil {
+		return cmd.FailErr(err, "get digest reference for run image")
+	}
+	targetData, err := platform.GetTargetMetadata(remoteRunImage)
+	if err != nil {
+		return cmd.FailErr(err, "read target data from run image")
+	}
+	cmd.DefaultLogger.Debugf("Run image info in analyzed metadata was: ")
+	cmd.DefaultLogger.Debugf(encoding.ToJSONMaybe(analyzedMD.RunImage))
+	analyzedMD.RunImage.Reference = digestRef.String()
+	analyzedMD.RunImage.TargetMetadata = targetData
+	cmd.DefaultLogger.Debugf("Run image info in analyzed metadata is: ")
+	cmd.DefaultLogger.Debugf(encoding.ToJSONMaybe(analyzedMD.RunImage))
+	return nil
+}
+
 func needsPulling(runImage *files.RunImage) bool {
-	return runImage != nil && runImage.Extend
+	if runImage == nil {
+		// sanity check to prevent panic, should be unreachable
+		return false
+	}
+	return runImage.Extend
 }
 
 func needsUpdating(runImage *files.RunImage) bool {
 	if runImage == nil {
+		// sanity check to prevent panic, should be unreachable
 		return false
 	}
-	if runImage.TargetMetadata != nil && runImage.TargetMetadata.OS != "" {
+	if runImage.Reference != "" && isPopulated(runImage.TargetMetadata) {
 		return false
 	}
 	return true
 }
 
+func isPopulated(metadata *files.TargetMetadata) bool {
+	return metadata != nil && metadata.OS != ""
+}
+
 func (r *restoreCmd) supportsBuildImageExtension() bool {
 	return r.PlatformAPI.AtLeast("0.10")
 }
@@ -201,9 +210,12 @@ func (r *restoreCmd) pullSparse(imageRef string) (imgutil.Image, error) {
 		return nil, fmt.Errorf("failed to create cache directory: %w", err)
 	}
 	// get remote image
-	remoteImage, err := remote.NewV1Image(imageRef, r.keychain)
+	remoteImage, err := remote.NewImage(imageRef, r.keychain, remote.FromBaseImage(imageRef))
 	if err != nil {
-		return nil, fmt.Errorf("failed to get remote image: %w", err)
+		return nil, fmt.Errorf("failed to initialize remote image: %w", err)
+	}
+	if !remoteImage.Found() {
+		return nil, fmt.Errorf("failed to get remote image")
 	}
 	// check for usable kaniko dir
 	if _, err := os.Stat(kanikoDir); err != nil {
@@ -213,13 +225,15 @@ func (r *restoreCmd) pullSparse(imageRef string) (imgutil.Image, error) {
 		return nil, nil
 	}
 	// save to disk
-	h, err := remoteImage.Digest()
+	h, err := remoteImage.UnderlyingImage().Digest()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get remote image digest: %w", err)
 	}
+	path := filepath.Join(baseCacheDir, h.String())
+	cmd.DefaultLogger.Debugf("Saving image metadata to %s...", path)
 	sparseImage, err := sparse.NewImage(
-		filepath.Join(baseCacheDir, h.String()),
-		remoteImage,
+		path,
+		remoteImage.UnderlyingImage(),
 		layout.WithMediaTypes(imgutil.DefaultTypes),
 	)
 	if err != nil {
@@ -228,32 +242,7 @@ func (r *restoreCmd) pullSparse(imageRef string) (imgutil.Image, error) {
 	if err = sparseImage.Save(); err != nil {
 		return nil, fmt.Errorf("failed to save sparse image: %w", err)
 	}
-	return sparseImage, nil
-}
-
-func digestReference(imageRef string, image imgutil.Image) (string, error) {
-	ir, err := name.ParseReference(imageRef)
-	if err != nil {
-		return "", err
-	}
-	_, err = name.NewDigest(ir.String())
-	if err == nil {
-		// if we already have a digest reference, return it
-		return imageRef, nil
-	}
-	id, err := image.Identifier()
-	if err != nil {
-		return "", err
-	}
-	digest, err := name.NewDigest(id.String())
-	if err != nil {
-		return "", err
-	}
-	digestRef, err := name.NewDigest(fmt.Sprintf("%s@%s", ir.Context().Name(), digest.DigestStr()), name.WeakValidation)
-	if err != nil {
-		return "", err
-	}
-	return digestRef.String(), nil
+	return remoteImage, nil
 }
 
 func (r *restoreCmd) restoresLayerMetadata() bool {