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 <[email protected]>

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)

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()).

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{

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)
 			})

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)