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)