diff --git a/pkg/meta/common/common.go b/pkg/meta/common/common.go index d37dc9e4..d3a3e6e4 100644 --- a/pkg/meta/common/common.go +++ b/pkg/meta/common/common.go @@ -275,8 +275,18 @@ func RemoveImageFromRepoMeta(repoMeta *proto_go.RepoMeta, repoBlobs *proto_go.Re queue := []string{descriptor.Digest} + // Check if blob info exists before accessing it to prevent nil pointer dereference. + // When a tag is skipped due to nil blob info, that tag remains in repoMeta.Tags but + // won't have any associated blobs in the result, creating metadata inconsistency. + // This is acceptable in GC/cleanup scenarios where data may already be inconsistent + // due to partial deletions or corruption. + descriptorBlobInfo := repoBlobs.Blobs[descriptor.Digest] + if descriptorBlobInfo == nil { + continue + } + updatedLastImage = mConvert.GetProtoEarlierUpdatedImage(updatedLastImage, &proto_go.RepoLastUpdatedImage{ - LastUpdated: repoBlobs.Blobs[descriptor.Digest].LastUpdated, + LastUpdated: descriptorBlobInfo.LastUpdated, MediaType: descriptor.MediaType, Digest: descriptor.Digest, Tag: tag, @@ -288,6 +298,9 @@ func RemoveImageFromRepoMeta(repoMeta *proto_go.RepoMeta, repoBlobs *proto_go.Re if _, found := updatedBlobs[currentBlob]; !found { blobInfo := repoBlobs.Blobs[currentBlob] + if blobInfo == nil { + continue + } updatedBlobs[currentBlob] = blobInfo updatedSize += blobInfo.Size diff --git a/pkg/meta/common/common_test.go b/pkg/meta/common/common_test.go index 125a5327..ba012c76 100644 --- a/pkg/meta/common/common_test.go +++ b/pkg/meta/common/common_test.go @@ -6,8 +6,10 @@ import ( "time" . "github.com/smartystreets/goconvey/convey" + "google.golang.org/protobuf/types/known/timestamppb" "zotregistry.dev/zot/v2/pkg/meta/common" + proto_go "zotregistry.dev/zot/v2/pkg/meta/proto/gen" mTypes "zotregistry.dev/zot/v2/pkg/meta/types" ) @@ -126,4 +128,395 @@ func TestUtils(t *testing.T) { So(res, ShouldEqual, false) }) + + Convey("RemoveImageFromRepoMeta", t, func() { + Convey("should handle nil blob info for descriptor digest and continue with other tags", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag1": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest1", + }, + "tag-missing": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:missing", + }, + "tag2": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest2", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + "sha256:manifest1": { + Size: 1000, + LastUpdated: timestamppb.New(now), + SubBlobs: []string{"sha256:layer1"}, + }, + "sha256:layer1": { + Size: 500, + }, + // Intentionally missing "sha256:missing" for tag-missing to test nil check + // The function should skip tag-missing and continue processing tag2 + "sha256:manifest2": { + Size: 2000, + LastUpdated: timestamppb.New(now.Add(time.Hour)), + SubBlobs: []string{"sha256:layer2"}, + }, + "sha256:layer2": { + Size: 800, + }, + }, + } + + // Remove tag1 (simulating actual usage pattern) + delete(repoMeta.Tags, "tag1") + + // Should not panic when tag-missing has nil blob info + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // tag-missing remains in metadata but has no blobs (inconsistent state, acceptable in GC scenarios) + So(resultMeta.Tags["tag-missing"], ShouldNotBeNil) + + // Should include blobs from tag2 (which has valid blob info) + So(len(resultBlobs.Blobs), ShouldEqual, 2) + So(resultBlobs.Blobs["sha256:manifest2"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:layer2"], ShouldNotBeNil) + + // Should have correct size from tag2 only + expectedSize := int64(2000 + 800) + So(resultMeta.Size, ShouldEqual, expectedSize) + + // Should have updated last image from tag2 + So(resultMeta.LastUpdatedImage, ShouldNotBeNil) + So(resultMeta.LastUpdatedImage.Digest, ShouldEqual, "sha256:manifest2") + }) + + Convey("should handle nil blob info in queue traversal and continue processing", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag1": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest1", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + "sha256:manifest1": { + Size: 1000, + LastUpdated: timestamppb.New(now), + // Mix of valid and missing sub-blobs to test that processing continues + SubBlobs: []string{"sha256:layer1", "sha256:missing-layer", "sha256:layer2"}, + }, + "sha256:layer1": { + Size: 500, + }, + // Intentionally missing "sha256:missing-layer" to trigger nil check in queue traversal + // The function should skip it and continue processing layer2 + "sha256:layer2": { + Size: 300, + SubBlobs: []string{"sha256:layer3"}, + }, + "sha256:layer3": { + Size: 200, + }, + }, + } + + // Remove the tag before calling RemoveImageFromRepoMeta (as done in actual usage) + delete(repoMeta.Tags, "tag1") + + // Should not panic when a sub-blob is nil + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // Verify tag1 was removed + So(resultMeta.Tags["tag1"], ShouldBeNil) + + // After removing tag1, no blobs should remain + So(len(resultBlobs.Blobs), ShouldEqual, 0) + }) + + Convey("should handle multiple nil blobs in deeply nested structure", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag-valid": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest1", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + "sha256:manifest1": { + Size: 1000, + LastUpdated: timestamppb.New(now), + // Multiple missing sub-blobs interspersed with valid ones + SubBlobs: []string{ + "sha256:missing1", + "sha256:layer1", + "sha256:missing2", + "sha256:layer2", + "sha256:missing3", + }, + }, + "sha256:layer1": { + Size: 500, + SubBlobs: []string{"sha256:missing4", "sha256:nested-layer"}, + }, + "sha256:layer2": { + Size: 300, + }, + "sha256:nested-layer": { + Size: 100, + }, + // Intentionally missing: missing1, missing2, missing3, missing4 + }, + } + + // Should not panic with multiple missing blobs at various levels + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "nonexistent") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "nonexistent") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // Should only include the valid blobs that were successfully traversed + So(len(resultBlobs.Blobs), ShouldEqual, 4) // manifest1, layer1, layer2, nested-layer + So(resultBlobs.Blobs["sha256:manifest1"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:layer1"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:layer2"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:nested-layer"], ShouldNotBeNil) + + // Verify correct size calculation (only valid blobs) + expectedSize := int64(1000 + 500 + 300 + 100) + So(resultMeta.Size, ShouldEqual, expectedSize) + }) + + Convey("should work correctly with valid blob info", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag1": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest1", + }, + "tag2": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest2", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + "sha256:manifest1": { + Size: 1000, + LastUpdated: timestamppb.New(now), + SubBlobs: []string{"sha256:layer1"}, + Vendors: []string{"vendor1"}, + Platforms: []*proto_go.Platform{{OS: "linux", Architecture: "amd64"}}, + }, + "sha256:layer1": { + Size: 500, + }, + "sha256:manifest2": { + Size: 2000, + LastUpdated: timestamppb.New(now.Add(time.Hour)), + SubBlobs: []string{"sha256:layer2"}, + }, + "sha256:layer2": { + Size: 800, + }, + }, + } + + // Remove the tag before calling RemoveImageFromRepoMeta (as done in actual usage) + delete(repoMeta.Tags, "tag1") + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // Verify tag1 was removed + So(resultMeta.Tags["tag1"], ShouldBeNil) + + // Should only include blobs from remaining tag2 (manifest2 and layer2) + So(len(resultBlobs.Blobs), ShouldEqual, 2) + So(resultBlobs.Blobs["sha256:manifest2"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:layer2"], ShouldNotBeNil) + + // Should calculate total size correctly (only tag2 blobs) + expectedSize := int64(2000 + 800) + So(resultMeta.Size, ShouldEqual, expectedSize) + + // Should have updated last image + So(resultMeta.LastUpdatedImage, ShouldNotBeNil) + }) + + Convey("should handle empty tags", func() { + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{}, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{}, + } + + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag1") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + So(resultMeta.Size, ShouldEqual, 0) + So(len(resultBlobs.Blobs), ShouldEqual, 0) + }) + + Convey("should skip tags with empty digest and continue processing", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag-empty": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "", // Empty digest - should be skipped + }, + "tag-valid": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest1", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + "sha256:manifest1": { + Size: 1000, + LastUpdated: timestamppb.New(now), + SubBlobs: []string{"sha256:layer1"}, + }, + "sha256:layer1": { + Size: 500, + }, + }, + } + + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag-empty") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "tag-empty") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // Should skip tag-empty and process tag-valid + So(len(resultBlobs.Blobs), ShouldEqual, 2) + So(resultBlobs.Blobs["sha256:manifest1"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:layer1"], ShouldNotBeNil) + + expectedSize := int64(1000 + 500) + So(resultMeta.Size, ShouldEqual, expectedSize) + }) + + Convey("should handle combined edge cases - empty digest, nil descriptor blob, and nil queue blob", func() { + now := time.Now() + repoMeta := &proto_go.RepoMeta{ + Name: "test-repo", + Tags: map[string]*proto_go.TagDescriptor{ + "tag-empty": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "", // Empty digest + }, + "tag-nil-descriptor": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:missing-descriptor", + }, + "tag-nil-in-queue": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:manifest-with-missing-blobs", + }, + "tag-valid": { + MediaType: "application/vnd.oci.image.manifest.v1+json", + Digest: "sha256:valid-manifest", + }, + }, + } + + repoBlobs := &proto_go.RepoBlobs{ + Blobs: map[string]*proto_go.BlobInfo{ + // Missing "sha256:missing-descriptor" to trigger descriptor nil check + "sha256:manifest-with-missing-blobs": { + Size: 1000, + LastUpdated: timestamppb.New(now), + SubBlobs: []string{"sha256:missing-in-queue", "sha256:valid-layer1"}, + }, + // Missing "sha256:missing-in-queue" to trigger queue nil check + "sha256:valid-layer1": { + Size: 300, + }, + "sha256:valid-manifest": { + Size: 2000, + LastUpdated: timestamppb.New(now.Add(2 * time.Hour)), + SubBlobs: []string{"sha256:valid-layer2"}, + }, + "sha256:valid-layer2": { + Size: 800, + }, + }, + } + + // Should not panic with multiple types of issues + So(func() { + common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "nonexistent") + }, ShouldNotPanic) + + resultMeta, resultBlobs := common.RemoveImageFromRepoMeta(repoMeta, repoBlobs, "nonexistent") + So(resultMeta, ShouldNotBeNil) + So(resultBlobs, ShouldNotBeNil) + + // Should include only valid blobs: + // - tag-valid's blobs (valid-manifest + valid-layer2) + // - tag-nil-in-queue's valid blobs (manifest-with-missing-blobs + valid-layer1) + So(len(resultBlobs.Blobs), ShouldEqual, 4) + So(resultBlobs.Blobs["sha256:valid-manifest"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:valid-layer2"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:manifest-with-missing-blobs"], ShouldNotBeNil) + So(resultBlobs.Blobs["sha256:valid-layer1"], ShouldNotBeNil) + + // Verify correct size (all valid blobs) + expectedSize := int64(2000 + 800 + 1000 + 300) + So(resultMeta.Size, ShouldEqual, expectedSize) + + // Last updated should be from the most recent valid blob + So(resultMeta.LastUpdatedImage, ShouldNotBeNil) + }) + }) } diff --git a/pkg/retention/candidate.go b/pkg/retention/candidate.go index ad470414..f1d42580 100644 --- a/pkg/retention/candidate.go +++ b/pkg/retention/candidate.go @@ -12,19 +12,26 @@ func GetCandidates(repoMeta mTypes.RepoMeta) []*types.Candidate { // collect all statistic of repo's manifests for tag, desc := range repoMeta.Tags { - for digestStr, stats := range repoMeta.Statistics { - if digestStr == desc.Digest { - candidate := &types.Candidate{ - MediaType: desc.MediaType, - DigestStr: digestStr, - Tag: tag, - PushTimestamp: stats.PushTimestamp, - PullTimestamp: stats.LastPullTimestamp, - } - - candidates = append(candidates, candidate) - } + // Check if statistics exist for this digest to prevent using zero-value statistics. + // When statistics are missing for a tag's digest, we skip creating a candidate for it. + // This prevents incorrect retention decisions based on zero-value timestamps (epoch time). + // The retention policy manager handles tags without statistics separately by keeping them + // (see GetRetainedTagsFromMetaDB in retention.go which explicitly retains tags not found + // in candidates list). + stats, hasStatistics := repoMeta.Statistics[desc.Digest] + if !hasStatistics { + continue } + + candidate := &types.Candidate{ + MediaType: desc.MediaType, + DigestStr: desc.Digest, + Tag: tag, + PushTimestamp: stats.PushTimestamp, + PullTimestamp: stats.LastPullTimestamp, + } + + candidates = append(candidates, candidate) } return candidates diff --git a/pkg/retention/candidate_test.go b/pkg/retention/candidate_test.go new file mode 100644 index 00000000..9f6ac43b --- /dev/null +++ b/pkg/retention/candidate_test.go @@ -0,0 +1,118 @@ +package retention_test + +import ( + "testing" + "time" + + . "github.com/smartystreets/goconvey/convey" + + mTypes "zotregistry.dev/zot/v2/pkg/meta/types" + "zotregistry.dev/zot/v2/pkg/retention" +) + +func TestGetCandidatesWithMissingStatistics(t *testing.T) { + Convey("GetCandidates should handle missing statistics gracefully", t, func() { + now := time.Now() + + Convey("With complete statistics", func() { + repoMeta := mTypes.RepoMeta{ + Name: "test-repo", + Tags: map[string]mTypes.Descriptor{ + "tag1": { + Digest: "sha256:digest1", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + "tag2": { + Digest: "sha256:digest2", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + }, + Statistics: map[string]mTypes.DescriptorStatistics{ + "sha256:digest1": { + PushTimestamp: now, + LastPullTimestamp: now, + DownloadCount: 5, + }, + "sha256:digest2": { + PushTimestamp: now.Add(-24 * time.Hour), + LastPullTimestamp: now.Add(-12 * time.Hour), + DownloadCount: 10, + }, + }, + } + + candidates := retention.GetCandidates(repoMeta) + + So(candidates, ShouldHaveLength, 2) + So(candidates[0].Tag, ShouldBeIn, "tag1", "tag2") + So(candidates[1].Tag, ShouldBeIn, "tag1", "tag2") + }) + + Convey("With missing statistics for one tag", func() { + repoMeta := mTypes.RepoMeta{ + Name: "test-repo", + Tags: map[string]mTypes.Descriptor{ + "tag1": { + Digest: "sha256:digest1", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + "tag2": { + Digest: "sha256:digest2", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + }, + Statistics: map[string]mTypes.DescriptorStatistics{ + "sha256:digest1": { + PushTimestamp: now, + LastPullTimestamp: now, + DownloadCount: 5, + }, + // tag2's digest has no statistics - simulates inconsistent metaDB state + }, + } + + candidates := retention.GetCandidates(repoMeta) + + // Should only return candidate for tag1, skipping tag2 with missing statistics + So(candidates, ShouldHaveLength, 1) + So(candidates[0].Tag, ShouldEqual, "tag1") + So(candidates[0].DigestStr, ShouldEqual, "sha256:digest1") + }) + + Convey("With no statistics at all", func() { + repoMeta := mTypes.RepoMeta{ + Name: "test-repo", + Tags: map[string]mTypes.Descriptor{ + "tag1": { + Digest: "sha256:digest1", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + }, + Statistics: map[string]mTypes.DescriptorStatistics{}, + } + + candidates := retention.GetCandidates(repoMeta) + + // Should return empty list - no candidates without statistics + So(candidates, ShouldHaveLength, 0) + }) + + Convey("With nil Statistics map", func() { + repoMeta := mTypes.RepoMeta{ + Name: "test-repo", + Tags: map[string]mTypes.Descriptor{ + "tag1": { + Digest: "sha256:digest1", + MediaType: "application/vnd.oci.image.manifest.v1+json", + }, + }, + Statistics: nil, + } + + candidates := retention.GetCandidates(repoMeta) + + // Should return empty list and not panic + So(candidates, ShouldHaveLength, 0) + }) + }) +}