From b7ab9dab166766256b382da89a62f6ce2644b672 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Tue, 18 Nov 2025 20:56:44 +0200 Subject: [PATCH] fix (metadb): make sure metadb statistics are initialized on image download, and minor metadb fixes for Docker v2 manifest compatibility (#3545) fix: make sure metadb statistics are initialized on image download, and minor metadb fixes for Docker v2 manifest compatibility Looking into potential causes of https://github.com/project-zot/zot/issues/3163 1. One possible reason is the statistics were not properly initialized in the first place because of (unknown and/or unavoidable) errors on image push. To workaround this add logic to initialize the statistics on the call to download them. 2. Some images have the download statistics while others dont, one cause could be a bug in the logic handling manifest mediatypes in the search extension. Add compatibility checks for Docker v2 manifest types in metadb convert functions, and more tests for covering the Docker mediatype use case. Side fixes: - Ensure PushedBy Statistics entries are properly initialized in SetRepoReference - Fix and issue in the image upload test functions, they were uploading docker images with oci mediatypes in call headers Signed-off-by: Andrei Aaron --- pkg/extensions/search/search_test.go | 315 +++++++++++++++++++++++++++ pkg/meta/boltdb/boltdb.go | 43 +++- pkg/meta/boltdb/boltdb_test.go | 105 +++++++++ pkg/meta/convert/convert.go | 51 +++-- pkg/meta/dynamodb/dynamodb.go | 43 +++- pkg/meta/dynamodb/dynamodb_test.go | 68 ++++++ pkg/meta/meta_test.go | 97 +++++++++ pkg/meta/redis/redis.go | 43 +++- pkg/meta/redis/redis_test.go | 81 +++++++ pkg/test/image-utils/images.go | 2 +- pkg/test/image-utils/upload.go | 39 +++- 11 files changed, 849 insertions(+), 38 deletions(-) diff --git a/pkg/extensions/search/search_test.go b/pkg/extensions/search/search_test.go index 631f0077..4879abe3 100644 --- a/pkg/extensions/search/search_test.go +++ b/pkg/extensions/search/search_test.go @@ -33,6 +33,7 @@ import ( "zotregistry.dev/zot/v2/pkg/api/config" "zotregistry.dev/zot/v2/pkg/api/constants" zcommon "zotregistry.dev/zot/v2/pkg/common" + "zotregistry.dev/zot/v2/pkg/compat" extconf "zotregistry.dev/zot/v2/pkg/extensions/config" "zotregistry.dev/zot/v2/pkg/extensions/monitoring" cveinfo "zotregistry.dev/zot/v2/pkg/extensions/search/cve" @@ -5333,6 +5334,7 @@ func TestMetaDBIndexOperations(t *testing.T) { baseURL := GetBaseURL(port) conf := config.New() conf.HTTP.Port = port + conf.HTTP.Compat = []compat.MediaCompatibility{compat.DockerManifestV2SchemaV2} conf.Storage.RootDirectory = dir conf.Storage.GC = false defaultVal := true @@ -5431,7 +5433,219 @@ func RunMetaDBIndexTests(baseURL, port string) { responseImage = responseImages[0] So(responseImage.IsSigned, ShouldBeFalse) + // Download count is 1 because SignImageUsingCosign fetches the manifest to sign it + So(responseImage.DownloadCount, ShouldEqual, 1) + + // Get initial repository download count - query repository separately + repoQuery := ` + { + GlobalSearch(query:"repo"){ + Repos { + Name DownloadCount + } + } + }` + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct := &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + repos := repoResponseStruct.GlobalSearchResult.GlobalSearch.Repos + So(repos, ShouldNotBeEmpty) + initialRepoDownloadCount := repos[0].DownloadCount + + // Test download count - download the index manifest 3 times + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // Verify download count increased at both image and repository level + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct = &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + responseImages = responseStruct.GlobalSearchResult.GlobalSearch.Images + So(responseImages, ShouldNotBeEmpty) + responseImage = responseImages[0] + // Started with 1 (from cosign signing), added 3 more downloads = 4 total + So(responseImage.DownloadCount, ShouldEqual, 4) + + // Verify repository-level download count also increased - query repository separately + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct = &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + repos = repoResponseStruct.GlobalSearchResult.GlobalSearch.Repos + So(repos, ShouldNotBeEmpty) + So(repos[0].DownloadCount, ShouldEqual, initialRepoDownloadCount+3) }) + + Convey("Push test index with Docker media types", func() { + const repo = "repo-docker" + + multiarchImage := CreateRandomMultiarch().AsDockerImage() + + err := UploadMultiarchImage(multiarchImage, baseURL, repo, "tag1") + So(err, ShouldBeNil) + + query := ` + { + GlobalSearch(query:"repo-docker:tag1"){ + Images { + RepoName Tag DownloadCount + IsSigned + Manifests { + Digest + ConfigDigest + Platform {Os Arch} + Layers {Size Digest} + LastUpdated + Size + } + } + } + }` + + resp, err := resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct := &zcommon.GlobalSearchResultResp{} + + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + + responseImages := responseStruct.GlobalSearchResult.GlobalSearch.Images + So(responseImages, ShouldNotBeEmpty) + responseImage := responseImages[0] + So(len(responseImage.Manifests), ShouldEqual, 3) + + err = signature.SignImageUsingCosign("repo-docker@"+multiarchImage.DigestStr(), port, false) + So(err, ShouldBeNil) + + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct = &zcommon.GlobalSearchResultResp{} + + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + + responseImages = responseStruct.GlobalSearchResult.GlobalSearch.Images + So(responseImages, ShouldNotBeEmpty) + responseImage = responseImages[0] + + So(responseImage.IsSigned, ShouldBeTrue) + + // remove signature + cosignTag := "sha256-" + multiarchImage.Digest().Encoded() + ".sig" + _, err = resty.R().Delete(baseURL + "/v2/" + repo + "/manifests/" + cosignTag) + So(err, ShouldBeNil) + + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct = &zcommon.GlobalSearchResultResp{} + + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + + responseImages = responseStruct.GlobalSearchResult.GlobalSearch.Images + So(responseImages, ShouldNotBeEmpty) + responseImage = responseImages[0] + + So(responseImage.IsSigned, ShouldBeFalse) + // Download count is 1 because SignImageUsingCosign fetches the manifest to sign it + initialDownloadCount := responseImage.DownloadCount + So(initialDownloadCount, ShouldEqual, 1) + + // Get initial repository download count - query repository separately + repoQuery := ` + { + GlobalSearch(query:"repo-docker"){ + Repos { + Name DownloadCount + } + } + }` + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct := &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + repos := repoResponseStruct.GlobalSearchResult.GlobalSearch.Repos + So(repos, ShouldNotBeEmpty) + initialRepoDownloadCount := repos[0].DownloadCount + + // Test download count - download the index manifest 3 times + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + repo + "/manifests/" + "tag1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // Verify download count increased at both image and repository level + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct = &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + responseImages = responseStruct.GlobalSearchResult.GlobalSearch.Images + So(responseImages, ShouldNotBeEmpty) + responseImage = responseImages[0] + // Started with initialDownloadCount of 1 (from SignImageUsingCosign), added 3 more downloads + So(responseImage.DownloadCount, ShouldEqual, initialDownloadCount+3) + + // Verify repository-level download count also increased - query repository separately + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct = &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + repos = repoResponseStruct.GlobalSearchResult.GlobalSearch.Repos + So(repos, ShouldNotBeEmpty) + So(repos[0].DownloadCount, ShouldEqual, initialRepoDownloadCount+3) + }) + Convey("Index base images", func() { // ---------------- BASE IMAGE ------------------- imageAMD64 := CreateImageWith().LayerBlobs([][]byte{ @@ -5916,6 +6130,26 @@ func TestMetaDBWhenReadingImages(t *testing.T) { So(err, ShouldBeNil) So(responseStruct.Images, ShouldNotBeEmpty) So(responseStruct.Images[0].DownloadCount, ShouldEqual, 3) + + // Verify repository-level download count also increased - query repository separately + repoQuery := ` + { + GlobalSearch(query:"repo1"){ + Repos { + Name DownloadCount + } + } + }` + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct := &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + So(repoResponseStruct.Repos, ShouldNotBeEmpty) + So(repoResponseStruct.Repos[0].DownloadCount, ShouldEqual, 3) }) Convey("Error when incrementing", func() { @@ -5930,6 +6164,87 @@ func TestMetaDBWhenReadingImages(t *testing.T) { So(resp.StatusCode(), ShouldEqual, http.StatusInternalServerError) }) }) + + Convey("Push test image with Docker media types", t, func() { + dir := t.TempDir() + + port := GetFreePort() + baseURL := GetBaseURL(port) + conf := config.New() + conf.HTTP.Port = port + conf.HTTP.Compat = []compat.MediaCompatibility{compat.DockerManifestV2SchemaV2} + conf.Storage.RootDirectory = dir + defaultVal := true + conf.Extensions = &extconf.ExtensionConfig{ + Search: &extconf.SearchConfig{BaseConfig: extconf.BaseConfig{Enable: &defaultVal}}, + } + + ctlr := api.NewController(conf) + + ctlrManager := NewControllerManager(ctlr) + ctlrManager.StartAndWait(port) + defer ctlrManager.StopServer() + + image := CreateImageWith().RandomLayers(1, 100).DefaultConfig().Build().AsDockerImage() + + err := UploadImage(image, baseURL, "repo2", "2.0.1") + So(err, ShouldBeNil) + + Convey("Download 3 times", func() { + resp, err := resty.R().Get(baseURL + "/v2/" + "repo2" + "/manifests/" + "2.0.1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + "repo2" + "/manifests/" + "2.0.1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = resty.R().Get(baseURL + "/v2/" + "repo2" + "/manifests/" + "2.0.1") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + query := ` + { + GlobalSearch(query:"repo2:2.0"){ + Images { + RepoName Tag DownloadCount + } + } + }` + + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + responseStruct := &zcommon.GlobalSearchResultResp{} + + err = json.Unmarshal(resp.Body(), responseStruct) + So(err, ShouldBeNil) + So(responseStruct.Images, ShouldNotBeEmpty) + So(responseStruct.Images[0].DownloadCount, ShouldEqual, 3) + + // Verify repository-level download count also increased - query repository separately + repoQuery := ` + { + GlobalSearch(query:"repo2"){ + Repos { + Name DownloadCount + } + } + }` + resp, err = resty.R().Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(repoQuery)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + repoResponseStruct := &zcommon.GlobalSearchResultResp{} + err = json.Unmarshal(resp.Body(), repoResponseStruct) + So(err, ShouldBeNil) + So(repoResponseStruct.Repos, ShouldNotBeEmpty) + So(repoResponseStruct.Repos[0].DownloadCount, ShouldEqual, 3) + }) + }) } func TestMetaDBWhenDeletingImages(t *testing.T) { diff --git a/pkg/meta/boltdb/boltdb.go b/pkg/meta/boltdb/boltdb.go index 2b712487..936a1e57 100644 --- a/pkg/meta/boltdb/boltdb.go +++ b/pkg/meta/boltdb/boltdb.go @@ -247,15 +247,25 @@ func (bdw *BoltDB) SetRepoReference(ctx context.Context, repo string, reference } } - if _, ok := protoRepoMeta.Statistics[imageMeta.Digest.String()]; !ok { - protoRepoMeta.Statistics[imageMeta.Digest.String()] = &proto_go.DescriptorStatistics{ + digestStr := imageMeta.Digest.String() + stats, ok := protoRepoMeta.Statistics[digestStr] + + if !ok { + stats = &proto_go.DescriptorStatistics{ DownloadCount: 0, LastPullTimestamp: ×tamppb.Timestamp{}, PushTimestamp: timestamppb.Now(), PushedBy: userid, } - } else if protoRepoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp.AsTime().IsZero() { - protoRepoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp = timestamppb.Now() + protoRepoMeta.Statistics[digestStr] = stats + } else { + if stats.PushTimestamp.AsTime().IsZero() { + stats.PushTimestamp = timestamppb.Now() + } + + if userid != "" && stats.PushedBy == "" { + stats.PushedBy = userid + } } if _, ok := protoRepoMeta.Signatures[imageMeta.Digest.String()]; !ok { @@ -1250,7 +1260,30 @@ func (bdw *BoltDB) UpdateStatsOnDownload(repo string, reference string) error { manifestStatistics, ok := protoRepoMeta.Statistics[manifestDigest] if !ok { - return zerr.ErrImageMetaNotFound + // Statistics entry doesn't exist - validate digest exists in this repository before creating it + // Check if digest is referenced in any tag for this repository + digestExists := false + + for _, tagDescriptor := range protoRepoMeta.Tags { + if tagDescriptor.Digest == manifestDigest { + digestExists = true + + break + } + } + + if !digestExists { + return zerr.ErrImageMetaNotFound + } + + // Statistics entry doesn't exist - create it + // This can happen if SetRepoReference failed or wasn't called + manifestStatistics = &proto_go.DescriptorStatistics{ + DownloadCount: 0, + LastPullTimestamp: ×tamppb.Timestamp{}, + PushTimestamp: ×tamppb.Timestamp{}, // Unknown push time + PushedBy: "", // Unknown pusher + } } manifestStatistics.DownloadCount++ diff --git a/pkg/meta/boltdb/boltdb_test.go b/pkg/meta/boltdb/boltdb_test.go index 7d4f53d2..c261e630 100644 --- a/pkg/meta/boltdb/boltdb_test.go +++ b/pkg/meta/boltdb/boltdb_test.go @@ -190,6 +190,111 @@ func TestWrapperErrors(t *testing.T) { err = boltdbWrapper.UpdateStatsOnDownload("repo", godigest.FromString("not-found").String()) So(err, ShouldNotBeNil) }) + + Convey("statistics entry missing but digest exists in tags - should create and increment", func() { + // Set repo reference to create tag + err := boltdbWrapper.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry to simulate missing Statistics + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + repoMetaBuck := tx.Bucket([]byte(boltdb.RepoMetaBuck)) + repoMetaBlob := repoMetaBuck.Get([]byte("repo")) + + if len(repoMetaBlob) == 0 { + return zerr.ErrRepoMetaNotFound + } + + var protoRepoMeta proto_go.RepoMeta + + err := proto.Unmarshal(repoMetaBlob, &protoRepoMeta) + if err != nil { + return err + } + + // Remove Statistics entry for the digest + delete(protoRepoMeta.Statistics, imageMeta.Digest.String()) + + repoMetaBlob, err = proto.Marshal(&protoRepoMeta) + if err != nil { + return err + } + + return repoMetaBuck.Put([]byte("repo"), repoMetaBlob) + }) + So(err, ShouldBeNil) + + // Verify Statistics entry doesn't exist + repoMeta, err := boltdbWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + _, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeFalse) + + // Update stats - should create Statistics entry and increment + err = boltdbWrapper.UpdateStatsOnDownload("repo", "tag") + So(err, ShouldBeNil) + + // Verify Statistics entry was created and incremented + repoMeta, err = boltdbWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + + // Update stats again - should increment existing entry + err = boltdbWrapper.UpdateStatsOnDownload("repo", "tag") + So(err, ShouldBeNil) + + repoMeta, err = boltdbWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 2) + }) + + Convey("statistics entry missing but digest exists in tags - using digest reference", func() { + // Set repo reference to create tag + err := boltdbWrapper.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + repoMetaBuck := tx.Bucket([]byte(boltdb.RepoMetaBuck)) + repoMetaBlob := repoMetaBuck.Get([]byte("repo")) + + if len(repoMetaBlob) == 0 { + return zerr.ErrRepoMetaNotFound + } + + var protoRepoMeta proto_go.RepoMeta + + err := proto.Unmarshal(repoMetaBlob, &protoRepoMeta) + if err != nil { + return err + } + + delete(protoRepoMeta.Statistics, imageMeta.Digest.String()) + + repoMetaBlob, err = proto.Marshal(&protoRepoMeta) + if err != nil { + return err + } + + return repoMetaBuck.Put([]byte("repo"), repoMetaBlob) + }) + So(err, ShouldBeNil) + + // Update stats using digest directly + err = boltdbWrapper.UpdateStatsOnDownload("repo", imageMeta.Digest.String()) + So(err, ShouldBeNil) + + // Verify Statistics entry was created + repoMeta, err := boltdbWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + }) }) Convey("GetReferrersInfo", func() { diff --git a/pkg/meta/convert/convert.go b/pkg/meta/convert/convert.go index fb75b171..28d0879a 100644 --- a/pkg/meta/convert/convert.go +++ b/pkg/meta/convert/convert.go @@ -35,15 +35,17 @@ func GetHistory(history []*proto_go.History) []ispec.History { } func GetImageArtifactType(imageMeta *proto_go.ImageMeta) string { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): manifestArtifactType := imageMeta.GetManifests()[0].GetManifest().GetArtifactType() if manifestArtifactType != "" { return manifestArtifactType } return imageMeta.GetManifests()[0].GetManifest().GetConfig().GetMediaType() - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return imageMeta.GetIndex().GetIndex().GetArtifactType() default: return "" @@ -51,10 +53,12 @@ func GetImageArtifactType(imageMeta *proto_go.ImageMeta) string { } func GetImageManifestSize(imageMeta *proto_go.ImageMeta) int64 { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): return imageMeta.GetManifests()[0].GetSize() - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return imageMeta.GetIndex().GetSize() default: return 0 @@ -62,10 +66,12 @@ func GetImageManifestSize(imageMeta *proto_go.ImageMeta) int64 { } func GetImageDigest(imageMeta *proto_go.ImageMeta) godigest.Digest { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): return godigest.Digest(imageMeta.GetManifests()[0].GetDigest()) - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return godigest.Digest(imageMeta.GetIndex().GetDigest()) default: return "" @@ -73,10 +79,12 @@ func GetImageDigest(imageMeta *proto_go.ImageMeta) godigest.Digest { } func GetImageDigestStr(imageMeta *proto_go.ImageMeta) string { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): return imageMeta.GetManifests()[0].GetDigest() - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return imageMeta.GetIndex().GetDigest() default: return "" @@ -84,10 +92,12 @@ func GetImageDigestStr(imageMeta *proto_go.ImageMeta) string { } func GetImageAnnotations(imageMeta *proto_go.ImageMeta) map[string]string { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): return imageMeta.GetManifests()[0].GetManifest().GetAnnotations() - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return imageMeta.GetIndex().GetIndex().GetAnnotations() default: return map[string]string{} @@ -95,14 +105,16 @@ func GetImageAnnotations(imageMeta *proto_go.ImageMeta) map[string]string { } func GetImageSubject(imageMeta *proto_go.ImageMeta) *ispec.Descriptor { - switch imageMeta.GetMediaType() { - case ispec.MediaTypeImageManifest: + mediaType := imageMeta.GetMediaType() + + switch { + case mediaType == ispec.MediaTypeImageManifest || compat.IsCompatibleManifestMediaType(mediaType): if imageMeta.GetManifests()[0].GetManifest().GetSubject() == nil { return nil } return GetDescriptorRef(imageMeta.GetManifests()[0].GetManifest().GetSubject()) - case ispec.MediaTypeImageIndex: + case mediaType == ispec.MediaTypeImageIndex || compat.IsCompatibleManifestListMediaType(mediaType): return GetDescriptorRef(imageMeta.GetIndex().GetIndex().GetSubject()) default: return nil @@ -530,7 +542,8 @@ func GetImageMeta(dbImageMeta *proto_go.ImageMeta) mTypes.ImageMeta { Digest: GetImageDigest(dbImageMeta), } - if dbImageMeta.GetMediaType() == ispec.MediaTypeImageIndex { + if dbImageMeta.GetMediaType() == ispec.MediaTypeImageIndex || + compat.IsCompatibleManifestListMediaType(dbImageMeta.GetMediaType()) { manifests := make([]ispec.Descriptor, 0, len(dbImageMeta.GetManifests())) for _, manifest := range dbImageMeta.GetIndex().GetIndex().GetManifests() { diff --git a/pkg/meta/dynamodb/dynamodb.go b/pkg/meta/dynamodb/dynamodb.go index fbc73736..818f4427 100644 --- a/pkg/meta/dynamodb/dynamodb.go +++ b/pkg/meta/dynamodb/dynamodb.go @@ -425,15 +425,25 @@ func (dwr *DynamoDB) SetRepoReference(ctx context.Context, repo string, referenc } } - if _, ok := repoMeta.Statistics[imageMeta.Digest.String()]; !ok { - repoMeta.Statistics[imageMeta.Digest.String()] = &proto_go.DescriptorStatistics{ + digestStr := imageMeta.Digest.String() + stats, ok := repoMeta.Statistics[digestStr] + + if !ok { + stats = &proto_go.DescriptorStatistics{ DownloadCount: 0, LastPullTimestamp: ×tamppb.Timestamp{}, PushTimestamp: timestamppb.Now(), PushedBy: userid, } - } else if repoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp.AsTime().IsZero() { - repoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp = timestamppb.Now() + repoMeta.Statistics[digestStr] = stats + } else { + if stats.PushTimestamp.AsTime().IsZero() { + stats.PushTimestamp = timestamppb.Now() + } + + if userid != "" && stats.PushedBy == "" { + stats.PushedBy = userid + } } if _, ok := repoMeta.Signatures[imageMeta.Digest.String()]; !ok { @@ -1157,7 +1167,30 @@ func (dwr *DynamoDB) UpdateStatsOnDownload(repo string, reference string) error manifestStatistics, ok := repoMeta.Statistics[descriptorDigest] if !ok { - return zerr.ErrImageMetaNotFound + // Statistics entry doesn't exist - validate digest exists in this repository before creating it + // Check if digest is referenced in any tag for this repository + digestExists := false + + for _, tagDescriptor := range repoMeta.Tags { + if tagDescriptor.Digest == descriptorDigest { + digestExists = true + + break + } + } + + if !digestExists { + return zerr.ErrImageMetaNotFound + } + + // Statistics entry doesn't exist - create it + // This can happen if SetRepoReference failed or wasn't called + manifestStatistics = &proto_go.DescriptorStatistics{ + DownloadCount: 0, + LastPullTimestamp: ×tamppb.Timestamp{}, + PushTimestamp: ×tamppb.Timestamp{}, // Unknown push time + PushedBy: "", // Unknown pusher + } } manifestStatistics.DownloadCount++ diff --git a/pkg/meta/dynamodb/dynamodb_test.go b/pkg/meta/dynamodb/dynamodb_test.go index f8b5cf9a..91e91acd 100644 --- a/pkg/meta/dynamodb/dynamodb_test.go +++ b/pkg/meta/dynamodb/dynamodb_test.go @@ -334,6 +334,74 @@ func TestWrapperErrors(t *testing.T) { err = dynamoWrapper.UpdateStatsOnDownload("repo", godigest.FromString("not-found").String()) //nolint: contextcheck So(err, ShouldNotBeNil) }) + + Convey("statistics entry missing but digest exists in tags - should create and increment", func() { + // Set repo reference to create tag + err := dynamoWrapper.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry to simulate missing Statistics + repoMeta, err := dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + delete(repoMeta.Statistics, imageMeta.Digest.String()) + + // Set repo meta back without Statistics + err = dynamoWrapper.SetRepoMeta("repo", repoMeta) + So(err, ShouldBeNil) + + // Verify Statistics entry doesn't exist + repoMeta, err = dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + _, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeFalse) + + // Update stats - should create Statistics entry and increment + err = dynamoWrapper.UpdateStatsOnDownload("repo", "tag") //nolint: contextcheck + So(err, ShouldBeNil) + + // Verify Statistics entry was created and incremented + repoMeta, err = dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + + // Update stats again - should increment existing entry + err = dynamoWrapper.UpdateStatsOnDownload("repo", "tag") //nolint: contextcheck + So(err, ShouldBeNil) + + repoMeta, err = dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 2) + }) + + Convey("statistics entry missing but digest exists in tags - using digest reference", func() { + // Set repo reference to create tag + err := dynamoWrapper.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry + repoMeta, err := dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + delete(repoMeta.Statistics, imageMeta.Digest.String()) + + // Set repo meta back without Statistics + err = dynamoWrapper.SetRepoMeta("repo", repoMeta) + So(err, ShouldBeNil) + + // Update stats using digest directly + err = dynamoWrapper.UpdateStatsOnDownload("repo", imageMeta.Digest.String()) //nolint: contextcheck + So(err, ShouldBeNil) + + // Verify Statistics entry was created + repoMeta, err = dynamoWrapper.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + }) }) Convey("GetReferrersInfo", func() { Convey("unmarshalProtoRepoMeta error", func() { diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 92e03b94..480f4611 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -847,6 +847,103 @@ func RunMetaDBTests(t *testing.T, metaDB mTypes.MetaDB, preparationFuncs ...func So(err, ShouldBeNil) So(*repoMeta.LastUpdatedImage.LastUpdated, ShouldEqual, time.Date(2011, 3, 1, 12, 0, 0, 0, time.UTC)) }) + + Convey("PushedBy field behavior", func() { + userAc := reqCtx.NewUserAccessControl() + userAc.SetUsername("test-user") + ctxWithUser := userAc.DeriveContext(context.Background()) + + emptyCtx := context.Background() + + Convey("PushedBy is set when userid provided and PushedBy is empty", func() { + // Set repo reference with userid + err := metaDB.SetRepoReference(ctxWithUser, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is set + repoMeta, err := metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "test-user") + }) + + Convey("PushedBy is NOT overwritten when it already has a value", func() { + // Set repo reference with first user + err := metaDB.SetRepoReference(ctxWithUser, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is set + repoMeta, err := metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "test-user") + + // Try to set repo reference again with different userid + userAc2 := reqCtx.NewUserAccessControl() + userAc2.SetUsername("different-user") + ctx2 := userAc2.DeriveContext(context.Background()) + + err = metaDB.SetRepoReference(ctx2, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is NOT overwritten + repoMeta, err = metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "test-user") // Still the original value + }) + + Convey("PushedBy is set when Statistics entry exists but PushedBy is empty", func() { + // Create repo reference without userid (empty context) + err := metaDB.SetRepoReference(emptyCtx, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is empty + repoMeta, err := metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "") + + // Set repo reference again with userid + err = metaDB.SetRepoReference(ctxWithUser, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is now set + repoMeta, err = metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "test-user") + }) + + Convey("PushedBy is NOT set when userid is empty", func() { + // Set repo reference without userid (empty context) + err := metaDB.SetRepoReference(emptyCtx, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is empty + repoMeta, err := metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "") + + // Set repo reference again without userid + err = metaDB.SetRepoReference(emptyCtx, repo1, tag1, imgData1) + So(err, ShouldBeNil) + + // Verify PushedBy is still empty + repoMeta, err = metaDB.GetRepoMeta(ctxWithUser, repo1) + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imgData1.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.PushedBy, ShouldEqual, "") + }) + }) }) Convey("Test RemoveRepoReference", func() { diff --git a/pkg/meta/redis/redis.go b/pkg/meta/redis/redis.go index b64639b6..ece03419 100644 --- a/pkg/meta/redis/redis.go +++ b/pkg/meta/redis/redis.go @@ -811,15 +811,25 @@ func (rc *RedisDB) SetRepoReference(ctx context.Context, repo string, } } - if _, ok := protoRepoMeta.Statistics[imageMeta.Digest.String()]; !ok { - protoRepoMeta.Statistics[imageMeta.Digest.String()] = &proto_go.DescriptorStatistics{ + digestStr := imageMeta.Digest.String() + stats, ok := protoRepoMeta.Statistics[digestStr] + + if !ok { + stats = &proto_go.DescriptorStatistics{ DownloadCount: 0, LastPullTimestamp: ×tamppb.Timestamp{}, PushTimestamp: timestamppb.Now(), PushedBy: userid, } - } else if protoRepoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp.AsTime().IsZero() { - protoRepoMeta.Statistics[imageMeta.Digest.String()].PushTimestamp = timestamppb.Now() + protoRepoMeta.Statistics[digestStr] = stats + } else { + if stats.PushTimestamp.AsTime().IsZero() { + stats.PushTimestamp = timestamppb.Now() + } + + if userid != "" && stats.PushedBy == "" { + stats.PushedBy = userid + } } if _, ok := protoRepoMeta.Signatures[imageMeta.Digest.String()]; !ok { @@ -1753,7 +1763,30 @@ func (rc *RedisDB) UpdateStatsOnDownload(repo string, reference string) error { manifestStatistics, ok := protoRepoMeta.Statistics[manifestDigest] if !ok { - return zerr.ErrImageMetaNotFound + // Statistics entry doesn't exist - validate digest exists in this repository before creating it + // Check if digest is referenced in any tag for this repository + digestExists := false + + for _, tagDescriptor := range protoRepoMeta.Tags { + if tagDescriptor.Digest == manifestDigest { + digestExists = true + + break + } + } + + if !digestExists { + return zerr.ErrImageMetaNotFound + } + + // Statistics entry doesn't exist - create it + // This can happen if SetRepoReference failed or wasn't called + manifestStatistics = &proto_go.DescriptorStatistics{ + DownloadCount: 0, + LastPullTimestamp: ×tamppb.Timestamp{}, + PushTimestamp: ×tamppb.Timestamp{}, // Unknown push time + PushedBy: "", // Unknown pusher + } } manifestStatistics.DownloadCount++ diff --git a/pkg/meta/redis/redis_test.go b/pkg/meta/redis/redis_test.go index bf1272be..6fcf7a6f 100644 --- a/pkg/meta/redis/redis_test.go +++ b/pkg/meta/redis/redis_test.go @@ -717,6 +717,87 @@ func TestWrapperErrors(t *testing.T) { err = metaDB.UpdateStatsOnDownload("repo", godigest.FromString("not-found").String()) So(err, ShouldNotBeNil) }) + + Convey("statistics entry missing but digest exists in tags - should create and increment", func() { + // Set repo reference to create tag + err := metaDB.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry to simulate missing Statistics + // Get proto blob directly from Redis + key := keyPrefix + ":" + redis.RepoMetaBucket + repoMetaBlob, err := client.HGet(ctx, key, "repo").Bytes() + So(err, ShouldBeNil) + + var protoRepoMeta proto_go.RepoMeta + err = proto.Unmarshal(repoMetaBlob, &protoRepoMeta) + So(err, ShouldBeNil) + delete(protoRepoMeta.Statistics, imageMeta.Digest.String()) + + repoMetaBlob, err = proto.Marshal(&protoRepoMeta) + So(err, ShouldBeNil) + err = setRepoMeta("repo", repoMetaBlob, client) + So(err, ShouldBeNil) + + // Verify Statistics entry doesn't exist + repoMeta, err := metaDB.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + _, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeFalse) + + // Update stats - should create Statistics entry and increment + err = metaDB.UpdateStatsOnDownload("repo", "tag") + So(err, ShouldBeNil) + + // Verify Statistics entry was created and incremented + repoMeta, err = metaDB.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + + // Update stats again - should increment existing entry + err = metaDB.UpdateStatsOnDownload("repo", "tag") + So(err, ShouldBeNil) + + repoMeta, err = metaDB.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists = repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 2) + }) + + Convey("statistics entry missing but digest exists in tags - using digest reference", func() { + // Set repo reference to create tag + err := metaDB.SetRepoReference(ctx, "repo", "tag", imageMeta) + So(err, ShouldBeNil) + + // Manually remove Statistics entry + key := keyPrefix + ":" + redis.RepoMetaBucket + repoMetaBlob, err := client.HGet(ctx, key, "repo").Bytes() + So(err, ShouldBeNil) + + var protoRepoMeta proto_go.RepoMeta + err = proto.Unmarshal(repoMetaBlob, &protoRepoMeta) + So(err, ShouldBeNil) + delete(protoRepoMeta.Statistics, imageMeta.Digest.String()) + + repoMetaBlob, err = proto.Marshal(&protoRepoMeta) + So(err, ShouldBeNil) + err = setRepoMeta("repo", repoMetaBlob, client) + So(err, ShouldBeNil) + + // Update stats using digest directly + err = metaDB.UpdateStatsOnDownload("repo", imageMeta.Digest.String()) + So(err, ShouldBeNil) + + // Verify Statistics entry was created + repoMeta, err := metaDB.GetRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + stats, exists := repoMeta.Statistics[imageMeta.Digest.String()] + So(exists, ShouldBeTrue) + So(stats.DownloadCount, ShouldEqual, 1) + }) }) Convey("GetReferrersInfo", func() { diff --git a/pkg/test/image-utils/images.go b/pkg/test/image-utils/images.go index 419df31c..da0386eb 100644 --- a/pkg/test/image-utils/images.go +++ b/pkg/test/image-utils/images.go @@ -193,7 +193,7 @@ func (img Image) AsDockerImage() Image { } img.ManifestDescriptor = ispec.Descriptor{ - MediaType: docker.MediaTypeImageConfig, + MediaType: docker.MediaTypeManifest, Digest: img.digestAlgorithm.FromBytes(manifestBlob), Size: int64(len(manifestBlob)), Data: manifestBlob, diff --git a/pkg/test/image-utils/upload.go b/pkg/test/image-utils/upload.go index 43bd520c..b5f4bf8d 100644 --- a/pkg/test/image-utils/upload.go +++ b/pkg/test/image-utils/upload.go @@ -118,8 +118,19 @@ func UploadImage(img Image, baseURL, repo, ref string) error { } } + // Use the media type from ManifestDescriptor, or fall back to Manifest.MediaType, or default to OCI + mediaType := img.ManifestDescriptor.MediaType + + if mediaType == "" { + mediaType = img.Manifest.MediaType + } + + if mediaType == "" { + mediaType = ispec.MediaTypeImageManifest + } + resp, err = resty.R(). - SetHeader("Content-type", ispec.MediaTypeImageManifest). + SetHeader("Content-type", mediaType). SetBody(manifestBlob). Put(baseURL + "/v2/" + repo + "/manifests/" + ref) @@ -216,9 +227,20 @@ func UploadImageWithBasicAuth(img Image, baseURL, repo, ref, user, password stri return err } + // Use the media type from ManifestDescriptor, or fall back to Manifest.MediaType, or default to OCI + mediaType := img.ManifestDescriptor.MediaType + + if mediaType == "" { + mediaType = img.Manifest.MediaType + } + + if mediaType == "" { + mediaType = ispec.MediaTypeImageManifest + } + _, err = resty.R(). SetBasicAuth(user, password). - SetHeader("Content-type", "application/vnd.oci.image.manifest.v1+json"). + SetHeader("Content-type", mediaType). SetBody(manifestBlob). Put(baseURL + "/v2/" + repo + "/manifests/" + ref) @@ -245,8 +267,19 @@ func UploadMultiarchImage(multiImage MultiarchImage, baseURL string, repo, ref s } } + // Use the media type from IndexDescriptor, or fall back to Index.MediaType, or default to OCI + mediaType := multiImage.IndexDescriptor.MediaType + + if mediaType == "" { + mediaType = multiImage.Index.MediaType + } + + if mediaType == "" { + mediaType = ispec.MediaTypeImageIndex + } + resp, err := resty.R(). - SetHeader("Content-type", ispec.MediaTypeImageIndex). + SetHeader("Content-type", mediaType). SetBody(indexBlob). Put(baseURL + "/v2/" + repo + "/manifests/" + ref)