Merge pull request #267 from buildpacks/fixes

Fixes for imgutil from recent refactor

cnb_image.go

@@ -10,7 +10,6 @@ import (
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/google/go-containerregistry/pkg/v1/tarball"
-	"github.com/google/go-containerregistry/pkg/v1/validate"
 )
 
 // CNBImageCore wraps a v1.Image and provides most of the methods necessary for the image to satisfy the Image interface.
@@ -184,11 +183,6 @@ func (i *CNBImageCore) UnderlyingImage() v1.Image {
 	return i.Image
 }
 
-func (i *CNBImageCore) Valid() bool {
-	err := validate.Image(i.Image)
-	return err == nil
-}
-
 // TBD Deprecated: Variant
 func (i *CNBImageCore) Variant() (string, error) {
 	configFile, err := getConfigFile(i.Image)

layout/layout.go

@@ -66,6 +66,11 @@ func (i *Image) Identifier() (imgutil.Identifier, error) {
 	return newLayoutIdentifier(i.repoPath, hash)
 }
 
+func (i *Image) Valid() bool {
+	// layout images may be invalid if they are missing layer data
+	return true
+}
+
 func (i *Image) Delete() error {
 	return os.RemoveAll(i.repoPath)
 }

local/local.go

@@ -46,6 +46,11 @@ func (i *Image) Identifier() (imgutil.Identifier, error) {
 	}, nil
 }
 
+func (i *Image) Valid() bool {
+	// local images are actually always invalid, because they are missing a manifest, but let's ignore that for now
+	return true
+}
+
 // GetLayer returns an io.ReadCloser with uncompressed layer data.
 // The layer will always have data, even if that means downloading ALL the image layers from the daemon.
 func (i *Image) GetLayer(diffID string) (io.ReadCloser, error) {

local/local_test.go

@@ -2101,18 +2101,20 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
 		})
 
 		when("previous image is configured and layers are reused", func() {
+			var prevImageName string
+
 			it.Before(func() {
 				var err error
 
-				prevImg, err := local.NewImage(newTestImageName(), dockerClient)
+				prevImageName = newTestImageName()
+				prevImg, err := local.NewImage(prevImageName, dockerClient)
 				h.AssertNil(t, err)
 
 				prevImgBase, err := h.CreateSingleFileLayerTar("/root", "root", daemonOS)
 				h.AssertNil(t, err)
 
 				h.AssertNil(t, prevImg.AddLayer(prevImgBase))
 				h.AssertNil(t, prevImg.Save())
-				defer h.DockerRmi(dockerClient, prevImg.Name())
 
 				img, err = local.NewImage(repoName, dockerClient, local.WithPreviousImage(prevImg.Name()))
 				h.AssertNil(t, err)
@@ -2131,6 +2133,10 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			it("creates an archive that can be imported and has correct diffIDs", saveFileTest)
+
+			it.After(func() {
+				defer h.DockerRmi(dockerClient, prevImageName)
+			})
 		})
 
 		when("base image is configured", func() {

local/store.go

@@ -216,24 +216,10 @@ func (s *Store) addImageToTar(tw *tar.Writer, image v1.Image, withName string) e
 		blankIdx   int
 	)
 	for _, layer := range layers {
-		var layerName string
-		size, err := layer.Size()
+		layerName, err := s.addLayerToTar(tw, layer, &blankIdx)
 		if err != nil {
 			return err
 		}
-		if size == -1 { // layer facade fronting empty layer
-			layerName = fmt.Sprintf("blank_%d", blankIdx)
-			blankIdx++
-			hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0}
-			if err := tw.WriteHeader(hdr); err != nil {
-				return err
-			}
-		} else {
-			layerName, err = s.addLayerToTar(tw, layer)
-			if err != nil {
-				return err
-			}
-		}
 		layerPaths = append(layerPaths, layerName)
 	}
 
@@ -250,32 +236,46 @@ func (s *Store) addImageToTar(tw *tar.Writer, image v1.Image, withName string) e
 	return addTextToTar(tw, manifestJSON, "manifest.json")
 }
 
-func (s *Store) addLayerToTar(tw *tar.Writer, layer v1.Layer) (string, error) {
-	layerDiffID, err := layer.DiffID()
+func (s *Store) addLayerToTar(tw *tar.Writer, layer v1.Layer, blankIdx *int) (string, error) {
+	// If the layer is a previous image layer that hasn't been downloaded yet,
+	// cause ALL the previous image layers to be downloaded by grabbing the ReadCloser.
+	layerReader, err := layer.Uncompressed()
 	if err != nil {
 		return "", err
 	}
-	withName := fmt.Sprintf("/%s.tar", layerDiffID.String())
+	defer layerReader.Close()
 
-	uncompressedSize, err := s.getLayerSize(layer)
+	var layerName string
+	size, err := layer.Size()
 	if err != nil {
 		return "", err
 	}
-	hdr := &tar.Header{Name: withName, Mode: 0644, Size: uncompressedSize}
-	if err = tw.WriteHeader(hdr); err != nil {
+	if size == -1 { // it's a base (always empty) layer
+		layerName = fmt.Sprintf("blank_%d", blankIdx)
+		*blankIdx++
+		hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0}
+		return layerName, tw.WriteHeader(hdr)
+	}
+	// it's a populated layer
+	layerDiffID, err := layer.DiffID()
+	if err != nil {
 		return "", err
 	}
+	layerName = fmt.Sprintf("/%s.tar", layerDiffID.String())
 
-	layerReader, err := layer.Uncompressed()
+	uncompressedSize, err := s.getLayerSize(layer)
 	if err != nil {
 		return "", err
 	}
-	defer layerReader.Close()
+	hdr := &tar.Header{Name: layerName, Mode: 0644, Size: uncompressedSize}
+	if err = tw.WriteHeader(hdr); err != nil {
+		return "", err
+	}
 	if _, err = io.Copy(tw, layerReader); err != nil {
 		return "", err
 	}
 
-	return withName, nil
+	return layerName, nil
 }
 
 // getLayerSize returns the uncompressed layer size.
@@ -405,7 +405,7 @@ func (s *Store) doDownloadLayersFor(identifier string) error {
 
 	imageReader, err := s.dockerClient.ImageSave(ctx, []string{identifier})
 	if err != nil {
-		return fmt.Errorf("saving base image with ID %q from the docker daemon: %w", identifier, err)
+		return fmt.Errorf("saving image with ID %q from the docker daemon: %w", identifier, err)
 	}
 	defer ensureReaderClosed(imageReader)
 

local/v1_facade.go

@@ -182,9 +182,9 @@ func toV1Config(dockerCfg *container.Config) v1.Config {
 var _ v1.Layer = &v1LayerFacade{}
 
 type v1LayerFacade struct {
-	diffID       v1.Hash
-	uncompressed func() (io.ReadCloser, error)
-	size         func() (int64, error)
+	diffID           v1.Hash
+	uncompressed     func() (io.ReadCloser, error)
+	uncompressedSize func() (int64, error)
 }
 
 func newEmptyLayer(diffID v1.Hash, store *Store) *v1LayerFacade {
@@ -197,7 +197,7 @@ func newEmptyLayer(diffID v1.Hash, store *Store) *v1LayerFacade {
 			}
 			return io.NopCloser(bytes.NewReader([]byte{})), nil
 		},
-		size: func() (int64, error) {
+		uncompressedSize: func() (int64, error) {
 			layer, err := store.LayerByDiffID(diffID)
 			if err == nil {
 				return layer.Size()
@@ -224,24 +224,17 @@ func newDownloadableEmptyLayer(diffID v1.Hash, store *Store, imageID string) *v1
 			}
 			return nil, err
 		},
-		size: func() (int64, error) {
+		uncompressedSize: func() (int64, error) {
 			layer, err := store.LayerByDiffID(diffID)
 			if err == nil {
 				return layer.Size()
 			}
-			if err = store.downloadLayersFor(imageID); err != nil {
-				return -1, err
-			}
-			layer, err = store.LayerByDiffID(diffID)
-			if err == nil {
-				return layer.Size()
-			}
-			return -1, err
+			return -1, nil
 		},
 	}
 }
 
-func newPopulatedLayer(diffID v1.Hash, fromPath string, sentinelSize int64) *v1LayerFacade {
+func newPopulatedLayer(diffID v1.Hash, fromPath string, uncompressedSize int64) *v1LayerFacade {
 	return &v1LayerFacade{
 		diffID: diffID,
 		uncompressed: func() (io.ReadCloser, error) {
@@ -251,8 +244,8 @@ func newPopulatedLayer(diffID v1.Hash, fromPath string, sentinelSize int64) *v1L
 			}
 			return f, nil
 		},
-		size: func() (int64, error) {
-			return sentinelSize, nil
+		uncompressedSize: func() (int64, error) {
+			return uncompressedSize, nil
 		},
 	}
 }
@@ -285,9 +278,10 @@ func (l *v1LayerFacade) Uncompressed() (io.ReadCloser, error) {
 	return l.uncompressed()
 }
 
-// Size returns a sentinel value indicating if the layer has data.
+// Size returns the uncompressed size.
+// If the layer is missing local data, it returns a sentinel value of -1.
 func (l *v1LayerFacade) Size() (int64, error) {
-	return l.size()
+	return l.uncompressedSize()
 }
 
 func (l *v1LayerFacade) MediaType() (v1types.MediaType, error) {

remote/remote.go

@@ -74,10 +74,6 @@ func (i *Image) Identifier() (imgutil.Identifier, error) {
 }
 
 // Valid returns true if the (saved) image is valid.
-// It differs from CNBImageCore.Valid() in that the latter validates the current working image (not what is already saved).
-// Additionally, when the repoName for the image is a manifest list, this method validates the entire index.
-// Finally, this method uses validate.Fast, whereas CNBImageCore.Valid() does not.
-// FIXME: see if this can be combined with CNBImageCore.Valid()
 func (i *Image) Valid() bool {
 	return i.valid() == nil
 }