From 2ce971d890838efc8b7734d25d961d44bcdca965 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Fri, 18 Aug 2023 16:04:09 -0700 Subject: [PATCH] Add delete target to image remove Adds atomicity to image delete when deleting from a list. Signed-off-by: Derek McGowan --- image_store.go | 35 ++++------------ images/image.go | 11 +++++ metadata/images.go | 23 +++++++++++ metadata/images_test.go | 88 +++++++++++++++++++++++++++++++++++----- services/images/local.go | 9 +++- 5 files changed, 128 insertions(+), 38 deletions(-) diff --git a/image_store.go b/image_store.go index 524a7a672..ff3f6ec35 100644 --- a/image_store.go +++ b/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, - } -} diff --git a/images/image.go b/images/image.go index 6acb93308..24cb5456e 100644 --- a/images/image.go +++ b/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) diff --git a/metadata/images.go b/metadata/images.go index b51d0d610..1fcdbb309 100644 --- a/metadata/images.go +++ b/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) diff --git a/metadata/images_test.go b/metadata/images_test.go index 4644e0018..43077279c 100644 --- a/metadata/images_test.go +++ b/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(testcase.name) - testcase.input.Target.Digest = testcase.original.Target.Digest - testcase.expected.Target.Digest = testcase.original.Target.Digest - } + testcase.original.Target.Digest = digest.FromString(fmt.Sprintf("%s-%d", testcase.name, testcase.original.Target.Size)) + testcase.input.Target.Digest = digest.FromString(fmt.Sprintf("%s-%d", testcase.name, testcase.input.Target.Size)) + testcase.expected.Target.Digest = testcase.input.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) + } }) } } diff --git a/services/images/local.go b/services/images/local.go index 0c934bf66..194b1a395 100644 --- a/services/images/local.go +++ b/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) }