Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #2 (2021-07-06 13:30:19)

Expand the PodmanEngine and refactor to use it
This expands the methods in the PodmanEngine (adds InspectImage) and
implements Save. It demonstrates the usage of these in the util functions
and the HasRequiredLabels check. This removes a lot of code from the
check and simplifies it dramatically. It also adds a unit test for the
labels check.

Signed-off-by: Brad P. Crochet <brad@redhat.com>

openshift-ci[bot]: @bcrochet: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
komish: Runs on my machine!

```
time="2021-07-08T13:15:12-05:00" level=info msg="check completed: BasedOnUbi" result=PASSED
{
    "image": "registry.access.redhat.com/ubi8/ubi:latest",
    "validation_lib_version": {
        "version": "0.0.0",
        "commit": "c891eab1641ca1fdad35285f323e75bfa8bc01ff"
    },
    "results": {
        "Passed": [
            {
                "description": "Checking if the container's base image is based on UBI",
                "level": "best",
                "knowledge_base_url": "https://connect.redhat.com/zones/containers/container-certification-policy-guide",
                "check_url": "https://connect.redhat.com/zones/containers/container-certification-policy-guide"
            }
        ],
        "Failed": [],
        "Errors": []
    }
}
```
openshift-ci[bot]: @bcrochet: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/HACK.md b/HACK.md
new file mode 100644
index 0000000..d979a44
--- /dev/null
+++ b/HACK.md
@@ -0,0 +1,5 @@
+Build the binary with the repo commit hash
+
+```bash
+go build -ldflags "-X github.com/redhat-openshift-ecosystem/openshift-preflight/version.commit=$(git rev-parse HEAD)" main.go
+```
diff --git a/certification/errors/errors.go b/certification/errors/errors.go
index a0e1fba..f5ae6b0 100644
--- a/certification/errors/errors.go
+++ b/certification/errors/errors.go
@@ -16,3 +16,4 @@ var ErrExtractingTarball = errors.New("failed to extract tarball")
 var ErrCreateTempDir = errors.New("failed to create temporary directory")
 var ErrRunContainerFailed = errors.New("failed to run command in container")
 var ErrInitializingLogger = errors.New("failed to initialize logger for config")
+var ErrImageInspectFailed = errors.New("failed to inspect image")
diff --git a/certification/internal/shell/engine.go b/certification/internal/shell/engine.go
index adea973..2d8c73d 100644
--- a/certification/internal/shell/engine.go
+++ b/certification/internal/shell/engine.go
@@ -35,7 +35,7 @@ func (e *CheckEngine) ExecuteChecks() {
 
 			if isRemote {
 				log.Info("downloading image")
-				err := GetContainerFromRegistry(e.Image)
+				_, err := GetContainerFromRegistry(e.Image)
 				if err != nil {
 					log.Error("unable to the container from the registry: ", err)
 					e.results.Errors = append(e.results.Errors, runtime.Result{Check: check, ElapsedTime: time.Since(checkStartTime)})
diff --git a/certification/internal/shell/has_required_labels.go b/certification/internal/shell/has_required_labels.go
index 614c572..4a32c54 100644
--- a/certification/internal/shell/has_required_labels.go
+++ b/certification/internal/shell/has_required_labels.go
@@ -1,11 +1,8 @@
 package shell
 
 import (
-	"encoding/json"
-	"os/exec"
-
-	"github.com/itchyny/gojq"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -13,56 +10,35 @@ type HasRequiredLabelsCheck struct{}
 
 func (p *HasRequiredLabelsCheck) Validate(image string) (bool, error) {
 	// TODO: if we're going have the image json on disk already, we should use it here instead of podman inspect-ing.
-	stdouterr, err := exec.Command("podman", "inspect", image).CombinedOutput()
-	if err != nil {
-		log.Error("unable to execute inspect on the image: ", err)
-		return false, err
-	}
-
-	// we must send gojq a []interface{}, so we have to convert our inspect output to that type
-	var inspectData []interface{}
-	err = json.Unmarshal(stdouterr, &inspectData)
+	labels, err := p.getDataForValidate(image)
 	if err != nil {
-		log.Error("unable to parse podman inspect data for image")
-		log.Debug("error marshaling podman inspect data: ", err)
-		log.Trace("failure in attempt to convert the raw bytes from `podman inspect` to a []interface{}")
 		return false, err
 	}
 
-	jqQueryString := ".[0] | .Labels | {name: .name, vendor: .vendor, version: .version, release: .release, summary: .summary, description: .description}"
+	return p.validate(labels)
+}
 
-	query, err := gojq.Parse(jqQueryString)
+func (p *HasRequiredLabelsCheck) getDataForValidate(image string) (map[string]string, error) {
+	inspectReport, err := podmanEngine.InspectImage(image, cli.ImageInspectOptions{})
 	if err != nil {
-		log.Error("unable to parse podman inspect data for image")
-		log.Debug("unable to successfully parse the gojq query string:", err)
-		return false, err
-	}
-
-	// gojq expects us to iterate in the event that our query returned multiple matching values, but we only expect one.
-	iter := query.Run(inspectData)
-	val, nextOk := iter.Next()
-
-	if !nextOk {
-		log.Warn("did not receive any label information when parsing container image")
-		// in this case, there was no data returned from jq, so we need to fail the check.
-		return false, nil
+		log.Error("unable to execute inspect on the image: ", err)
+		return nil, err
 	}
+	return inspectReport.Images[0].Config.Labels, nil
+}
 
-	// gojq can return an error in iteration, so we need to check for that.
-	if err, ok := val.(error); ok {
-		log.Error("unable to parse podman inspect data for image")
-		log.Debug("unable to successfully parse the podman inspect output with the query string provided:", err)
-		// this is an error, as we didn't get the proper input from `podman inspect`
-		return false, err
+func (p *HasRequiredLabelsCheck) validate(labels map[string]string) (bool, error) {
+	requiredLabels := []string{"name", "vendor", "version", "release", "summary", "description"}
+	missingLabels := []string{}
+	for _, label := range requiredLabels {
+		if labels[label] == "" {
+			missingLabels = append(missingLabels, label)
+		}
 	}
 
-	labels := val.(map[string]interface{})
-
-	for _, label := range labels {
-		if label == nil {
-			log.Warn("an expected label is missing:", label)
-			return false, nil
-		}
+	if len(missingLabels) > 0 {
+		log.Warn("expected labels are missing:", missingLabels)
+		return false, nil
 	}
 
 	return true, nil
diff --git a/certification/internal/shell/podman.go b/certification/internal/shell/podman.go
index 4cb7fe7..ee69025 100644
--- a/certification/internal/shell/podman.go
+++ b/certification/internal/shell/podman.go
@@ -2,8 +2,11 @@ package shell
 
 import (
 	"bytes"
+	"encoding/json"
+	"fmt"
 	"os/exec"
 
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/errors"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
 )
 
@@ -33,5 +36,35 @@ func (pe PodmanCLIEngine) Run(opts cli.ImageRunOptions) (*cli.ImageRunReport, er
 }
 
 func (pe PodmanCLIEngine) Save(nameOrID string, tags []string, opts cli.ImageSaveOptions) error {
+	cmdArgs := []string{"save", "--output", opts.Destination}
+	cmdArgs = append(cmdArgs, nameOrID)
+	_, err := exec.Command("podman", cmdArgs...).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("%w: %s", errors.ErrSaveContainerFailed, err)
+	}
 	return nil
 }
+
+func (pe PodmanCLIEngine) InspectImage(rawImage string, opts cli.ImageInspectOptions) (*cli.ImageInspectReport, error) {
+	cmdArgs := []string{"image", "inspect"}
+	if opts.LogLevel != "" {
+		cmdArgs = append(cmdArgs, "--log-level", opts.LogLevel)
+	}
+	cmdArgs = append(cmdArgs, rawImage)
+
+	cmd := exec.Command("podman", cmdArgs...)
+	var stdout, stderr bytes.Buffer
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	err := cmd.Run()
+	if err != nil {
+		return nil, fmt.Errorf("%w: %s", errors.ErrImageInspectFailed, err)
+	}
+
+	var inspectData []cli.PodmanImage
+	err = json.Unmarshal(stdout.Bytes(), &inspectData)
+	if err != nil {
+		return nil, fmt.Errorf("%w: %s", errors.ErrImageInspectFailed, err)
+	}
+	return &cli.ImageInspectReport{Images: inspectData}, nil
+}
diff --git a/certification/internal/shell/util.go b/certification/internal/shell/util.go
index 980e0ac..d99e881 100644
--- a/certification/internal/shell/util.go
+++ b/certification/internal/shell/util.go
@@ -1,7 +1,6 @@
 package shell
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"os/exec"
@@ -9,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/errors"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
 )
 
 func ExtractContainerTar(tarball string) (string, error) {
@@ -34,27 +34,23 @@ func ExtractContainerTar(tarball string) (string, error) {
 	return outputDir, nil
 }
 
-func GetContainerFromRegistry(imageLoc string) error {
-	_, err := exec.Command("podman", "pull", imageLoc).CombinedOutput()
+func GetContainerFromRegistry(imageLoc string) (string, error) {
+	pullReport, err := podmanEngine.Pull(imageLoc, cli.ImagePullOptions{})
 	if err != nil {
-		return fmt.Errorf("%w: %s", errors.ErrGetRemoteContainerFailed, err)
+		return "", fmt.Errorf("%w: %s", errors.ErrGetRemoteContainerFailed, err)
 	}
 
-	return nil
+	return pullReport.StdoutErr, nil
 }
 
-// Calls podman to save the image to a temporary directory in /tmp/preflight-???, where
-// ??? will be a a generated string. Should be follwed with:
+// Calls podman to save the image to a temporary directory in /tmp/preflight-{randomString}.
+// Should be followed with:
 // defer os.RemoveAll(tarballDir)
 // Returns the locataion of the tarball.
-func SaveContainerToFilesystem(imageLoc string) (string, error) {
-	cmd := exec.Command("podman", "image", "inspect", "--format", "{{.Id}}", imageLoc)
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-	err := cmd.Run()
+func SaveContainerToFilesystem(podmanEngine cli.PodmanEngine, imageLog string) (string, error) {
+	inspectReport, err := podmanEngine.InspectImage(imageLog, cli.ImageInspectOptions{})
 	if err != nil {
-		return "", fmt.Errorf("%w: %s", errors.ErrExtractingTarball, err)
+		return "", fmt.Errorf("%w: %s", errors.ErrImageInspectFailed, err)
 	}
 
 	tempdir, err := os.MkdirTemp(os.TempDir(), "preflight-*")
@@ -62,9 +58,9 @@ func SaveContainerToFilesystem(imageLoc string) (string, error) {
 		return "", fmt.Errorf("%w: %s", errors.ErrCreateTempDir, err)
 	}
 
-	imgSig := stdout.String()
+	imgSig := inspectReport.Images[0].Id
 	tarpath := filepath.Join(tempdir, imgSig+".tar")
-	_, err = exec.Command("podman", "save", imageLoc, "--output", tarpath).CombinedOutput()
+	err = podmanEngine.Save(imgSig, []string{}, cli.ImageSaveOptions{Destination: tarpath})
 	if err != nil {
 		return "", fmt.Errorf("%w: %s", errors.ErrSaveContainerFailed, err)
 	}
diff --git a/cli/podman.go b/cli/podman.go
index 01676b8..921d704 100644
--- a/cli/podman.go
+++ b/cli/podman.go
@@ -1,5 +1,13 @@
 package cli
 
+type ImageInspectOptions struct {
+	LogLevel string
+}
+
+type ImageInspectReport struct {
+	Images []PodmanImage
+}
+
 type ImagePullOptions struct {
 	LogLevel string
 }
@@ -21,10 +29,22 @@ type ImageRunReport struct {
 }
 
 type ImageSaveOptions struct {
-	LogLevel string
+	LogLevel    string
+	Image       string
+	Destination string
+}
+
+type PodmanImage struct {
+	Id     string
+	Config PodmanImageConfig
+}
+
+type PodmanImageConfig struct {
+	Labels map[string]string
 }
 
 type PodmanEngine interface {
+	InspectImage(rawImage string, opts ImageInspectOptions) (*ImageInspectReport, error)
 	Pull(rawImage string, opts ImagePullOptions) (*ImagePullReport, error)
 	Run(opts ImageRunOptions) (*ImageRunReport, error)
 	Save(nameOrID string, tags []string, opts ImageSaveOptions) error


Test patch:
diff --git a/certification/internal/shell/base_on_ubi_test.go b/certification/internal/shell/base_on_ubi_test.go
index 6841fdd..12acad9 100644
--- a/certification/internal/shell/base_on_ubi_test.go
+++ b/certification/internal/shell/base_on_ubi_test.go
@@ -15,7 +15,6 @@ import (
 var _ = Describe("BaseOnUBI", func() {
 	var (
 		baseOnUbiCheck BaseOnUBICheck
-		originalEngine cli.PodmanEngine
 		fakeEngine     cli.PodmanEngine
 	)
 
@@ -26,10 +25,6 @@ NAME="Red Hat Enterprise Linux"
 `,
 			RunReportStderr: "",
 		}
-		originalEngine = podmanEngine
-	})
-	AfterEach(func() {
-		podmanEngine = originalEngine
 	})
 	Describe("Checking for UBI as a base", func() {
 		Context("When it is based on UBI", func() {
diff --git a/certification/internal/shell/fake_podman_engine_test.go b/certification/internal/shell/fake_podman_engine_test.go
deleted file mode 100644
index bdda741..0000000
--- a/certification/internal/shell/fake_podman_engine_test.go
+++ /dev/null
@@ -1,55 +0,0 @@
-package shell
-
-import (
-	"errors"
-
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
-)
-
-// This struct is meant to implement cli.PodmanEngine
-// It is used for unit testing, allowing the package-level
-// variable of podmanEngine to be overridden in test files
-
-type FakePodmanEngine struct {
-	RunReportStdout     string
-	RunReportStderr     string
-	PullReportStdouterr string
-}
-
-func (fpe FakePodmanEngine) Run(opts cli.ImageRunOptions) (*cli.ImageRunReport, error) {
-	runReport := cli.ImageRunReport{
-		Stdout: fpe.RunReportStdout,
-		Stderr: fpe.RunReportStderr,
-	}
-	return &runReport, nil
-}
-
-func (fpe FakePodmanEngine) Pull(rawImage string, opts cli.ImagePullOptions) (*cli.ImagePullReport, error) {
-	pullReport := cli.ImagePullReport{
-		StdoutErr: fpe.PullReportStdouterr,
-	}
-
-	return &pullReport, nil
-}
-
-func (fpe FakePodmanEngine) Save(nameOrID string, tags []string, opts cli.ImageSaveOptions) error {
-	return nil
-}
-
-type BadPodmanEngine struct{}
-
-func (bpe BadPodmanEngine) Run(cli.ImageRunOptions) (*cli.ImageRunReport, error) {
-	runReport := cli.ImageRunReport{
-		Stdout: "Bad stadout",
-		Stderr: "Bad stderr",
-	}
-	return &runReport, errors.New("the Podman Run has failed")
-}
-
-func (bpe BadPodmanEngine) Pull(rawImage string, opts cli.ImagePullOptions) (*cli.ImagePullReport, error) {
-	return nil, errors.New("the Podman Pull has failed")
-}
-
-func (bpe BadPodmanEngine) Save(nameOrID string, tags []string, opts cli.ImageSaveOptions) error {
-	return errors.New("the Podman Save has failed")
-}
diff --git a/certification/internal/shell/has_required_labels_test.go b/certification/internal/shell/has_required_labels_test.go
new file mode 100644
index 0000000..c302239
--- /dev/null
+++ b/certification/internal/shell/has_required_labels_test.go
@@ -0,0 +1,72 @@
+package shell
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
+)
+
+var _ = Describe("HasRequiredLabels", func() {
+	var (
+		hasRequiredLabelsCheck HasRequiredLabelsCheck
+		fakeEngine             cli.PodmanEngine
+	)
+
+	BeforeEach(func() {
+		labels := map[string]string{
+			"name":        "name",
+			"vendor":      "vendor",
+			"version":     "version",
+			"release":     "release",
+			"summary":     "summary",
+			"description": "description",
+		}
+		fakeEngine = FakePodmanEngine{
+			ImageInspectReport: cli.ImageInspectReport{
+				Images: []cli.PodmanImage{
+					{
+						Id: "imageid",
+						Config: cli.PodmanImageConfig{
+							Labels: labels,
+						},
+					},
+				},
+			},
+		}
+		podmanEngine = fakeEngine
+	})
+
+	Describe("Checking for required labels", func() {
+		Context("When it has required labels", func() {
+			It("should pass Validate", func() {
+				ok, err := hasRequiredLabelsCheck.Validate("dummy/image")
+				Expect(err).ToNot(HaveOccurred())
+				Expect(ok).To(BeTrue())
+			})
+		})
+		Context("When it does not have required labels", func() {
+			BeforeEach(func() {
+				engine := fakeEngine.(FakePodmanEngine)
+				delete(engine.ImageInspectReport.Images[0].Config.Labels, "description")
+			})
+			It("should not succeed the check", func() {
+				ok, err := hasRequiredLabelsCheck.Validate("dummy/image")
+				Expect(err).ToNot(HaveOccurred())
+				Expect(ok).To(BeFalse())
+			})
+		})
+	})
+	Describe("Checking that PodMan errors are handled correctly", func() {
+		BeforeEach(func() {
+			fakeEngine = BadPodmanEngine{}
+			podmanEngine = fakeEngine
+		})
+		Context("When PodMan throws an error", func() {
+			It("should fail Validate and return an error", func() {
+				ok, err := hasRequiredLabelsCheck.Validate("dummy/image")
+				Expect(err).To(HaveOccurred())
+				Expect(ok).To(BeFalse())
+			})
+		})
+	})
+})
diff --git a/certification/internal/shell/shell_suite_test.go b/certification/internal/shell/shell_suite_test.go
index a6bfcbe..9db56cf 100644
--- a/certification/internal/shell/shell_suite_test.go
+++ b/certification/internal/shell/shell_suite_test.go
@@ -1,10 +1,12 @@
 package shell
 
 import (
+	"errors"
 	"testing"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/cli"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -17,3 +19,75 @@ func init() {
 	log.SetFormatter(&log.TextFormatter{})
 	log.SetLevel(log.TraceLevel)
 }
+
+var (
+	originalEngine cli.PodmanEngine
+)
+
+var _ = BeforeSuite(func() {
+	originalEngine = podmanEngine
+})
+
+var _ = AfterSuite(func() {
+	podmanEngine = originalEngine
+})
+
+// This struct is meant to implement cli.PodmanEngine
+// It is used for unit testing, allowing the package-level
+// variable of podmanEngine to be overridden in test files
+
+type FakePodmanEngine struct {
+	RunReportStdout     string
+	RunReportStderr     string
+	PullReportStdouterr string
+	ImageInspectReport  cli.ImageInspectReport
+}
+
+func (fpe FakePodmanEngine) Run(opts cli.ImageRunOptions) (*cli.ImageRunReport, error) {
+	runReport := cli.ImageRunReport{
+		Stdout: fpe.RunReportStdout,
+		Stderr: fpe.RunReportStderr,
+	}
+	return &runReport, nil
+}
+
+func (fpe FakePodmanEngine) Pull(rawImage string, opts cli.ImagePullOptions) (*cli.ImagePullReport, error) {
+	pullReport := cli.ImagePullReport{
+		StdoutErr: fpe.PullReportStdouterr,
+	}
+
+	return &pullReport, nil
+}
+
+func (fpe FakePodmanEngine) Save(nameOrID string, tags []string, opts cli.ImageSaveOptions) error {
+	return nil
+}
+
+func (fpe FakePodmanEngine) InspectImage(rawImage string, opts cli.ImageInspectOptions) (*cli.ImageInspectReport, error) {
+	return &fpe.ImageInspectReport, nil
+}
+
+type BadPodmanEngine struct{}
+
+func (bpe BadPodmanEngine) Run(cli.ImageRunOptions) (*cli.ImageRunReport, error) {
+	runReport := cli.ImageRunReport{
+		Stdout: "Bad stadout",
+		Stderr: "Bad stderr",
+	}
+	return &runReport, errors.New("the Podman Run has failed")
+}
+
+func (bpe BadPodmanEngine) Pull(rawImage string, opts cli.ImagePullOptions) (*cli.ImagePullReport, error) {
+	pullReport := cli.ImagePullReport{
+		StdoutErr: "Bad stdouterr",
+	}
+	return &pullReport, errors.New("the Podman Pull has failed")
+}
+
+func (bpe BadPodmanEngine) Save(nameOrID string, tags []string, opts cli.ImageSaveOptions) error {
+	return errors.New("the Podman Save has failed")
+}
+
+func (bpe BadPodmanEngine) InspectImage(rawImage string, opts cli.ImageInspectOptions) (*cli.ImageInspectReport, error) {
+	return nil, errors.New("the Podman Inspect Image has failed")
+}