Merge pull request #51629 from vvoland/c8d-fix-images

c8d/inspect: Fix image inspect for incomplete images

daemon/containerd/image_inspect.go

@@ -2,6 +2,7 @@ package containerd
 
 import (
 	"context"
+	"fmt"
 	"sync/atomic"
 	"time"
 
@@ -63,13 +64,6 @@ func (i *ImageService) ImageInspect(ctx context.Context, refOrID string, opts im
 		}
 	}
 
-	var img dockerspec.DockerOCIImage
-	if multi.Best != nil {
-		if err := multi.Best.ReadConfig(ctx, &img); err != nil {
-			return nil, err
-		}
-	}
-
 	parent, err := i.getImageLabelByDigest(ctx, target.Digest, imageLabelClassicBuilderParent)
 	if err != nil {
 		log.G(ctx).WithError(err).Warn("failed to determine Parent property")
@@ -104,29 +98,36 @@ func (i *ImageService) ImageInspect(ctx context.Context, refOrID string, opts im
 		GraphDriverLegacy: &storage.DriverData{Name: i.snapshotter},
 	}
 
+	var img dockerspec.DockerOCIImage
 	if multi.Best != nil {
-		imgConfig := img.Config
-		resp.Author = img.Author
-		resp.Config = &imgConfig
-		resp.Architecture = img.Architecture
-		resp.Variant = img.Variant
-		resp.Os = img.OS
-		resp.OsVersion = img.OSVersion
+		if err := multi.Best.ReadConfig(ctx, &img); err != nil && !cerrdefs.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to read image config: %w", err)
+		}
+	}
 
-		if len(img.History) > 0 {
-			resp.Comment = img.History[len(img.History)-1].Comment
-		}
+	// Copy the config
+	imgConfig := img.Config
+	resp.Config = &imgConfig
 
-		if img.Created != nil {
-			resp.Created = img.Created.Format(time.RFC3339Nano)
-		}
+	resp.Author = img.Author
+	resp.Architecture = img.Architecture
+	resp.Variant = img.Variant
+	resp.Os = img.OS
+	resp.OsVersion = img.OSVersion
 
-		resp.RootFS = imagetypes.RootFS{
-			Type: img.RootFS.Type,
-		}
-		for _, layer := range img.RootFS.DiffIDs {
-			resp.RootFS.Layers = append(resp.RootFS.Layers, layer.String())
-		}
+	if len(img.History) > 0 {
+		resp.Comment = img.History[len(img.History)-1].Comment
+	}
+
+	if img.Created != nil {
+		resp.Created = img.Created.Format(time.RFC3339Nano)
+	}
+
+	resp.RootFS = imagetypes.RootFS{
+		Type: img.RootFS.Type,
+	}
+	for _, layer := range img.RootFS.DiffIDs {
+		resp.RootFS.Layers = append(resp.RootFS.Layers, layer.String())
 	}
 
 	return resp, nil

daemon/containerd/image_inspect_test.go

@@ -61,6 +61,32 @@ func TestImageInspect(t *testing.T) {
 		}
 	})
 
+	t.Run("inspect image with one layer missing", func(t *testing.T) {
+		ctx := logtest.WithT(ctx, t)
+		service := fakeImageService(t, ctx, cs)
+
+		img := toContainerdImage(t, specialimage.MultiLayer)
+
+		_, err := service.images.Create(ctx, img)
+		assert.NilError(t, err)
+
+		// Get the manifest to access the layers
+		mfst, err := c8dimages.Manifest(ctx, cs, img.Target, nil)
+		assert.NilError(t, err)
+		assert.Check(t, len(mfst.Layers) > 0, "image should have at least one layer")
+
+		// Delete the last layer from the content store
+		lastLayer := mfst.Layers[len(mfst.Layers)-1]
+		err = cs.Delete(ctx, lastLayer.Digest)
+		assert.NilError(t, err)
+
+		inspect, err := service.ImageInspect(ctx, img.Name, imagebackend.ImageInspectOpts{})
+		assert.NilError(t, err)
+
+		assert.Check(t, inspect.Config != nil)
+		assert.Check(t, is.Len(inspect.RootFS.Layers, len(mfst.Layers)))
+	})
+
 	t.Run("inspect image with platform parameter", func(t *testing.T) {
 		ctx := logtest.WithT(ctx, t)
 		service := fakeImageService(t, ctx, cs)

daemon/containerd/image_list.go

@@ -305,34 +305,33 @@ func (i *ImageService) multiPlatformSummary(ctx context.Context, img c8dimages.I
 			mfstSummary.ImageData.Platform = *target.Platform
 		}
 
-		if !available {
-			return nil
-		}
-
 		var dockerImage dockerspec.DockerOCIImage
-		if err := img.ReadConfig(ctx, &dockerImage); err != nil {
+		if err := img.ReadConfig(ctx, &dockerImage); err != nil && !cerrdefs.IsNotFound(err) {
 			logger.WithError(err).Warn("failed to read image config")
-			return nil
 		}
 
-		if target.Platform == nil {
-			mfstSummary.ImageData.Platform = dockerImage.Platform
+		if dockerImage.Platform.OS != "" {
+			if target.Platform == nil {
+				mfstSummary.ImageData.Platform = dockerImage.Platform
+			}
+			logger = logger.WithField("platform", mfstSummary.ImageData.Platform)
 		}
-		logger = logger.WithField("platform", mfstSummary.ImageData.Platform)
 
-		chainIDs := identity.ChainIDs(dockerImage.RootFS.DiffIDs)
+		if dockerImage.RootFS.DiffIDs != nil {
+			chainIDs := identity.ChainIDs(dockerImage.RootFS.DiffIDs)
 
-		snapshotUsage, err := img.SnapshotUsage(ctx, i.snapshotterService(i.snapshotter))
-		if err != nil {
-			logger.WithFields(log.Fields{"error": err}).Warn("failed to determine platform specific unpacked size")
+			snapshotUsage, err := img.SnapshotUsage(ctx, i.snapshotterService(i.snapshotter))
+			if err != nil {
+				logger.WithFields(log.Fields{"error": err}).Warn("failed to determine platform specific unpacked size")
+			}
+			unpackedSize := snapshotUsage.Size
+
+			mfstSummary.ImageData.Size.Unpacked = unpackedSize
+			mfstSummary.Size.Total += unpackedSize
+			summary.TotalSize += unpackedSize
+
+			summary.AllChainIDs = append(summary.AllChainIDs, chainIDs...)
 		}
-		unpackedSize := snapshotUsage.Size
-
-		mfstSummary.ImageData.Size.Unpacked = unpackedSize
-		mfstSummary.Size.Total += unpackedSize
-		summary.TotalSize += unpackedSize
-
-		summary.AllChainIDs = append(summary.AllChainIDs, chainIDs...)
 
 		for _, c := range i.containers.List() {
 			if c.ImageManifest != nil && c.ImageManifest.Digest == target.Digest {
@@ -360,8 +359,9 @@ func (i *ImageService) multiPlatformSummary(ctx context.Context, img c8dimages.I
 	if err != nil {
 		if errors.Is(err, errNotManifestOrIndex) {
 			log.G(ctx).WithFields(log.Fields{
-				"error": err,
-				"image": img.Name,
+				"error":      err,
+				"image":      img.Name,
+				"descriptor": img.Target,
 			}).Warn("unexpected image target (neither a manifest nor index)")
 		} else {
 			return nil, err
@@ -443,15 +443,6 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 		}
 	}
 
-	cfgDesc, err := imageManifest.Image.Config(ctx)
-	if err != nil {
-		return nil, err
-	}
-	var cfg configLabels
-	if err := readJSON(ctx, contentStore, cfgDesc, &cfg); err != nil {
-		return nil, err
-	}
-
 	var unpackedSize int64
 	if snapshotUsage, err := imageManifest.SnapshotUsage(ctx, i.snapshotterService(i.snapshotter)); err != nil {
 		log.G(ctx).WithFields(log.Fields{"image": imageManifest.Name(), "error": err}).Warn("failed to calculate unpacked size of image")
@@ -474,7 +465,6 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 		RepoDigests: repoDigests,
 		RepoTags:    repoTags,
 		Size:        totalSize,
-		Labels:      cfg.Config.Labels,
 		// -1 indicates that the value has not been set (avoids ambiguity
 		// between 0 (default) and "not set". We cannot use a pointer (nil)
 		// for this, as the JSON representation uses "omitempty", which would
@@ -482,9 +472,25 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
 		SharedSize: -1,
 		Containers: -1,
 	}
+
+	var cfg configLabels
+	if err := imageManifest.ReadConfig(ctx, &cfg); err != nil {
+		if !cerrdefs.IsNotFound(err) {
+			log.G(ctx).WithFields(log.Fields{
+				"image": imageManifest.Name(),
+				"error": err,
+			}).Warn("failed to read image config")
+		}
+	}
+
 	if cfg.Created != nil {
 		summary.Created = cfg.Created.Unix()
 	}
+	if cfg.Config.Labels != nil {
+		summary.Labels = cfg.Config.Labels
+	} else {
+		summary.Labels = map[string]string{}
+	}
 
 	return summary, nil
 }

integration/image/inspect_test.go

@@ -82,6 +82,74 @@ func TestImageInspectDescriptor(t *testing.T) {
 	assert.Check(t, inspect.Descriptor.Size > 0)
 }
 
+// Regression test for: https://github.com/moby/moby/issues/51566
+//
+// This can be reproduced with two image that share the same uncompressed layer
+// but have a different compressed blob is pulled.
+//
+// Example:
+// ```
+// docker pull nginx@sha256:3b7732505933ca591ce4a6d860cb713ad96a3176b82f7979a8dfa9973486a0d6
+// docker pull gotenberg/gotenberg@sha256:b116a40a1c24917e2bf3e153692da5acd2e78e7cd67e1b2d243b47c178f31c90
+// ```
+//
+// In this case, it's the base debian trixie image that's used as a base.
+// They're effectively the same layer (unpacked diff ID
+// `sha256:1d46119d249f7719e1820e24a311aa7c453f166f714969cffe89504678eaa447`),
+// but different compressed blobs:
+//
+// # nginx
+// {
+// "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
+// "size": 29777766,
+// "digest": "sha256:8c7716127147648c1751940b9709b6325f2256290d3201662eca2701cadb2cdf"
+// }
+//
+// # gotenberg
+// {
+// "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+// "size": 30781333,
+// "digest": "sha256:b96413fb491a5ed179bb2746ff3be6cbddd72e14c6503bea80d58e579a3b92bc"
+// },
+func TestImageInspectWithoutSomeBlobs(t *testing.T) {
+	t.Skip("TODO(vvoland): Come up with minimal images for this test")
+
+	skip.If(t, testEnv.DaemonInfo.OSType != "linux", "The test images are Linux-only")
+
+	ctx := setupTest(t)
+	apiClient := testEnv.APIClient()
+
+	const baseImage = "nginx@sha256:3b7732505933ca591ce4a6d860cb713ad96a3176b82f7979a8dfa9973486a0d6"
+	const childImage = "gotenberg/gotenberg:8.24@sha256:b116a40a1c24917e2bf3e153692da5acd2e78e7cd67e1b2d243b47c178f31c90"
+
+	// Pull the base image first and then the child image
+	for _, image := range []string{baseImage, childImage} {
+		rdr, err := apiClient.ImagePull(ctx, image, client.ImagePullOptions{})
+		assert.NilError(t, err)
+		assert.NilError(t, rdr.Wait(ctx))
+
+		t.Cleanup(func() {
+			_, _ = apiClient.ImageRemove(ctx, image, client.ImageRemoveOptions{})
+		})
+	}
+
+	var raw bytes.Buffer
+	inspect, err := apiClient.ImageInspect(ctx, childImage, client.ImageInspectWithRawResponse(&raw))
+	assert.NilError(t, err)
+
+	var rawJson map[string]any
+	err = json.Unmarshal(raw.Bytes(), &rawJson)
+	assert.NilError(t, err)
+
+	configVal, hasConfig := rawJson["Config"]
+	assert.Check(t, hasConfig, "Config field should exist in JSON response")
+	if assert.Check(t, configVal != nil, "Config should not be null in JSON response") {
+		assert.Check(t, is.DeepEqual(inspect.Config.Cmd, []string{"gotenberg"}))
+		assert.Check(t, inspect.Os != "")
+		assert.Check(t, inspect.Architecture != "")
+	}
+}
+
 func TestImageInspectWithPlatform(t *testing.T) {
 	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "The test image is a Linux image")
 	ctx := setupTest(t)