fix: gc for untagged docker manifests (#3349)

- fixes #3347: removeUntaggedManifests() did not consider compatible manifest types
- add AsDockerImage() to Image and MultiarchImage for testing
- extend TestGarbageCollectAndRetentionMetaDB to test docker image and multiarch image

Signed-off-by: Stephan Merker <stephan.merker@sap.com>

pkg/storage/gc/gc.go

@@ -485,7 +485,8 @@ func (gc GarbageCollect) removeUntaggedManifests(repo string, index *ispec.Index
 		}
 
 		// remove untagged images
-		if desc.MediaType == ispec.MediaTypeImageManifest || desc.MediaType == ispec.MediaTypeImageIndex {
+		if desc.MediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(desc.MediaType) ||
+			desc.MediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(desc.MediaType) {
 			_, ok := getDescriptorTag(desc)
 			if !ok {
 				gced, err = gc.gcManifest(repo, index, desc, "", "", gc.opts.ImageRetention.Delay)

pkg/storage/gc/gc_test.go

@@ -20,6 +20,7 @@ import (
 	"gopkg.in/resty.v1"
 
 	"zotregistry.dev/zot/pkg/api/config"
+	"zotregistry.dev/zot/pkg/compat"
 	"zotregistry.dev/zot/pkg/extensions/monitoring"
 	zlog "zotregistry.dev/zot/pkg/log"
 	"zotregistry.dev/zot/pkg/meta"
@@ -71,6 +72,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 			var imgStore storageTypes.ImageStore
 
 			var metaDB mTypes.MetaDB
+			compat := []compat.MediaCompatibility{compat.DockerManifestV2SchemaV2}
 
 			if testcase.storageType == storageConstants.S3StorageDriverName {
 				tskip.SkipDynamo(t)
@@ -140,13 +142,13 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					panic(err)
 				}
 
-				imgStore = s3.NewImageStore(rootDir, cacheDir, true, false, log, metrics, nil, store, nil, nil, nil)
+				imgStore = s3.NewImageStore(rootDir, cacheDir, true, false, log, metrics, nil, store, nil, compat, nil)
 			} else {
 				// Create temporary directory
 				rootDir := t.TempDir()
 
 				// Create ImageStore
-				imgStore = local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, nil, nil)
+				imgStore = local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, compat, nil)
 
 				// init metaDB
 				params := boltdb.DBParameters{
@@ -243,6 +245,15 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 				err = WriteMultiArchImageToFileSystem(gcUntagged4, "gc-test4", gcUntagged4.DigestStr(), storeController)
 				So(err, ShouldBeNil)
 
+				// docker images
+				gcDocker1 := CreateRandomImage().AsDockerImage()
+				err = WriteImageToFileSystem(gcDocker1, "gc-docker1", "0.0.1", storeController)
+				So(err, ShouldBeNil)
+
+				gcDocker2 := CreateRandomMultiarch().AsDockerImage()
+				err = WriteMultiArchImageToFileSystem(gcDocker2, "gc-docker2", "0.0.1", storeController)
+				So(err, ShouldBeNil)
+
 				// for image retention testing
 				// old images
 				gcOld1 := CreateRandomImage()
@@ -361,6 +372,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					err = gc.CleanRepo(ctx, "gc-test4")
 					So(err, ShouldBeNil)
 
+					err = gc.CleanRepo(ctx, "gc-docker1")
+					So(err, ShouldBeNil)
+
+					err = gc.CleanRepo(ctx, "gc-docker2")
+					So(err, ShouldBeNil)
+
 					err = gc.CleanRepo(ctx, "retention")
 					So(err, ShouldBeNil)
 
@@ -412,6 +429,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					_, _, _, err = imgStore.GetImageManifest("gc-test4", refOfRef4.DigestStr())
 					So(err, ShouldBeNil)
 
+					_, _, _, err = imgStore.GetImageManifest("gc-docker1", gcDocker1.DigestStr())
+					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker2", gcDocker2.DigestStr())
+					So(err, ShouldBeNil)
+
 					_, _, _, err = imgStore.GetImageManifest("retention", "0.0.1")
 					So(err, ShouldBeNil)
 
@@ -465,6 +488,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					err = gc.CleanRepo(ctx, "gc-test4")
 					So(err, ShouldBeNil)
 
+					err = gc.CleanRepo(ctx, "gc-docker1")
+					So(err, ShouldBeNil)
+
+					err = gc.CleanRepo(ctx, "gc-docker2")
+					So(err, ShouldBeNil)
+
 					err = gc.CleanRepo(ctx, "retention")
 					So(err, ShouldBeNil)
 
@@ -515,6 +544,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 
 					_, _, _, err = imgStore.GetImageManifest("gc-test4", refOfRef4.DigestStr())
 					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker1", gcDocker1.DigestStr())
+					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker2", gcDocker2.DigestStr())
+					So(err, ShouldBeNil)
 				})
 
 				Convey("gc all tags, untagged, and afterwards referrers", func() {
@@ -560,6 +595,50 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(repos, ShouldContain, "gc-test2")
 					So(repos, ShouldContain, "gc-test3")
 					So(repos, ShouldContain, "gc-test4")
+					So(repos, ShouldContain, "gc-docker1")
+					So(repos, ShouldContain, "gc-docker2")
+					So(repos, ShouldContain, "retention")
+				})
+
+				Convey("gc all tags for docker repo", func() {
+					gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{
+						Delay: 1 * time.Millisecond,
+						ImageRetention: config.ImageRetention{
+							Delay: 1 * time.Millisecond,
+							Policies: []config.RetentionPolicy{
+								{
+									Repositories:    []string{"gc-docker*"},
+									DeleteReferrers: true,
+									DeleteUntagged:  &trueVal,
+									KeepTags: []config.KeepTagsPolicy{
+										{
+											Patterns: []string{"v1"}, // should not match any tag
+										},
+									},
+								},
+							},
+						},
+					}, audit, log)
+
+					err := gc.CleanRepo(ctx, "gc-docker1")
+					So(err, ShouldBeNil)
+					err = gc.CleanRepo(ctx, "gc-docker2")
+					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker1", gcDocker1.DigestStr())
+					So(err, ShouldNotBeNil)
+					_, _, _, err = imgStore.GetImageManifest("gc-docker2", gcDocker2.DigestStr())
+					So(err, ShouldNotBeNil)
+
+					// now repo should get gc'ed
+					repos, err := imgStore.GetRepositories()
+					So(err, ShouldBeNil)
+					So(repos, ShouldContain, "gc-test1")
+					So(repos, ShouldContain, "gc-test2")
+					So(repos, ShouldContain, "gc-test3")
+					So(repos, ShouldContain, "gc-test4")
+					So(repos, ShouldNotContain, "gc-docker1")
+					So(repos, ShouldNotContain, "gc-docker2")
 					So(repos, ShouldContain, "retention")
 				})
 
@@ -584,9 +663,9 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					}, audit, log)
 
 					processedRepos := make(map[string]struct{})
-					expectedRepos := []string{"gc-test1", "gc-test2", "gc-test3", "gc-test4", "retention"}
+					expectedRepos := []string{"gc-docker1", "gc-docker2", "gc-test1", "gc-test2", "gc-test3", "gc-test4", "retention"}
 
-					for i := range 10 {
+					for i := range 2 * len(expectedRepos) {
 						t.Logf("index %d, processed repos %v", i, processedRepos)
 
 						// we need to check if GetNextRepository returns each repository just once, and empty string afterwards
@@ -594,7 +673,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 						t.Logf("index %d, repo '%s'", i, repo)
 						So(err, ShouldBeNil)
 
-						if i >= 5 {
+						if i >= len(expectedRepos) {
 							So(repo, ShouldEqual, "")
 
 							continue
@@ -627,6 +706,8 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(repos, ShouldContain, "gc-test2")
 					So(repos, ShouldNotContain, "gc-test3")
 					So(repos, ShouldContain, "gc-test4")
+					So(repos, ShouldContain, "gc-docker1")
+					So(repos, ShouldContain, "gc-docker2")
 					So(repos, ShouldContain, "retention")
 				})
 
@@ -670,6 +751,8 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(repos, ShouldContain, "gc-test2")
 					So(repos, ShouldContain, "gc-test3")
 					So(repos, ShouldContain, "gc-test4")
+					So(repos, ShouldContain, "gc-docker1")
+					So(repos, ShouldContain, "gc-docker2")
 					So(repos, ShouldContain, "retention")
 
 					tags, err := imgStore.GetImageTags("gc-test1")
@@ -713,6 +796,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					_, _, _, err = imgStore.GetImageManifest("gc-test3", "0.0.1")
 					So(err, ShouldBeNil)
 
+					_, _, _, err = imgStore.GetImageManifest("gc-docker1", "0.0.1")
+					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker2", "0.0.1")
+					So(err, ShouldBeNil)
+
 					_, _, _, err = imgStore.GetImageManifest("retention", "0.0.1")
 					So(err, ShouldBeNil)
 
@@ -768,6 +857,12 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					_, _, _, err = imgStore.GetImageManifest("gc-test3", "0.0.1")
 					So(err, ShouldBeNil)
 
+					_, _, _, err = imgStore.GetImageManifest("gc-docker1", "0.0.1")
+					So(err, ShouldBeNil)
+
+					_, _, _, err = imgStore.GetImageManifest("gc-docker2", "0.0.1")
+					So(err, ShouldBeNil)
+
 					_, _, _, err = imgStore.GetImageManifest("retention", "0.0.1")
 					So(err, ShouldBeNil)
 
@@ -1026,6 +1121,8 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(repos, ShouldContain, "gc-test1")
 					So(repos, ShouldContain, "gc-test2")
 					So(repos, ShouldContain, "gc-test3")
+					So(repos, ShouldContain, "gc-docker1")
+					So(repos, ShouldContain, "gc-docker2")
 					So(repos, ShouldContain, "retention")
 				})
 

pkg/test/image-utils/images.go

@@ -7,6 +7,7 @@ import (
 	"strconv"
 	"time"
 
+	docker "github.com/distribution/distribution/v3/manifest/schema2"
 	godigest "github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -173,6 +174,35 @@ func (img Image) AsImageMeta() mTypes.ImageMeta {
 	}
 }
 
+func (img Image) AsDockerImage() Image {
+	// Convert manifest media type
+	img.Manifest.MediaType = docker.MediaTypeManifest
+
+	// Convert config media type
+	img.Manifest.Config.MediaType = docker.MediaTypeImageConfig
+	img.ConfigDescriptor.MediaType = docker.MediaTypeImageConfig
+
+	// Convert layer media types
+	for i := range img.Manifest.Layers {
+		img.Manifest.Layers[i].MediaType = docker.MediaTypeLayer
+	}
+
+	manifestBlob, err := json.Marshal(img.Manifest)
+	if err != nil {
+		panic("unreachable: ispec.Manifest should always be marshable")
+	}
+
+	img.ManifestDescriptor = ispec.Descriptor{
+		MediaType: docker.MediaTypeImageConfig,
+		Digest:    img.digestAlgorithm.FromBytes(manifestBlob),
+		Size:      int64(len(manifestBlob)),
+		Data:      manifestBlob,
+		Platform:  img.ConfigDescriptor.Platform,
+	}
+
+	return img
+}
+
 type Layer struct {
 	Blob      []byte
 	MediaType string

pkg/test/image-utils/images_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"testing"
 
+	docker "github.com/distribution/distribution/v3/manifest/schema2"
 	godigest "github.com/opencontainers/go-digest"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	. "github.com/smartystreets/goconvey/convey"
@@ -197,3 +198,19 @@ func TestImageMethods(t *testing.T) {
 		})
 	})
 }
+
+func TestConvertImageToDocker(t *testing.T) {
+	Convey("AsDockerImage", t, func() {
+		Convey("DefaultImage", func() {
+			img := CreateDefaultImage()
+			dockerImage := img.AsDockerImage()
+
+			So(dockerImage.Manifest.MediaType, ShouldEqual, docker.MediaTypeManifest)
+			So(dockerImage.ConfigDescriptor.MediaType, ShouldEqual, docker.MediaTypeImageConfig)
+
+			for _, layer := range dockerImage.Manifest.Layers {
+				So(layer.MediaType, ShouldEqual, docker.MediaTypeLayer)
+			}
+		})
+	})
+}

pkg/test/image-utils/multiarch.go

@@ -3,6 +3,7 @@ package image
 import (
 	"encoding/json"
 
+	dockerList "github.com/distribution/distribution/v3/manifest/manifestlist"
 	godigest "github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -75,6 +76,34 @@ func (mi MultiarchImage) AsImageMeta() mTypes.ImageMeta {
 	}
 }
 
+func (mi MultiarchImage) AsDockerImage() MultiarchImage {
+	for i := range mi.Images {
+		mi.Images[i] = mi.Images[i].AsDockerImage()
+	}
+
+	mi.Index.MediaType = dockerList.MediaTypeManifestList
+	for i := range mi.Index.Manifests {
+		mi.Index.Manifests[i].Digest = mi.Images[i].ManifestDescriptor.Digest
+		mi.Index.Manifests[i].Size = mi.Images[i].ManifestDescriptor.Size
+		mi.Index.Manifests[i].MediaType = mi.Images[i].ManifestDescriptor.MediaType
+	}
+
+	indexBlob, err := json.Marshal(mi.Index)
+	if err != nil {
+		panic("unreachable: ispec.Index should always be marshable")
+	}
+
+	mi.IndexDescriptor.MediaType = dockerList.MediaTypeManifestList
+	mi.IndexDescriptor = ispec.Descriptor{
+		MediaType: dockerList.MediaTypeManifestList,
+		Digest:    mi.digestAlgorithm.FromBytes(indexBlob),
+		Size:      int64(len(indexBlob)),
+		Data:      indexBlob,
+	}
+
+	return mi
+}
+
 type ImagesBuilder interface {
 	Images(images []Image) MultiarchBuilder
 	RandomImages(count int) MultiarchBuilder

pkg/test/image-utils/write.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 
 	godigest "github.com/opencontainers/go-digest"
-	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 
 	stypes "zotregistry.dev/zot/pkg/storage/types"
 )
@@ -52,7 +51,7 @@ func WriteImageToFileSystem(image Image, repoName, ref string, storeController s
 		return err
 	}
 
-	_, _, err = store.PutImageManifest(repoName, ref, ispec.MediaTypeImageManifest, manifestBlob)
+	_, _, err = store.PutImageManifest(repoName, ref, image.Manifest.MediaType, manifestBlob)
 	if err != nil {
 		return err
 	}
@@ -82,7 +81,7 @@ func WriteMultiArchImageToFileSystem(multiarchImage MultiarchImage, repoName, re
 		return err
 	}
 
-	_, _, err = store.PutImageManifest(repoName, ref, ispec.MediaTypeImageIndex,
+	_, _, err = store.PutImageManifest(repoName, ref, multiarchImage.Index.MediaType,
 		indexBlob)
 
 	return err