Add delete target to image remove

Adds atomicity to image delete when deleting from a list.

Signed-off-by: Derek McGowan <derek@mcg.dev>

image_store.go

@@ -20,14 +20,12 @@ import (
 	"context"
 
 	imagesapi "github.com/containerd/containerd/api/services/images/v1"
-	"github.com/containerd/containerd/api/types"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/images"
+	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/pkg/epoch"
 	"github.com/containerd/containerd/protobuf"
 	ptypes "github.com/containerd/containerd/protobuf/types"
-	"github.com/opencontainers/go-digest"
-	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
@@ -108,11 +106,14 @@ func (s *remoteImages) Delete(ctx context.Context, name string, opts ...images.D
 			return err
 		}
 	}
-	_, err := s.client.Delete(ctx, &imagesapi.DeleteImageRequest{
+	req := &imagesapi.DeleteImageRequest{
 		Name: name,
 		Sync: do.Synchronous,
-	})
+	}
-
+	if do.Target != nil {
+		req.Target = oci.DescriptorToProto(*do.Target)
+	}
+	_, err := s.client.Delete(ctx, req)
 	return errdefs.FromGRPC(err)
 }
 
@@ -120,7 +121,7 @@ func imageToProto(image *images.Image) *imagesapi.Image {
 	return &imagesapi.Image{
 		Name:      image.Name,
 		Labels:    image.Labels,
-		Target:    descToProto(&image.Target),
+		Target:    oci.DescriptorToProto(image.Target),
 		CreatedAt: protobuf.ToTimestamp(image.CreatedAt),
 		UpdatedAt: protobuf.ToTimestamp(image.UpdatedAt),
 	}
@@ -130,7 +131,7 @@ func imageFromProto(imagepb *imagesapi.Image) images.Image {
 	return images.Image{
 		Name:      imagepb.Name,
 		Labels:    imagepb.Labels,
-		Target:    descFromProto(imagepb.Target),
+		Target:    oci.DescriptorFromProto(imagepb.Target),
 		CreatedAt: protobuf.FromTimestamp(imagepb.CreatedAt),
 		UpdatedAt: protobuf.FromTimestamp(imagepb.UpdatedAt),
 	}
@@ -146,21 +147,3 @@ func imagesFromProto(imagespb []*imagesapi.Image) []images.Image {
 
 	return images
 }
-
-func descFromProto(desc *types.Descriptor) ocispec.Descriptor {
-	return ocispec.Descriptor{
-		MediaType:   desc.MediaType,
-		Size:        desc.Size,
-		Digest:      digest.Digest(desc.Digest),
-		Annotations: desc.Annotations,
-	}
-}
-
-func descToProto(desc *ocispec.Descriptor) *types.Descriptor {
-	return &types.Descriptor{
-		MediaType:   desc.MediaType,
-		Size:        desc.Size,
-		Digest:      desc.Digest.String(),
-		Annotations: desc.Annotations,
-	}
-}

images/image.go

@@ -58,6 +58,7 @@ type Image struct {
 // DeleteOptions provide options on image delete
 type DeleteOptions struct {
 	Synchronous bool
+	Target      *ocispec.Descriptor
 }
 
 // DeleteOpt allows configuring a delete operation
@@ -72,6 +73,16 @@ func SynchronousDelete() DeleteOpt {
 	}
 }
 
+// DeleteTarget is used to specify the target value an image is expected
+// to have when deleting. If the image has a different target, then
+// NotFound is returned.
+func DeleteTarget(target *ocispec.Descriptor) DeleteOpt {
+	return func(ctx context.Context, o *DeleteOptions) error {
+		o.Target = target
+		return nil
+	}
+}
+
 // Store and interact with images
 type Store interface {
 	Get(ctx context.Context, name string) (Image, error)

metadata/images.go

@@ -266,6 +266,13 @@ func (s *imageStore) Delete(ctx context.Context, name string, opts ...images.Del
 		return err
 	}
 
+	var options images.DeleteOptions
+	for _, opt := range opts {
+		if err := opt(ctx, &options); err != nil {
+			return err
+		}
+	}
+
 	return update(ctx, s.db, func(tx *bolt.Tx) error {
 		bkt := getImagesBucket(tx, namespace)
 		if bkt == nil {
@@ -276,6 +283,22 @@ func (s *imageStore) Delete(ctx context.Context, name string, opts ...images.Del
 			return err
 		}
 
+		if options.Target != nil && options.Target.Digest != "" {
+			ibkt := bkt.Bucket([]byte(name))
+			if ibkt == nil {
+				return fmt.Errorf("image %q: %w", name, errdefs.ErrNotFound)
+			}
+
+			var check images.Image
+			if err := readImage(&check, ibkt); err != nil {
+				return fmt.Errorf("image %q: %w", name, err)
+			}
+
+			if check.Target.Digest != options.Target.Digest {
+				return fmt.Errorf("image %q has target %v, not %v: %w", name, check.Target.Digest, options.Target.Digest, errdefs.ErrNotFound)
+			}
+		}
+
 		if err = bkt.DeleteBucket([]byte(name)); err != nil {
 			if err == bolt.ErrBucketNotFound {
 				err = fmt.Errorf("image %q: %w", name, errdefs.ErrNotFound)

metadata/images_test.go

@@ -154,10 +154,11 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 		name       string
 		original   images.Image
 		createerr  error
-		input      images.Image
+		input      images.Image // Input target size determines target digest, base image uses 10
 		fieldpaths []string
 		expected   images.Image
 		cause      error
+		deleteerr  error
 	}{
 		{
 			name:     "Touch",
@@ -239,7 +240,7 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 					"boo": "boo",
 				},
 				Target: ocispec.Descriptor{
-					Size:      20,                                 // ignored
+					Size:      10,
 					MediaType: "application/vnd.oci.blab+ignored", // make sure other stuff is ignored
 					Annotations: map[string]string{
 						"not": "bar",
@@ -272,7 +273,7 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 					"boo": "boo",
 				},
 				Target: ocispec.Descriptor{
-					Size:      20,                                 // ignored
+					Size:      10,
 					MediaType: "application/vnd.oci.blab+ignored", // make sure other stuff is ignored
 					Annotations: map[string]string{
 						"foo": "boo",
@@ -303,7 +304,7 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 					"baz": "bunk",
 				},
 				Target: ocispec.Descriptor{
-					Size:      20,                                 // ignored
+					Size:      10,
 					MediaType: "application/vnd.oci.blab+ignored", // make sure other stuff is ignored
 					Annotations: map[string]string{
 						"foo": "bar",
@@ -335,7 +336,7 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 					"baz": "bunk",
 				},
 				Target: ocispec.Descriptor{
-					Size:      20,                                 // ignored
+					Size:      10,
 					MediaType: "application/vnd.oci.blab+ignored", // make sure other stuff is ignored
 					Annotations: map[string]string{
 						"foo": "baz",
@@ -360,7 +361,7 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 			},
 		},
 		{
-			name:     "ReplaceTarget", // target must be updated as a unit
+			name:     "ReplaceTargetTypeAndAnnotations", // target must be updated as a unit
 			original: imageBase(),
 			input: images.Image{
 				Target: ocispec.Descriptor{
@@ -386,6 +387,57 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:     "ReplaceTargetFieldpath", // target must be updated as a unit
+			original: imageBase(),
+			input: images.Image{
+				Target: ocispec.Descriptor{
+					Size:      20,
+					MediaType: "application/vnd.oci.blab+replaced",
+					Annotations: map[string]string{
+						"fox": "dog",
+					},
+				},
+			},
+			fieldpaths: []string{"target"},
+			expected: images.Image{
+				Labels: map[string]string{ // Labels not updated
+					"foo": "bar",
+					"baz": "boo",
+				},
+				Target: ocispec.Descriptor{
+					Size:      20,
+					MediaType: "application/vnd.oci.blab+replaced",
+					Annotations: map[string]string{
+						"fox": "dog",
+					},
+				},
+			},
+			deleteerr: errdefs.ErrNotFound,
+		},
+		{
+			name:     "ReplaceTarget", // target must be updated as a unit
+			original: imageBase(),
+			input: images.Image{
+				Target: ocispec.Descriptor{
+					Size:      20,
+					MediaType: "application/vnd.oci.blab+replaced",
+					Annotations: map[string]string{
+						"fox": "dog",
+					},
+				},
+			},
+			expected: images.Image{
+				Target: ocispec.Descriptor{
+					Size:      20,
+					MediaType: "application/vnd.oci.blab+replaced",
+					Annotations: map[string]string{
+						"fox": "dog",
+					},
+				},
+			},
+			deleteerr: errdefs.ErrNotFound,
+		},
 		{
 			name:     "EmptySize",
 			original: imageBase(),
@@ -486,11 +538,9 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 			}
 			testcase.expected.Name = testcase.name
 
-			if testcase.original.Target.Digest == "" {
+			testcase.original.Target.Digest = digest.FromString(fmt.Sprintf("%s-%d", testcase.name, testcase.original.Target.Size))
-				testcase.original.Target.Digest = digest.FromString(testcase.name)
+			testcase.input.Target.Digest = digest.FromString(fmt.Sprintf("%s-%d", testcase.name, testcase.input.Target.Size))
-				testcase.input.Target.Digest = testcase.original.Target.Digest
+			testcase.expected.Target.Digest = testcase.input.Target.Digest
-				testcase.expected.Target.Digest = testcase.original.Target.Digest
-			}
 
 			// Create
 			now := time.Now()
@@ -538,6 +588,22 @@ func TestImagesCreateUpdateDelete(t *testing.T) {
 			}
 
 			checkImagesEqual(t, &result, &testcase.expected, "get after failed to get expected result")
+
+			if testcase.original.Target.Digest != testcase.expected.Target.Digest {
+				t.Log("Delete should fail")
+			}
+			// Delete
+			err = store.Delete(ctx, testcase.original.Name, images.DeleteTarget(&testcase.original.Target))
+			if err != nil {
+				if testcase.deleteerr == nil {
+					t.Fatal(err)
+				}
+				if !errors.Is(err, testcase.deleteerr) {
+					t.Fatal("unexpected error", err, ", expected", testcase.deleteerr)
+				}
+			} else if testcase.deleteerr != nil {
+				t.Fatal("no error on deleted, expected", testcase.deleteerr)
+			}
 		})
 	}
 }

services/images/local.go

@@ -176,7 +176,14 @@ func (l *local) Update(ctx context.Context, req *imagesapi.UpdateImageRequest, _
 func (l *local) Delete(ctx context.Context, req *imagesapi.DeleteImageRequest, _ ...grpc.CallOption) (*ptypes.Empty, error) {
 	log.G(ctx).WithField("name", req.Name).Debugf("delete image")
 
-	if err := l.store.Delete(ctx, req.Name); err != nil {
+	var opts []images.DeleteOpt
+	if req.Target != nil {
+		desc := descFromProto(req.Target)
+		opts = append(opts, images.DeleteTarget(&desc))
+	}
+
+	// Sync option handled here after event is published
+	if err := l.store.Delete(ctx, req.Name, opts...); err != nil {
 		return nil, errdefs.ToGRPC(err)
 	}