From 008527b7bb014289247c7a40fbd0cf93cd96620c Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Thu, 13 Nov 2025 19:26:18 +0200 Subject: [PATCH] fix: gracefully handle manifests missing from storage (prepare for sparse indexes) (#3503) GC and scrub should not stop if a manifest or index is missing from storage. Other similar changes are also included. WRT metadb, the missing manifests cannot be added, and the results returned from metadb do not include the descriptors for these manifests. Signed-off-by: Andrei Aaron --- pkg/extensions/search/cve/scan_test.go | 5 + pkg/extensions/search/search_test.go | 92 +++++++ pkg/extensions/sync/destination.go | 22 ++ pkg/meta/boltdb/boltdb.go | 7 + pkg/meta/boltdb/boltdb_test.go | 158 ++++++++++- pkg/meta/dynamodb/dynamodb.go | 75 +++-- pkg/meta/dynamodb/dynamodb_test.go | 18 +- pkg/meta/meta_test.go | 69 +++++ pkg/meta/parse.go | 12 +- pkg/meta/parse_test.go | 74 ++++- pkg/meta/redis/redis.go | 7 + pkg/meta/redis/redis_test.go | 55 +++- pkg/storage/common/common.go | 36 +++ pkg/storage/common/common_test.go | 160 +++++++++++ pkg/storage/gc/gc.go | 54 ++++ pkg/storage/gc/gc_internal_test.go | 86 +++++- pkg/storage/local/local_test.go | 11 +- pkg/storage/scrub.go | 71 ++++- pkg/storage/scrub_test.go | 364 +++++++++++++++++++++---- pkg/storage/storage_test.go | 2 +- 20 files changed, 1240 insertions(+), 138 deletions(-) diff --git a/pkg/extensions/search/cve/scan_test.go b/pkg/extensions/search/cve/scan_test.go index e030b6b3..dd6db8d3 100644 --- a/pkg/extensions/search/cve/scan_test.go +++ b/pkg/extensions/search/cve/scan_test.go @@ -384,6 +384,11 @@ func TestScanGeneratorWithMockedData(t *testing.T) { //nolint: gocyclo return false, err } + // If all manifests are missing (e.g., from an index), Manifests will be empty + if len(manifestData.Manifests) == 0 { + return false, nil + } + for _, imageLayer := range manifestData.Manifests[0].Manifest.Layers { switch imageLayer.MediaType { case ispec.MediaTypeImageLayerGzip, ispec.MediaTypeImageLayer, string(regTypes.DockerLayer): diff --git a/pkg/extensions/search/search_test.go b/pkg/extensions/search/search_test.go index 4c76d527..631f0077 100644 --- a/pkg/extensions/search/search_test.go +++ b/pkg/extensions/search/search_test.go @@ -7406,6 +7406,98 @@ type repoRef struct { Tag string } +func TestSearchWithMissingManifest(t *testing.T) { + Convey("Search with missing manifest", t, func() { + dir := t.TempDir() + + // 1. Write the image to the disk + log := log.NewTestLogger() + storeCtlr := ociutils.GetDefaultStoreController(dir, log) + + // Create a multiarch image with exactly 2 manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + + // Write the multiarch image to filesystem + err := WriteMultiArchImageToFileSystem(multiarchImage, "testrepo", "latest", storeCtlr) + So(err, ShouldBeNil) + + // Get the image store to access index and manifests + imageStore := storeCtlr.GetDefaultImageStore() + + // Get the index content to find all manifest digests + indexBlob, err := imageStore.GetIndexContent("testrepo") + So(err, ShouldBeNil) + + var indexContent ispec.Index + err = json.Unmarshal(indexBlob, &indexContent) + So(err, ShouldBeNil) + + So(len(indexContent.Manifests), ShouldBeGreaterThanOrEqualTo, 2) + + // Get the first manifest digest to delete + firstManifestDigest := indexContent.Manifests[0].Digest + + // Get the second manifest digest (should remain valid) + secondManifestDigest := indexContent.Manifests[1].Digest + + // 2. Delete the manifest from the disk + manifestBlobPath := path.Join(dir, "testrepo", "blobs", "sha256", firstManifestDigest.Encoded()) + err = os.Remove(manifestBlobPath) + So(err, ShouldBeNil) + + // 3. Start the controller (MetaDB parsing would be done in the background) + port := GetFreePort() + baseURL := GetBaseURL(port) + conf := config.New() + conf.HTTP.Port = port + conf.Storage.RootDirectory = dir + defaultVal := true + conf.Extensions = &extconf.ExtensionConfig{ + Search: &extconf.SearchConfig{BaseConfig: extconf.BaseConfig{Enable: &defaultVal}}, + } + + conf.Extensions.Search.CVE = nil + + ctlr := api.NewController(conf) + + ctlrManager := NewControllerManager(ctlr) + ctlrManager.StartAndWait(port) + defer ctlrManager.StopServer() + + // Search for the repository + query := ` + { + GlobalSearch(query:"testrepo:latest"){ + Images { + RepoName Tag + Manifests { + Digest + } + } + } + }` + + 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) + + // Verify we found the image + So(responseStruct.GlobalSearchResult.GlobalSearch.Images, ShouldNotBeEmpty) + foundImage := responseStruct.GlobalSearchResult.GlobalSearch.Images[0] + So(foundImage.RepoName, ShouldEqual, "testrepo") + So(foundImage.Tag, ShouldEqual, "latest") + + // Verify only the valid manifest is found in search results (missing one was skipped by ParseStorage) + So(len(foundImage.Manifests), ShouldEqual, 1) + So(foundImage.Manifests[0].Digest, ShouldEqual, secondManifestDigest.String()) + }) +} + func deleteUsedImages(repoTags []repoRef, baseURL string) { for _, image := range repoTags { status, err := DeleteImage(image.Repo, image.Tag, baseURL) diff --git a/pkg/extensions/sync/destination.go b/pkg/extensions/sync/destination.go index 784fb352..be13ed83 100644 --- a/pkg/extensions/sync/destination.go +++ b/pkg/extensions/sync/destination.go @@ -12,6 +12,7 @@ import ( "path" "strings" + "github.com/distribution/distribution/v3/registry/storage/driver" godigest "github.com/opencontainers/go-digest" ispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/regclient/regclient/types/mediatype" @@ -227,11 +228,27 @@ func (registry *DestinationRegistry) copyManifest(repo string, desc ispec.Descri return err } + var firstMissingErr error + for _, manifest := range indexManifest.Manifests { reference := GetDescriptorReference(manifest) manifestBuf, err := tempImageStore.GetBlobContent(repo, manifest.Digest) if err != nil { + // Handle missing manifest blobs gracefully - log warning and continue with other manifests + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + if firstMissingErr == nil { + firstMissingErr = err + } + + registry.log.Warn().Err(err).Str("dir", path.Join(tempImageStore.RootDir(), repo)). + Str("digest", manifest.Digest.String()). + Msg("skipping missing manifest blob in image index, continuing sync with other manifests") + + continue + } + registry.log.Error().Str("errorType", common.TypeOf(err)). Err(err).Str("dir", path.Join(tempImageStore.RootDir(), repo)).Str("digest", manifest.Digest.String()). Msg("failed find manifest which is part of an image index") @@ -254,6 +271,11 @@ func (registry *DestinationRegistry) copyManifest(repo string, desc ispec.Descri } } + // Return error if we encountered any missing manifests + if firstMissingErr != nil { + return firstMissingErr + } + _, _, err := imageStore.PutImageManifest(repo, reference, desc.MediaType, manifestContent) if err != nil { registry.log.Error().Str("errorType", common.TypeOf(err)).Str("repo", repo).Str("reference", reference). diff --git a/pkg/meta/boltdb/boltdb.go b/pkg/meta/boltdb/boltdb.go index 87e74f17..2b712487 100644 --- a/pkg/meta/boltdb/boltdb.go +++ b/pkg/meta/boltdb/boltdb.go @@ -500,6 +500,11 @@ func getAllContainedMeta(imageBuck *bbolt.Bucket, imageIndexData *proto_go.Image imageManifestData, err := getProtoImageMeta(imageBuck, manifest.Digest) if err != nil { + // Skip manifests that don't have MetaDB entries (missing from storage) + if errors.Is(err, zerr.ErrImageMetaNotFound) { + continue + } + return imageMetaList, manifestDataList, err } @@ -511,6 +516,8 @@ func getAllContainedMeta(imageBuck *bbolt.Bucket, imageIndexData *proto_go.Image compat.IsCompatibleManifestListMediaType(imageManifestData.MediaType) { partialImageDataList, partialManifestDataList, err := getAllContainedMeta(imageBuck, imageManifestData) if err != nil { + // getAllContainedMeta skips missing items internally, so any error returned + // is a real error that should be propagated return imageMetaList, manifestDataList, err } diff --git a/pkg/meta/boltdb/boltdb_test.go b/pkg/meta/boltdb/boltdb_test.go index cc35fff3..7d4f53d2 100644 --- a/pkg/meta/boltdb/boltdb_test.go +++ b/pkg/meta/boltdb/boltdb_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "encoding/base64" + "errors" "math" "testing" "time" @@ -32,6 +33,8 @@ func (its imgTrustStore) VerifySignature( return "", time.Time{}, false, nil } +var errImageMetaBucketNotFound = errors.New("ImageMeta bucket not found") + func TestWrapperErrors(t *testing.T) { image := CreateDefaultImage() imageMeta := image.AsImageMeta() @@ -302,23 +305,63 @@ func TestWrapperErrors(t *testing.T) { So(err, ShouldNotBeNil) }) - Convey("image is index, fail to get manifests", func() { + Convey("image is index, missing manifests are skipped gracefully", func() { + err := boltdbWrapper.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) + So(err, ShouldBeNil) + + // Missing manifests are skipped gracefully, so GetFullImageMeta succeeds + // but returns an index with no manifests + fullImageMeta, err := boltdbWrapper.GetFullImageMeta(ctx, "repo", "tag") + So(err, ShouldBeNil) + So(len(fullImageMeta.Manifests), ShouldEqual, 0) + }) + + Convey("image is index, corrupted manifest data returns error", func() { + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + multiarchImageMeta := multiarchImage.AsImageMeta() err := boltdbWrapper.SetImageMeta(multiarchImageMeta.Digest, multiarchImageMeta) So(err, ShouldBeNil) - err = boltdbWrapper.SetRepoMeta("repo", mTypes.RepoMeta{ - Name: "repo", - Tags: map[mTypes.Tag]mTypes.Descriptor{ - "tag": { - MediaType: ispec.MediaTypeImageIndex, - Digest: multiarchImageMeta.Digest.String(), - }, - }, + // Store the first manifest normally + firstManifest := multiarchImage.Images[0] + firstManifestMeta := firstManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(firstManifestMeta.Digest, firstManifestMeta) + So(err, ShouldBeNil) + + // Store the second manifest normally first, then corrupt it + secondManifest := multiarchImage.Images[1] + secondManifestMeta := secondManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(secondManifestMeta.Digest, secondManifestMeta) + So(err, ShouldBeNil) + + secondManifestDigest := secondManifest.ManifestDescriptor.Digest + + // Corrupt the data for the second manifest by storing invalid protobuf data + // This will cause getProtoImageMeta to return an unmarshaling error + // which is not ErrImageMetaNotFound, so it will propagate through getAllContainedMeta + corruptedData := []byte("invalid protobuf data") + + // Access BoltDB directly to corrupt the data + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + imageBuck := tx.Bucket([]byte(boltdb.ImageMetaBuck)) + if imageBuck == nil { + return errImageMetaBucketNotFound + } + // Store corrupted protobuf data + return imageBuck.Put([]byte(secondManifestDigest.String()), corruptedData) }) So(err, ShouldBeNil) - _, err = boltdbWrapper.GetFullImageMeta(ctx, "repo", "tag") + err = boltdbWrapper.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) + So(err, ShouldBeNil) + + // GetFullImageMeta should return an error due to corrupted manifest data + // The error from getAllContainedMeta should propagate + fullImageMeta, err := boltdbWrapper.GetFullImageMeta(ctx, "repo", "tag") So(err, ShouldNotBeNil) + // Should still return a FullImageMeta object (even with error) + So(fullImageMeta, ShouldNotBeNil) }) }) @@ -443,6 +486,54 @@ func TestWrapperErrors(t *testing.T) { _, err = boltdbWrapper.FilterTags(ctx, mTypes.AcceptAllRepoTag, mTypes.AcceptAllImageMeta) So(err, ShouldBeNil) }) + + Convey("getAllContainedMeta error for index is joined and processing continues", func() { + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + multiarchImageMeta := multiarchImage.AsImageMeta() + err := boltdbWrapper.SetImageMeta(multiarchImageMeta.Digest, multiarchImageMeta) + So(err, ShouldBeNil) + + // Store the first manifest normally + firstManifest := multiarchImage.Images[0] + firstManifestMeta := firstManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(firstManifestMeta.Digest, firstManifestMeta) + So(err, ShouldBeNil) + + // Store the second manifest normally first, then corrupt it + secondManifest := multiarchImage.Images[1] + secondManifestMeta := secondManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(secondManifestMeta.Digest, secondManifestMeta) + So(err, ShouldBeNil) + + secondManifestDigest := secondManifest.ManifestDescriptor.Digest + + // Corrupt the data for the second manifest by storing invalid protobuf data + // This will cause getProtoImageMeta to return an unmarshaling error + // which is not ErrImageMetaNotFound, so it will propagate through getAllContainedMeta + corruptedData := []byte("invalid protobuf data") + + // Access BoltDB directly to corrupt the data + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + imageBuck := tx.Bucket([]byte(boltdb.ImageMetaBuck)) + if imageBuck == nil { + return errImageMetaBucketNotFound + } + // Store corrupted protobuf data + return imageBuck.Put([]byte(secondManifestDigest.String()), corruptedData) + }) + So(err, ShouldBeNil) + + err = boltdbWrapper.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) + So(err, ShouldBeNil) + + // FilterTags should return an error due to corrupted manifest data + // The error from getAllContainedMeta should be joined with viewError + images, err := boltdbWrapper.FilterTags(ctx, mTypes.AcceptAllRepoTag, mTypes.AcceptAllImageMeta) + So(err, ShouldNotBeNil) + // Should still return some images (the first valid manifest might be processed) + So(images, ShouldNotBeNil) + }) }) Convey("SearchRepos", func() { @@ -470,6 +561,53 @@ func TestWrapperErrors(t *testing.T) { }) }) + Convey("GetImageMeta", func() { + Convey("image is index, getAllContainedMeta error returns error", func() { + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + multiarchImageMeta := multiarchImage.AsImageMeta() + err := boltdbWrapper.SetImageMeta(multiarchImageMeta.Digest, multiarchImageMeta) + So(err, ShouldBeNil) + + // Store the first manifest normally + firstManifest := multiarchImage.Images[0] + firstManifestMeta := firstManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(firstManifestMeta.Digest, firstManifestMeta) + So(err, ShouldBeNil) + + // Store the second manifest normally first, then corrupt it + secondManifest := multiarchImage.Images[1] + secondManifestMeta := secondManifest.AsImageMeta() + err = boltdbWrapper.SetImageMeta(secondManifestMeta.Digest, secondManifestMeta) + So(err, ShouldBeNil) + + secondManifestDigest := secondManifest.ManifestDescriptor.Digest + + // Corrupt the data for the second manifest by storing invalid protobuf data + // This will cause getProtoImageMeta to return an unmarshaling error + // which is not ErrImageMetaNotFound, so it will propagate through getAllContainedMeta + corruptedData := []byte("invalid protobuf data") + + // Access BoltDB directly to corrupt the data + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + imageBuck := tx.Bucket([]byte(boltdb.ImageMetaBuck)) + if imageBuck == nil { + return errImageMetaBucketNotFound + } + // Store corrupted protobuf data + return imageBuck.Put([]byte(secondManifestDigest.String()), corruptedData) + }) + So(err, ShouldBeNil) + + // GetImageMeta should return an error due to corrupted manifest data + // The error from getAllContainedMeta should propagate + imageMeta, err := boltdbWrapper.GetImageMeta(multiarchImageMeta.Digest) + So(err, ShouldNotBeNil) + // Should still return an ImageMeta object (even with error) + So(imageMeta, ShouldNotBeNil) + }) + }) + Convey("SetRepoReference", func() { Convey("getProtoRepoMeta errors", func() { err := setRepoMeta("repo", badProtoBlob, boltdbWrapper.DB) diff --git a/pkg/meta/dynamodb/dynamodb.go b/pkg/meta/dynamodb/dynamodb.go index 428c67dd..fbc73736 100644 --- a/pkg/meta/dynamodb/dynamodb.go +++ b/pkg/meta/dynamodb/dynamodb.go @@ -1358,6 +1358,27 @@ func (dwr *DynamoDB) FilterImageMeta(ctx context.Context, digests []string, return nil, err } + // Check if all requested digests were found + // FilterImageMeta should return an error if any digest is missing (unlike getAllContainedMeta) + if len(imageMetaAttributes) != len(digests) { + // Build a map of found digests to identify which ones are missing + foundDigests := make(map[string]bool) + + for _, attributes := range imageMetaAttributes { + var digest string + if err := attributevalue.Unmarshal(attributes["TableKey"], &digest); err == nil { + foundDigests[digest] = true + } + } + + // Find the first missing digest + for _, digest := range digests { + if !foundDigests[digest] { + return nil, fmt.Errorf("%w for digest %s", zerr.ErrImageMetaNotFound, digest) + } + } + } + results := map[string]mTypes.ImageMeta{} for _, attributes := range imageMetaAttributes { @@ -2021,10 +2042,7 @@ func (dwr *DynamoDB) fetchImageMetaAttributesByDigest(ctx context.Context, diges return nil, err } - if len(resp.Responses[dwr.ImageMetaTablename]) != size { - return nil, zerr.ErrImageMetaNotFound - } - + // Missing manifests are allowed - append whatever was found batchedResp = append(batchedResp, resp.Responses[dwr.ImageMetaTablename]...) start = end } @@ -2045,13 +2063,12 @@ func (dwr *DynamoDB) fetchImageMetaAttributesByDigest(ctx context.Context, diges respMap[digest] = item } + // Only include digests that were actually found (missing ones are skipped gracefully) for _, digest := range digests { imageMeta, ok := respMap[digest] - if !ok { - return nil, fmt.Errorf("%w for digest %s", zerr.ErrImageMetaNotFound, digest) + if ok { + orderedResp = append(orderedResp, imageMeta) } - - orderedResp = append(orderedResp, imageMeta) } return orderedResp, nil @@ -2228,30 +2245,28 @@ func (dwr *DynamoDB) createVersionTable() error { return err } - if err == nil { - mdAttributeValue, err := attributevalue.Marshal(version.CurrentVersion) - if err != nil { - return err - } + mdAttributeValue, err := attributevalue.Marshal(version.CurrentVersion) + if err != nil { + return err + } - _, err = dwr.Client.UpdateItem(context.TODO(), &dynamodb.UpdateItemInput{ - ExpressionAttributeNames: map[string]string{ - "#V": "Version", + _, err = dwr.Client.UpdateItem(context.TODO(), &dynamodb.UpdateItemInput{ + ExpressionAttributeNames: map[string]string{ + "#V": "Version", + }, + ExpressionAttributeValues: map[string]types.AttributeValue{ + ":Version": mdAttributeValue, + }, + Key: map[string]types.AttributeValue{ + "TableKey": &types.AttributeValueMemberS{ + Value: version.DBVersionKey, }, - ExpressionAttributeValues: map[string]types.AttributeValue{ - ":Version": mdAttributeValue, - }, - Key: map[string]types.AttributeValue{ - "TableKey": &types.AttributeValueMemberS{ - Value: version.DBVersionKey, - }, - }, - TableName: aws.String(dwr.VersionTablename), - UpdateExpression: aws.String("SET #V = :Version"), - }) - if err != nil { - return err - } + }, + TableName: aws.String(dwr.VersionTablename), + UpdateExpression: aws.String("SET #V = :Version"), + }) + if err != nil { + return err } return nil diff --git a/pkg/meta/dynamodb/dynamodb_test.go b/pkg/meta/dynamodb/dynamodb_test.go index 41bcf040..f8b5cf9a 100644 --- a/pkg/meta/dynamodb/dynamodb_test.go +++ b/pkg/meta/dynamodb/dynamodb_test.go @@ -383,12 +383,15 @@ func TestWrapperErrors(t *testing.T) { _, err := dynamoWrapper.GetImageMeta(testDigest) So(err, ShouldNotBeNil) }) - Convey("image index, get manifest meta fails", func() { + Convey("image index, missing manifests are skipped gracefully", func() { err := dynamoWrapper.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) So(err, ShouldBeNil) - _, err = dynamoWrapper.GetImageMeta(multiarchImageMeta.Digest) //nolint: contextcheck - So(err, ShouldNotBeNil) + // Missing manifests are skipped gracefully, so GetImageMeta succeeds + // but returns an index with no manifests + imageMeta, err := dynamoWrapper.GetImageMeta(multiarchImageMeta.Digest) //nolint: contextcheck + So(err, ShouldBeNil) + So(len(imageMeta.Manifests), ShouldEqual, 0) }) }) Convey("GetFullImageMeta", func() { @@ -429,7 +432,7 @@ func TestWrapperErrors(t *testing.T) { So(err, ShouldNotBeNil) }) - Convey("image is index, fail to get manifests", func() { + Convey("image is index, missing manifests are skipped gracefully", func() { err := dynamoWrapper.SetImageMeta(multiarchImageMeta.Digest, multiarchImageMeta) //nolint: contextcheck So(err, ShouldBeNil) @@ -444,8 +447,11 @@ func TestWrapperErrors(t *testing.T) { }) So(err, ShouldBeNil) - _, err = dynamoWrapper.GetFullImageMeta(ctx, "repo", "tag") - So(err, ShouldNotBeNil) + // Missing manifests are skipped gracefully, so GetFullImageMeta succeeds + // but returns an index with no manifests + fullImageMeta, err := dynamoWrapper.GetFullImageMeta(ctx, "repo", "tag") + So(err, ShouldBeNil) + So(len(fullImageMeta.Manifests), ShouldEqual, 0) }) }) diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index f1b49c62..92e03b94 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -572,6 +572,75 @@ func RunMetaDBTests(t *testing.T, metaDB mTypes.MetaDB, preparationFuncs ...func So(fullImageMeta.Digest.String(), ShouldResemble, multi.DigestStr()) }) + Convey("GetFullImageMeta with nested index missing manifests", func() { + // Create a nested index structure: + // - Top-level index contains a nested index and a manifest + // - The nested index references manifests that are missing from MetaDB + // - The manifest in the top-level index exists + // Create a valid manifest + validImage := CreateDefaultImage() + validImageMeta := validImage.AsImageMeta() + err := metaDB.SetImageMeta(validImageMeta.Digest, validImageMeta) + So(err, ShouldBeNil) + + // Create a nested index that references missing manifests + nestedIndex := CreateMultiarchWith().Images([]Image{CreateRandomImage()}).Build() + nestedIndexMeta := nestedIndex.AsImageMeta() + // Store the nested index metadata (but not its manifests - they're missing) + err = metaDB.SetImageMeta(nestedIndexMeta.Digest, nestedIndexMeta) + So(err, ShouldBeNil) + + // Create a top-level index that contains: + // 1. The nested index (with missing manifests) + // 2. The valid manifest + topLevelIndexContent := ispec.Index{ + Versioned: specs.Versioned{SchemaVersion: 2}, + MediaType: ispec.MediaTypeImageIndex, + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageManifest, + Digest: validImageMeta.Digest, + Size: validImageMeta.Size, + }, + { + MediaType: ispec.MediaTypeImageIndex, + Digest: nestedIndexMeta.Digest, + Size: nestedIndexMeta.Size, + }, + }, + } + + // Create and store the top-level index metadata + topLevelIndexBlob, err := json.Marshal(topLevelIndexContent) + So(err, ShouldBeNil) + + // Calculate digest from the index content + topLevelIndexDigest := godigest.FromBytes(topLevelIndexBlob) + + topLevelIndexMeta := mTypes.ImageMeta{ + MediaType: ispec.MediaTypeImageIndex, + Digest: topLevelIndexDigest, + Size: int64(len(topLevelIndexBlob)), + Index: &topLevelIndexContent, + Manifests: []mTypes.ManifestMeta{}, + } + + err = metaDB.SetImageMeta(topLevelIndexMeta.Digest, topLevelIndexMeta) + So(err, ShouldBeNil) + + err = metaDB.SetRepoReference(ctx, "repo", "tag", topLevelIndexMeta) + So(err, ShouldBeNil) + + // GetFullImageMeta should succeed, skipping the nested index with missing manifests + // but including the valid manifest from the top-level index + fullImageMeta, err := metaDB.GetFullImageMeta(ctx, "repo", "tag") + So(err, ShouldBeNil) + // Should have the valid manifest from the top-level index + // The nested index with missing manifests should be skipped + So(len(fullImageMeta.Manifests), ShouldEqual, 1) + So(fullImageMeta.Manifests[0].Digest, ShouldEqual, validImageMeta.Digest) + }) + Convey("Set/Get RepoMeta", func() { err := metaDB.SetRepoMeta("repo", mTypes.RepoMeta{ Name: "repo", diff --git a/pkg/meta/parse.go b/pkg/meta/parse.go index 2f469e0d..50bc0184 100644 --- a/pkg/meta/parse.go +++ b/pkg/meta/parse.go @@ -6,6 +6,7 @@ import ( "errors" "time" + "github.com/distribution/distribution/v3/registry/storage/driver" godigest "github.com/opencontainers/go-digest" ispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -143,8 +144,17 @@ func ParseRepo(repo string, metaDB mTypes.MetaDB, storeController stypes.StoreCo continue } - manifestBlob, _, _, err := imageStore.GetImageManifest(repo, manifest.Digest.String()) + manifestBlob, err := imageStore.GetBlobContent(repo, manifest.Digest) if err != nil { + // Handle missing blobs gracefully - log warning and continue with other manifests + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + log.Warn().Err(err).Str("repository", repo).Str("digest", manifest.Digest.String()). + Msg("skipping missing manifest blob, continuing repo parse") + + continue + } + log.Error().Err(err).Str("repository", repo).Str("digest", manifest.Digest.String()). Msg("failed to get blob for image") diff --git a/pkg/meta/parse_test.go b/pkg/meta/parse_test.go index ba5c792a..8a47baa2 100644 --- a/pkg/meta/parse_test.go +++ b/pkg/meta/parse_test.go @@ -16,6 +16,7 @@ import ( ispec "github.com/opencontainers/image-spec/specs-go/v1" . "github.com/smartystreets/goconvey/convey" + zerr "zotregistry.dev/zot/v2/errors" rediscfg "zotregistry.dev/zot/v2/pkg/api/config/redis" zcommon "zotregistry.dev/zot/v2/pkg/common" "zotregistry.dev/zot/v2/pkg/extensions/monitoring" @@ -165,13 +166,14 @@ func TestParseStorageErrors(t *testing.T) { So(err, ShouldBeNil) }) - Convey("imageStore.GetImageManifest errors", func() { + Convey("imageStore.GetBlobContent non-missing error", func() { + manifestDigest := godigest.FromString("digest") imageStore.GetIndexContentFn = func(repo string) ([]byte, error) { return getIndexBlob(ispec.Index{ Manifests: []ispec.Descriptor{ { MediaType: ispec.MediaTypeImageManifest, - Digest: godigest.FromString("digest"), + Digest: manifestDigest, Annotations: map[string]string{ ispec.AnnotationRefName: "tag", }, @@ -179,11 +181,75 @@ func TestParseStorageErrors(t *testing.T) { }, }), nil } - imageStore.GetImageManifestFn = func(repo, reference string) ([]byte, godigest.Digest, string, error) { - return nil, "", "", ErrTestError + imageStore.GetBlobContentFn = func(repo string, digest godigest.Digest) ([]byte, error) { + // Return a non-missing error (not ErrBlobNotFound or PathNotFoundError) + return nil, ErrTestError } err := meta.ParseRepo("repo", metaDB, storeController, log) So(err, ShouldNotBeNil) + So(err, ShouldEqual, ErrTestError) + }) + + Convey("imageStore.GetImageManifest missing blob - graceful handling", func() { + digest1 := godigest.FromString("digest1") + digest2 := godigest.FromString("digest2") + imageStore.GetIndexContentFn = func(repo string) ([]byte, error) { + return getIndexBlob(ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageManifest, + Digest: digest1, + Annotations: map[string]string{ + ispec.AnnotationRefName: "tag1", + }, + }, + { + MediaType: ispec.MediaTypeImageManifest, + Digest: digest2, + Annotations: map[string]string{ + ispec.AnnotationRefName: "tag2", + }, + }, + }, + }), nil + } + callCount := 0 + setRepoRefCount := 0 + // Create a valid image for the second manifest + validImage := CreateRandomImage() + manifestBlob, _ := json.Marshal(validImage.Manifest) + configBlob, _ := json.Marshal(validImage.Config) + imageStore.GetBlobContentFn = func(repo string, digest godigest.Digest) ([]byte, error) { + callCount++ + // First manifest is missing, second one succeeds + if digest == digest1 { + return nil, zerr.ErrBlobNotFound + } + + if digest == digest2 { + // Return valid manifest for second one + return manifestBlob, nil + } + // Return config blob when requested + if digest == validImage.ConfigDescriptor.Digest { + return configBlob, nil + } + + return nil, zerr.ErrBlobNotFound + } + metaDB.SetRepoReferenceFn = func(ctx context.Context, repo, reference string, imageMeta mTypes.ImageMeta) error { + setRepoRefCount++ + // Verify it's only called for tag2 (the second manifest) + So(reference, ShouldEqual, "tag2") + + return nil + } + err := meta.ParseRepo("repo", metaDB, storeController, log) + So(err, ShouldBeNil) + // Should have called GetBlobContent for both manifests (and config) + So(callCount, ShouldEqual, 3) + // Should have called SetRepoReference only once for the second manifest (first was skipped) + So(setRepoRefCount, ShouldEqual, 1) }) Convey("manifestMetaIsPresent true", func() { diff --git a/pkg/meta/redis/redis.go b/pkg/meta/redis/redis.go index a3223b3a..b64639b6 100644 --- a/pkg/meta/redis/redis.go +++ b/pkg/meta/redis/redis.go @@ -2197,6 +2197,11 @@ func (rc *RedisDB) getAllContainedMeta(ctx context.Context, imageIndexData *prot imageManifestData, err := rc.getProtoImageMeta(ctx, manifest.Digest) if err != nil { + // Skip manifests that don't have MetaDB entries (missing from storage) + if errors.Is(err, zerr.ErrImageMetaNotFound) { + continue + } + return imageMetaList, manifestDataList, err } @@ -2208,6 +2213,8 @@ func (rc *RedisDB) getAllContainedMeta(ctx context.Context, imageIndexData *prot compat.IsCompatibleManifestListMediaType(imageManifestData.MediaType) { partialImageDataList, partialManifestDataList, err := rc.getAllContainedMeta(ctx, imageManifestData) if err != nil { + // getAllContainedMeta skips missing items internally, so any error returned + // is a real error that should be propagated return imageMetaList, manifestDataList, err } diff --git a/pkg/meta/redis/redis_test.go b/pkg/meta/redis/redis_test.go index c2996c7e..bf1272be 100644 --- a/pkg/meta/redis/redis_test.go +++ b/pkg/meta/redis/redis_test.go @@ -857,23 +857,56 @@ func TestWrapperErrors(t *testing.T) { So(err, ShouldNotBeNil) }) - Convey("image is index, fail to get manifests", func() { + Convey("image is index, missing manifests are skipped gracefully", func() { + err := metaDB.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) + So(err, ShouldBeNil) + + // Missing manifests are skipped gracefully, so GetFullImageMeta succeeds + // but returns an index with no manifests + fullImageMeta, err := metaDB.GetFullImageMeta(ctx, "repo", "tag") + So(err, ShouldBeNil) + So(len(fullImageMeta.Manifests), ShouldEqual, 0) + }) + + Convey("image is index, corrupted manifest data returns error", func() { + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + multiarchImageMeta := multiarchImage.AsImageMeta() err := metaDB.SetImageMeta(multiarchImageMeta.Digest, multiarchImageMeta) So(err, ShouldBeNil) - err = metaDB.SetRepoMeta("repo", mTypes.RepoMeta{ - Name: "repo", - Tags: map[mTypes.Tag]mTypes.Descriptor{ - "tag": { - MediaType: ispec.MediaTypeImageIndex, - Digest: multiarchImageMeta.Digest.String(), - }, - }, - }) + // Store the first manifest normally + firstManifest := multiarchImage.Images[0] + firstManifestMeta := firstManifest.AsImageMeta() + err = metaDB.SetImageMeta(firstManifestMeta.Digest, firstManifestMeta) So(err, ShouldBeNil) - _, err = metaDB.GetFullImageMeta(ctx, "repo", "tag") + // Store the second manifest normally first, then corrupt it + secondManifest := multiarchImage.Images[1] + secondManifestMeta := secondManifest.AsImageMeta() + err = metaDB.SetImageMeta(secondManifestMeta.Digest, secondManifestMeta) + So(err, ShouldBeNil) + + secondManifestDigest := secondManifest.ManifestDescriptor.Digest + + // Corrupt the data for the second manifest by storing invalid protobuf data + // This will cause getProtoImageMeta to return an unmarshaling error + // which is not ErrImageMetaNotFound, so it will propagate through getAllContainedMeta + corruptedData := []byte("invalid protobuf data") + + // Access Redis directly to corrupt the data using the helper function pattern + err = setImageMeta(secondManifestDigest, corruptedData, client) + So(err, ShouldBeNil) + + err = metaDB.SetRepoReference(ctx, "repo", "tag", multiarchImageMeta) + So(err, ShouldBeNil) + + // GetFullImageMeta should return an error due to corrupted manifest data + // The error from getAllContainedMeta should propagate + fullImageMeta, err := metaDB.GetFullImageMeta(ctx, "repo", "tag") So(err, ShouldNotBeNil) + // Should still return a FullImageMeta object (even with error) + So(fullImageMeta, ShouldNotBeNil) }) }) diff --git a/pkg/storage/common/common.go b/pkg/storage/common/common.go index 3487e6dc..571da481 100644 --- a/pkg/storage/common/common.go +++ b/pkg/storage/common/common.go @@ -464,6 +464,15 @@ func PruneImageManifestsFromIndex(imgStore storageTypes.ImageStore, repo string, for _, otherIndex := range otherImgIndexes { oindex, err := GetImageIndex(imgStore, repo, otherIndex.Digest, log) if err != nil { + // Handle missing blobs gracefully - log warning and continue with other indexes + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + log.Warn().Err(err).Str("repository", repo).Str("digest", otherIndex.Digest.String()). + Msg("skipping missing image index blob, continuing with other indexes") + + continue + } + return nil, err } @@ -520,6 +529,15 @@ func isBlobReferencedInImageManifest(imgStore storageTypes.ImageStore, repo stri manifestContent, err := GetImageManifest(imgStore, repo, mdigest, log) if err != nil { + // Handle missing blobs gracefully - treat as not referenced and continue + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + log.Warn().Err(err).Str("repo", repo).Str("digest", mdigest.String()).Str("component", "gc"). + Msg("skipping missing manifest blob, treating as not referenced") + + return false, nil + } + log.Error().Err(err).Str("repo", repo).Str("digest", mdigest.String()).Str("component", "gc"). Msg("failed to read manifest image") @@ -554,6 +572,15 @@ func IsBlobReferencedInImageIndex(imgStore storageTypes.ImageStore, repo string, indexImage, err := GetImageIndex(imgStore, repo, desc.Digest, log) if err != nil { + // Handle missing blobs gracefully - treat as not referenced and continue + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + log.Warn().Err(err).Str("repository", repo).Str("digest", desc.Digest.String()). + Msg("skipping missing image index blob, treating as not referenced") + + continue + } + log.Error().Err(err).Str("repository", repo).Str("digest", desc.Digest.String()). Msg("failed to read multiarch(index) image") @@ -787,6 +814,15 @@ func GetBlobDescriptorFromIndex(imgStore storageTypes.ImageStore, index ispec.In case ispec.MediaTypeImageIndex: indexImage, err := GetImageIndex(imgStore, repo, desc.Digest, log) if err != nil { + // Handle missing blobs gracefully - skip this index and continue searching + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + log.Warn().Err(err).Str("repository", repo).Str("digest", desc.Digest.String()). + Msg("skipping missing image index blob, continuing search for blob descriptor") + + continue + } + return ispec.Descriptor{}, err } diff --git a/pkg/storage/common/common_test.go b/pkg/storage/common/common_test.go index 17e8c1da..a3c09764 100644 --- a/pkg/storage/common/common_test.go +++ b/pkg/storage/common/common_test.go @@ -610,3 +610,163 @@ func TestDedupeGeneratorErrors(t *testing.T) { So(task, ShouldBeNil) }) } + +func TestPruneImageManifestsFromIndexMissingIndex(t *testing.T) { + log := log.NewTestLogger() + + Convey("Missing nested index blob is skipped gracefully", t, func(c C) { + // Create a main index + mainIndexDigest := godigest.FromString("main-index") + manifest1Digest := godigest.FromString("manifest1") + mainIndex := ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageManifest, + Digest: manifest1Digest, + }, + }, + } + mainIndexBlob, err := json.Marshal(mainIndex) + So(err, ShouldBeNil) + + // Create other indexes list with one missing index + // The missing index would have referenced manifest1, but since it's missing, + // manifest1 should still be pruned (removed) if it has no tag + otherImgIndexes := []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageIndex, + Digest: godigest.FromString("missing-index"), + Size: 100, + }, + } + + imgStore := &mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + if digest == mainIndexDigest { + return mainIndexBlob, nil + } + // Return ErrBlobNotFound for the missing nested index + return nil, zerr.ErrBlobNotFound + }, + } + + // PruneImageManifestsFromIndex should skip the missing nested index and continue + // outIndex contains a manifest without a tag, so it should be pruned + outIndex := ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageManifest, + Digest: manifest1Digest, + // No tag annotation, so it will be pruned + }, + }, + } + + prunedManifests, err := common.PruneImageManifestsFromIndex( + imgStore, "repo", mainIndexDigest, outIndex, otherImgIndexes, log) + So(err, ShouldBeNil) + // The manifest should be pruned (removed) since it has no tag and the missing index + // couldn't be checked to see if it references this manifest + // The important thing is that the function succeeded (didn't error) despite the missing index + So(len(prunedManifests), ShouldEqual, 0) + }) +} + +func TestIsBlobReferencedInImageManifestMissingManifest(t *testing.T) { + log := log.NewTestLogger() + + Convey("Missing manifest blob is treated as not referenced", t, func(c C) { + blobDigest := godigest.FromString("blob-digest") + missingManifestDigest := godigest.FromString("missing-manifest") + + imgStore := &mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + // Return ErrBlobNotFound for the missing manifest + return nil, zerr.ErrBlobNotFound + }, + } + + // Create an index with a manifest descriptor pointing to a missing manifest + // IsBlobReferencedInImageIndex will call isBlobReferencedInImageManifest internally + index := ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageManifest, + Digest: missingManifestDigest, + Size: 100, + }, + }, + } + + // isBlobReferencedInImageManifest should treat missing manifest as not referenced + referenced, err := common.IsBlobReferencedInImageIndex(imgStore, "repo", blobDigest, index, log) + So(err, ShouldBeNil) + So(referenced, ShouldBeFalse) + }) +} + +func TestIsBlobReferencedInImageIndexNonMissingError(t *testing.T) { + log := log.NewTestLogger() + + Convey("Non-missing error when reading nested index returns error", t, func(c C) { + blobDigest := godigest.FromString("blob-digest") + nestedIndexDigest := godigest.FromString("nested-index") + + // Create an index with a nested index descriptor + index := ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageIndex, + Digest: nestedIndexDigest, + Size: 100, + }, + }, + } + + imgStore := &mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + // Return a non-missing error (not ErrBlobNotFound or PathNotFoundError) + return nil, ErrTestError + }, + } + + // IsBlobReferencedInImageIndex should return the error (not skip it) + referenced, err := common.IsBlobReferencedInImageIndex(imgStore, "repo", blobDigest, index, log) + So(err, ShouldNotBeNil) + So(err, ShouldEqual, ErrTestError) + So(referenced, ShouldBeFalse) + }) +} + +func TestGetBlobDescriptorFromIndexMissingNestedIndex(t *testing.T) { + log := log.NewTestLogger() + + Convey("Missing nested index blob is skipped gracefully", t, func(c C) { + blobDigest := godigest.FromString("blob-digest") + missingIndexDigest := godigest.FromString("missing-nested-index") + + // Create an index that contains a nested index (which will be missing) + topLevelIndex := ispec.Index{ + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageIndex, + Digest: missingIndexDigest, + Size: 100, + }, + }, + } + + imgStore := &mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + // Return ErrBlobNotFound for the missing nested index + return nil, zerr.ErrBlobNotFound + }, + } + + // GetBlobDescriptorFromIndex should skip the missing nested index and continue + // Since the blob is not found, it should return ErrBlobNotFound + _, err := common.GetBlobDescriptorFromIndex(imgStore, topLevelIndex, "repo", blobDigest, log) + So(err, ShouldNotBeNil) + So(err, ShouldEqual, zerr.ErrBlobNotFound) + }) +} diff --git a/pkg/storage/gc/gc.go b/pkg/storage/gc/gc.go index c658365e..ddcc1068 100644 --- a/pkg/storage/gc/gc.go +++ b/pkg/storage/gc/gc.go @@ -236,6 +236,15 @@ func (gc GarbageCollect) removeIndexReferrers(repo string, rootIndex *ispec.Inde if (desc.MediaType == ispec.MediaTypeImageIndex) || compat.IsCompatibleManifestListMediaType(desc.MediaType) { indexImage, err := common.GetImageIndex(gc.imgStore, repo, desc.Digest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", desc.Digest.String()). + Msg("skipping missing image index blob, continuing GC") + + continue + } + gc.log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", desc.Digest.String()). Msg("failed to read multiarch(index) image") @@ -265,6 +274,15 @@ func (gc GarbageCollect) removeIndexReferrers(repo string, rootIndex *ispec.Inde } else if (desc.MediaType == ispec.MediaTypeImageManifest) || compat.IsCompatibleManifestMediaType(desc.MediaType) { image, err := common.GetImageManifest(gc.imgStore, repo, desc.Digest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repo", repo).Str("digest", desc.Digest.String()). + Msg("skipping missing image manifest blob, continuing GC") + + continue + } + gc.log.Error().Err(err).Str("module", "gc").Str("repo", repo).Str("digest", desc.Digest.String()). Msg("failed to read manifest image") @@ -536,6 +554,15 @@ func (gc GarbageCollect) identifyManifestsReferencedInIndex(index ispec.Index, r if (desc.MediaType == ispec.MediaTypeImageIndex) || compat.IsCompatibleManifestListMediaType(desc.MediaType) { indexImage, err := common.GetImageIndex(gc.imgStore, repo, desc.Digest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repository", repo). + Str("digest", desc.Digest.String()).Msg("skipping missing image index blob, continuing GC") + + continue + } + gc.log.Error().Err(err).Str("module", "gc").Str("repository", repo). Str("digest", desc.Digest.String()).Msg("failed to read multiarch(index) image") @@ -556,6 +583,15 @@ func (gc GarbageCollect) identifyManifestsReferencedInIndex(index ispec.Index, r } else if (desc.MediaType == ispec.MediaTypeImageManifest) || compat.IsCompatibleManifestMediaType(desc.MediaType) { image, err := common.GetImageManifest(gc.imgStore, repo, desc.Digest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repo", repo). + Str("digest", desc.Digest.String()).Msg("skipping missing image manifest blob, continuing GC") + + continue + } + gc.log.Error().Err(err).Str("module", "gc").Str("repo", repo). Str("digest", desc.Digest.String()).Msg("failed to read manifest image") @@ -718,6 +754,15 @@ func (gc GarbageCollect) addImageIndexBlobsToReferences(repo string, mdigest god ) error { index, err := common.GetImageIndex(gc.imgStore, repo, mdigest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", mdigest.String()). + Msg("skipping missing image index blob, continuing GC") + + return nil + } + gc.log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", mdigest.String()). Msg("failed to read manifest image") @@ -743,6 +788,15 @@ func (gc GarbageCollect) addImageManifestBlobsToReferences(repo string, mdigest ) error { manifestContent, err := common.GetImageManifest(gc.imgStore, repo, mdigest, gc.log) if err != nil { + // Handle missing blobs (not found) gracefully + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + gc.log.Warn().Err(err).Str("module", "gc").Str("repository", repo). + Str("digest", mdigest.String()).Msg("skipping missing image manifest blob, continuing GC") + + return nil + } + gc.log.Error().Err(err).Str("module", "gc").Str("repository", repo). Str("digest", mdigest.String()).Msg("failed to read manifest image") diff --git a/pkg/storage/gc/gc_internal_test.go b/pkg/storage/gc/gc_internal_test.go index 64f0952e..30111d4b 100644 --- a/pkg/storage/gc/gc_internal_test.go +++ b/pkg/storage/gc/gc_internal_test.go @@ -14,6 +14,7 @@ import ( ispec "github.com/opencontainers/image-spec/specs-go/v1" . "github.com/smartystreets/goconvey/convey" + zerr "zotregistry.dev/zot/v2/errors" "zotregistry.dev/zot/v2/pkg/api/config" zcommon "zotregistry.dev/zot/v2/pkg/common" "zotregistry.dev/zot/v2/pkg/extensions/monitoring" @@ -63,7 +64,8 @@ func TestGarbageCollectManifestErrors(t *testing.T) { }, }, audit, log) - Convey("trigger repo not found in addImageIndexBlobsToReferences()", func() { + Convey("trigger missing blob in addImageIndexBlobsToReferences()", func() { + // GC should continue when blobs are missing (not found), not return an error err := gc.addIndexBlobsToReferences(repoName, ispec.Index{ Manifests: []ispec.Descriptor{ { @@ -72,10 +74,11 @@ func TestGarbageCollectManifestErrors(t *testing.T) { }, }, }, map[string]bool{}) - So(err, ShouldNotBeNil) + So(err, ShouldBeNil) }) - Convey("trigger repo not found in addImageManifestBlobsToReferences()", func() { + Convey("trigger missing blob in addImageManifestBlobsToReferences()", func() { + // GC should continue when blobs are missing (not found), not return an error err := gc.addIndexBlobsToReferences(repoName, ispec.Index{ Manifests: []ispec.Descriptor{ { @@ -84,7 +87,7 @@ func TestGarbageCollectManifestErrors(t *testing.T) { }, }, }, map[string]bool{}) - So(err, ShouldNotBeNil) + So(err, ShouldBeNil) }) content := []byte("this is a blob") @@ -137,8 +140,12 @@ func TestGarbageCollectManifestErrors(t *testing.T) { So(err, ShouldBeNil) }() + // Note: Permission denied from Stat() is converted to ErrBlobNotFound in originalBlobInfo, + // so we can't distinguish it from missing blobs. GC treats missing blobs gracefully, + // so permission denied from Stat() will also be treated as missing (return nil). + // Permission denied from ReadFile() will still return an error. err = gc.addIndexBlobsToReferences(repoName, index, map[string]bool{}) - So(err, ShouldNotBeNil) + So(err, ShouldBeNil) }) Convey("trigger GetImageManifest error in AddIndexBlobsToReferences", func() { @@ -706,5 +713,74 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) { }) So(err, ShouldNotBeNil) }) + + Convey("Missing nested index blob in removeIndexReferrers is skipped gracefully", func() { + // Create a top-level index that contains a nested index + // The nested index blob will be missing + topLevelIndex := ispec.Index{ + MediaType: ispec.MediaTypeImageIndex, + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageIndex, + Digest: godigest.FromString("missing-nested-index"), + Size: 100, + }, + }, + } + + imgStore := mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + // Return ErrBlobNotFound for the missing nested index + return nil, zerr.ErrBlobNotFound + }, + } + + gcOptions.ImageRetention = config.ImageRetention{ + Policies: []config.RetentionPolicy{ + { + Repositories: []string{"**"}, + DeleteReferrers: true, + }, + }, + } + + gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, gcOptions, audit, log) + + // removeIndexReferrers should skip the missing nested index and continue + gced, err := gc.removeIndexReferrers(repoName, &topLevelIndex, topLevelIndex) + So(err, ShouldBeNil) + So(gced, ShouldBeFalse) + }) + + Convey("Missing nested index blob in identifyManifestsReferencedInIndex is skipped gracefully", func() { + // Create a top-level index that contains a nested index + // The nested index blob will be missing + topLevelIndex := ispec.Index{ + MediaType: ispec.MediaTypeImageIndex, + Manifests: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageIndex, + Digest: godigest.FromString("missing-nested-index"), + Size: 100, + }, + }, + } + + imgStore := mocks.MockedImageStore{ + GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) { + // Return ErrBlobNotFound for the missing nested index + return nil, zerr.ErrBlobNotFound + }, + } + + gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, gcOptions, audit, log) + + // identifyManifestsReferencedInIndex should skip the missing nested index and continue + referenced := make(map[godigest.Digest]bool) + err := gc.identifyManifestsReferencedInIndex(topLevelIndex, repoName, referenced) + So(err, ShouldBeNil) + // No manifests should be marked as referenced since the nested index is missing + So(len(referenced), ShouldEqual, 0) + }) }) } diff --git a/pkg/storage/local/local_test.go b/pkg/storage/local/local_test.go index bd31a426..4147f8a4 100644 --- a/pkg/storage/local/local_test.go +++ b/pkg/storage/local/local_test.go @@ -1969,7 +1969,7 @@ func TestGarbageCollectForImageStore(t *testing.T) { ctx := context.Background() - Convey("Garbage collect error for repo with config removed", func() { + Convey("Garbage collect continues when manifest blob is missing", func() { logFile, _ := os.CreateTemp("", "zot-log*.txt") defer os.Remove(logFile.Name()) // clean up @@ -2006,15 +2006,18 @@ func TestGarbageCollectForImageStore(t *testing.T) { panic(err) } + // GC should succeed (return nil) when manifest blobs are missing + // This is the expected behavior - GC continues gracefully when blobs are not found err = gc.CleanRepo(ctx, repoName) - So(err, ShouldNotBeNil) + So(err, ShouldBeNil) time.Sleep(500 * time.Millisecond) data, err := os.ReadFile(logFile.Name()) So(err, ShouldBeNil) - So(string(data), ShouldContainSubstring, - "failed to run GC for "+path.Join(imgStore.RootDir(), repoName)) + // Verify that GC logged a warning about skipping the missing manifest + So(string(data), ShouldContainSubstring, "skipping missing") + So(string(data), ShouldContainSubstring, manifestDigest.String()) }) Convey("Garbage collect error - not enough permissions to access index.json", func() { diff --git a/pkg/storage/scrub.go b/pkg/storage/scrub.go index 6b0a80de..fb4a6921 100644 --- a/pkg/storage/scrub.go +++ b/pkg/storage/scrub.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/olekukonko/tablewriter" "github.com/olekukonko/tablewriter/tw" godigest "github.com/opencontainers/go-digest" @@ -31,6 +32,9 @@ const ( statusWidth = 8 affectedBlobWidth = 64 errorWidth = 8 + + statusAffected = "affected" + statusOK = "ok" ) type ScrubImageResult struct { @@ -116,8 +120,14 @@ func CheckRepo(ctx context.Context, imageName string, imgStore storageTypes.Imag layers, err := checkImage(manifest, imgStore, imageName, tag, scrubbedManifests) if err == nil && len(layers) > 0 { // CheckLayers doesn't use locks + // Only update result if current status is not "affected" to preserve subject/blob issues + currentRes, exists := scrubbedManifests[manifest.Digest] + imgRes := CheckLayers(imageName, tag, layers, imgStore) - scrubbedManifests[manifest.Digest] = imgRes + + if !exists || currentRes.Status != statusAffected { + scrubbedManifests[manifest.Digest] = imgRes + } } // ignore the manifest if it isn't found @@ -197,9 +207,24 @@ func scrubManifest( } // check all manifests + indexAffected := false + for _, man := range idx.Manifests { buf, err := imgStore.GetBlobContent(imageName, man.Digest) if err != nil { + // Handle missing blobs gracefully - mark as affected but continue processing + var pathNotFoundErr driver.PathNotFoundError + if errors.Is(err, zerr.ErrBlobNotFound) || errors.As(err, &pathNotFoundErr) { + imgRes := getResult(imageName, tag, man.Digest, err) + scrubbedManifests[man.Digest] = imgRes + scrubbedManifests[manifest.Digest] = imgRes + indexAffected = true + + // Continue checking other manifests instead of returning + continue + } + + // For other errors, mark as affected and return imgRes := getResult(imageName, tag, man.Digest, zerr.ErrBadBlobDigest) scrubbedManifests[man.Digest] = imgRes scrubbedManifests[manifest.Digest] = imgRes @@ -223,14 +248,28 @@ func scrubManifest( } } - // at this point, before starting to check the subject we can consider the index is ok - scrubbedManifests[manifest.Digest] = getResult(imageName, tag, "", nil) + // at this point, before starting to check the subject, mark index as ok only if not affected + // Also check if result was already set to avoid overwriting "affected" status + currentIndexRes, indexResExists := scrubbedManifests[manifest.Digest] + if !indexAffected && (!indexResExists || currentIndexRes.Status != statusAffected) { + scrubbedManifests[manifest.Digest] = getResult(imageName, tag, "", nil) + } // check subject if exists if idx.Subject != nil { buf, err := imgStore.GetBlobContent(imageName, idx.Subject.Digest) if err != nil { - imgRes := getResult(imageName, tag, idx.Subject.Digest, zerr.ErrBadBlobDigest) + // Handle missing blobs gracefully - mark as affected but continue processing + var pathNotFoundErr driver.PathNotFoundError + + resultErr := err + + if !errors.Is(err, zerr.ErrBlobNotFound) && !errors.As(err, &pathNotFoundErr) { + // For other errors, use generic error + resultErr = zerr.ErrBadBlobDigest + } + + imgRes := getResult(imageName, tag, idx.Subject.Digest, resultErr) scrubbedManifests[idx.Subject.Digest] = imgRes scrubbedManifests[manifest.Digest] = imgRes @@ -244,8 +283,12 @@ func scrubManifest( subjectRes := scrubbedManifests[idx.Subject.Digest] - scrubbedManifests[manifest.Digest] = newScrubImageResult(imageName, tag, subjectRes.Status, - subjectRes.AffectedBlob, subjectRes.Error) + // Only update index status if it's not already "affected" to preserve missing manifest/blob issues + currentIndexRes, indexResExists := scrubbedManifests[manifest.Digest] + if !indexResExists || currentIndexRes.Status != statusAffected { + scrubbedManifests[manifest.Digest] = newScrubImageResult(imageName, tag, subjectRes.Status, + subjectRes.AffectedBlob, subjectRes.Error) + } return layers, err } @@ -263,7 +306,17 @@ func scrubManifest( if err == nil && man.Subject != nil { buf, err := imgStore.GetBlobContent(imageName, man.Subject.Digest) if err != nil { - imgRes := getResult(imageName, tag, man.Subject.Digest, zerr.ErrBadBlobDigest) + // Handle missing blobs gracefully - mark as affected but continue processing + var pathNotFoundErr driver.PathNotFoundError + + resultErr := err + + if !errors.Is(err, zerr.ErrBlobNotFound) && !errors.As(err, &pathNotFoundErr) { + // For other errors, use generic error + resultErr = zerr.ErrBadBlobDigest + } + + imgRes := getResult(imageName, tag, man.Subject.Digest, resultErr) scrubbedManifests[man.Subject.Digest] = imgRes scrubbedManifests[manifest.Digest] = imgRes @@ -340,10 +393,10 @@ func CheckLayers( func getResult(imageName, tag string, affectedBlobDigest godigest.Digest, err error) ScrubImageResult { if err != nil { - return newScrubImageResult(imageName, tag, "affected", affectedBlobDigest.Encoded(), err.Error()) + return newScrubImageResult(imageName, tag, statusAffected, affectedBlobDigest.Encoded(), err.Error()) } - return newScrubImageResult(imageName, tag, "ok", "", "") + return newScrubImageResult(imageName, tag, statusOK, "", "") } func newScrubImageResult(imageName, tag, status, affectedBlob, err string) ScrubImageResult { diff --git a/pkg/storage/scrub_test.go b/pkg/storage/scrub_test.go index 78de961b..617e85cd 100644 --- a/pkg/storage/scrub_test.go +++ b/pkg/storage/scrub_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "os" "path" "regexp" "strings" @@ -347,6 +348,122 @@ func RunCheckAllBlobsIntegrityTests( //nolint: thelper So(actual, ShouldContainSubstring, fmt.Sprintf("test 1.0 affected %s blob not found", layerDig)) }) + Convey("Scrub index with missing manifest blob - graceful handling", func() { + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + err = WriteMultiArchImageToFileSystem(multiarchImage, repoName, "2.0", storeCtlr) + So(err, ShouldBeNil) + + // Get the index to find the index manifest digest + idx, err := common.GetIndex(imgStore, repoName, log) + So(err, ShouldBeNil) + + // Find the index manifest + var indexManifestDesc ispec.Descriptor + + for _, desc := range idx.Manifests { + if desc.MediaType == ispec.MediaTypeImageIndex { + indexManifestDesc = desc + + break + } + } + + // Get the index content to find the manifest digests within it + indexBlob, err := imgStore.GetBlobContent(repoName, indexManifestDesc.Digest) + So(err, ShouldBeNil) + + var indexContent ispec.Index + err = json.Unmarshal(indexBlob, &indexContent) + So(err, ShouldBeNil) + + // Delete one of the manifest blobs within the index (but not all) + missingManifestDig := indexContent.Manifests[0].Digest.Encoded() + missingManifestFile := path.Join(imgStore.RootDir(), repoName, "/blobs/sha256", missingManifestDig) + err = driver.Delete(missingManifestFile) + So(err, ShouldBeNil) + + buff := bytes.NewBufferString("") + + res, err := storeCtlr.CheckAllBlobsIntegrity(context.Background()) + res.PrintScrubResults(buff) + So(err, ShouldBeNil) + + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + + // Should mark the index as affected due to missing manifest + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, "test 2.0 affected") + // Should continue processing and report the missing manifest + So(actual, ShouldContainSubstring, missingManifestDig) + }) + + Convey("Scrub index with non-missing error on manifest blob via file permissions", func() { + // Skip for non-local storage (S3/DynamoDB) since we can't chmod remote files + if driver.Name() != storageConstants.LocalStorageDriverName { + return + } + + // Create a multiarch image with multiple manifests + multiarchImage := CreateMultiarchWith().RandomImages(2).Build() + err = WriteMultiArchImageToFileSystem(multiarchImage, repoName, "2.1", storeCtlr) + So(err, ShouldBeNil) + + // Get the index to find the index manifest digest + idx, err := common.GetIndex(imgStore, repoName, log) + So(err, ShouldBeNil) + + // Find the index manifest + var indexManifestDesc ispec.Descriptor + + for _, desc := range idx.Manifests { + if desc.MediaType == ispec.MediaTypeImageIndex { + indexManifestDesc = desc + + break + } + } + + // Get the index content to find the manifest digests within it + indexBlob, err := imgStore.GetBlobContent(repoName, indexManifestDesc.Digest) + So(err, ShouldBeNil) + + var indexContent ispec.Index + err = json.Unmarshal(indexBlob, &indexContent) + So(err, ShouldBeNil) + + // Remove read permissions on one of the manifest blobs to cause a permission denied error (non-missing error) + manifestDig := indexContent.Manifests[0].Digest.Encoded() + manifestFile := path.Join(imgStore.RootDir(), repoName, "/blobs/sha256", manifestDig) + err = os.Chmod(manifestFile, 0o000) + So(err, ShouldBeNil) + + // Restore permissions after test + defer func() { + _ = os.Chmod(manifestFile, 0o644) + }() + + buff := bytes.NewBufferString("") + + res, err := storeCtlr.CheckAllBlobsIntegrity(context.Background()) + res.PrintScrubResults(buff) + So(err, ShouldBeNil) + + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + + // Should mark the index as affected due to non-missing error on manifest + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, "test 2.1 affected") + // Should report the manifest digest as affected blob + So(actual, ShouldContainSubstring, manifestDig) + // Should have "bad blob digest" error + So(actual, ShouldContainSubstring, "bad blob digest") + }) + Convey("Scrub index", func() { newImage := CreateRandomImage() newManifestDigest := newImage.ManifestDescriptor.Digest @@ -544,6 +661,41 @@ func RunCheckAllBlobsIntegrityTests( //nolint: thelper So(actual, ShouldContainSubstring, "test 0.0.1 ok") }) + Convey("preserve affected status when CheckLayers would overwrite it", func() { + // Create an image with a subject + index, err := common.GetIndex(imgStore, repoName, log) + So(err, ShouldBeNil) + + manifestDescriptor, ok := common.GetManifestDescByReference(index, image.ManifestDescriptor.Digest.String()) + So(ok, ShouldBeTrue) + + subjectImage := CreateDefaultImageWith().Subject(&manifestDescriptor).Build() + err = WriteImageToFileSystem(subjectImage, repoName, "0.0.3", storeCtlr) + So(err, ShouldBeNil) + + // Delete the subject manifest to mark it as affected + subjectManifestDig := manifestDescriptor.Digest.Encoded() + subjectManifestFile := path.Join(imgStore.RootDir(), repoName, "/blobs/sha256", subjectManifestDig) + err = driver.Delete(subjectManifestFile) + So(err, ShouldBeNil) + + buff := bytes.NewBufferString("") + + res, err := storeCtlr.CheckAllBlobsIntegrity(context.Background()) + res.PrintScrubResults(buff) + So(err, ShouldBeNil) + + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + + // The manifest with the missing subject should be marked as affected + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, "test 0.0.3 affected") + // Even if CheckLayers would pass, the affected status from the missing subject should be preserved + So(actual, ShouldContainSubstring, subjectManifestDig) + }) + Convey("the subject of the current manifest doesn't exist", func() { index, err := common.GetIndex(imgStore, repoName, log) So(err, ShouldBeNil) @@ -623,75 +775,173 @@ func RunCheckAllBlobsIntegrityTests( //nolint: thelper So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") So(actual, ShouldContainSubstring, "test 0.0.2 affected") }) - }) - Convey("test errors", func() { - mockedImgStore := mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{repoName}, nil - }, - ValidateRepoFn: func(name string) (bool, error) { - return false, nil - }, - } + Convey("test errors", func() { + mockedImgStore := mocks.MockedImageStore{ + GetRepositoriesFn: func() ([]string, error) { + return []string{repoName}, nil + }, + ValidateRepoFn: func(name string) (bool, error) { + return false, nil + }, + } - storeController := storage.StoreController{} - storeController.DefaultStore = mockedImgStore + storeController := storage.StoreController{} + storeController.DefaultStore = mockedImgStore - _, err := storeController.CheckAllBlobsIntegrity(context.Background()) - So(err, ShouldNotBeNil) - So(err, ShouldEqual, zerr.ErrRepoBadLayout) + _, err := storeController.CheckAllBlobsIntegrity(context.Background()) + So(err, ShouldNotBeNil) + So(err, ShouldEqual, zerr.ErrRepoBadLayout) - mockedImgStore = mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{repoName}, nil - }, - GetIndexContentFn: func(repo string) ([]byte, error) { - return []byte{}, errUnexpectedError - }, - } + mockedImgStore = mocks.MockedImageStore{ + GetRepositoriesFn: func() ([]string, error) { + return []string{repoName}, nil + }, + GetIndexContentFn: func(repo string) ([]byte, error) { + return []byte{}, errUnexpectedError + }, + } - storeController.DefaultStore = mockedImgStore + storeController.DefaultStore = mockedImgStore - _, err = storeController.CheckAllBlobsIntegrity(context.Background()) - So(err, ShouldNotBeNil) - So(err, ShouldEqual, errUnexpectedError) + _, err = storeController.CheckAllBlobsIntegrity(context.Background()) + So(err, ShouldNotBeNil) + So(err, ShouldEqual, errUnexpectedError) - manifestDigest := godigest.FromString("abcd") + manifestDigest := godigest.FromString("abcd") - mockedImgStore = mocks.MockedImageStore{ - GetRepositoriesFn: func() ([]string, error) { - return []string{repoName}, nil - }, - GetIndexContentFn: func(repo string) ([]byte, error) { - var index ispec.Index - index.SchemaVersion = 2 - index.Manifests = []ispec.Descriptor{ - { - MediaType: "InvalidMediaType", - Digest: manifestDigest, - Size: int64(100), - Annotations: map[string]string{ispec.AnnotationRefName: "1.0"}, - }, - } + mockedImgStore = mocks.MockedImageStore{ + GetRepositoriesFn: func() ([]string, error) { + return []string{repoName}, nil + }, + GetIndexContentFn: func(repo string) ([]byte, error) { + var index ispec.Index + index.SchemaVersion = 2 + index.Manifests = []ispec.Descriptor{ + { + MediaType: "InvalidMediaType", + Digest: manifestDigest, + Size: int64(100), + Annotations: map[string]string{ispec.AnnotationRefName: "1.0"}, + }, + } - return json.Marshal(index) - }, - } + return json.Marshal(index) + }, + } - storeController.DefaultStore = mockedImgStore + storeController.DefaultStore = mockedImgStore - res, err := storeController.CheckAllBlobsIntegrity(context.Background()) - So(err, ShouldBeNil) + res, err := storeController.CheckAllBlobsIntegrity(context.Background()) + So(err, ShouldBeNil) - buff := bytes.NewBufferString("") - res.PrintScrubResults(buff) + buff := bytes.NewBufferString("") + res.PrintScrubResults(buff) - space := regexp.MustCompile(`\s+`) - str := space.ReplaceAllString(buff.String(), " ") - actual := strings.TrimSpace(str) - So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") - So(actual, ShouldContainSubstring, fmt.Sprintf("%s 1.0 affected %s invalid manifest content", - repoName, manifestDigest.Encoded())) + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, fmt.Sprintf("%s 1.0 affected %s invalid manifest content", + repoName, manifestDigest.Encoded())) + }) + + Convey("scrub with non-missing error on manifest subject blob via file permissions", func() { + // Skip for non-local storage (S3/DynamoDB) since we can't chmod remote files + if driver.Name() != storageConstants.LocalStorageDriverName { + return + } + + index, err := common.GetIndex(imgStore, repoName, log) + So(err, ShouldBeNil) + + manifestDescriptor, ok := common.GetManifestDescByReference(index, image.ManifestDescriptor.Digest.String()) + So(ok, ShouldBeTrue) + + // Create an image with a subject + subjectImage := CreateDefaultImageWith().Subject(&manifestDescriptor).Build() + err = WriteImageToFileSystem(subjectImage, repoName, "0.0.6", storeCtlr) + So(err, ShouldBeNil) + + // Get the subject manifest digest + subjectManifestDig := manifestDescriptor.Digest.Encoded() + subjectManifestFile := path.Join(imgStore.RootDir(), repoName, "/blobs/sha256", subjectManifestDig) + + // Remove read permissions to cause a permission denied error (non-missing error) + err = os.Chmod(subjectManifestFile, 0o000) + So(err, ShouldBeNil) + + // Restore permissions after test + defer func() { + _ = os.Chmod(subjectManifestFile, 0o644) + }() + + buff := bytes.NewBufferString("") + + res, err := storeCtlr.CheckAllBlobsIntegrity(context.Background()) + res.PrintScrubResults(buff) + So(err, ShouldBeNil) + + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + + // Should mark the manifest as affected due to non-missing error on subject + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, "test 0.0.6 affected") + // Should report the subject digest as affected blob + So(actual, ShouldContainSubstring, subjectManifestDig) + // Should have "bad blob digest" error + So(actual, ShouldContainSubstring, "bad blob digest") + }) + + Convey("scrub with non-missing error on index subject blob via file permissions", func() { + // Skip for non-local storage (S3/DynamoDB) since we can't chmod remote files + if driver.Name() != storageConstants.LocalStorageDriverName { + return + } + + index, err := common.GetIndex(imgStore, repoName, log) + So(err, ShouldBeNil) + + manifestDescriptor, ok := common.GetManifestDescByReference(index, image.ManifestDescriptor.Digest.String()) + So(ok, ShouldBeTrue) + + // Create a multiarch image with a subject + err = WriteMultiArchImageToFileSystem(CreateMultiarchWith().RandomImages(1).Subject(&manifestDescriptor).Build(), + repoName, "0.0.7", storeCtlr) + So(err, ShouldBeNil) + + // Get the subject manifest digest + subjectManifestDig := manifestDescriptor.Digest.Encoded() + subjectManifestFile := path.Join(imgStore.RootDir(), repoName, "/blobs/sha256", subjectManifestDig) + + // Remove read permissions to cause a permission denied error (non-missing error) + err = os.Chmod(subjectManifestFile, 0o000) + So(err, ShouldBeNil) + + // Restore permissions after test + defer func() { + _ = os.Chmod(subjectManifestFile, 0o644) + }() + + buff := bytes.NewBufferString("") + + res, err := storeCtlr.CheckAllBlobsIntegrity(context.Background()) + res.PrintScrubResults(buff) + So(err, ShouldBeNil) + + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(buff.String(), " ") + actual := strings.TrimSpace(str) + + // Should mark the index as affected due to non-missing error on subject + So(actual, ShouldContainSubstring, "REPOSITORY TAG STATUS AFFECTED BLOB ERROR") + So(actual, ShouldContainSubstring, "test 0.0.7 affected") + // Should report the subject digest as affected blob + So(actual, ShouldContainSubstring, subjectManifestDig) + // Should have "bad blob digest" error + So(actual, ShouldContainSubstring, "bad blob digest") + }) }) } diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index 076635de..2c1eccef 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -1504,7 +1504,7 @@ func TestDeleteBlobsInUse(t *testing.T) { So(err, ShouldBeNil) ok, err := storageCommon.IsBlobReferenced(imgStore, repoName, unusedDigest, log) - So(err, ShouldNotBeNil) + So(err, ShouldBeNil) So(ok, ShouldBeFalse) }) }