From 211ac3b60dc33232ae2b43d2f95e182fbe56dd10 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Sun, 25 Aug 2024 14:49:19 +0000 Subject: [PATCH] fix(delete manifest): distinct behaviors for delete by tag vb delete by digest In case of delete by tag only the tag is removed, the manifest itself would continue to be accessible by digest. In case of delete by digest the manifest would be completely removed (provided it is not used by an index or another reference). Signed-off-by: Andrei Aaron --- pkg/api/controller_test.go | 3 +++ pkg/storage/common/common.go | 24 +++++++++++++++++++++++- pkg/storage/imagestore/imagestore.go | 3 ++- pkg/storage/local/local_test.go | 22 ++++++++++++++++++++-- pkg/storage/s3/s3_test.go | 20 +++++++++++++++++++- 5 files changed, 67 insertions(+), 5 deletions(-) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 1edf8f1b5..b882d87fd 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -8710,6 +8710,9 @@ func TestManifestImageIndex(t *testing.T) { So(resp.Body(), ShouldNotBeEmpty) resp, err = resty.R().Delete(baseURL + "/v2/index/manifests/test:index1") So(err, ShouldBeNil) + // Manifest is in use, tag is deleted, but manifest is not + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + So(resp.Body(), ShouldBeEmpty) resp, err = resty.R().SetHeader("Content-Type", ispec.MediaTypeImageIndex). Get(baseURL + "/v2/index/manifests/test:index1") So(err, ShouldBeNil) diff --git a/pkg/storage/common/common.go b/pkg/storage/common/common.go index 450090199..e28239d59 100644 --- a/pkg/storage/common/common.go +++ b/pkg/storage/common/common.go @@ -330,17 +330,21 @@ func RemoveManifestDescByReference(index *ispec.Index, reference string, detectC ) (ispec.Descriptor, error) { var removedManifest ispec.Descriptor - var found bool + var found, foundAsTag bool // keep track if the manifest was found by digest or by tag + manifestDigestCounts := map[godigest.Digest]int{} // Keep track of the number of references for a specific digest foundCount := 0 var outIndex ispec.Index for _, manifest := range index.Manifests { + manifestDigestCounts[manifest.Digest]++ + tag, ok := manifest.Annotations[ispec.AnnotationRefName] if ok && tag == reference { removedManifest = manifest found = true + foundAsTag = true foundCount++ continue @@ -361,6 +365,19 @@ func RemoveManifestDescByReference(index *ispec.Index, reference string, detectC return ispec.Descriptor{}, zerr.ErrManifestNotFound } + // In case of delete by digest we remove the manifest right away from storage + // but in case of delete by tag we want to only remove the tag + // and handle the manifest based on retention rules later. + // If there are more than one tags with the same digest we want to keep the others. + // If and only if there are no other tags except the one we want to remove + // we need to add a new descriptor without a tag name to keep track of + // the manifest in index.json so it remains accessible by digest. + if foundAsTag && manifestDigestCounts[removedManifest.Digest] == 1 { + newManifest := removedManifest + delete(newManifest.Annotations, ispec.AnnotationRefName) + outIndex.Manifests = append(outIndex.Manifests, newManifest) + } + index.Manifests = outIndex.Manifests return removedManifest, nil @@ -515,6 +532,11 @@ func IsBlobReferencedInImageIndex(imgStore storageTypes.ImageStore, repo string, switch desc.MediaType { case ispec.MediaTypeImageIndex: + if digest == desc.Digest { + // no need to look further if we have a match + return true, nil + } + indexImage, err := GetImageIndex(imgStore, repo, desc.Digest, log) if err != nil { log.Error().Err(err).Str("repository", repo).Str("digest", desc.Digest.String()). diff --git a/pkg/storage/imagestore/imagestore.go b/pkg/storage/imagestore/imagestore.go index 64f0f5d6d..a295184a4 100644 --- a/pkg/storage/imagestore/imagestore.go +++ b/pkg/storage/imagestore/imagestore.go @@ -626,7 +626,8 @@ func (is *ImageStore) deleteImageManifest(repo, reference string, detectCollisio /* check if manifest is referenced in image indexes, do not allow index images manipulations (ie. remove manifest being part of an image index) */ - if manifestDesc.MediaType == ispec.MediaTypeImageManifest { + if zcommon.IsDigest(reference) && + (manifestDesc.MediaType == ispec.MediaTypeImageManifest || manifestDesc.MediaType == ispec.MediaTypeImageIndex) { for _, mDesc := range index.Manifests { if mDesc.MediaType == ispec.MediaTypeImageIndex { if ok, _ := common.IsBlobReferencedInImageIndex(is, repo, manifestDesc.Digest, ispec.Index{ diff --git a/pkg/storage/local/local_test.go b/pkg/storage/local/local_test.go index ce603fef9..610b68045 100644 --- a/pkg/storage/local/local_test.go +++ b/pkg/storage/local/local_test.go @@ -1239,11 +1239,11 @@ func TestDedupeLinks(t *testing.T) { manifestBuf, err = json.Marshal(manifest) So(err, ShouldBeNil) - digest = godigest.FromBytes(manifestBuf) + manifestDigest2 := godigest.FromBytes(manifestBuf) _, _, err = imgStore.PutImageManifest("dedupe2", "1.0", ispec.MediaTypeImageManifest, manifestBuf) So(err, ShouldBeNil) - _, _, _, err = imgStore.GetImageManifest("dedupe2", digest.String()) + _, _, _, err = imgStore.GetImageManifest("dedupe2", manifestDigest2.String()) So(err, ShouldBeNil) // verify that dedupe with hard links happened @@ -1267,6 +1267,15 @@ func TestDedupeLinks(t *testing.T) { err = imgStore.DeleteBlob("dedupe1", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest1)) So(err, ShouldBeNil) + // only the tag was removed, but not the digest, this call should error + err = imgStore.DeleteBlob("dedupe2", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest2)) + So(err, ShouldNotBeNil) + + // Delete the manifest + err = imgStore.DeleteImageManifest("dedupe2", manifestDigest2.String(), false) + So(err, ShouldBeNil) + + // The call should succeed err = imgStore.DeleteBlob("dedupe2", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest2)) So(err, ShouldBeNil) }) @@ -1422,6 +1431,15 @@ func TestDedupeLinks(t *testing.T) { err = imgStore.DeleteBlob("dedupe1", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest1)) So(err, ShouldBeNil) + // only the tag was removed, but not the digest, this call should error + err = imgStore.DeleteBlob("dedupe2", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest2)) + So(err, ShouldNotBeNil) + + // Delete the manifest + err = imgStore.DeleteImageManifest("dedupe2", manifestDigest2.String(), false) + So(err, ShouldBeNil) + + // The call should succeed err = imgStore.DeleteBlob("dedupe2", godigest.NewDigestFromEncoded(godigest.SHA256, blobDigest2)) So(err, ShouldBeNil) }) diff --git a/pkg/storage/s3/s3_test.go b/pkg/storage/s3/s3_test.go index 85d0fbeb2..0a3a5f30c 100644 --- a/pkg/storage/s3/s3_test.go +++ b/pkg/storage/s3/s3_test.go @@ -1744,12 +1744,21 @@ func TestS3Dedupe(t *testing.T) { err = imgStore.DeleteImageManifest("dedupe1", manifestDigest.String(), false) So(err, ShouldBeNil) + // delete tag, but not manifest err = imgStore.DeleteImageManifest("dedupe2", "1.0", false) So(err, ShouldBeNil) + // Delete should succeed as the manifest was deleted err = imgStore.DeleteBlob("dedupe1", blobDigest1) So(err, ShouldBeNil) + // Delete should fail, as the blob is referenced by an untagged manifest + err = imgStore.DeleteBlob("dedupe2", blobDigest2) + So(err, ShouldEqual, zerr.ErrBlobReferenced) + + err = imgStore.DeleteImageManifest("dedupe2", manifestDigest.String(), false) + So(err, ShouldBeNil) + err = imgStore.DeleteBlob("dedupe2", blobDigest2) So(err, ShouldBeNil) }) @@ -1790,12 +1799,21 @@ func TestS3Dedupe(t *testing.T) { err = imgStore.DeleteImageManifest("dedupe1", manifestDigest.String(), false) So(err, ShouldBeNil) + // delete tag, but not manifest err = imgStore.DeleteImageManifest("dedupe2", "1.0", false) So(err, ShouldBeNil) + // delete should succeed as the manifest was deleted err = imgStore.DeleteBlob("dedupe1", blobDigest1) So(err, ShouldBeNil) + // delete should fail, as the blob is referenced by an untagged manifest + err = imgStore.DeleteBlob("dedupe2", blobDigest2) + So(err, ShouldEqual, zerr.ErrBlobReferenced) + + err = imgStore.DeleteImageManifest("dedupe2", manifestDigest.String(), false) + So(err, ShouldBeNil) + err = imgStore.DeleteBlob("dedupe2", blobDigest2) So(err, ShouldBeNil) }) @@ -1832,7 +1850,7 @@ func TestS3Dedupe(t *testing.T) { err = imgStore.DeleteImageManifest("dedupe1", manifestDigest.String(), false) So(err, ShouldBeNil) - err = imgStore.DeleteImageManifest("dedupe2", "1.0", false) + err = imgStore.DeleteImageManifest("dedupe2", manifestDigest.String(), false) So(err, ShouldBeNil) err = imgStore.DeleteBlob("dedupe1", blobDigest1)