fix(delete manifest): distinct behaviors for delete by tag vb delete by digest (#2626)

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 <aaaron@luxoft.com>

cmd/zb/helper.go

@@ -25,25 +25,27 @@ import (
 	"zotregistry.dev/zot/pkg/common"
 )
 
-func makeHTTPGetRequest(url string, resultPtr interface{}, client *resty.Client) error {
+func makeHTTPGetRequest(url string, resultPtr interface{}, client *resty.Client) (http.Header, error) {
 	resp, err := client.R().Get(url)
 	if err != nil {
-		return err
+		return http.Header{}, err
 	}
 
+	header := resp.Header()
+
 	if resp.StatusCode() != http.StatusOK {
 		log.Printf("unable to make GET request on %s, response status code: %d", url, resp.StatusCode())
 
-		return fmt.Errorf("%w: Expected: %d, Got: %d, Body: '%s'", zerr.ErrBadHTTPStatusCode, http.StatusOK,
+		return header, fmt.Errorf("%w: Expected: %d, Got: %d, Body: '%s'", zerr.ErrBadHTTPStatusCode, http.StatusOK,
 			resp.StatusCode(), string(resp.Body()))
 	}
 
 	err = json.Unmarshal(resp.Body(), resultPtr)
 	if err != nil {
-		return err
+		return header, err
 	}
 
-	return nil
+	return header, nil
 }
 
 func makeHTTPDeleteRequest(url string, client *resty.Client) error {
@@ -67,7 +69,7 @@ func deleteTestRepo(repos []string, url string, client *resty.Client) error {
 		var tags common.ImageTags
 
 		// get tags
-		err := makeHTTPGetRequest(fmt.Sprintf("%s/v2/%s/tags/list", url, repo), &tags, client)
+		_, err := makeHTTPGetRequest(fmt.Sprintf("%s/v2/%s/tags/list", url, repo), &tags, client)
 		if err != nil {
 			return err
 		}
@@ -76,13 +78,15 @@ func deleteTestRepo(repos []string, url string, client *resty.Client) error {
 			var manifest ispec.Manifest
 
 			// first get tag manifest to get containing blobs
-			err := makeHTTPGetRequest(fmt.Sprintf("%s/v2/%s/manifests/%s", url, repo, tag), &manifest, client)
+			header, err := makeHTTPGetRequest(fmt.Sprintf("%s/v2/%s/manifests/%s", url, repo, tag), &manifest, client)
 			if err != nil {
 				return err
 			}
 
+			manifestDigest := header.Get("Docker-Content-Digest")
+
 			// delete manifest so that we don't trigger BlobInUse error
-			err = makeHTTPDeleteRequest(fmt.Sprintf("%s/v2/%s/manifests/%s", url, repo, tag), client)
+			err = makeHTTPDeleteRequest(fmt.Sprintf("%s/v2/%s/manifests/%s", url, repo, manifestDigest), client)
 			if err != nil {
 				return err
 			}

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

@@ -554,11 +554,15 @@ func (is *ImageStore) PutImageManifest(repo, reference, mediaType string, //noli
 	dir := path.Join(is.rootDir, repo, ispec.ImageBlobsDir, mDigest.Algorithm().String())
 	manifestPath := path.Join(dir, mDigest.Encoded())
 
+	binfo, err := is.storeDriver.Stat(manifestPath)
+	if err != nil || binfo.Size() != desc.Size {
+		// The blob isn't already there, or it is corrupted, and needs a correction
 		if _, err = is.storeDriver.WriteFile(manifestPath, body); err != nil {
 			is.log.Error().Err(err).Str("file", manifestPath).Msg("failed to write")
 
 			return "", "", err
 		}
+	}
 
 	err = common.UpdateIndexWithPrunedImageManifests(is, &index, repo, desc, oldDgst, is.log)
 	if err != nil {
@@ -566,6 +570,14 @@ func (is *ImageStore) PutImageManifest(repo, reference, mediaType string, //noli
 	}
 
 	// now update "index.json"
+	for midx, manifest := range index.Manifests {
+		_, ok := manifest.Annotations[ispec.AnnotationRefName]
+		if !ok && manifest.Digest.String() == desc.Digest.String() {
+			// matching descriptor does not have a tag, we need to remove it and add the new descriptor
+			index.Manifests = append(index.Manifests[:midx], index.Manifests[midx+1:]...)
+		}
+	}
+
 	index.Manifests = append(index.Manifests, desc)
 
 	// update the descriptors artifact type in order to check for signatures when applying the linter
@@ -626,7 +638,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

@@ -1307,12 +1307,12 @@ func TestS3Dedupe(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)
 
 		fi1, err := storeDriver.Stat(context.Background(), path.Join(testDir, "dedupe1", "blobs", "sha256",
@@ -1336,12 +1336,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", manifestDigest2.String(), false)
+			So(err, ShouldBeNil)
+
 			err = imgStore.DeleteBlob("dedupe2", blobDigest2)
 			So(err, ShouldBeNil)
 		})
@@ -1351,7 +1360,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", manifestDigest2.String(), false)
 			So(err, ShouldBeNil)
 
 			// if we delete blob1, the content should be moved to blob2
@@ -1470,12 +1479,12 @@ func TestS3Dedupe(t *testing.T) {
 			manifestBuf, err = json.Marshal(manifest)
 			So(err, ShouldBeNil)
 
-			digest = godigest.FromBytes(manifestBuf)
+			manifestDigest3 := godigest.FromBytes(manifestBuf)
 			_, _, err = imgStore.PutImageManifest("dedupe3", "1.0", ispec.MediaTypeImageManifest,
 				manifestBuf)
 			So(err, ShouldBeNil)
 
-			_, _, _, err = imgStore.GetImageManifest("dedupe3", digest.String())
+			_, _, _, err = imgStore.GetImageManifest("dedupe3", manifestDigest3.String())
 			So(err, ShouldBeNil)
 
 			fi1, err := storeDriver.Stat(context.Background(), path.Join(testDir, "dedupe1", "blobs", "sha256",
@@ -1500,10 +1509,10 @@ 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", manifestDigest2.String(), false)
 				So(err, ShouldBeNil)
 
-				err = imgStore.DeleteImageManifest("dedupe3", "1.0", false)
+				err = imgStore.DeleteImageManifest("dedupe3", manifestDigest3.String(), false)
 				So(err, ShouldBeNil)
 
 				err = imgStore.DeleteBlob("dedupe1", blobDigest1)
@@ -1715,12 +1724,12 @@ func TestS3Dedupe(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)
 
 		fi1, err := storeDriver.Stat(context.Background(), path.Join(testDir, "dedupe1", "blobs", "sha256",
@@ -1744,12 +1753,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", manifestDigest2.String(), false)
+			So(err, ShouldBeNil)
+
 			err = imgStore.DeleteBlob("dedupe2", blobDigest2)
 			So(err, ShouldBeNil)
 		})
@@ -1790,12 +1808,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", manifestDigest2.String(), false)
+				So(err, ShouldBeNil)
+
 				err = imgStore.DeleteBlob("dedupe2", blobDigest2)
 				So(err, ShouldBeNil)
 			})
@@ -1832,7 +1859,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", manifestDigest2.String(), false)
 			So(err, ShouldBeNil)
 
 			err = imgStore.DeleteBlob("dedupe1", blobDigest1)