Merge pull request #778 from ecordell/fix-api-labels

fix(olm): use hashes for provided api labels

Documentation/install/local-values.yaml

@@ -1,4 +1,4 @@
-installType: "upstream"
+installType: upstream
 rbacApiVersion: rbac.authorization.k8s.io
 namespace: local
 writeStatusName: '""'

deploy/chart/values.yaml

@@ -1,3 +1,4 @@
+installType: upstream
 rbacApiVersion: rbac.authorization.k8s.io
 namespace: operator-lifecycle-manager
 catalog_namespace: operator-lifecycle-manager
@@ -18,7 +19,7 @@ olm:
 
 catalog:
   replicaCount: 1
-  commandArgs: -configmapServerImage=quay.io/operatorframework/configmap-operator-registry:latest
+  commandArgs: -configmapServerImage=quay.io/operator-framework/configmap-operator-registry:latest
   image:
     ref: quay.io/operator-framework/olm:master
     pullPolicy: Always

pkg/controller/operators/olm/operator.go

@@ -335,7 +335,9 @@ func (a *Operator) syncObject(obj interface{}) (syncError error) {
 	}
 
 	// Requeue CSVs with provided and required labels (for CRDs)
-	if labelSets := a.apiLabeler.LabelSetsFor(metaObj); len(labelSets) > 0 {
+	if labelSets, err := a.apiLabeler.LabelSetsFor(metaObj); err != nil {
+		logger.WithError(err).Warn("couldn't create label set")
+	} else if len(labelSets) > 0 {
 		logger.Debug("requeueing providing/requiring csvs")
 		a.requeueCSVsByLabelSet(logger, labelSets...)
 	}
@@ -757,16 +759,19 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 	}
 
 	// Ensure required and provided API labels
-	updated, err := a.ensureLabels(out, a.apiLabeler.LabelSetsFor(operatorSurface)...)
-	if err != nil {
-		logger.WithError(err).Warn("issue ensuring csv api labels")
-		syncError = err
-		return
+	if labelSets, err := a.apiLabeler.LabelSetsFor(operatorSurface); err != nil {
+		logger.WithError(err).Warn("couldn't create label set")
+	} else if len(labelSets) > 0 {
+		updated, err := a.ensureLabels(out, labelSets...)
+		if err != nil {
+			logger.WithError(err).Warn("issue ensuring csv api labels")
+			syncError = err
+			return
+		}
+		// Update the underlying value of out to preserve changes
+		*out = *updated
 	}
 
-	// Update the underlying value of out to preserve changes
-	*out = *updated
-
 	// Check if the current CSV is being replaced, return with replacing status if so
 	if err := a.checkReplacementsAndUpdateStatus(out); err != nil {
 		logger.WithError(err).Info("replacement check")
@@ -1318,7 +1323,9 @@ func (a *Operator) handleDeletion(obj interface{}) {
 	a.requeueOwnerCSVs(metaObj)
 
 	// Requeue CSVs with provided and required labels (for CRDs)
-	if labelSets := a.apiLabeler.LabelSetsFor(metaObj); len(labelSets) > 0 {
+	if labelSets, err := a.apiLabeler.LabelSetsFor(metaObj); err != nil {
+		logger.WithError(err).Warn("couldn't create label set")
+	} else if len(labelSets) > 0 {
 		logger.Debug("requeueing providing/requiring csvs")
 		a.requeueCSVsByLabelSet(logger, labelSets...)
 	}

pkg/controller/operators/olm/operator_test.go

@@ -15,7 +15,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/metrics"
+	"github.com/operator-framework/operator-registry/pkg/registry"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -39,6 +39,8 @@ import (
 	apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
 	kagg "k8s.io/kube-aggregator/pkg/client/informers/externalversions"
 
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/metrics"
+
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha2"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
@@ -740,6 +742,9 @@ func TestTransitionCSV(t *testing.T) {
 	logrus.SetLevel(logrus.DebugLevel)
 	namespace := "ns"
 
+	apiHash, err := resolver.APIKeyToGVKHash(registry.APIKey{Group: "g1", Version: "v1", Kind: "c1"})
+	require.NoError(t, err)
+
 	defaultOperatorGroup := &v1alpha2.OperatorGroup{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "OperatorGroup",
@@ -2349,7 +2354,7 @@ func TestTransitionCSV(t *testing.T) {
 						[]*v1beta1.CustomResourceDefinition{},
 						v1alpha1.CSVPhaseSucceeded,
 					), defaultTemplateAnnotations), labels.Set{
-						resolver.APILabelKeyPrefix + "c1.v1.g1": "provided",
+						resolver.APILabelKeyPrefix + apiHash: "provided",
 					}),
 					csvWithLabels(csvWithAnnotations(csv("csv1",
 						namespace,
@@ -2360,7 +2365,7 @@ func TestTransitionCSV(t *testing.T) {
 						[]*v1beta1.CustomResourceDefinition{},
 						v1alpha1.CSVPhaseReplacing,
 					), defaultTemplateAnnotations), labels.Set{
-						resolver.APILabelKeyPrefix + "c1.v1.g1": "provided",
+						resolver.APILabelKeyPrefix + apiHash: "provided",
 					}),
 					csvWithLabels(csvWithAnnotations(csv("csv2",
 						namespace,
@@ -2371,7 +2376,7 @@ func TestTransitionCSV(t *testing.T) {
 						[]*v1beta1.CustomResourceDefinition{},
 						v1alpha1.CSVPhaseReplacing,
 					), defaultTemplateAnnotations), labels.Set{
-						resolver.APILabelKeyPrefix + "c1.v1.g1": "provided",
+						resolver.APILabelKeyPrefix + apiHash: "provided",
 					}),
 				},
 				clientObjs: []runtime.Object{defaultOperatorGroup},
@@ -2891,7 +2896,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 		[]*v1beta1.CustomResourceDefinition{crd},
 		[]*v1beta1.CustomResourceDefinition{},
 		v1alpha1.CSVPhaseNone,
-	), labels.Set{resolver.APILabelKeyPrefix + "c1.v1.fake.api.group": "provided"})
+	), labels.Set{resolver.APILabelKeyPrefix + "9f4c46c37bdff8d0": "provided"})
 
 	serverVersion := version.Get().String()
 	// after state transitions from operatorgroups, this is the operator csv we expect

pkg/controller/registry/resolver/labeler.go

@@ -1,8 +1,7 @@
 package resolver
 
 import (
-	"fmt"
-
+	"github.com/operator-framework/operator-registry/pkg/registry"
 	extv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/labels"
 )
@@ -14,39 +13,54 @@ const (
 
 // LabelSetsFor returns API label sets for the given object.
 // Concrete types other than OperatorSurface and CustomResource definition no-op.
-func LabelSetsFor(obj interface{}) []labels.Set {
-	var labelSets []labels.Set
+func LabelSetsFor(obj interface{}) ([]labels.Set, error) {
 	switch v := obj.(type) {
 	case OperatorSurface:
-		labelSets = labelSetsForOperatorSurface(v)
+		return labelSetsForOperatorSurface(v)
 	case *extv1beta1.CustomResourceDefinition:
-		labelSets = labelSetsForCRD(v)
+		return labelSetsForCRD(v)
+	default:
+		return nil, nil
 	}
-
-	return labelSets
 }
 
-func labelSetsForOperatorSurface(surface OperatorSurface) []labels.Set {
+func labelSetsForOperatorSurface(surface OperatorSurface) ([]labels.Set, error) {
 	labelSet := labels.Set{}
 	for key := range surface.ProvidedAPIs().StripPlural() {
-		labelSet[APILabelKeyPrefix+APIKeyToGVKString(key)] = "provided"
+		hash, err := APIKeyToGVKHash(key)
+		if err != nil {
+			return nil, err
+		}
+		labelSet[APILabelKeyPrefix+hash] = "provided"
 	}
 	for key := range surface.RequiredAPIs().StripPlural() {
-		labelSet[APILabelKeyPrefix+APIKeyToGVKString(key)] = "required"
+		hash, err := APIKeyToGVKHash(key)
+		if err != nil {
+			return nil, err
+		}
+		labelSet[APILabelKeyPrefix+hash] = "required"
 	}
 
-	return []labels.Set{labelSet}
+	return []labels.Set{labelSet}, nil
 }
 
-func labelSetsForCRD(crd *extv1beta1.CustomResourceDefinition) []labels.Set {
+func labelSetsForCRD(crd *extv1beta1.CustomResourceDefinition) ([]labels.Set, error) {
 	labelSets := []labels.Set{}
 	if crd == nil {
-		return labelSets
+		return labelSets, nil
 	}
 
 	// Add label sets for each version
 	for _, version := range crd.Spec.Versions {
-		key := fmt.Sprintf("%s%s.%s.%s", APILabelKeyPrefix, crd.Spec.Names.Kind, version.Name, crd.Spec.Group)
+		hash, err := APIKeyToGVKHash(registry.APIKey{
+			Group:   crd.Spec.Group,
+			Version: version.Name,
+			Kind:    crd.Spec.Names.Kind,
+		})
+		if err != nil {
+			return nil, err
+		}
+		key := APILabelKeyPrefix + hash
 		sets := []labels.Set{
 			{
 				key: "provided",
@@ -58,5 +72,5 @@ func labelSetsForCRD(crd *extv1beta1.CustomResourceDefinition) []labels.Set {
 		labelSets = append(labelSets, sets...)
 	}
 
-	return labelSets
+	return labelSets, nil
 }

pkg/controller/registry/resolver/labeler_test.go

@@ -34,10 +34,10 @@ func TestLabelSetsFor(t *testing.T) {
 			}),
 			expected: []labels.Set{
 				{
-					APILabelKeyPrefix + "Ghost.v1alpha1.ghouls": "provided",
+					APILabelKeyPrefix + "6435ab0d7c6bda64": "provided",
 				},
 				{
-					APILabelKeyPrefix + "Ghost.v1alpha1.ghouls": "required",
+					APILabelKeyPrefix + "6435ab0d7c6bda64": "required",
 				},
 			},
 		},
@@ -50,7 +50,7 @@ func TestLabelSetsFor(t *testing.T) {
 			},
 			expected: []labels.Set{
 				{
-					APILabelKeyPrefix + "Ghost.v1alpha1.ghouls": "provided",
+					APILabelKeyPrefix + "6435ab0d7c6bda64": "provided",
 				},
 			},
 		},
@@ -66,15 +66,17 @@ func TestLabelSetsFor(t *testing.T) {
 			},
 			expected: []labels.Set{
 				{
-					APILabelKeyPrefix + "Ghost.v1alpha1.ghouls":  "provided",
-					APILabelKeyPrefix + "Goblin.v1alpha1.ghouls": "required",
+					APILabelKeyPrefix + "6435ab0d7c6bda64": "provided",
+					APILabelKeyPrefix + "557c9f42470aa352": "required",
 				},
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			require.ElementsMatch(t, tt.expected, LabelSetsFor(tt.obj))
+			labelSets, err := LabelSetsFor(tt.obj)
+			require.NoError(t, err)
+			require.ElementsMatch(t, tt.expected, labelSets)
 		})
 	}
 }

pkg/controller/registry/resolver/operators.go

@@ -2,6 +2,7 @@ package resolver
 
 import (
 	"fmt"
+	"hash/fnv"
 	"sort"
 	"strings"
 
@@ -58,6 +59,14 @@ func APIKeyToGVKString(key opregistry.APIKey) string {
 	return strings.Join([]string{key.Kind, key.Version, key.Group}, ".")
 }
 
+func APIKeyToGVKHash(key opregistry.APIKey) (string, error) {
+	hash := fnv.New64a()
+	if _, err := hash.Write([]byte(APIKeyToGVKString(key))); err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%x", hash.Sum64()), nil
+}
+
 func (s APISet) String() string {
 	gvkStrs := make([]string, len(s))
 	i := 0

pkg/lib/labeler/labeler.go

@@ -7,13 +7,13 @@ import (
 // Labeler can provide label sets that describe an object
 type Labeler interface {
 	// LabelSetsFor returns label sets that describe the given object
-	LabelSetsFor(obj interface{}) []labels.Set
+	LabelSetsFor(obj interface{}) ([]labels.Set, error)
 }
 
 // Func is a function type that implements the Labeler interface
-type Func func(obj interface{}) []labels.Set
+type Func func(obj interface{}) ([]labels.Set, error)
 
 // LabelSetsFor calls LabelSetsFor on itself to satisfy the Labeler interface
-func (l Func) LabelSetsFor(obj interface{}) []labels.Set {
+func (l Func) LabelSetsFor(obj interface{}) ([]labels.Set, error) {
 	return l(obj)
 }