Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the default platform for select sources based on host arch #152

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Set the default platform for select sources based on host arch #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bea3bc0
set the default platform for select sources based on host arch
wagoodman Jan 12, 2023
ccfa506
decompose into smaller function and add tests for setting default pla…
wagoodman Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package stereoscope
import (
"context"
"fmt"
"runtime"

"github.com/anchore/go-logger"

Expand Down Expand Up @@ -101,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()
platformSelectionUnsupported := fmt.Errorf("specified platform=%q however image source=%q does not support selecting platform", cfg.Platform.String(), source.String())

if err := setPlatform(source, &cfg, runtime.GOARCH); err != nil {
return nil, err
}

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:
Expand All @@ -129,28 +130,45 @@ 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:
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")
}
return provider, nil
}

func setPlatform(source image.Source, cfg *config, defaultArch string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: setDefaultPlatform or similar, since it does nothing if platform is set?

Copy link
Contributor Author

@wagoodman wagoodman Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's probably the right name 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, on second thought, this is also verifying that a non-default, non-nil platform is allowed to be set by the user... which goes beyond setting the default. There is probably a better name for this but setPlatform and setDefaultPlatform both don't quite fit the bill... but setPlatform is slightly less incorrect, so I'll leave it for now.

// 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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be an error? could it just safely be ignored here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that the user explicitly asked for an image with the given platform, we don't want to analyze a different image than what was intended. Logging an error would allow for the analysis to continue, and say in syft an SBOM to be produced, but the SBOM config would show a platform that differs than that of what was really analyzed (which seems wrong)

}

case image.DockerDaemonSource, image.PodmanDaemonSource, image.OciRegistrySource:
if cfg.Platform == nil {
cfg.Platform = defaultPlatform
}

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;
// note: the source where the image should be referenced from is automatically inferred.
func GetImage(ctx context.Context, userStr string, options ...Option) (*image.Image, error) {
Expand Down
191 changes: 191 additions & 0 deletions client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
package stereoscope

import (
"github.com/anchore/stereoscope/pkg/image"
"github.com/scylladb/go-set/i8set"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"testing"
)

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())
}
}