Use default platform in convert

Before this change, the SOCI cli defaulted to converting all platforms.
This is inconsistent with nerdctl's pull, push, and convert functions.
It also leads to weird issues like this not working:

```
nerdctl pull docker.io/library/busybox:latest
soci convert docker.io/library/busybox:latest other:latest-soci
```

the solution is either to pull with `--all-platforms` or convert with
`--platform`. The latter had an additional bug that convert touched all
manifests, even if you didn't convert them.

This change makes SOCI default to single platform conversions and fixes
the manifests bug. SOCI convert now behaves like nerdctl
pull/convert/push.

Signed-off-by: Kern Walster <walster@amazon.com>

Author: Kern Walster

cmd/soci/commands/convert.go

@@ -27,7 +27,6 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/containerd/reference"
-	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/urfave/cli"
 )
 
@@ -147,13 +146,7 @@ var ConvertCommand = cli.Command{
 		}
 		defer done(ctx)
 
-		var platforms []v1.Platform
-		explicitPlatforms := cliContext.StringSlice(internal.PlatformFlagKey)
-		if len(explicitPlatforms) > 0 {
-			platforms, err = internal.GetPlatforms(ctx, cliContext, srcImg, cs)
-		} else {
-			platforms, err = images.Platforms(ctx, cs, srcImg.Target)
-		}
+		platforms, err := internal.GetPlatforms(ctx, cliContext, srcImg, cs)
 		if err != nil {
 			return err
 		}

cmd/soci/commands/create.go

@@ -23,6 +23,7 @@ import (
 	"github.com/awslabs/soci-snapshotter/cmd/soci/commands/internal"
 	"github.com/awslabs/soci-snapshotter/soci"
 	"github.com/awslabs/soci-snapshotter/soci/store"
+	"github.com/containerd/platforms"
 	"github.com/urfave/cli"
 )
 
@@ -100,6 +101,9 @@ var CreateCommand = cli.Command{
 		if err != nil {
 			return err
 		}
+		if len(ps) == 0 {
+			ps = append(ps, platforms.DefaultSpec())
+		}
 
 		artifactsDb, err := soci.NewDB(soci.ArtifactsDbPath(cliContext.GlobalString("root")))
 		if err != nil {

cmd/soci/commands/index/list.go

@@ -17,6 +17,7 @@
 package index
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -26,6 +27,7 @@ import (
 
 	"github.com/awslabs/soci-snapshotter/cmd/soci/commands/internal"
 	"github.com/awslabs/soci-snapshotter/soci"
+	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/platforms"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
@@ -119,6 +121,9 @@ var listCommand = cli.Command{
 			for _, plat := range plats {
 				desc, err := soci.GetImageManifestDescriptor(ctx, cs, img.Target, platforms.OnlyStrict(plat))
 				if err != nil {
+					if errors.Is(err, errdefs.ErrNotFound) {
+						return fmt.Errorf("image manifest for platform %s: %w", platforms.Format(plat), err)
+					}
 					return err
 				}
 				filters = append(filters, originalDigestFilter(desc.Digest.String()))

cmd/soci/commands/internal/platform.go

@@ -47,7 +47,7 @@ var PlatformFlags = []cli.Flag{
 // The order of preference is:
 // 1) all platforms supported by the image if the `all-plaforms` flag is set
 // 2) the set of platforms specified by the `platform` flag
-// 3) the default platform
+// 3) An empty platform slice. The consumer is responsible for setting an appropriate platform
 //
 // This method is not suitable for situations where the default should be all supported platforms (e.g. the `soci index list` command)
 func GetPlatforms(ctx context.Context, cliContext *cli.Context, img images.Image, cs content.Store) ([]ocispec.Platform, error) {
@@ -56,7 +56,7 @@ func GetPlatforms(ctx context.Context, cliContext *cli.Context, img images.Image
 	}
 	ps := cliContext.StringSlice(PlatformFlagKey)
 	if len(ps) == 0 {
-		return []ocispec.Platform{platforms.DefaultSpec()}, nil
+		return []ocispec.Platform{}, nil
 	}
 	var result []ocispec.Platform
 	for _, p := range ps {

cmd/soci/commands/push.go

@@ -31,6 +31,7 @@ import (
 	"github.com/awslabs/soci-snapshotter/soci"
 	"github.com/awslabs/soci-snapshotter/soci/store"
 	"github.com/containerd/containerd/reference"
+	"github.com/containerd/platforms"
 	dockercliconfig "github.com/docker/cli/cli/config"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/urfave/cli"
@@ -98,6 +99,9 @@ if they are available in the snapshotter's local content store.
 		if err != nil {
 			return err
 		}
+		if len(ps) == 0 {
+			ps = append(ps, platforms.DefaultSpec())
+		}
 
 		artifactsDb, err := soci.NewDB(soci.ArtifactsDbPath(cliContext.GlobalString("root")))
 		if err != nil {

integration/convert_test.go

@@ -146,6 +146,24 @@ func TestConvert(t *testing.T) {
 				convertedRef := imgRef + "-soci"
 
 				sh.X("nerdctl", "pull", "--all-platforms", imgRef)
+				sh.X("soci", "convert", "--all-platforms", "--min-layer-size", "0", imgRef, convertedRef)
+
+				imgDigest := getImageDigest(sh, imgRef)
+				convertedDigest := getImageDigest(sh, convertedRef)
+
+				validateConversion(t, sh, imgDigest, convertedDigest)
+			})
+		}
+	})
+
+	t.Run("single platform conversion", func(t *testing.T) {
+		for _, imageName := range convertImages {
+			t.Run(imageName, func(t *testing.T) {
+				rebootContainerd(t, sh, "", "")
+				imgRef := dockerhub(imageName).ref
+				convertedRef := imgRef + "-soci"
+
+				sh.X("nerdctl", "pull", imgRef)
 				sh.X("soci", "convert", "--min-layer-size", "0", imgRef, convertedRef)
 
 				imgDigest := getImageDigest(sh, imgRef)
@@ -165,8 +183,8 @@ func TestConvert(t *testing.T) {
 				convertedRef2 := imgRef + "-soci-2"
 
 				sh.X("nerdctl", "pull", "--all-platforms", imgRef)
-				sh.X("soci", "convert", imgRef, convertedRef1)
-				sh.X("soci", "convert", convertedRef1, convertedRef2)
+				sh.X("soci", "convert", "--all-platforms", imgRef, convertedRef1)
+				sh.X("soci", "convert", "--all-platforms", convertedRef1, convertedRef2)
 
 				convertedDigest1 := getImageDigest(sh, convertedRef1)
 				convertedDigest2 := getImageDigest(sh, convertedRef2)
@@ -177,7 +195,7 @@ func TestConvert(t *testing.T) {
 		}
 	})
 
-	t.Run("single platform conversion", func(t *testing.T) {
+	t.Run("single image manifest conversion", func(t *testing.T) {
 		images := []string{cloudwatchAgentx86ImageRef}
 		for _, imgRef := range images {
 			t.Run(imgRef, func(t *testing.T) {
@@ -215,21 +233,89 @@ func TestConvert(t *testing.T) {
 
 }
 
+type convertAndPushTestConfig struct {
+	PullPlatforms    string
+	ConvertPlatforms string
+	PushPlatforms    string
+}
+
+func (c convertAndPushTestConfig) String() string {
+	return strings.Join([]string{c.PullPlatforms, c.ConvertPlatforms, c.PushPlatforms}, ",")
+}
+
 func TestConvertAndPush(t *testing.T) {
 	registryConfig := newRegistryConfig()
 	sh, done := newShellWithRegistry(t, registryConfig)
 	defer done()
-	for _, imageName := range convertImages {
-		t.Run(imageName, func(t *testing.T) {
-			rebootContainerd(t, sh, "", "")
-			img := dockerhub(imageName)
-			convertedImg := registryConfig.mirror(imageName)
 
-			sh.X("nerdctl", "pull", "--all-platforms", img.ref)
-			sh.X("soci", "convert", img.ref, convertedImg.ref)
-			sh.X("nerdctl", "login", "--username", registryConfig.user, "--password", registryConfig.pass, convertedImg.ref)
-			sh.X("nerdctl", "push", "--all-platforms", convertedImg.ref)
-		})
+	imageName := nginxImage
+
+	platformOptions := map[string][]string{
+		"one": {"--platform", "linux/x86_64"},
+		"all": {"--all-platforms"},
+	}
+
+	convertFailureMessages := map[convertAndPushTestConfig]string{
+		// Any config not in this list should succeed on convert (no error message)
+		{PullPlatforms: "one", ConvertPlatforms: "all", PushPlatforms: "one"}: "not found",
+		{PullPlatforms: "one", ConvertPlatforms: "all", PushPlatforms: "all"}: "not found",
+	}
+
+	pushFailureMessages := map[convertAndPushTestConfig]string{
+		// Any config not in this list should succeed on push (no error message)
+		{PullPlatforms: "one", ConvertPlatforms: "one", PushPlatforms: "all"}: "not found",
+	}
+
+	for pullPlatform := range platformOptions {
+		for convertPlatform := range platformOptions {
+			for pushPlatform := range platformOptions {
+				test := convertAndPushTestConfig{
+					PullPlatforms:    pullPlatform,
+					ConvertPlatforms: convertPlatform,
+					PushPlatforms:    pushPlatform,
+				}
+
+				t.Run(test.String(), func(t *testing.T) {
+					rebootContainerd(t, sh, "", "")
+					img := dockerhub(imageName)
+					convertedImg := registryConfig.mirror(imageName)
+
+					pull := []string{"nerdctl", "pull"}
+					convert := []string{"soci", "convert"}
+					push := []string{"nerdctl", "push"}
+
+					pull = append(pull, platformOptions[pullPlatform]...)
+					convert = append(convert, platformOptions[convertPlatform]...)
+					push = append(push, platformOptions[pushPlatform]...)
+
+					convertFailureMessage, expectedConvertFailure := convertFailureMessages[test]
+					pushFailureMessage := pushFailureMessages[test]
+
+					sh.X(append(pull, img.ref)...)
+					o, err := sh.CombinedOLog(append(convert, img.ref, convertedImg.ref)...)
+					validateErrorOutput(t, "convert", string(o), err, convertFailureMessage)
+					if expectedConvertFailure {
+						// If we expected a convert error and we got the correct one, then the test is done.
+						// We should push because we know it will fail.
+						return
+					}
+					sh.X("nerdctl", "login", "--username", registryConfig.user, "--password", registryConfig.pass, convertedImg.ref)
+					o, err = sh.CombinedOLog(append(push, convertedImg.ref)...)
+					validateErrorOutput(t, "push", string(o), err, pushFailureMessage)
+				})
+
+			}
+		}
+	}
+}
+
+func validateErrorOutput(t *testing.T, name string, o string, err error, expectedError string) {
+	if expectedError != "" {
+		if !strings.Contains(o, expectedError) {
+			t.Fatalf("%s: expected error %q, got %q", name, expectedError, o)
+		}
+	} else if err != nil {
+		t.Fatalf("%s: unexpected error: %v", name, err)
 	}
 }
 

soci/soci_convert.go

@@ -28,6 +28,7 @@ import (
 	"github.com/awslabs/soci-snapshotter/soci/store"
 	"github.com/awslabs/soci-snapshotter/util/ociutil"
 	"github.com/containerd/containerd/images"
+	"github.com/containerd/errdefs"
 	"github.com/containerd/platforms"
 	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go"
@@ -45,7 +46,9 @@ type convertConfig struct {
 // ConvertWithPlatforms sets the platforms that will be indexed during conversion
 func ConvertWithPlatforms(platforms ...ocispec.Platform) ConvertOption {
 	return func(cc *convertConfig) error {
-		cc.platforms = platforms
+		if len(platforms) != 0 {
+			cc.platforms = platforms
+		}
 		return nil
 	}
 }
@@ -82,17 +85,26 @@ func (b *IndexBuilder) Convert(ctx context.Context, img images.Image, opts ...Co
 	if len(allPlatforms) == 0 {
 		return nil, errors.New("image does not support any platforms")
 	}
-	if images.IsManifestType(img.Target.MediaType) && img.Target.Platform == nil {
+	defaultPlatform := platforms.DefaultSpec()
+	if images.IsManifestType(img.Target.MediaType) {
 		// If the image's target descriptor is a single manifest, then it will not
 		// contain a platform because that information is stored in the image config instead.
 		// If we directly use this descriptor in the converted image,
 		// runtimes will not be able to pull the correct manifest.
 		// We know the actual platform by inspecting the image config in `images.Platforms`,
 		// so we add that to the target descriptor.
-		img.Target.Platform = &allPlatforms[0]
+		imagePlatform := allPlatforms[0]
+		img.Target.Platform = &imagePlatform
+
+		// We also set the default platform as the image's platform.
+		// We shouldn't force the user to specify the platform if
+		// we know the only platform the image supports. This allows users
+		// to convert single platform images on non-native hardware
+		// (e.g. converting an arm64 image on an amd64 machine without explicitly specifying the platform)
+		defaultPlatform = imagePlatform
 	}
 	convertCfg := convertConfig{
-		platforms: allPlatforms,
+		platforms: []ocispec.Platform{defaultPlatform},
 		gcRoot:    true,
 	}
 	for _, opt := range opts {
@@ -226,10 +238,33 @@ func (b *IndexBuilder) newOciIndex(ctx context.Context, img images.Image) (ocisp
 func (b *IndexBuilder) annotateImages(ctx context.Context, ociIndex *ocispec.Index, sociIndexes []*IndexWithMetadata) error {
 	for i := 0; i < len(ociIndex.Manifests); i++ {
 		manifestDesc := &ociIndex.Manifests[i]
+
+		var indexWithMetadata *IndexWithMetadata
+		idx := slices.IndexFunc(sociIndexes, func(i *IndexWithMetadata) bool { return i.ManifestDesc.Digest == manifestDesc.Digest })
+		if idx >= 0 {
+			indexWithMetadata = sociIndexes[idx]
+		}
+
 		// images.Manifest validates the mediatype, no need to do it ourselves like
 		// we did when loading the OCI index
 		manifest, err := images.Manifest(ctx, b.contentStore, *manifestDesc, nil)
 		if err != nil {
+			// If the manifest is not found:
+			//   if there is an index for the image: error. The manifest was unexpectedly deleted
+			//   if there is not an index for the image: skip.
+			//       In this case, we are working with a multi-platform image where the user only
+			//       pulled a subset of the platforms. We should convert the platforms
+			//       requested and not touch anything else. The user can still push the image as a reduced
+			//       platform (i.e. without --all-platforms).
+			//       This is the case of:
+			//           nerdctl pull $IMAGE
+			//           soci convert $IMAGE $IMAGE-soci
+			//           nerdctl push $IMAGE-soci
+			if errors.Is(err, errdefs.ErrNotFound) {
+				if indexWithMetadata == nil {
+					continue
+				}
+			}
 			return err
 		}
 		// Some Registries don't like mixing Docker V2 manifests with OCI image manifests.
@@ -242,10 +277,7 @@ func (b *IndexBuilder) annotateImages(ctx context.Context, ociIndex *ocispec.Ind
 		// We also aren't modifying image contents at all, so we don't need to modify the config contents.
 		manifest.Config.MediaType = ocispec.MediaTypeImageConfig
 
-		idx := slices.IndexFunc(sociIndexes, func(i *IndexWithMetadata) bool { return i.ManifestDesc.Digest == manifestDesc.Digest })
-		if idx >= 0 {
-			indexWithMetadata := sociIndexes[idx]
-
+		if indexWithMetadata != nil {
 			if manifest.Annotations == nil {
 				manifest.Annotations = make(map[string]string)
 			}

soci/soci_index.go

@@ -34,14 +34,14 @@ import (
 	"github.com/awslabs/soci-snapshotter/ztoc"
 	"github.com/awslabs/soci-snapshotter/ztoc/compression"
 	"github.com/containerd/containerd/content"
+	"github.com/containerd/errdefs"
+	"oras.land/oras-go/v2/errdef"
 
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/log"
 	"github.com/containerd/platforms"
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
-
-	"oras.land/oras-go/v2/errdef"
 )
 
 const (
@@ -501,6 +501,9 @@ func (b *IndexBuilder) build(ctx context.Context, img images.Image, buildCfg bui
 	// images.Manifest, images.Children will error out when reading the manifest blob (this happens on containerd side)
 	imgManifestDesc, err := GetImageManifestDescriptor(ctx, b.contentStore, img.Target, platformMatcher)
 	if err != nil {
+		if errors.Is(err, errdefs.ErrNotFound) {
+			return nil, fmt.Errorf("image manifest for %s: %w", platforms.Format(buildCfg.platform), err)
+		}
 		return nil, err
 	}
 	manifest, err := images.Manifest(ctx, b.contentStore, img.Target, platformMatcher)
@@ -726,8 +729,11 @@ func GetImageManifestDescriptor(ctx context.Context, cs content.Store, imageTarg
 				return &manifest, nil
 			}
 		}
-		return nil, errors.New("image manifest not found")
+		return nil, errdefs.ErrNotFound
 	} else if images.IsManifestType(imageTarget.MediaType) {
+		if imageTarget.Platform != nil && !platform.Match(*imageTarget.Platform) {
+			return nil, errdefs.ErrNotFound
+		}
 		return &imageTarget, nil
 	}