Skip to content

Commit

Permalink
Process image manifest if platform not specified
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@bcrochet
bcrochet committed Jul 27, 2023
1 parent b4ad7f3 commit 79d63f0
Show file tree
Hide file tree
Showing 10 changed files with 209 additions and 100 deletions.
165 changes: 108 additions & 57 deletions cmd/preflight/cmd/check_container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}
Expand All @@ -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")
Expand Down Expand Up @@ -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.
Expand Down
57 changes: 48 additions & 9 deletions cmd/preflight/cmd/check_container_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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())
})
})
Expand Down Expand Up @@ -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())
})
})
Expand All @@ -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()

Expand All @@ -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())
})
Expand All @@ -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()

Expand All @@ -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())
})
Expand Down
11 changes: 10 additions & 1 deletion container/check_container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -151,4 +159,5 @@ type containerCheck struct {
pyxisHost string
platform string
insecure bool
manifestListDigest string
}
Loading

0 comments on commit 79d63f0

Please sign in to comment.