From 5b5049bf4d3a99df9a2b1c31d5d52ddff7b5cec2 Mon Sep 17 00:00:00 2001 From: William Murphy Date: Fri, 9 Jun 2023 15:05:19 -0400 Subject: [PATCH] Change platform selection logic (#189) This reverts commit d7551b7f46f53179922d6229709d3d1602881080. Defaulting to platform for all providers resulted in syft having unnecessary when pulling an image that had a particular digest, if that digest didn't match the architecture of the host running the pull. Revert commit d7551b7f46f53179922d6229709d3d1602881080, which introduced that error, but then add platform defaulting logic back for the OCI Registry Provider, since defaulting there has been specifically requested. Add integ tests to cover the new behavior. Also, update integ tests to use the manifest ID for assertsion, since the RepoDigests array can be empty. Signed-off-by: Will Murphy --- client.go | 51 ++++---- client_test.go | 193 ------------------------------ test/integration/platform_test.go | 94 +++++++++++---- 3 files changed, 97 insertions(+), 241 deletions(-) delete mode 100644 client_test.go diff --git a/client.go b/client.go index ce0f4d16..ea8ee736 100644 --- a/client.go +++ b/client.go @@ -102,13 +102,13 @@ func GetImageFromSource(ctx context.Context, imgStr string, source image.Source, func selectImageProvider(imgStr string, source image.Source, cfg config) (image.Provider, error) { var provider image.Provider tempDirGenerator := rootTempDirGenerator.NewGenerator() - - if err := setPlatform(source, &cfg, runtime.GOARCH); err != nil { - return nil, err - } + platformSelectionUnsupported := fmt.Errorf("specified platform=%q however image source=%q does not support selecting platform", cfg.Platform.String(), source.String()) switch source { case image.DockerTarballSource: + if cfg.Platform != nil { + return nil, platformSelectionUnsupported + } // note: the imgStr is the path on disk to the tar file provider = docker.NewProviderFromTarball(imgStr, tempDirGenerator) case image.DockerDaemonSource: @@ -130,12 +130,22 @@ func selectImageProvider(imgStr string, source image.Source, cfg config) (image. return nil, err } case image.OciDirectorySource: + if cfg.Platform != nil { + return nil, platformSelectionUnsupported + } provider = oci.NewProviderFromPath(imgStr, tempDirGenerator) case image.OciTarballSource: + if cfg.Platform != nil { + return nil, platformSelectionUnsupported + } provider = oci.NewProviderFromTarball(imgStr, tempDirGenerator) case image.OciRegistrySource: + defaultPlatformIfNil(&cfg) provider = oci.NewProviderFromRegistry(imgStr, tempDirGenerator, cfg.Registry, cfg.Platform) case image.SingularitySource: + if cfg.Platform != nil { + return nil, platformSelectionUnsupported + } provider = sif.NewProviderFromPath(imgStr, tempDirGenerator) default: return nil, fmt.Errorf("unable to determine image source") @@ -143,30 +153,19 @@ func selectImageProvider(imgStr string, source image.Source, cfg config) (image. return provider, nil } -func setPlatform(source image.Source, cfg *config, defaultArch string) error { - // we should override the platform based on the host architecture if the user did not specify a platform - // see https://github.com/anchore/stereoscope/issues/149 for more details - defaultPlatform, err := image.NewPlatform(defaultArch) - if err != nil { - log.WithFields("error", err).Warnf("unable to set default platform to %q", runtime.GOARCH) - defaultPlatform = nil - } - - switch source { - case image.DockerTarballSource, image.OciDirectorySource, image.OciTarballSource, image.SingularitySource: - if cfg.Platform != nil { - return fmt.Errorf("specified platform=%q however image source=%q does not support selecting platform", cfg.Platform.String(), source.String()) - } - - case image.DockerDaemonSource, image.PodmanDaemonSource, image.OciRegistrySource: - if cfg.Platform == nil { - cfg.Platform = defaultPlatform +// defaultPlatformIfNil sets the platform to use the host's architecture +// if no platform was specified. The OCI registry provider uses "linux/amd64" +// as a hard-coded default platform, which has surprised customers +// running stereoscope on non-amd64 hosts. If platform is already +// set on the config, or the code can't generate a matching platform, +// do nothing. +func defaultPlatformIfNil(cfg *config) { + if cfg.Platform == nil { + p, err := image.NewPlatform(fmt.Sprintf("linux/%s", runtime.GOARCH)) + if err == nil { + cfg.Platform = p } - - default: - return fmt.Errorf("unable to determine image source to select platform") } - return nil } // GetImage parses the user provided image string and provides an image object; diff --git a/client_test.go b/client_test.go deleted file mode 100644 index 82a420e5..00000000 --- a/client_test.go +++ /dev/null @@ -1,193 +0,0 @@ -package stereoscope - -import ( - "testing" - - "github.com/scylladb/go-set/i8set" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/anchore/stereoscope/pkg/image" -) - -func Test_setPlatform(t *testing.T) { - - expectedSources := i8set.New() - for _, s := range image.AllSources { - expectedSources.Add(int8(s)) - } - actualSources := i8set.New() - - tests := []struct { - name string - source image.Source - defaultArch string - initialPlatform *image.Platform - wantPlatform *image.Platform - wantErr require.ErrorAssertionFunc - }{ - // allow defaults --------------------------------------------------------- - { - name: "docker daemon", - source: image.DockerDaemonSource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - }, - { - name: "docker daemon (do not override)", - source: image.DockerDaemonSource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "arm64", // not different than default arch - OS: "linux", - }, - wantPlatform: &image.Platform{ - Architecture: "arm64", // note: did not change - OS: "linux", - }, - }, - { - name: "podman daemon", - source: image.PodmanDaemonSource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - }, - { - name: "podman daemon (do not override)", - source: image.PodmanDaemonSource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "arm64", // not different than default arch - OS: "linux", - }, - wantPlatform: &image.Platform{ - Architecture: "arm64", // note: did not change - OS: "linux", - }, - }, - { - name: "OCI registry", - source: image.OciRegistrySource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - }, - { - name: "OCI registry (do not override)", - source: image.OciRegistrySource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "arm64", // not different than default arch - OS: "linux", - }, - wantPlatform: &image.Platform{ - Architecture: "arm64", // note: did not change - OS: "linux", - }, - }, - // disallow defaults --------------------------------------------------------- - { - name: "docker tarball", - source: image.DockerTarballSource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: nil, - }, - { - name: "docker tarball (override fails)", - source: image.DockerTarballSource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - wantErr: require.Error, - }, - { - name: "OCI dir", - source: image.OciDirectorySource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: nil, - }, - { - name: "OCI dir (override fails)", - source: image.OciDirectorySource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - wantErr: require.Error, - }, - { - name: "OCI tarball", - source: image.OciTarballSource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: nil, - }, - { - name: "OCI tarball (override fails)", - source: image.OciTarballSource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - wantErr: require.Error, - }, - { - name: "singularity", - source: image.SingularitySource, - defaultArch: "amd64", - initialPlatform: nil, - wantPlatform: nil, - }, - { - name: "singularity (override fails)", - source: image.SingularitySource, - defaultArch: "amd64", - initialPlatform: &image.Platform{ - Architecture: "amd64", - OS: "linux", - }, - wantErr: require.Error, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.wantErr == nil { - tt.wantErr = require.NoError - } - - actualSources.Add(int8(tt.source)) - cfg := config{ - Platform: tt.initialPlatform, - } - err := setPlatform(tt.source, &cfg, tt.defaultArch) - tt.wantErr(t, err) - if err != nil { - return - } - - assert.Equal(t, tt.wantPlatform, cfg.Platform) - }) - } - - diff := i8set.Difference(expectedSources, actualSources) - if !diff.IsEmpty() { - t.Errorf("missing test cases for sources: %v", diff.List()) - } -} diff --git a/test/integration/platform_test.go b/test/integration/platform_test.go index 91e2d503..1206e62e 100644 --- a/test/integration/platform_test.go +++ b/test/integration/platform_test.go @@ -2,8 +2,9 @@ package integration import ( "context" + "encoding/json" "fmt" - "strings" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -14,6 +15,11 @@ import ( ) func TestPlatformSelection(t *testing.T) { + /* + All digests were obtained by: + $ docker image pull --platform $PLATFORM busybox:1.31 + $ docker image inspect busybox:1.31 | jq -r '.[0].Id' + */ imageName := "busybox:1.31" tests := []struct { source image.Source @@ -26,37 +32,49 @@ func TestPlatformSelection(t *testing.T) { source: image.OciRegistrySource, architecture: "arm64", os: "linux", - expectedDigest: "sha256:1ee006886991ad4689838d3a288e0dd3fd29b70e276622f16b67a8922831a853", // direct from repo manifest + expectedDigest: "sha256:19d689bc58fd64da6a46d46512ea965a12b6bfb5b030400e21bc0a04c4ff155e", + }, + { + source: image.OciRegistrySource, + architecture: "s390x", + os: "linux", + expectedDigest: "sha256:5bf065699d2e6ddb8b5f7e30f02edc3cfe15d7400e7101b5b505d61fde01f84c", }, { source: image.OciRegistrySource, architecture: "amd64", os: "linux", - expectedDigest: "sha256:95cf004f559831017cdf4628aaf1bb30133677be8702a8c5f2994629f637a209", // direct from repo manifest + expectedDigest: "sha256:1c35c441208254cb7c3844ba95a96485388cef9ccc0646d562c7fc026e04c807", }, { source: image.DockerDaemonSource, architecture: "arm64", os: "linux", - expectedDigest: "sha256:dcd4bbdd7ea2360002c684968429a2105997c3ce5821e84bfc2703c5ec984971", // from generated manifest + expectedDigest: "sha256:19d689bc58fd64da6a46d46512ea965a12b6bfb5b030400e21bc0a04c4ff155e", }, { source: image.DockerDaemonSource, architecture: "amd64", os: "linux", - expectedDigest: "sha256:79d3cb76a5a8ba402af164ace87bd0f3e0759979f94caf7247745126359711da", // from generated manifest + expectedDigest: "sha256:1c35c441208254cb7c3844ba95a96485388cef9ccc0646d562c7fc026e04c807", + }, + { + source: image.DockerDaemonSource, + architecture: "s390x", + os: "linux", + expectedDigest: "sha256:5bf065699d2e6ddb8b5f7e30f02edc3cfe15d7400e7101b5b505d61fde01f84c", }, { source: image.PodmanDaemonSource, architecture: "arm64", os: "linux", - expectedDigest: "sha256:dcd4bbdd7ea2360002c684968429a2105997c3ce5821e84bfc2703c5ec984971", // from generated manifest + expectedDigest: "sha256:19d689bc58fd64da6a46d46512ea965a12b6bfb5b030400e21bc0a04c4ff155e", }, { source: image.PodmanDaemonSource, architecture: "amd64", os: "linux", - expectedDigest: "sha256:79d3cb76a5a8ba402af164ace87bd0f3e0759979f94caf7247745126359711da", // from generated manifest + expectedDigest: "sha256:1c35c441208254cb7c3844ba95a96485388cef9ccc0646d562c7fc026e04c807", }, } @@ -71,21 +89,53 @@ func TestPlatformSelection(t *testing.T) { tt.expectedErr(t, err) require.NotNil(t, img) - assert.Equal(t, tt.os, img.Metadata.OS) - assert.Equal(t, tt.architecture, img.Metadata.Architecture) - found := false - if img.Metadata.ManifestDigest == tt.expectedDigest { - found = true - } - for _, d := range img.Metadata.RepoDigests { - if found { - break - } - if strings.Contains(d, tt.expectedDigest) { - found = true - } - } - assert.True(t, found, "could not find repo digest that matches the expected digest:\nfound manifest digest: %+v\nfound repo digests: %+v\nexpected digest: %+v", img.Metadata.ManifestDigest, img.Metadata.RepoDigests, tt.expectedDigest) + assertArchAndOs(t, img, tt.os, tt.architecture) + assert.Equal(t, tt.expectedDigest, img.Metadata.ID) }) } } + +func TestDigestThatNarrowsToOnePlatform(t *testing.T) { + // This digest is busybox:1.31 on linux/s390x + // Test assumes that the host running these tests _isn't_ linux/s390x, but the behavior + // should be the same regardless. + imageStrWithDigest := "busybox:1.31@sha256:91c15b1ba6f408a648be60f8c047ef79058f26fa640025f374281f31c8704387" + tests := []struct { + name string + source image.Source + }{ + { + name: "docker", + source: image.DockerDaemonSource, + }, + { + name: "registry", + source: image.OciRegistrySource, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + img, err := stereoscope.GetImageFromSource(context.TODO(), imageStrWithDigest, tt.source) + assert.NoError(t, err) + assertArchAndOs(t, img, "linux", "s390x") + }) + } +} + +func TestDefaultPlatformWithOciRegistry(t *testing.T) { + img, err := stereoscope.GetImageFromSource(context.TODO(), "busybox:1.31", image.OciRegistrySource) + require.NoError(t, err) + assertArchAndOs(t, img, "linux", runtime.GOARCH) +} + +func assertArchAndOs(t *testing.T, img *image.Image, os string, architecture string) { + type platform struct { + Architecture string `json:"architecture"` + Os string `json:"os"` + } + var got platform + err := json.Unmarshal(img.Metadata.RawConfig, &got) + require.NoError(t, err) + assert.Equal(t, os, got.Os) + assert.Equal(t, architecture, got.Architecture) +}