pkg/verify: Refactor signature fetching only use a Store interface

This consolidates around abstract signature retrieval, removing the
split between stores and locations which we grew in 9bbf366
(verify: Refactor the verify package to have common code, 2019-12-09, #279).
This will make it easier to make future changes like parallel HTTP(S)
signature retrieval retrieval [1].

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1840343

Author: wking

pkg/cvo/cvo.go

@@ -46,7 +46,9 @@ import (
 	"github.com/openshift/cluster-version-operator/pkg/payload/precondition"
 	preconditioncv "github.com/openshift/cluster-version-operator/pkg/payload/precondition/clusterversion"
 	"github.com/openshift/cluster-version-operator/pkg/verify"
-	"github.com/openshift/cluster-version-operator/pkg/verify/verifyconfigmap"
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
+	"github.com/openshift/cluster-version-operator/pkg/verify/store/configmap"
+	"github.com/openshift/cluster-version-operator/pkg/verify/store/serial"
 )
 
 const (
@@ -312,8 +314,8 @@ func loadConfigMapVerifierDataFromUpdate(update *payload.Update, clientBuilder v
 
 		// allow the verifier to consult the cluster for signature data, and also configure
 		// a process that writes signatures back to that store
-		signatureStore := verifyconfigmap.NewStore(configMapClient, nil)
-		verifier = verifier.WithStores(signatureStore)
+		signatureStore := configmap.NewStore(configMapClient, nil)
+		verifier.Store = &serial.Store{Stores: []store.Store{signatureStore, verifier.Store}}
 		persister := verify.NewSignatureStorePersister(signatureStore, verifier)
 		return verifier, persister, nil
 	}

pkg/verify/configmap.go

@@ -9,6 +9,9 @@ import (
 	"github.com/pkg/errors"
 	"golang.org/x/crypto/openpgp"
 	"k8s.io/klog"
+
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
+	"github.com/openshift/cluster-version-operator/pkg/verify/store/serial"
 )
 
 // ReleaseAnnotationConfigMapVerifier is an annotation set on a config map in the
@@ -48,7 +51,7 @@ const ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-co
 // store and the lookup order is internally defined.
 func NewFromConfigMapData(src string, data map[string]string, clientBuilder ClientBuilder) (*ReleaseVerifier, error) {
 	verifiers := make(map[string]openpgp.EntityList)
-	var stores []*url.URL
+	var stores []store.Store
 	for k, v := range data {
 		switch {
 		case strings.HasPrefix(k, "verifier-public-key-"):
@@ -63,7 +66,16 @@ func NewFromConfigMapData(src string, data map[string]string, clientBuilder Clie
 			if err != nil || (u.Scheme != "http" && u.Scheme != "https" && u.Scheme != "file") {
 				return nil, fmt.Errorf("%s has an invalid key %q: must be a valid URL with scheme file://, http://, or https://", src, k)
 			}
-			stores = append(stores, u)
+			if u.Scheme == "file" {
+				stores = append(stores, &fileStore{
+					directory: u.Path,
+				})
+			} else {
+				stores = append(stores, &httpStore{
+					uri:        u,
+					httpClient: clientBuilder.HTTPClient,
+				})
+			}
 		default:
 			klog.Warningf("An unexpected key was found in %s and will be ignored (expected store-* or verifier-public-key-*): %s", src, k)
 		}
@@ -75,7 +87,7 @@ func NewFromConfigMapData(src string, data map[string]string, clientBuilder Clie
 		return nil, fmt.Errorf("%s did not provide any GPG public keys to verify signatures from and cannot be used", src)
 	}
 
-	return NewReleaseVerifier(verifiers, stores, clientBuilder), nil
+	return NewReleaseVerifier(verifiers, &serial.Store{Stores: stores}), nil
 }
 
 func loadArmoredOrUnarmoredGPGKeyRing(data []byte) (openpgp.EntityList, error) {

pkg/verify/verifyconfigmap/store.go renamed to pkg/verify/store/configmap/configmap.go

@@ -1,4 +1,4 @@
-package verifyconfigmap
+package configmap
 
 import (
 	"context"
@@ -16,6 +16,8 @@ import (
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog"
+
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
 )
 
 // ReleaseLabelConfigMap is a label applied to a configmap inside the
@@ -87,10 +89,10 @@ func digestToKeyPrefix(digest string) (string, error) {
 	return fmt.Sprintf("%s-%s", algo, hash), nil
 }
 
-// DigestSignatures returns a list of signatures that match the request
+// Signatures returns a list of signatures that match the request
 // digest out of config maps labelled with ReleaseLabelConfigMap in the
 // openshift-config-managed namespace.
-func (s *Store) DigestSignatures(ctx context.Context, digest string) ([][]byte, error) {
+func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error {
 	// avoid repeatedly reloading config maps
 	items := s.mostRecentConfigMaps()
 	r := s.limiter.Reserve()
@@ -100,31 +102,36 @@ func (s *Store) DigestSignatures(ctx context.Context, digest string) ([][]byte,
 		})
 		if err != nil {
 			s.rememberMostRecentConfigMaps([]corev1.ConfigMap{})
-			return nil, err
+			return err
 		}
 		items = configMaps.Items
 		s.rememberMostRecentConfigMaps(configMaps.Items)
 	}
 
 	prefix, err := digestToKeyPrefix(digest)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var signatures [][]byte
 	for _, cm := range items {
 		klog.V(4).Infof("searching for %s in signature config map %s", prefix, cm.ObjectMeta.Name)
 		for k, v := range cm.BinaryData {
 			if strings.HasPrefix(k, prefix) {
 				klog.V(4).Infof("key %s from signature config map %s matches %s", k, cm.ObjectMeta.Name, digest)
-				signatures = append(signatures, v)
+				done, err := fn(ctx, v, nil)
+				if err != nil || done {
+					return err
+				}
+				if err := ctx.Err(); err != nil {
+					return err
+				}
 			}
 		}
 	}
-	return signatures, nil
+	return nil
 }
 
-// Store attempts to persist the provided signatures into a form DigestSignatures will
+// Store attempts to persist the provided signatures into a form Signatures will
 // retrieve.
 func (s *Store) Store(ctx context.Context, signaturesByDigest map[string][][]byte) error {
 	cm := &corev1.ConfigMap{

pkg/verify/store/memory/memory.go

@@ -0,0 +1,36 @@
+// Package memory implements an in-memory signature store.  This is
+// mostly useful for testing.
+package memory
+
+import (
+	"context"
+
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
+)
+
+// Store provides access to signatures stored in memory.
+type Store struct {
+	// Data maps digests to slices of signatures.
+	Data map[string][][]byte
+}
+
+// Signatures fetches signatures for the provided digest.
+func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error {
+	for _, signature := range s.Data[digest] {
+		done, err := fn(ctx, signature, nil)
+		if err != nil || done {
+			return err
+		}
+		if err := ctx.Err(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// String returns a description of where this store finds
+// signatures.
+func (s *Store) String() string {
+	return "in-memory signature store"
+}

pkg/verify/store/serial/serial.go

@@ -0,0 +1,56 @@
+// Package serial combines several signature stores in a single store.
+// Signatures are searched in each substore in turn until a match is
+// found.
+package serial
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
+)
+
+// Store provides access to signatures stored in sub-stores.
+type Store struct {
+	Stores []store.Store
+}
+
+// Signatures fetches signatures for the provided digest.
+func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error {
+	allDone := false
+
+	wrapper := func(ctx context.Context, signature []byte, errIn error) (done bool, err error) {
+		done, err = fn(ctx, signature, errIn)
+		if done {
+			allDone = true
+		}
+		return done, err
+	}
+
+	for _, store := range s.Stores {
+		err := store.Signatures(ctx, name, digest, wrapper)
+		if err != nil || allDone {
+			return err
+		}
+		if err := ctx.Err(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// String returns a description of where this store finds
+// signatures.
+func (s *Store) String() string {
+	wrapped := "no stores"
+	if len(s.Stores) > 0 {
+		names := make([]string, 0, len(s.Stores))
+		for _, store := range s.Stores {
+			names = append(names, store.String())
+		}
+		wrapped = strings.Join(names, ", ")
+	}
+	return fmt.Sprintf("serial signature store wrapping %s", wrapped)
+}

pkg/verify/store/serial/serial_test.go

@@ -0,0 +1,101 @@
+package serial
+
+import (
+	"context"
+	"errors"
+	"reflect"
+	"regexp"
+	"testing"
+
+	"github.com/openshift/cluster-version-operator/pkg/verify/store"
+	"github.com/openshift/cluster-version-operator/pkg/verify/store/memory"
+)
+
+func TestStore(t *testing.T) {
+	ctx := context.Background()
+	serial := &Store{
+		Stores: []store.Store{
+			&memory.Store{
+				Data: map[string][][]byte{
+					"sha256:123": {
+						[]byte("store-1-sig-1"),
+						[]byte("store-1-sig-2"),
+					},
+				},
+			},
+			&memory.Store{
+				Data: map[string][][]byte{
+					"sha256:123": {
+						[]byte("store-2-sig-1"),
+						[]byte("store-2-sig-2"),
+					},
+				},
+			},
+		},
+	}
+
+	for _, testCase := range []struct {
+		name               string
+		doneSignature      string
+		doneError          error
+		expectedSignatures []string
+		expectedError      *regexp.Regexp
+	}{
+		{
+			name: "all",
+			expectedSignatures: []string{
+				"store-1-sig-1",
+				"store-1-sig-2",
+				"store-2-sig-1",
+				"store-2-sig-2",
+			},
+		},
+		{
+			name:          "done early",
+			doneSignature: "store-2-sig-1",
+			expectedSignatures: []string{
+				"store-1-sig-1",
+				"store-1-sig-2",
+				"store-2-sig-1",
+			},
+		},
+		{
+			name:          "error early",
+			doneSignature: "store-2-sig-1",
+			doneError:     errors.New("test error"),
+			expectedSignatures: []string{
+				"store-1-sig-1",
+				"store-1-sig-2",
+				"store-2-sig-1",
+			},
+			expectedError: regexp.MustCompile("^test error$"),
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			signatures := []string{}
+			err := serial.Signatures(ctx, "name", "sha256:123", func(ctx context.Context, signature []byte, errIn error) (done bool, err error) {
+				if errIn != nil {
+					return false, errIn
+				}
+				signatures = append(signatures, string(signature))
+				if string(signature) == testCase.doneSignature {
+					return true, testCase.doneError
+				}
+				return false, nil
+			})
+			if err == nil {
+				if testCase.expectedError != nil {
+					t.Fatalf("signatures succeeded when we expected %s", testCase.expectedError)
+				}
+			} else if testCase.expectedError == nil {
+				t.Fatalf("signatures failed when we expected success: %v", err)
+			} else if !testCase.expectedError.MatchString(err.Error()) {
+				t.Fatalf("signatures failed with %v (expected %s)", err, testCase.expectedError)
+			}
+
+			if !reflect.DeepEqual(signatures, testCase.expectedSignatures) {
+				t.Fatalf("signatures gathered %v when we expected %v", signatures, testCase.expectedSignatures)
+			}
+		})
+	}
+}

pkg/verify/store/store.go

@@ -0,0 +1,27 @@
+// Package store defines generic interfaces for signature stores.
+package store
+
+import (
+	"context"
+)
+
+// Callback returns true if an acceptable signature has been found, or
+// an error if the loop should be aborted.  If there was a problem
+// retrieving the signature, the incoming error will describe the
+// problem and the function can decide how to handle that error.
+type Callback func(ctx context.Context, signature []byte, errIn error) (done bool, err error)
+
+// Store provides access to signatures by digest.
+type Store interface {
+
+	// Signatures fetches signatures for the provided digest, feeding
+	// them into the provided callback until an acceptable signature is
+	// found or an error occurs.  Not finding any acceptable signatures
+	// is not an error; it is up to the caller to handle that case.
+	Signatures(ctx context.Context, name string, digest string, fn Callback) error
+
+	// String returns a description of where this store finds
+	// signatures.  The string is a short clause intended for display in
+	// a description of the verifier.
+	String() string
+}