Repository: redhat-openshift-ecosystem/openshift-preflight (go)

Issue:
HasUniqueTag check needs to migrate away from Skopeo
## Bug Description
This is the last check that does not use Crane directly. It will either need access to Crane (to call ListTags) or have the tags passed in via the ImageReference.


Pull request:
Introduce an interface to list tags
The implementation is backed by Crane, but is used via an interface that
abstracts Crane, so that the implementation could be something else, should
that be necessary, without having to change the check.

Fixes #243

Signed-off-by: Brad P. Crochet <brad@redhat.com>

Patch:
diff --git a/certification/engine/engine.go b/certification/engine/engine.go
index 022061a..86309d2 100644
--- a/certification/engine/engine.go
+++ b/certification/engine/engine.go
@@ -78,7 +78,7 @@ var validateOperatorBundle certification.Check = operatorpol.NewValidateOperator
 
 // Container checks
 var hasLicenseCheck certification.Check = &containerpol.HasLicenseCheck{}
-var hasUniqueTagCheck certification.Check = containerpol.NewHasUniqueTagCheck(internal.NewSkopeoEngine())
+var hasUniqueTagCheck certification.Check = containerpol.NewHasUniqueTagCheck(internal.NewCraneEngine())
 var maxLayersCheck certification.Check = &containerpol.MaxLayersCheck{}
 var hasNoProhibitedCheck certification.Check = &containerpol.HasNoProhibitedPackagesCheck{}
 var hasRequiredLabelsCheck certification.Check = &containerpol.HasRequiredLabelsCheck{}
diff --git a/certification/internal/cli/skopeo.go b/certification/internal/cli/skopeo.go
deleted file mode 100644
index 6ba7917..0000000
--- a/certification/internal/cli/skopeo.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package cli
-
-type SkopeoListTagsReport struct {
-	Stdout string
-	Stderr string
-	Tags   []string
-}
-
-type SkopeoEngine interface {
-	ListTags(rawImage string) (*SkopeoListTagsReport, error)
-}
diff --git a/certification/internal/engine/crane.go b/certification/internal/engine/crane.go
new file mode 100644
index 0000000..8599630
--- /dev/null
+++ b/certification/internal/engine/crane.go
@@ -0,0 +1,16 @@
+package engine
+
+import "github.com/google/go-containerregistry/pkg/crane"
+
+type craneEngine struct{}
+
+func NewCraneEngine() *craneEngine {
+	return &craneEngine{}
+}
+
+func (c *craneEngine) ListTags(imageURI string) ([]string, error) {
+	// prepare crane runtime options
+	options := make([]crane.Option, 0)
+
+	return crane.ListTags(imageURI, options...)
+}
diff --git a/certification/internal/engine/skopeo.go b/certification/internal/engine/skopeo.go
deleted file mode 100644
index dd8daf7..0000000
--- a/certification/internal/engine/skopeo.go
+++ /dev/null
@@ -1,61 +0,0 @@
-package engine
-
-import (
-	"bytes"
-	"encoding/json"
-	"fmt"
-	"os/exec"
-
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/cli"
-	containerutil "github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/utils/container"
-	log "github.com/sirupsen/logrus"
-)
-
-func NewSkopeoEngine() *cli.SkopeoEngine {
-	var engine cli.SkopeoEngine = SkopeoEngine{}
-	return &engine
-}
-
-type SkopeoEngine struct{}
-
-func (e SkopeoEngine) ListTags(image string) (*cli.SkopeoListTagsReport, error) {
-
-	imageName, err := containerutil.ImageName(image)
-
-	if err != nil {
-		log.Error("unable to parse the image name", err)
-		log.Debug("error parsing the image name: ", err)
-		log.Trace("failure in attempt to parse the image name to strip tag/digest")
-		return nil, err
-	}
-
-	cmdArgs := []string{"list-tags", "docker://" + imageName}
-
-	log.Trace("running skopeo with the following invocation", cmdArgs)
-	cmd := exec.Command("skopeo", cmdArgs...)
-
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-	err = cmd.Run()
-
-	if err != nil {
-		return &cli.SkopeoListTagsReport{Stdout: stdout.String(), Stderr: stderr.String()}, err
-	}
-
-	var report cli.SkopeoListTagsReport
-
-	err = json.Unmarshal(stdout.Bytes(), &report)
-	if err != nil {
-		log.Error("unable to parse skopeo list-tags data for image: ", err)
-		log.Debug("error marshaling skopeo list-tags data: ", err)
-		log.Trace("failure in attempt to convert the raw bytes from `skopeo list-tags` to a [map[string]interface{}")
-		return nil, err
-	}
-	report.Stdout = stdout.String()
-	report.Stderr = stderr.String()
-
-	log.Debugf(fmt.Sprintf("detected these tags for %s: %s", imageName, report.Tags))
-
-	return &report, nil
-}
diff --git a/certification/internal/policy/container/has_unique_tag.go b/certification/internal/policy/container/has_unique_tag.go
index 879d622..114182a 100644
--- a/certification/internal/policy/container/has_unique_tag.go
+++ b/certification/internal/policy/container/has_unique_tag.go
@@ -4,21 +4,18 @@ import (
 	"strings"
 
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/cli"
-	log "github.com/sirupsen/logrus"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/service"
 )
 
+func NewHasUniqueTagCheck(tagLister service.TagLister) *HasUniqueTagCheck {
+	return &HasUniqueTagCheck{TagLister: tagLister}
+}
+
 // HasUniqueTagCheck evaluates the image to ensure that it has a tag other than
 // the latest tag, which is considered to be a "floating" tag and may not accurately
 // represent the same image over time.
 type HasUniqueTagCheck struct {
-	SkopeoEngine cli.SkopeoEngine
-}
-
-func NewHasUniqueTagCheck(skopeoEngine *cli.SkopeoEngine) *HasUniqueTagCheck {
-	return &HasUniqueTagCheck{
-		SkopeoEngine: *skopeoEngine,
-	}
+	TagLister service.TagLister
 }
 
 func (p *HasUniqueTagCheck) Validate(imgRef certification.ImageReference) (bool, error) {
@@ -30,15 +27,7 @@ func (p *HasUniqueTagCheck) Validate(imgRef certification.ImageReference) (bool,
 }
 
 func (p *HasUniqueTagCheck) getDataToValidate(image string) ([]string, error) {
-
-	runReport, err := p.SkopeoEngine.ListTags(image)
-
-	if err != nil {
-		log.Error("unable to execute skopeo on the image: ", err)
-		return nil, err
-	}
-
-	return runReport.Tags, nil
+	return p.TagLister.ListTags(image)
 }
 
 func (p *HasUniqueTagCheck) validate(tags []string) (bool, error) {
diff --git a/certification/internal/service/service.go b/certification/internal/service/service.go
new file mode 100644
index 0000000..9a217be
--- /dev/null
+++ b/certification/internal/service/service.go
@@ -0,0 +1,5 @@
+package service
+
+type TagLister interface {
+	ListTags(imageURI string) ([]string, error)
+}
diff --git a/certification/internal/utils/container/helper.go b/certification/internal/utils/container/helper.go
index c04fd60..33d2038 100644
--- a/certification/internal/utils/container/helper.go
+++ b/certification/internal/utils/container/helper.go
@@ -8,7 +8,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"regexp"
 	"sort"
 
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/errors"
@@ -116,22 +115,6 @@ func Annotation(annotations map[string]string, key string) (string, error) {
 	return value, nil
 }
 
-func ImageName(image string) (string, error) {
-	re, err := regexp.Compile(`(?P<Image>[^@:]+)[@|:]+.*`)
-
-	if err != nil {
-		log.Error("unable to parse the image name: ", err)
-		log.Debug("error parsing the image name: ", err)
-		log.Trace("failure in attempt to parse the image name to strip tag/digest")
-		return "", err
-	}
-	if len(re.FindStringSubmatch(image)) != 0 {
-		return re.FindStringSubmatch(image)[1], nil
-	}
-
-	return "", errors.ErrInvalidImageName
-}
-
 type metadata struct {
 	Annotations map[string]string
 }


Test patch:
diff --git a/certification/internal/policy/container/container_suite_test.go b/certification/internal/policy/container/container_suite_test.go
index 4ef29ec..878b186 100644
--- a/certification/internal/policy/container/container_suite_test.go
+++ b/certification/internal/policy/container/container_suite_test.go
@@ -1,7 +1,6 @@
 package container
 
 import (
-	"errors"
 	"io"
 	"testing"
 
@@ -9,7 +8,6 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/types"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/cli"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -59,23 +57,3 @@ type SkopeoData struct {
 	Repository string
 	Tags       []string
 }
-
-func (fse FakeSkopeoEngine) ListTags(image string) (*cli.SkopeoListTagsReport, error) {
-	skopeoReport := cli.SkopeoListTagsReport{
-		Stdout: fse.SkopeoReportStdout,
-		Stderr: fse.SkopeoReportStderr,
-		Tags:   fse.Tags,
-	}
-	return &skopeoReport, nil
-}
-
-type BadSkopeoEngine struct{}
-
-func (bse BadSkopeoEngine) ListTags(string) (*cli.SkopeoListTagsReport, error) {
-	skopeoReport := cli.SkopeoListTagsReport{
-		Stdout: "Bad Stdout",
-		Stderr: "Bad stderr",
-		Tags:   []string{""},
-	}
-	return &skopeoReport, errors.New("the Skopeo ListTags has failed")
-}
diff --git a/certification/internal/policy/container/has_unique_tag_test.go b/certification/internal/policy/container/has_unique_tag_test.go
index 3b7c132..6a4e192 100644
--- a/certification/internal/policy/container/has_unique_tag_test.go
+++ b/certification/internal/policy/container/has_unique_tag_test.go
@@ -4,8 +4,6 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/cli"
-	log "github.com/sirupsen/logrus"
 )
 
 // podmanEngine is a package-level variable. In some tests, we
@@ -13,23 +11,23 @@ import (
 // In the unhappy path, we override it with an engine that returns
 // nothing but errors.
 
+type fakeTagLister struct {
+	Tags []string
+}
+
+func (ftl *fakeTagLister) ListTags(_ string) ([]string, error) {
+	return ftl.Tags, nil
+}
+
 var _ = Describe("UniqueTag", func() {
 	var (
 		hasUniqueTagCheck HasUniqueTagCheck
-		fakeEngine        cli.SkopeoEngine
 	)
 
-	BeforeEach(func() {
-		fakeEngine = FakeSkopeoEngine{
-			SkopeoReportStdout: "",
-			Tags:               validImageTags(),
-			SkopeoReportStderr: "",
-		}
-	})
 	Describe("Checking for unique tags", func() {
 		Context("When it has tags other than latest", func() {
 			BeforeEach(func() {
-				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeEngine)
+				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeTagLister{Tags: validImageTags()})
 			})
 			It("should pass Validate", func() {
 				ok, err := hasUniqueTagCheck.Validate(certification.ImageReference{ImageURI: "dummy/image"})
@@ -39,34 +37,15 @@ var _ = Describe("UniqueTag", func() {
 		})
 		Context("When it has only latest tag", func() {
 			BeforeEach(func() {
-				engine := fakeEngine.(FakeSkopeoEngine)
-				engine.SkopeoReportStdout = ""
-				engine.Tags = invalidImageTags()
-				engine.SkopeoReportStderr = ""
-				fakeEngine = engine
-				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeEngine)
+				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeTagLister{Tags: invalidImageTags()})
 			})
 			It("should not pass Validate", func() {
-				log.Errorf("Run Report:")
 				ok, err := hasUniqueTagCheck.Validate(certification.ImageReference{ImageURI: "dummy/image"})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ok).To(BeFalse())
 			})
 		})
 	})
-	Describe("Checking that SkopeoEngine errors are handled correctly", func() {
-		BeforeEach(func() {
-			fakeEngine = BadSkopeoEngine{}
-			hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeEngine)
-		})
-		Context("When Skopeo throws an error", func() {
-			It("should fail Validate and return an error", func() {
-				ok, err := hasUniqueTagCheck.Validate(certification.ImageReference{ImageURI: "dummy/image"})
-				Expect(err).To(HaveOccurred())
-				Expect(ok).To(BeFalse())
-			})
-		})
-	})
 })
 
 func validImageTags() []string {