From f0557f076e9f54ac3bd41c94de71a709bcf8e379 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 6 Feb 2024 17:36:48 -0500 Subject: [PATCH] Handle multiple artifacts correctly When saving artifacts to OCI layout directories to use as supplemental refernces, use a different directory for each artifact(!) When supplementing a reference in CopySpecificImages mode, only mark a supplemental reference as the source for a specified instance if it actually corresponds to the instance's digest. If an artifact manifest doesn't have any file layers, remember to catalog the empty descriptor that we add to its layer list to keep it from being empty, as recommended. Extend libimage/manifests.TestReference() to ensure that we're able to copy everything that we should be able to copy. Signed-off-by: Nalin Dahyabhai --- libimage/manifests/manifests.go | 22 ++++--- libimage/manifests/manifests_test.go | 88 ++++++++++++++++++++++++++-- pkg/supplemented/supplemented.go | 9 ++- 3 files changed, 105 insertions(+), 14 deletions(-) diff --git a/libimage/manifests/manifests.go b/libimage/manifests/manifests.go index b6461a6c3..aab804538 100644 --- a/libimage/manifests/manifests.go +++ b/libimage/manifests/manifests.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path/filepath" + "strconv" "strings" "time" @@ -284,10 +285,8 @@ func (l *list) Reference(store storage.Store, multiple cp.ImageListSelection, in } case cp.CopySpecificImages: for instance := range l.instances { - for _, allowed := range instances { - if instance == allowed { - whichInstances = append(whichInstances, instance) - } + if slices.Contains(instances, instance) { + whichInstances = append(whichInstances, instance) } } } @@ -304,8 +303,11 @@ func (l *list) Reference(store storage.Store, multiple cp.ImageListSelection, in if err != nil { return nil, err } + subdir := 0 for artifactManifestDigest, contents := range l.artifacts.Manifests { // create the blobs directory + subdir++ + tmp := filepath.Join(tmp, strconv.Itoa(subdir)) blobsDir := filepath.Join(tmp, "blobs", artifactManifestDigest.Algorithm().String()) if err := os.MkdirAll(blobsDir, 0o700); err != nil { return nil, fmt.Errorf("creating directory for blobs: %w", err) @@ -891,13 +893,11 @@ func (l *list) AddArtifact(ctx context.Context, sys *types.SystemContext, option } l.artifacts.Manifests[artifactManifestDigest] = string(artifactManifestBytes) l.artifacts.Layers[artifactManifestDigest] = nil + l.artifacts.Configs[artifactManifestDigest] = artifactManifest.Config.Digest if configFilePath != "" { - l.artifacts.Configs[artifactManifestDigest] = artifactManifest.Config.Digest l.artifacts.Detached[artifactManifest.Config.Digest] = configFilePath l.artifacts.Files[artifactManifestDigest] = append(l.artifacts.Files[artifactManifestDigest], configFilePath) - } - if len(artifactManifest.Config.Data) != 0 { - l.artifacts.Configs[artifactManifestDigest] = artifactManifest.Config.Digest + } else { l.artifacts.Blobs[artifactManifest.Config.Digest] = slices.Clone(artifactManifest.Config.Data) } for filePath, fileDigest := range fileDigests { @@ -905,6 +905,12 @@ func (l *list) AddArtifact(ctx context.Context, sys *types.SystemContext, option l.artifacts.Detached[fileDigest] = filePath l.artifacts.Files[artifactManifestDigest] = append(l.artifacts.Files[artifactManifestDigest], filePath) } + for _, layer := range layers { + if len(layer.Data) != 0 { + l.artifacts.Blobs[layer.Digest] = slices.Clone(layer.Data) + l.artifacts.Layers[artifactManifestDigest] = append(l.artifacts.Layers[artifactManifestDigest], layer.Digest) + } + } // Add this artifact manifest to the image index. if err := l.AddInstance(artifactManifestDigest, int64(len(artifactManifestBytes)), artifactManifest.MediaType, options.Platform.OS, options.Platform.Architecture, options.Platform.OSVersion, options.Platform.OSFeatures, options.Platform.Variant, nil, nil); err != nil { return "", fmt.Errorf("adding artifact manifest for %q to image index: %w", files, err) diff --git a/libimage/manifests/manifests_test.go b/libimage/manifests/manifests_test.go index 0640e4f5c..e29c90772 100644 --- a/libimage/manifests/manifests_test.go +++ b/libimage/manifests/manifests_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -17,8 +18,10 @@ import ( "github.com/containerd/containerd/platforms" "github.com/containers/common/pkg/manifests" cp "github.com/containers/image/v5/copy" + "github.com/containers/image/v5/directory" "github.com/containers/image/v5/manifest" "github.com/containers/image/v5/pkg/compression" + "github.com/containers/image/v5/signature" "github.com/containers/image/v5/transports" "github.com/containers/image/v5/transports/alltransports" "github.com/containers/image/v5/types" @@ -471,6 +474,26 @@ func TestReference(t *testing.T) { } }() + policy, err := signature.NewPolicyFromFile(sys.SignaturePolicyPath) + assert.NoErrorf(t, err, "NewPolicyFromFile()") + policyContext, err := signature.NewPolicyContext(policy) + assert.NoErrorf(t, err, "NewPolicyContext()") + destRef, err := directory.NewReference(filepath.Join(dir, "directory")) + assert.NoErrorf(t, err, "NewReference()") + checkCopy := func(ref types.ImageReference, selection cp.ImageListSelection, instances []digest.Digest) error { + if ref == nil { + return errors.New("no reference") + } + copyOptions := &cp.Options{ + ImageListSelection: selection, + SourceCtx: sys, + DestinationCtx: sys, + Instances: instances, + } + // t.Helper() + _, err := cp.Image(ctx, policyContext, destRef, ref, copyOptions) + return err + } ref, err := alltransports.ParseImageName(otherListImage) assert.NoErrorf(t, err, "ParseImageName(%q)", otherListImage) @@ -478,22 +501,33 @@ func TestReference(t *testing.T) { _, err = list.Add(ctx, ppc64sys, ref, false) assert.NoError(t, err, "list.Add(all=false)") + smallJSON := filepath.Join(dir, "small.json") + err = os.WriteFile(smallJSON, []byte(`{"slice":[1, 2, 3]}`), 0o600) + assert.NoError(t, err) + + minimumJSON := filepath.Join(dir, "minimum.json") + err = os.WriteFile(minimumJSON, []byte("{}"), 0o600) + assert.NoError(t, err) + emptyJSON := filepath.Join(dir, "empty.json") err = os.WriteFile(emptyJSON, []byte(""), 0o600) assert.NoError(t, err) + artifactOptions := AddArtifactOptions{ ConfigFile: emptyJSON, } _, err = list.AddArtifact(ctx, &types.SystemContext{}, artifactOptions) assert.NoErrorf(t, err, "list.AddArtifact(file=%s)", emptyJSON) + artifactOptions = AddArtifactOptions{ ConfigDescriptor: &v1.DescriptorEmptyJSON, } - _, err = list.AddArtifact(ctx, &types.SystemContext{}, artifactOptions) - assert.NoError(t, err, "list.AddArtifact(empty descriptor)") + minimumArtifactDigest, err := list.AddArtifact(ctx, &types.SystemContext{}, artifactOptions, minimumJSON) + assert.NoError(t, err, "list.AddArtifact(file=%s)", minimumJSON) + artifactOptions = AddArtifactOptions{} - _, err = list.AddArtifact(ctx, &types.SystemContext{}, artifactOptions) - assert.NoError(t, err, "list.AddArtifact(no descriptor, no file)") + smallArtifactDigest, err := list.AddArtifact(ctx, &types.SystemContext{}, artifactOptions, smallJSON) + assert.NoError(t, err, "list.AddArtifact(file=%s)", smallJSON) listRef, err := list.Reference(store, cp.CopyAllImages, nil) assert.Error(t, err, "list.Reference(never saved)") @@ -515,6 +549,18 @@ func TestReference(t *testing.T) { assert.Error(t, err, "list.Reference(never saved)") assert.Nilf(t, listRef, "list.Reference(never saved)") + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest}) + assert.Error(t, err, "list.Reference(never saved)") + assert.Nilf(t, listRef, "list.Reference(never saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{minimumArtifactDigest}) + assert.Error(t, err, "list.Reference(saved)") + + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest, smallArtifactDigest}) + assert.Error(t, err, "list.Reference(never saved)") + assert.Nilf(t, listRef, "list.Reference(never saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{minimumArtifactDigest, smallArtifactDigest}) + assert.Error(t, err, "list.Reference(saved)") + _, err = list.SaveToImage(store, "", []string{listImageName}, "") assert.NoError(t, err, "SaveToImage") @@ -538,28 +584,62 @@ func TestReference(t *testing.T) { assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{minimumArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest, smallArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{minimumArtifactDigest, smallArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + _, err = list.Add(ctx, sys, ref, true) assert.NoError(t, err, "list.Add(all=true)") listRef, err = list.Reference(store, cp.CopyAllImages, nil) assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopyAllImages, nil) + assert.NoError(t, err, "list.Reference(saved)") listRef, err = list.Reference(store, cp.CopySystemImage, nil) assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySystemImage, nil) + assert.NoError(t, err, "list.Reference(saved)") listRef, err = list.Reference(store, cp.CopySpecificImages, nil) assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, nil) + assert.NoError(t, err, "list.Reference(saved)") listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest}) assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest}) + assert.NoError(t, err, "list.Reference(saved)") listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest}) assert.NoError(t, err, "list.Reference(saved)") assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest}) + assert.NoError(t, err, "list.Reference(saved)") + + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + + listRef, err = list.Reference(store, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest, smallArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") + assert.NotNilf(t, listRef, "list.Reference(saved)") + err = checkCopy(listRef, cp.CopySpecificImages, []digest.Digest{otherListAmd64Digest, otherListArm64Digest, minimumArtifactDigest, smallArtifactDigest}) + assert.NoError(t, err, "list.Reference(saved)") } func TestPushManifest(t *testing.T) { diff --git a/pkg/supplemented/supplemented.go b/pkg/supplemented/supplemented.go index 6ae9a4160..0f1db4bf4 100644 --- a/pkg/supplemented/supplemented.go +++ b/pkg/supplemented/supplemented.go @@ -14,6 +14,7 @@ import ( multierror "github.com/hashicorp/go-multierror" digest "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" + "golang.org/x/exp/slices" ) // supplementedImageReference groups multiple references together. @@ -139,7 +140,7 @@ func (s *supplementedImageReference) NewImageSource(ctx context.Context, sys *ty } sources[manifestDigest] = src - // Parse the manifest as a list of images. + // Parse the manifest as a list of images and artifacts. list, err := manifest.ListFromBlob(manifestBytes, manifestType) if err != nil { return fmt.Errorf("parsing manifest blob %q as a %q: %w", string(manifestBytes), manifestType, err) @@ -155,7 +156,11 @@ func (s *supplementedImageReference) NewImageSource(ctx context.Context, sys *ty } chaseInstances = []digest.Digest{instance} case cp.CopySpecificImages: - chaseInstances = s.instances + for _, instance := range list.Instances() { + if slices.Contains(s.instances, instance) { + chaseInstances = append(chaseInstances, instance) + } + } case cp.CopyAllImages: chaseInstances = list.Instances() }