manifest: merge manifest lists

jedevc · jedevc · commit 577dc075f87b · 2023-01-31T13:50:49.000Z
Previously, merging multiple manifest lists together was not supported
as a use case. However, with new versions of buildkit, manifest lists
will be created by default. To support these new manifest lists, we
should support merging manifest lists, just as we support this with
manifests today.

To do this, we refactor the manifest store to save and load groups of
manifests, instead of only one. On disk, these manifests take the form
of suffixed indexes, e.g. "_2", "_3", etc (note that we ignore "_1",
which is permitted to allow a backwards-compatible change with the
previous disk format).

Then, only the commands need small updates - "create" now looks up
multiple manifests and saves multiple manifests, while "annotate" now
annotates all the matching manifests.

Signed-off-by: Justin Chadwell &lt;me@jedevc.com&gt;
diff --git a/cli/command/manifest/annotate.go b/cli/command/manifest/annotate.go
@@ -58,38 +58,40 @@ func runManifestAnnotate(dockerCli command.Cli, opts annotateOptions) error {
 	}
 
 	manifestStore := dockerCli.ManifestStore()
-	imageManifest, err := manifestStore.Get(targetRef, imgRef)
+	imageManifests, err := manifestStore.Get(targetRef, imgRef)
 	switch {
 	case errors.Is(err, types.ErrManifestNotFound):
 		return fmt.Errorf("manifest for image %s does not exist in %s", opts.image, opts.target)
 	case err != nil:
 		return err
 	}
 
-	// Update the mf
-	if imageManifest.Descriptor.Platform == nil {
-		imageManifest.Descriptor.Platform = new(ocispec.Platform)
-	}
-	if opts.os != "" {
-		imageManifest.Descriptor.Platform.OS = opts.os
-	}
-	if opts.arch != "" {
-		imageManifest.Descriptor.Platform.Architecture = opts.arch
-	}
-	for _, osFeature := range opts.osFeatures {
-		imageManifest.Descriptor.Platform.OSFeatures = appendIfUnique(imageManifest.Descriptor.Platform.OSFeatures, osFeature)
-	}
-	if opts.variant != "" {
-		imageManifest.Descriptor.Platform.Variant = opts.variant
-	}
-	if opts.osVersion != "" {
-		imageManifest.Descriptor.Platform.OSVersion = opts.osVersion
-	}
-
-	if !isValidOSArch(imageManifest.Descriptor.Platform.OS, imageManifest.Descriptor.Platform.Architecture) {
-		return errors.Errorf("manifest entry for image has unsupported os/arch combination: %s/%s", opts.os, opts.arch)
+	for i, imageManifest := range imageManifests {
+		// Update the mf
+		if imageManifest.Descriptor.Platform == nil {
+			imageManifest.Descriptor.Platform = new(ocispec.Platform)
+		}
+		if opts.os != "" {
+			imageManifest.Descriptor.Platform.OS = opts.os
+		}
+		if opts.arch != "" {
+			imageManifest.Descriptor.Platform.Architecture = opts.arch
+		}
+		for _, osFeature := range opts.osFeatures {
+			imageManifest.Descriptor.Platform.OSFeatures = appendIfUnique(imageManifest.Descriptor.Platform.OSFeatures, osFeature)
+		}
+		if opts.variant != "" {
+			imageManifest.Descriptor.Platform.Variant = opts.variant
+		}
+		if opts.osVersion != "" {
+			imageManifest.Descriptor.Platform.OSVersion = opts.osVersion
+		}
+		if !isValidOSArch(imageManifest.Descriptor.Platform.OS, imageManifest.Descriptor.Platform.Architecture) {
+			return errors.Errorf("manifest entry for image has unsupported os/arch combination: %s/%s", opts.os, opts.arch)
+		}
+		imageManifests[i] = imageManifest
 	}
-	return manifestStore.Save(targetRef, imgRef, imageManifest)
+	return manifestStore.Save(targetRef, imgRef, imageManifests...)
 }
 
 func appendIfUnique(list []string, str string) []string {
diff --git a/cli/command/manifest/create_list.go b/cli/command/manifest/create_list.go
@@ -69,11 +69,11 @@ func createManifestList(dockerCli command.Cli, args []string, opts createOpts) e
 			return err
 		}
 
-		manifest, err := getManifest(ctx, dockerCli, targetRef, namedRef, opts.insecure)
+		manifests, err := getManifests(ctx, dockerCli, targetRef, namedRef, opts.insecure)
 		if err != nil {
 			return err
 		}
-		if err := manifestStore.Save(targetRef, namedRef, manifest); err != nil {
+		if err := manifestStore.Save(targetRef, namedRef, manifests...); err != nil {
 			return err
 		}
 	}
diff --git a/cli/command/manifest/inspect.go b/cli/command/manifest/inspect.go
@@ -55,40 +55,28 @@ func runInspect(dockerCli command.Cli, opts inspectOptions) error {
 		return err
 	}
 
-	// If list reference is provided, display the local manifest in a list
+	var listRef reference.Named
 	if opts.list != "" {
-		listRef, err := normalizeReference(opts.list)
+		listRef, err = normalizeReference(opts.list)
 		if err != nil {
 			return err
 		}
-
-		imageManifest, err := dockerCli.ManifestStore().Get(listRef, namedRef)
-		if err != nil {
-			return err
-		}
-		return printManifest(dockerCli, imageManifest, opts)
-	}
-
-	// Try a local manifest list first
-	localManifestList, err := dockerCli.ManifestStore().GetList(namedRef)
-	if err == nil {
-		return printManifestList(dockerCli, namedRef, localManifestList, opts)
 	}
 
-	// Next try a remote manifest
 	ctx := context.Background()
-	registryClient := dockerCli.RegistryClient(opts.insecure)
-	imageManifest, err := registryClient.GetManifest(ctx, namedRef)
-	if err == nil {
-		return printManifest(dockerCli, imageManifest, opts)
-	}
-
-	// Finally try a remote manifest list
-	manifestList, err := registryClient.GetManifestList(ctx, namedRef)
+	manifests, err := getManifests(ctx, dockerCli, listRef, namedRef, opts.insecure)
 	if err != nil {
 		return err
 	}
-	return printManifestList(dockerCli, namedRef, manifestList, opts)
+
+	switch len(manifests) {
+	case 0:
+		return nil
+	case 1:
+		return printManifest(dockerCli, manifests[0], opts)
+	default:
+		return printManifestList(dockerCli, namedRef, manifests, opts)
+	}
 }
 
 func printManifest(dockerCli command.Cli, manifest types.ImageManifest, opts inspectOptions) error {
diff --git a/cli/command/manifest/util.go b/cli/command/manifest/util.go
@@ -67,29 +67,42 @@ func normalizeReference(ref string) (reference.Named, error) {
 	return namedRef, nil
 }
 
-// getManifest from the local store, and fallback to the remote registry if it
+// getManifests from the local store, and fallback to the remote registry if it
 // doesn't exist locally
-func getManifest(ctx context.Context, dockerCli command.Cli, listRef, namedRef reference.Named, insecure bool) (types.ImageManifest, error) {
+func getManifests(ctx context.Context, dockerCli command.Cli, listRef, namedRef reference.Named, insecure bool) ([]types.ImageManifest, error) {
 	// load from the local store
 	if listRef != nil {
 		data, err := dockerCli.ManifestStore().Get(listRef, namedRef)
 		if err == nil {
 			return data, nil
 		} else if !errors.Is(err, types.ErrManifestNotFound) {
-			return types.ImageManifest{}, err
+			return nil, err
 		}
 	}
+	datas, err := dockerCli.ManifestStore().GetList(namedRef)
+	if err == nil {
+		return datas, nil
+	} else if !errors.Is(err, types.ErrManifestNotFound) {
+		return nil, err
+	}
 
 	// load from the remote registry
 	client := dockerCli.RegistryClient(insecure)
 	if client != nil {
 		data, err := client.GetManifest(ctx, namedRef)
 		if err == nil {
-			return data, nil
+			return []types.ImageManifest{data}, nil
+		} else if !errors.Is(err, types.ErrManifestNotFound) {
+			return nil, err
+		}
+
+		datas, err = client.GetManifestList(ctx, namedRef)
+		if err == nil {
+			return datas, nil
 		} else if !errors.Is(err, types.ErrManifestNotFound) {
-			return types.ImageManifest{}, err
+			return nil, err
 		}
 	}
 
-	return types.ImageManifest{}, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", namedRef)
+	return nil, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", namedRef)
 }
diff --git a/cli/manifest/store/store.go b/cli/manifest/store/store.go
@@ -2,6 +2,7 @@ package store
 
 import (
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -17,9 +18,9 @@ import (
 // Store manages local storage of image distribution manifests
 type Store interface {
 	Remove(listRef reference.Reference) error
-	Get(listRef reference.Reference, manifest reference.Reference) (types.ImageManifest, error)
+	Get(listRef reference.Reference, manifest reference.Reference) ([]types.ImageManifest, error)
 	GetList(listRef reference.Reference) ([]types.ImageManifest, error)
-	Save(listRef reference.Reference, manifest reference.Reference, image types.ImageManifest) error
+	Save(listRef reference.Reference, manifest reference.Reference, image ...types.ImageManifest) error
 }
 
 // fsStore manages manifest files stored on the local filesystem
@@ -39,9 +40,30 @@ func (s *fsStore) Remove(listRef reference.Reference) error {
 }
 
 // Get returns the local manifest
-func (s *fsStore) Get(listRef reference.Reference, manifest reference.Reference) (types.ImageManifest, error) {
+func (s *fsStore) Get(listRef reference.Reference, manifest reference.Reference) ([]types.ImageManifest, error) {
+	var imgs []types.ImageManifest
 	filename := manifestToFilename(s.root, listRef.String(), manifest.String())
-	return s.getFromFilename(manifest, filename)
+
+	img, err := s.getFromFilename(manifest, filename)
+	if err != nil {
+		return nil, err
+	}
+	imgs = append(imgs, img)
+
+	i := 2
+	for {
+		img, err := s.getFromFilename(manifest, fmt.Sprintf("%s_%d", filename, i))
+		if errors.Is(err, types.ErrManifestNotFound) {
+			break
+		} else if err != nil {
+			return nil, err
+		}
+		imgs = append(imgs, img)
+
+		i++
+	}
+
+	return imgs, nil
 }
 
 func (s *fsStore) getFromFilename(ref reference.Reference, filename string) (types.ImageManifest, error) {
@@ -126,16 +148,34 @@ func (s *fsStore) listManifests(transaction string) ([]string, error) {
 }
 
 // Save a manifest as part of a local manifest list
-func (s *fsStore) Save(listRef reference.Reference, manifest reference.Reference, image types.ImageManifest) error {
+func (s *fsStore) Save(listRef reference.Reference, manifest reference.Reference, images ...types.ImageManifest) error {
 	if err := s.createManifestListDirectory(listRef.String()); err != nil {
 		return err
 	}
+	if len(images) == 0 {
+		return nil
+	}
+
 	filename := manifestToFilename(s.root, listRef.String(), manifest.String())
-	bytes, err := json.Marshal(image)
+	bytes, err := json.Marshal(images[0])
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(filename, bytes, 0o644)
+	if err := os.WriteFile(filename, bytes, 0o644); err != nil {
+		return err
+	}
+
+	for i, image := range images[1:] {
+		bytes, err := json.Marshal(image)
+		if err != nil {
+			return err
+		}
+		if err := os.WriteFile(fmt.Sprintf("%s_%d", filename, i+2), bytes, 0o644); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func (s *fsStore) createManifestListDirectory(transaction string) error {
diff --git a/cli/manifest/store/store_test.go b/cli/manifest/store/store_test.go
@@ -55,14 +55,14 @@ func TestStoreRemove(t *testing.T) {
 func TestStoreSaveAndGet(t *testing.T) {
 	store := NewStore(t.TempDir())
 	listRef := ref("list")
-	data := types.ImageManifest{Ref: sref(t, "abcdef")}
-	err := store.Save(listRef, ref("exists"), data)
+	data := []types.ImageManifest{{Ref: sref(t, "abcdef")}}
+	err := store.Save(listRef, ref("exists"), data...)
 	assert.NilError(t, err)
 
 	testcases := []struct {
 		listRef     reference.Reference
 		manifestRef reference.Reference
-		expected    types.ImageManifest
+		expected    []types.ImageManifest
 		expectedErr error
 	}{
 		{

Original file line number	Diff line number	Diff line change
`@@ -69,11 +69,11 @@ func createManifestList(dockerCli command.Cli, args []string, opts createOpts) e`
`69`	`69`	`return err`
`70`	`70`	`}`
`71`	`71`
`72`		`- manifest, err := getManifest(ctx, dockerCli, targetRef, namedRef, opts.insecure)`
	`72`	`+ manifests, err := getManifests(ctx, dockerCli, targetRef, namedRef, opts.insecure)`
`73`	`73`	`if err != nil {`
`74`	`74`	`return err`
`75`	`75`	`}`
`76`		`- if err := manifestStore.Save(targetRef, namedRef, manifest); err != nil {`
	`76`	`+ if err := manifestStore.Save(targetRef, namedRef, manifests...); err != nil {`
`77`	`77`	`return err`
`78`	`78`	`}`
`79`	`79`	`}`