cmd/operator-sdk/scorecard: change source of truth for bundle metadata

changelog/fragments/scorecard-metadata-source-of-truth.yaml

@@ -0,0 +1,5 @@
+entries:
+  - description: >
+      Use on-disk bundle metadata in scorecard (either unpacked from an image or directly if a directory is passed)
+      as the source of truth for bundle metadata, and not image labels which are informative only.
+    kind: change

cmd/operator-sdk/scorecard/cmd.go

@@ -73,7 +73,7 @@ If the argument holds an image tag, it must be present remotely.`,
 	scorecardCmd.Flags().StringVarP(&c.config, "config", "c", "", "path to scorecard config file")
 	scorecardCmd.Flags().StringVarP(&c.namespace, "namespace", "n", "default", "namespace to run the test images in")
 	scorecardCmd.Flags().StringVarP(&c.outputFormat, "output", "o", "text",
-		"Output format for results.  Valid values: text, json")
+		"Output format for results. Valid values: text, json")
 	scorecardCmd.Flags().StringVarP(&c.serviceAccount, "service-account", "s", "default",
 		"Service account to use for tests")
 	scorecardCmd.Flags().BoolVarP(&c.list, "list", "L", false,
@@ -110,26 +110,20 @@ func (c *scorecardCmd) printOutput(output v1alpha3.TestList) error {
 
 func (c *scorecardCmd) run() (err error) {
 	// Extract bundle image contents if bundle is inferred to be an image.
-	var bundleLabels registryutil.Labels
 	if _, err = os.Stat(c.bundle); err != nil && errors.Is(err, os.ErrNotExist) {
-		if c.bundle, bundleLabels, err = getBundlePathAndLabelsFromImage(c.bundle); err != nil {
+		if c.bundle, err = extractBundleImage(c.bundle); err != nil {
 			log.Fatal(err)
 		}
 		defer func() {
 			if err := os.RemoveAll(c.bundle); err != nil {
 				log.Error(err)
 			}
 		}()
-	} else {
-		// Search for the metadata dir since we cannot assume its path, and
-		// use that metadata as the source of truth when testing.
-		metadataDir, err := registryutil.FindMetadataDir(c.bundle)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if bundleLabels, err = registryutil.GetMetadataLabels(metadataDir); err != nil {
-			log.Fatal(err)
-		}
+	}
+
+	metadata, _, err := registryutil.FindBundleMetadata(c.bundle)
+	if err != nil {
+		log.Fatal(err)
 	}
 
 	o := scorecard.Scorecard{
@@ -158,7 +152,7 @@ func (c *scorecardCmd) run() (err error) {
 			ServiceAccount: c.serviceAccount,
 			Namespace:      c.namespace,
 			BundlePath:     c.bundle,
-			BundleLabels:   bundleLabels,
+			BundleMetadata: metadata,
 		}
 
 		// Only get the client if running tests.
@@ -212,26 +206,13 @@ func discardLogger() *log.Logger {
 	return logger
 }
 
-// getBundlePathAndLabelsFromImage returns bundleImage's path on disk post-
-// extraction and image labels.
-func getBundlePathAndLabelsFromImage(bundleImage string) (string, registryutil.Labels, error) {
+// extractBundleImage returns bundleImage's path on disk post-extraction.
+func extractBundleImage(bundleImage string) (string, error) {
 	// Discard bundle extraction logs unless user sets verbose mode.
 	logger := log.NewEntry(discardLogger())
 	if viper.GetBool(flags.VerboseOpt) {
 		logger = log.WithFields(log.Fields{"bundle": bundleImage})
 	}
 	// FEAT: enable explicit local image extraction.
-	bundlePath, err := registryutil.ExtractBundleImage(context.TODO(), logger, bundleImage, false)
-	if err != nil {
-		return "", nil, err
-	}
-
-	// Get image labels from bundleImage locally, since the bundle extraction
-	// already pulled the image.
-	bundleLabels, err := registryutil.GetImageLabels(context.TODO(), logger, bundleImage, true)
-	if err != nil {
-		return "", nil, err
-	}
-
-	return bundlePath, bundleLabels, nil
+	return registryutil.ExtractBundleImage(context.TODO(), logger, bundleImage, false)
 }

images/scorecard-test/cmd/test/main.go

@@ -44,12 +44,12 @@ func main() {
 	}
 
 	// Read the pod's untar'd bundle from a well-known path.
-	cfg, err := apimanifests.GetBundleFromDir(scorecard.PodBundleRoot)
+	bundle, err := apimanifests.GetBundleFromDir(scorecard.PodBundleRoot)
 	if err != nil {
 		log.Fatal(err.Error())
 	}
 
-	labels, err := registryutil.GetMetadataLabels(scorecard.PodLabelsDir)
+	metadata, _, err := registryutil.FindBundleMetadata(scorecard.PodBundleRoot)
 	if err != nil {
 		log.Fatal(err.Error())
 	}
@@ -58,17 +58,17 @@ func main() {
 
 	switch entrypoint[0] {
 	case tests.OLMBundleValidationTest:
-		result = tests.BundleValidationTest(scorecard.PodBundleRoot, labels)
+		result = tests.BundleValidationTest(scorecard.PodBundleRoot, metadata)
 	case tests.OLMCRDsHaveValidationTest:
-		result = tests.CRDsHaveValidationTest(cfg)
+		result = tests.CRDsHaveValidationTest(bundle)
 	case tests.OLMCRDsHaveResourcesTest:
-		result = tests.CRDsHaveResourcesTest(cfg)
+		result = tests.CRDsHaveResourcesTest(bundle)
 	case tests.OLMSpecDescriptorsTest:
-		result = tests.SpecDescriptorsTest(cfg)
+		result = tests.SpecDescriptorsTest(bundle)
 	case tests.OLMStatusDescriptorsTest:
-		result = tests.StatusDescriptorsTest(cfg)
+		result = tests.StatusDescriptorsTest(bundle)
 	case tests.BasicCheckSpecTest:
-		result = tests.CheckSpecTest(cfg)
+		result = tests.CheckSpecTest(bundle)
 	default:
 		result = printValidTests()
 	}

internal/registry/image.go

@@ -21,10 +21,9 @@ import (
 	"os"
 	"path/filepath"
 
-	log "github.com/sirupsen/logrus"
-
 	registryimage "github.com/operator-framework/operator-registry/pkg/image"
 	"github.com/operator-framework/operator-registry/pkg/image/containerdregistry"
+	log "github.com/sirupsen/logrus"
 )
 
 // ExtractBundleImage returns a bundle directory containing files extracted
@@ -73,3 +72,32 @@ func ExtractBundleImage(ctx context.Context, logger *log.Entry, image string, lo
 
 	return bundleDir, nil
 }
+
+// GetImageLabels returns the set of labels on image.
+func GetImageLabels(ctx context.Context, logger *log.Entry, image string, local bool) (map[string]string, error) {
+	// Create a containerd registry for socket-less image layer reading.
+	reg, err := containerdregistry.NewRegistry(containerdregistry.WithLog(logger))
+	if err != nil {
+		return nil, fmt.Errorf("error creating new image registry: %v", err)
+	}
+	defer func() {
+		if err := reg.Destroy(); err != nil {
+			logger.WithError(err).Warn("Error destroying local cache")
+		}
+	}()
+
+	// Pull the image if it isn't present locally.
+	if !local {
+		if err := reg.Pull(ctx, registryimage.SimpleReference(image)); err != nil {
+			return nil, fmt.Errorf("error pulling image %s: %v", image, err)
+		}
+	}
+
+	// Query the image reference for its labels.
+	labels, err := reg.Labels(ctx, registryimage.SimpleReference(image))
+	if err != nil {
+		return nil, fmt.Errorf("error reading image %s labels: %v", image, err)
+	}
+
+	return labels, err
+}

internal/registry/labels.go

@@ -15,17 +15,15 @@
 package registry
 
 import (
-	"context"
 	"errors"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 
-	registryimage "github.com/operator-framework/operator-registry/pkg/image"
-	"github.com/operator-framework/operator-registry/pkg/image/containerdregistry"
 	registrybundle "github.com/operator-framework/operator-registry/pkg/lib/bundle"
 	log "github.com/sirupsen/logrus"
+	"github.com/spf13/afero"
 
 	// TODO: replace `gopkg.in/yaml.v3` with `sigs.k8s.io/yaml` once operator-registry has `json` tags in the
 	// annotations struct.
@@ -49,80 +47,67 @@ func (ls Labels) getLabel(key string) (string, bool) {
 	return value, hasLabel
 }
 
-// GetImageLabels returns the set of labels on image.
-func GetImageLabels(ctx context.Context, logger *log.Entry, image string, local bool) (Labels, error) {
-	// Create a containerd registry for socket-less image layer reading.
-	reg, err := containerdregistry.NewRegistry(containerdregistry.WithLog(logger))
-	if err != nil {
-		return nil, fmt.Errorf("error creating new image registry: %v", err)
-	}
-	defer func() {
-		if err := reg.Destroy(); err != nil {
-			logger.WithError(err).Warn("Error destroying local cache")
-		}
-	}()
+// FindBundleMetadata walks bundleRoot searching for metadata (ex. annotations.yaml),
+// and returns metadata and its path if found. If one is not found, an error is returned.
+func FindBundleMetadata(bundleRoot string) (Labels, string, error) {
+	return findBundleMetadata(afero.NewOsFs(), bundleRoot)
+}
 
-	// Pull the image if it isn't present locally.
-	if !local {
-		if err := reg.Pull(ctx, registryimage.SimpleReference(image)); err != nil {
-			return nil, fmt.Errorf("error pulling image %s: %v", image, err)
-		}
+func findBundleMetadata(fs afero.Fs, bundleRoot string) (Labels, string, error) {
+	// Check the default path first, and return annotations if they were found or an error if that error
+	// is not because the path does not exist (it exists or there was an unmarshalling error).
+	annotationsPath := filepath.Join(bundleRoot, registrybundle.MetadataDir, registrybundle.AnnotationsFile)
+	annotations, err := readAnnotations(fs, annotationsPath)
+	if (err == nil && len(annotations) != 0) || (err != nil && !errors.Is(err, os.ErrNotExist)) {
+		return annotations, annotationsPath, err
 	}
 
-	// Query the image reference for its labels.
-	labels, err := reg.Labels(ctx, registryimage.SimpleReference(image))
-	if err != nil {
-		return nil, fmt.Errorf("error reading image %s labels: %v", image, err)
-	}
-
-	return labels, err
-}
-
-// FindMetadataDir walks bundleRoot searching for metadata, and returns that directory if found.
-// If one is not found, an error is returned.
-func FindMetadataDir(bundleRoot string) (metadataDir string, err error) {
-	err = filepath.Walk(bundleRoot, func(path string, info os.FileInfo, err error) error {
-		if err != nil || !info.IsDir() {
+	// Annotations are not at the default path, so search recursively.
+	annotations = make(Labels)
+	annotationsPath = ""
+	err = afero.Walk(fs, bundleRoot, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
 			return err
 		}
-		// Already found the first metadata dir, do not overwrite it.
-		if metadataDir != "" {
+		// Skip directories and hidden files, or if annotations were already found.
+		if len(annotations) != 0 || info.IsDir() || strings.HasPrefix(path, ".") {
 			return nil
 		}
-		// The annotations file is well-defined.
-		_, err = os.Stat(filepath.Join(path, registrybundle.AnnotationsFile))
-		if err == nil || errors.Is(err, os.ErrExist) {
-			metadataDir = path
+
+		annotationsPath = path
+		// Ignore this error, since we only care if any annotations are returned.
+		if annotations, err = readAnnotations(fs, path); err != nil {
+			log.Debug(err)
 		}
 		return nil
 	})
 	if err != nil {
-		return "", err
+		return nil, "", err
 	}
-	if metadataDir == "" {
-		return "", fmt.Errorf("metadata dir not found in %s", bundleRoot)
+
+	if len(annotations) == 0 {
+		return nil, "", fmt.Errorf("metadata not found in %s", bundleRoot)
 	}
 
-	return metadataDir, nil
+	return annotations, annotationsPath, nil
 }
 
-// GetMetadataLabels reads annotations from file(s) in metadataDir and returns them as Labels.
-func GetMetadataLabels(metadataDir string) (Labels, error) {
+// readAnnotations reads annotations from file(s) in bundleRoot and returns them as Labels.
+func readAnnotations(fs afero.Fs, annotationsPath string) (Labels, error) {
 	// The annotations file is well-defined.
-	annotationsPath := filepath.Join(metadataDir, registrybundle.AnnotationsFile)
-	b, err := ioutil.ReadFile(annotationsPath)
+	b, err := afero.ReadFile(fs, annotationsPath)
 	if err != nil {
 		return nil, err
 	}
 
 	// Use the arbitrarily-labelled bundle representation of the annotations file
 	// for forwards and backwards compatibility.
-	meta := registrybundle.AnnotationMetadata{
+	annotations := registrybundle.AnnotationMetadata{
 		Annotations: make(map[string]string),
 	}
-	if err = yaml.Unmarshal(b, &meta); err != nil {
-		return nil, err
+	if err = yaml.Unmarshal(b, &annotations); err != nil {
+		return nil, fmt.Errorf("error unmarshalling potential bundle metadata %s: %v", annotationsPath, err)
 	}
 
-	return meta.Annotations, nil
+	return annotations.Annotations, nil
 }