From 79d63f024ae7e76a81a22dde59861b533aac70f6 Mon Sep 17 00:00:00 2001 From: "Brad P. Crochet" Date: Mon, 5 Jun 2023 17:01:48 -0400 Subject: [PATCH] Process image manifest if platform not specified If --platform is not given on 'check container', preflight will now process an manifest list if that's what the URL points to. If platform is specified, only process that platform. It will not do anything different yet for submission. It would be the equivalent of running preflight on each of the images in the manifest, with the 'arch' specified. This fixes the reported issue. However, more will be implemented when Pyxis and all other systems are ready to assosciate the images with an actual manifest list. Fixes #955 Signed-off-by: Brad P. Crochet --- cmd/preflight/cmd/check_container.go | 165 ++++++++++++++-------- cmd/preflight/cmd/check_container_test.go | 57 ++++++-- container/check_container.go | 11 +- internal/engine/engine.go | 43 +++--- internal/engine/engine_test.go | 2 +- internal/image/types.go | 13 +- internal/pyxis/types.go | 11 +- internal/runtime/config.go | 1 + internal/runtime/config_test.go | 4 +- operator/check_operator.go | 2 +- 10 files changed, 209 insertions(+), 100 deletions(-) diff --git a/cmd/preflight/cmd/check_container.go b/cmd/preflight/cmd/check_container.go index e13d4d3e0..abb882738 100644 --- a/cmd/preflight/cmd/check_container.go +++ b/cmd/preflight/cmd/check_container.go @@ -18,11 +18,15 @@ import ( "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/cli" "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/formatters" "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/lib" + "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/log" "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/runtime" "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/viper" "github.com/redhat-openshift-ecosystem/openshift-preflight/version" "github.com/go-logr/logr" + "github.com/google/go-containerregistry/pkg/crane" + cranev1 "github.com/google/go-containerregistry/pkg/v1" + cranev1types "github.com/google/go-containerregistry/pkg/v1/types" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -80,8 +84,8 @@ func checkContainerCmd(runpreflight runPreflight) *cobra.Command { "URL paramater. This value may differ from the PID on the overview page. (env: PFLT_CERTIFICATION_PROJECT_ID)")) _ = viper.BindPFlag("certification_project_id", flags.Lookup("certification-project-id")) - checkContainerCmd.Flags().String("platform", rt.GOARCH, "Architecture of image to pull. Defaults to current platform.") - _ = viper.BindPFlag("platform", checkContainerCmd.Flags().Lookup("platform")) + flags.String("platform", rt.GOARCH, "Architecture of image to pull. Defaults to runtime platform.") + _ = viper.BindPFlag("platform", flags.Lookup("platform")) return checkContainerCmd } @@ -103,85 +107,131 @@ func checkContainerRunE(cmd *cobra.Command, args []string, runpreflight runPrefl return fmt.Errorf("invalid configuration: %w", err) } - artifactsWriter, err := artifacts.NewFilesystemWriter(artifacts.WithDirectory(cfg.Artifacts)) + manifestBytes, err := crane.Manifest(containerImage) if err != nil { - return err + return fmt.Errorf("could not retrieve manifest: %w", err) } - - // Add the artifact writer to the context for use by checks. - ctx = artifacts.ContextWithWriter(ctx, artifactsWriter) - - formatter, err := formatters.NewByName(formatters.DefaultFormat) + manifest, err := cranev1.ParseManifest(bytes.NewBuffer(manifestBytes)) if err != nil { - return err + return fmt.Errorf("could not parse manifest: %w", err) } - opts := generateContainerCheckOptions(cfg) - - checkcontainer := container.NewCheck( - containerImage, - opts..., - ) + platformChanged := cmd.Flags().Lookup("platform").Changed - pc := lib.NewPyxisClient(ctx, cfg.CertificationProjectID, cfg.PyxisAPIToken, cfg.PyxisHost) - resultSubmitter := lib.ResolveSubmitter(pc, cfg.CertificationProjectID, cfg.DockerConfig, cfg.LogFile) + containerImagePlatforms := []string{cfg.Platform} - // Run the container check. - cmd.SilenceUsage = true - - err = runpreflight( - ctx, - checkcontainer.Run, - cli.CheckConfig{ - IncludeJUnitResults: cfg.WriteJUnit, - SubmitResults: cfg.Submit, - }, - formatter, - &runtime.ResultWriterFile{}, - resultSubmitter, - ) + // If platform param is not changed, it means that a platform was not specified on the + // command line. Therefore, we should process all platforms in the manifest list. + // As long as what is poinged to is a manifest list. Otherwise, it will just be the + // currnt runtime platform. + if isImageList(manifest.MediaType) && !platformChanged { + logger.V(log.DBG).Info("manifest list detected, checking all platforms in manifest") + manifest, err := cranev1.ParseIndexManifest(bytes.NewBuffer(manifestBytes)) + if err != nil { + return fmt.Errorf("could not parse index manifest: %w", err) + } - if err != nil { - return err + // Look up the manifestListDigest and set it in the config + manifestListDigest, err := crane.Digest(containerImage) + if err != nil { + return fmt.Errorf("could not get manifest list digest") + } + cfg.ManifestListDigest = manifestListDigest + + // Preflight was given a manifest list. --platform was not specified. + // Therefore, all platforms in the manifest list should be processed. + // Create a new slice since the original was for a single platform. + containerImagePlatforms = make([]string, 0, len(manifest.Manifests)) + for _, img := range manifest.Manifests { + containerImagePlatforms = append(containerImagePlatforms, img.Platform.Architecture) + } } - // checking for offline flag, if present tar up the contents of the artifacts directory - if cfg.Offline { - src := artifactsWriter.Path() - var buf bytes.Buffer - - // check to see if a tar file already exist to account for someone re-running - exists, err := artifactsWriter.Exists(check.DefaultArtifactsTarFileName) + for _, platform := range containerImagePlatforms { + logger.Info(fmt.Sprintf("running checks for %s for platform %s", containerImage, platform)) + artifactsWriter, err := artifacts.NewFilesystemWriter(artifacts.WithDirectory(filepath.Join(cfg.Artifacts, platform))) if err != nil { - return fmt.Errorf("unable to check if tar already exists: %v", err) + return err } - // remove the tar file if it exists - if exists { - err = artifactsWriter.Remove(check.DefaultArtifactsTarFileName) - if err != nil { - return fmt.Errorf("unable to remove existing tar: %v", err) - } - } + // Add the artifact writer to the context for use by checks. + ctx := artifacts.ContextWithWriter(ctx, artifactsWriter) - // tar the directory - err = artifactsTar(ctx, src, &buf) + formatter, err := formatters.NewByName(formatters.DefaultFormat) if err != nil { - return fmt.Errorf("unable to tar up artifacts directory: %v", err) + return err } - // writing the tar file to disk - _, err = artifactsWriter.WriteFile(check.DefaultArtifactsTarFileName, &buf) - if err != nil { - return fmt.Errorf("could not artifacts tar to artifacts dir: %w", err) + opts := generateContainerCheckOptions(cfg) + opts = append(opts, container.WithPlatform(platform)) + + checkcontainer := container.NewCheck( + containerImage, + opts..., + ) + + pc := lib.NewPyxisClient(ctx, cfg.CertificationProjectID, cfg.PyxisAPIToken, cfg.PyxisHost) + resultSubmitter := lib.ResolveSubmitter(pc, cfg.CertificationProjectID, cfg.DockerConfig, cfg.LogFile) + + // Run the container check. + cmd.SilenceUsage = true + + if err := runpreflight( + ctx, + checkcontainer.Run, + cli.CheckConfig{ + IncludeJUnitResults: cfg.WriteJUnit, + SubmitResults: cfg.Submit, + }, + formatter, + &runtime.ResultWriterFile{}, + resultSubmitter, + ); err != nil { + return err } - logger.Info("artifact tar written to disk", "filename", check.DefaultArtifactsTarFileName) + // checking for offline flag, if present tar up the contents of the artifacts directory + if cfg.Offline { + src := artifactsWriter.Path() + var buf bytes.Buffer + + // check to see if a tar file already exist to account for someone re-running + exists, err := artifactsWriter.Exists(check.DefaultArtifactsTarFileName) + if err != nil { + return fmt.Errorf("unable to check if tar already exists: %v", err) + } + + // remove the tar file if it exists + if exists { + err = artifactsWriter.Remove(check.DefaultArtifactsTarFileName) + if err != nil { + return fmt.Errorf("unable to remove existing tar: %v", err) + } + } + + // tar the directory + err = artifactsTar(ctx, src, &buf) + if err != nil { + return fmt.Errorf("unable to tar up artifacts directory: %v", err) + } + + // writing the tar file to disk + _, err = artifactsWriter.WriteFile(check.DefaultArtifactsTarFileName, &buf) + if err != nil { + return fmt.Errorf("could not artifacts tar to artifacts dir: %w", err) + } + + logger.Info("artifact tar written to disk", "filename", check.DefaultArtifactsTarFileName) + } } return nil } +func isImageList(mediaType cranev1types.MediaType) bool { + return mediaType == cranev1types.DockerManifestList || mediaType == cranev1types.OCIImageIndex +} + func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error { if len(args) != 1 { return fmt.Errorf("a container image positional argument is required") @@ -251,6 +301,7 @@ func generateContainerCheckOptions(cfg *runtime.Config) []container.Option { // Always add PyxisHost, since the value is always set in viper config parsing. container.WithPyxisHost(cfg.PyxisHost), container.WithPlatform(cfg.Platform), + container.WithManifestListDigest(cfg.ManifestListDigest), } // set auth information if both are present in config. diff --git a/cmd/preflight/cmd/check_container_test.go b/cmd/preflight/cmd/check_container_test.go index 0c60d460b..6eff7a8a5 100644 --- a/cmd/preflight/cmd/check_container_test.go +++ b/cmd/preflight/cmd/check_container_test.go @@ -4,8 +4,14 @@ import ( "bytes" "context" "errors" + "fmt" + "io" + "log" + "net/http/httptest" + "net/url" "os" "path/filepath" + "runtime" "github.com/redhat-openshift-ecosystem/openshift-preflight/artifacts" "github.com/redhat-openshift-ecosystem/openshift-preflight/certification" @@ -16,17 +22,44 @@ import ( "github.com/redhat-openshift-ecosystem/openshift-preflight/internal/viper" "github.com/go-logr/logr" + "github.com/google/go-containerregistry/pkg/crane" + "github.com/google/go-containerregistry/pkg/registry" + "github.com/google/go-containerregistry/pkg/v1/random" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) var _ = Describe("Check Container Command", func() { + var src string + var s *httptest.Server + var u *url.URL + BeforeEach(func() { + // Set up a fake registry. + registryLogger := log.New(io.Discard, "", log.Ldate) + s = httptest.NewServer(registry.New(registry.Logger(registryLogger))) + DeferCleanup(func() { + s.Close() + }) + + var err error + u, err = url.Parse(s.URL) + Expect(err).ToNot(HaveOccurred()) + + src = fmt.Sprintf("%s/test/crane", u.Host) + + // Expected values. + img, err := random.Image(1024, 5) + Expect(err).ToNot(HaveOccurred()) + + err = crane.Push(img, src) + Expect(err).ToNot(HaveOccurred()) + }) BeforeEach(createAndCleanupDirForArtifactsAndLogs) Context("when running the check container subcommand", func() { Context("With all of the required parameters", func() { It("should reach the core logic, but throw an error because of the placeholder values for the container image", func() { - _, err := executeCommand(checkContainerCmd(mockRunPreflightReturnNil), "example.com/example/image:mytag") + _, err := executeCommand(checkContainerCmd(mockRunPreflightReturnNil), src) Expect(err).To(HaveOccurred()) }) }) @@ -153,13 +186,13 @@ certification_project_id: mycertid` Context("when running the check container subcommand with a logger provided", func() { Context("with all of the required parameters", func() { It("should reach the core logic, and execute the mocked RunPreflight", func() { - _, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), "example.com/example/image:mytag") + _, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), src) Expect(err).ToNot(HaveOccurred()) }) }) Context("with all of the required parameters with error mocked", func() { It("should reach the core logic, and execute the mocked RunPreflight and return error", func() { - _, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnErr), logr.Discard(), "example.com/example/image:mytag") + _, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnErr), logr.Discard(), src) Expect(err).To(HaveOccurred()) }) }) @@ -172,12 +205,15 @@ certification_project_id: mycertid` Expect(err).ToNot(HaveOccurred()) DeferCleanup(os.RemoveAll, tmpDir) + platformDir := filepath.Join(tmpDir, runtime.GOARCH) + Expect(os.Mkdir(platformDir, 0o755)).Should(Succeed()) + // creating test files on in the tmpDir so the tar function has files to tar - f1, err := os.Create(filepath.Join(tmpDir, "test-file-1.json")) + f1, err := os.Create(filepath.Join(platformDir, "test-file-1.json")) Expect(err).ToNot(HaveOccurred()) defer f1.Close() - f2, err := os.Create(filepath.Join(tmpDir, "test-file-1.json")) + f2, err := os.Create(filepath.Join(platformDir, "test-file-1.json")) Expect(err).ToNot(HaveOccurred()) defer f2.Close() @@ -188,7 +224,7 @@ certification_project_id: mycertid` DeferCleanup(viper.Instance().Set, "offline", false) }) It("should reach core logic, and the additional offline logic", func() { - out, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), "example.com/example/image:mytag") + out, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), src) Expect(err).ToNot(HaveOccurred()) Expect(out).ToNot(BeNil()) }) @@ -199,12 +235,15 @@ certification_project_id: mycertid` Expect(err).ToNot(HaveOccurred()) DeferCleanup(os.RemoveAll, tmpDir) + platformDir := filepath.Join(tmpDir, runtime.GOARCH) + Expect(os.Mkdir(platformDir, 0o755)).Should(Succeed()) + // creating test files on in the tmpDir so the tar function has files to tar - f1, err := os.Create(filepath.Join(tmpDir, "test-file-1.json")) + f1, err := os.Create(filepath.Join(platformDir, "test-file-1.json")) Expect(err).ToNot(HaveOccurred()) defer f1.Close() - f2, err := os.Create(filepath.Join(tmpDir, "test-file-1.json")) + f2, err := os.Create(filepath.Join(platformDir, "test-file-1.json")) Expect(err).ToNot(HaveOccurred()) defer f2.Close() @@ -220,7 +259,7 @@ certification_project_id: mycertid` DeferCleanup(viper.Instance().Set, "offline", false) }) It("should reach the additional offline logic, and remove existing tar file", func() { - out, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), "example.com/example/image:mytag") + out, err := executeCommandWithLogger(checkContainerCmd(mockRunPreflightReturnNil), logr.Discard(), src) Expect(err).ToNot(HaveOccurred()) Expect(out).ToNot(BeNil()) }) diff --git a/container/check_container.go b/container/check_container.go index 5dd2507de..59e1571d0 100644 --- a/container/check_container.go +++ b/container/check_container.go @@ -70,7 +70,7 @@ func (c *containerCheck) Run(ctx context.Context) (certification.Results, error) return certification.Results{}, fmt.Errorf("%w: %s", preflighterr.ErrCannotInitializeChecks, err) } - eng, err := engine.New(ctx, c.image, checks, nil, c.dockerconfigjson, false, pol == policy.PolicyScratch, c.insecure, c.platform) + eng, err := engine.New(ctx, c.image, checks, nil, c.dockerconfigjson, false, pol == policy.PolicyScratch, c.insecure, c.platform, c.manifestListDigest) if err != nil { return certification.Results{}, err } @@ -143,6 +143,14 @@ func WithInsecureConnection() Option { } } +// WithManifestListDigest signifies that we have a manifest list and should add +// this digest to any Pyxis calls +func WithManifestListDigest(manifestListDigest string) Option { + return func(cc *containerCheck) { + cc.manifestListDigest = manifestListDigest + } +} + type containerCheck struct { image string dockerconfigjson string @@ -151,4 +159,5 @@ type containerCheck struct { pyxisHost string platform string insecure bool + manifestListDigest string } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 7c7b09ad5..30b542790 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -72,6 +72,9 @@ type CraneEngine struct { // the registry crane connects with. Insecure bool + // ManifestListDigest is the sha256 digest for the manifest list + ManifestListDigest string + imageRef image.ImageReference results certification.Results } @@ -185,12 +188,13 @@ func (c *CraneEngine) ExecuteChecks(ctx context.Context) error { // store the image internals in the engine image reference to pass to validations. c.imageRef = image.ImageReference{ - ImageURI: c.Image, - ImageFSPath: containerFSPath, - ImageInfo: img, - ImageRegistry: reference.Context().RegistryStr(), - ImageRepository: reference.Context().RepositoryStr(), - ImageTagOrSha: reference.Identifier(), + ImageURI: c.Image, + ImageFSPath: containerFSPath, + ImageInfo: img, + ImageRegistry: reference.Context().RegistryStr(), + ImageRepository: reference.Context().RepositoryStr(), + ImageTagOrSha: reference.Identifier(), + ManifestListDigest: c.ManifestListDigest, } if err := writeCertImage(ctx, c.imageRef); err != nil { @@ -504,10 +508,11 @@ func writeCertImage(ctx context.Context, imageRef image.ImageReference) error { repositories := make([]pyxis.Repository, 0, 1) repositories = append(repositories, pyxis.Repository{ - PushDate: addedDate, - Registry: imageRef.ImageRegistry, - Repository: imageRef.ImageRepository, - Tags: tags, + PushDate: addedDate, + Registry: imageRef.ImageRegistry, + Repository: imageRef.ImageRepository, + Tags: tags, + ManifestListDigest: imageRef.ManifestListDigest, }) certImage := pyxis.CertImage{ @@ -682,16 +687,18 @@ func New(ctx context.Context, isScratch bool, insecure bool, platform string, + manifestListDigest string, ) (CheckEngine, error) { return &CraneEngine{ - Kubeconfig: kubeconfig, - DockerConfig: dockerconfig, - Image: image, - Checks: checks, - IsBundle: isBundle, - IsScratch: isScratch, - Platform: platform, - Insecure: insecure, + Kubeconfig: kubeconfig, + DockerConfig: dockerconfig, + Image: image, + Checks: checks, + IsBundle: isBundle, + IsScratch: isScratch, + Platform: platform, + Insecure: insecure, + ManifestListDigest: manifestListDigest, }, nil } diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 892e3a133..86cf7e8f2 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -266,7 +266,7 @@ var _ = Describe("Tag and digest binding information function", func() { var _ = Describe("CheckInitialization", func() { When("initializing the engine", func() { It("should not return an error", func() { - _, err := New(context.TODO(), "example.com/some/image:latest", []check.Check{}, nil, "", false, false, false, goruntime.GOARCH) + _, err := New(context.TODO(), "example.com/some/image:latest", []check.Check{}, nil, "", false, false, false, goruntime.GOARCH, "") Expect(err).ToNot(HaveOccurred()) }) }) diff --git a/internal/image/types.go b/internal/image/types.go index 8f7476a37..39a3b49cd 100644 --- a/internal/image/types.go +++ b/internal/image/types.go @@ -4,10 +4,11 @@ import v1 "github.com/google/go-containerregistry/pkg/v1" // ImageReference holds all things image-related type ImageReference struct { - ImageURI string - ImageFSPath string - ImageInfo v1.Image - ImageRepository string - ImageRegistry string - ImageTagOrSha string + ImageURI string + ImageFSPath string + ImageInfo v1.Image + ImageRepository string + ImageRegistry string + ImageTagOrSha string + ManifestListDigest string } diff --git a/internal/pyxis/types.go b/internal/pyxis/types.go index 9d8351bea..d27450595 100644 --- a/internal/pyxis/types.go +++ b/internal/pyxis/types.go @@ -60,11 +60,12 @@ type ParsedData struct { } type Repository struct { - Published bool `json:"published" default:"false"` - PushDate string `json:"push_date,omitempty"` // time.Now - Registry string `json:"registry,omitempty"` - Repository string `json:"repository,omitempty"` - Tags []Tag `json:"tags,omitempty"` + Published bool `json:"published" default:"false"` + PushDate string `json:"push_date,omitempty"` // time.Now + Registry string `json:"registry,omitempty"` + Repository string `json:"repository,omitempty"` + Tags []Tag `json:"tags,omitempty"` + ManifestListDigest string `json:"manifest_list_digest,omitempty"` } type Label struct { diff --git a/internal/runtime/config.go b/internal/runtime/config.go index 5d08e0837..d4cc8ba00 100644 --- a/internal/runtime/config.go +++ b/internal/runtime/config.go @@ -27,6 +27,7 @@ type Config struct { Platform string Insecure bool Offline bool + ManifestListDigest string // Operator-Specific Fields Namespace string ServiceAccount string diff --git a/internal/runtime/config_test.go b/internal/runtime/config_test.go index fd432a8e0..4593cc335 100644 --- a/internal/runtime/config_test.go +++ b/internal/runtime/config_test.go @@ -64,12 +64,12 @@ var _ = Describe("Viper to Runtime Config", func() { }) }) - It("should only have 23 struct keys for tests to be valid", func() { + It("should only have 24 struct keys for tests to be valid", func() { // If this test fails, it means a developer has added or removed // keys from runtime.Config, and so these tests may no longer be // accurate in confirming that the derived configuration from viper // matches. keys := reflect.TypeOf(Config{}).NumField() - Expect(keys).To(Equal(23)) + Expect(keys).To(Equal(24)) }) }) diff --git a/operator/check_operator.go b/operator/check_operator.go index 5083dc9a0..bbfe51057 100644 --- a/operator/check_operator.go +++ b/operator/check_operator.go @@ -59,7 +59,7 @@ func (c operatorCheck) Run(ctx context.Context) (certification.Results, error) { return certification.Results{}, fmt.Errorf("%w: %s", preflighterr.ErrCannotInitializeChecks, err) } - eng, err := engine.New(ctx, c.image, checks, c.kubeconfig, c.dockerConfigFilePath, true, true, c.insecure, goruntime.GOARCH) + eng, err := engine.New(ctx, c.image, checks, c.kubeconfig, c.dockerConfigFilePath, true, true, c.insecure, goruntime.GOARCH, "") if err != nil { return certification.Results{}, err }