Make provided API ClusterRoles be owned by the corresponding API.

The existing CSV owner labels lead to a neverending battle for
ownership in clusters with more than one CSV providing the same
API. The amount of additional work generated by the owner label
conflict manifested (not exclusively) as excessive operator
installation latency in affected clusters.

This conflict is resolved by abandoning CSV owner labels in favor of
OwnerReferences to the providing APIService or
CustomResourceDefinition.

Aggregation of API ClusterRoles to the providing OperatorGroup
ClusterRoles is also changed. Instead of labeling API ClusterRoles
based on the names of OperatorGroups providing the associated API,
each API ClusterRole is labeled based on the name of the API
itself. OperatorGroup ClusterRoles accumulate one label selector per
provided API.

Author: benluddy

pkg/api/apis/operators/operatorgroup_types.go

@@ -74,6 +74,8 @@ type OperatorGroupList struct {
 }
 
 func (o *OperatorGroup) BuildTargetNamespaces() string {
-	sort.Strings(o.Status.Namespaces)
-	return strings.Join(o.Status.Namespaces, ",")
+	ns := make([]string, len(o.Status.Namespaces))
+	copy(ns, o.Status.Namespaces)
+	sort.Strings(ns)
+	return strings.Join(ns, ",")
 }

pkg/api/apis/operators/v1/operatorgroup_types.go

@@ -74,8 +74,10 @@ type OperatorGroupList struct {
 }
 
 func (o *OperatorGroup) BuildTargetNamespaces() string {
-	sort.Strings(o.Status.Namespaces)
-	return strings.Join(o.Status.Namespaces, ",")
+	ns := make([]string, len(o.Status.Namespaces))
+	copy(ns, o.Status.Namespaces)
+	sort.Strings(ns)
+	return strings.Join(ns, ",")
 }
 
 // IsServiceAccountSpecified returns true if the spec has a service account name specified.

pkg/controller/operators/olm/operator.go

@@ -1093,7 +1093,7 @@ func (a *Operator) operatorGroupFromAnnotations(logger *logrus.Entry, csv *v1alp
 	}
 
 	// Target namespaces don't match
-	if targets != strings.Join(operatorGroup.Status.Namespaces, ",") {
+	if targets != operatorGroup.BuildTargetNamespaces() {
 		logger.Info("olm.targetNamespaces annotation doesn't match operatorgroup status")
 		return nil
 	}
@@ -1105,21 +1105,21 @@ func (a *Operator) operatorGroupForCSV(csv *v1alpha1.ClusterServiceVersion, logg
 	now := a.now()
 
 	// Attempt to associate an OperatorGroup with the CSV.
-	operatorGroups, err := a.client.OperatorsV1().OperatorGroups(csv.GetNamespace()).List(metav1.ListOptions{})
+	operatorGroups, err := a.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(csv.GetNamespace()).List(labels.Everything())
 	if err != nil {
 		logger.Errorf("error occurred while attempting to associate csv with operatorgroup")
 		return nil, err
 	}
 	var operatorGroup *v1.OperatorGroup
 
-	switch len(operatorGroups.Items) {
+	switch len(operatorGroups) {
 	case 0:
 		err = fmt.Errorf("csv in namespace with no operatorgroups")
 		logger.Warn(err)
 		csv.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonNoOperatorGroup, err.Error(), now, a.recorder)
 		return nil, err
 	case 1:
-		operatorGroup = &operatorGroups.Items[0]
+		operatorGroup = operatorGroups[0]
 		logger = logger.WithField("opgroup", operatorGroup.GetName())
 		if a.operatorGroupAnnotationsDiffer(&csv.ObjectMeta, operatorGroup) {
 			a.setOperatorGroupAnnotations(&csv.ObjectMeta, operatorGroup, true)
@@ -1223,13 +1223,16 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 	}
 
 	// Check for intersecting provided APIs in intersecting OperatorGroups
-	options := metav1.ListOptions{
-		FieldSelector: fmt.Sprintf("metadata.name!=%s,metadata.namespace!=%s", operatorGroup.GetName(), operatorGroup.GetNamespace()),
+	allGroups, err := a.lister.OperatorsV1().OperatorGroupLister().List(labels.Everything())
+	otherGroups := make([]v1.OperatorGroup, 0, len(allGroups))
+	for _, g := range allGroups {
+		if g.GetName() != operatorGroup.GetName() || g.GetNamespace() != operatorGroup.GetNamespace() {
+			otherGroups = append(otherGroups, *g)
+		}
 	}
-	otherGroups, err := a.client.OperatorsV1().OperatorGroups(metav1.NamespaceAll).List(options)
 
 	groupSurface := resolver.NewOperatorGroup(operatorGroup)
-	otherGroupSurfaces := resolver.NewOperatorGroupSurfaces(otherGroups.Items...)
+	otherGroupSurfaces := resolver.NewOperatorGroupSurfaces(otherGroups...)
 	providedAPIs := operatorSurface.ProvidedAPIs().StripPlural()
 
 	switch result := a.apiReconciler.Reconcile(providedAPIs, groupSurface, otherGroupSurfaces...); {

pkg/controller/operators/olm/operatorgroup.go

@@ -19,15 +19,14 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
+	"github.com/operator-framework/operator-registry/pkg/registry"
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
 
 const (
-	operatorGroupAggregrationKeyPrefix = "olm.opgroup.permissions/aggregate-to-"
-	kubeRBACAggregationKeyPrefix       = "rbac.authorization.k8s.io/aggregate-to-"
-	AdminSuffix                        = "admin"
-	EditSuffix                         = "edit"
-	ViewSuffix                         = "view"
+	AdminSuffix = "admin"
+	EditSuffix  = "edit"
+	ViewSuffix  = "view"
 )
 
 var (
@@ -42,6 +41,14 @@ var (
 	}
 )
 
+func aggregationLabelFromAPIKey(k opregistry.APIKey, suffix string) (string, error) {
+	hash, err := resolver.APIKeyToGVKHash(k)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("olm.opgroup.permissions/aggregate-to-%s-%s", hash, suffix), nil
+}
+
 func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	op, ok := obj.(*v1.OperatorGroup)
 	if !ok {
@@ -115,12 +122,6 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	}
 	logger.Debug("OperatorGroup CSV annotation completed")
 
-	if err := a.ensureOpGroupClusterRoles(op); err != nil {
-		logger.WithError(err).Warn("failed to ensure operatorgroup clusterroles")
-		return err
-	}
-	logger.Debug("operatorgroup clusterroles ensured")
-
 	// Requeue all CSVs that provide the same APIs (including those removed). This notifies conflicting CSVs in
 	// intersecting groups that their conflict has possibly been resolved, either through resizing or through
 	// deletion of the conflicting CSV.
@@ -135,6 +136,12 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		providedAPIsForGroup[api] = struct{}{}
 	}
 
+	if err := a.ensureOpGroupClusterRoles(op, providedAPIsForGroup); err != nil {
+		logger.WithError(err).Warn("failed to ensure operatorgroup clusterroles")
+		return err
+	}
+	logger.Debug("operatorgroup clusterroles ensured")
+
 	csvs, err := a.findCSVsThatProvideAnyOf(providedAPIsForGroup)
 	if err != nil {
 		logger.WithError(err).Warn("could not find csvs that provide group apis")
@@ -295,32 +302,43 @@ func (a *Operator) pruneProvidedAPIs(group *v1.OperatorGroup, groupProvidedAPIs
 }
 
 // ensureProvidedAPIClusterRole ensures that a clusterrole exists (admin, edit, or view) for a single provided API Type
-func (a *Operator) ensureProvidedAPIClusterRole(operatorGroup *v1.OperatorGroup, csv *v1alpha1.ClusterServiceVersion, namePrefix, suffix string, verbs []string, group, resource string, resourceNames []string) error {
+func (a *Operator) ensureProvidedAPIClusterRole(operatorGroup *v1.OperatorGroup, csv *v1alpha1.ClusterServiceVersion, namePrefix, suffix string, verbs []string, group, resource string, resourceNames []string, api ownerutil.Owner, key opregistry.APIKey) error {
+	aggregationLabel, err := aggregationLabelFromAPIKey(key, suffix)
+	if err != nil {
+		return err
+	}
 	clusterRole := &rbacv1.ClusterRole{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: namePrefix + suffix,
 			Labels: map[string]string{
-				kubeRBACAggregationKeyPrefix + suffix:       "true",
-				operatorGroupAggregrationKeyPrefix + suffix: operatorGroup.GetName(),
+				// Matches aggregation rules on the bootstrap ClusterRoles.
+				// https://github.com/kubernetes/kubernetes/blob/61847eab61788fb0543b4cf147773c9da646ed2f/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go#L232
+				fmt.Sprintf("rbac.authorization.k8s.io/aggregate-to-%s", suffix): "true",
+				aggregationLabel: "true",
 			},
+			OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(api)},
 		},
 		Rules: []rbacv1.PolicyRule{{Verbs: verbs, APIGroups: []string{group}, Resources: []string{resource}, ResourceNames: resourceNames}},
 	}
-	err := ownerutil.AddOwnerLabels(clusterRole, csv)
-	if err != nil {
-		return err
-	}
-	existingCR, err := a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
-	if k8serrors.IsAlreadyExists(err) {
-		if existingCR != nil && reflect.DeepEqual(existingCR.Labels, clusterRole.Labels) && reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) {
+
+	existingCR, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name)
+	if existingCR == nil {
+		existingCR, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
+		if err == nil {
 			return nil
 		}
-		if _, err = a.opClient.UpdateClusterRole(clusterRole); err != nil {
-			a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole)
+		if !k8serrors.IsAlreadyExists(err) {
+			a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
 			return err
 		}
-	} else if err != nil {
-		a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
+	}
+
+	if existingCR != nil && reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) && ownerutil.IsOwnedBy(existingCR, api) && labels.Equals(existingCR.Labels, clusterRole.Labels) {
+		return nil
+	}
+
+	if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil {
+		a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole)
 		return err
 	}
 	return nil
@@ -329,27 +347,37 @@ func (a *Operator) ensureProvidedAPIClusterRole(operatorGroup *v1.OperatorGroup,
 // ensureClusterRolesForCSV ensures that ClusterRoles for writing and reading provided APIs exist for each operator
 func (a *Operator) ensureClusterRolesForCSV(csv *v1alpha1.ClusterServiceVersion, operatorGroup *v1.OperatorGroup) error {
 	for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
+		crd, err := a.lister.APIExtensionsV1beta1().CustomResourceDefinitionLister().Get(owned.Name)
+		if err != nil {
+			return fmt.Errorf("crd %q not found: %s", owned.Name, err.Error())
+		}
 		nameGroupPair := strings.SplitN(owned.Name, ".", 2) // -> etcdclusters etcd.database.coreos.com
 		if len(nameGroupPair) != 2 {
 			return fmt.Errorf("invalid parsing of name '%v', got %v", owned.Name, nameGroupPair)
 		}
 		plural := nameGroupPair[0]
 		group := nameGroupPair[1]
 		namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
-
+		key := registry.APIKey{Group: group, Version: owned.Version, Kind: owned.Kind, Plural: plural}
 		for suffix, verbs := range VerbsForSuffix {
-			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, group, plural, nil); err != nil {
+			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, group, plural, nil, crd, key); err != nil {
 				return err
 			}
 		}
-		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
+		if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}, crd, key); err != nil {
 			return err
 		}
 	}
 	for _, owned := range csv.Spec.APIServiceDefinitions.Owned {
+		svcName := strings.Join([]string{owned.Version, owned.Group}, ".")
+		svc, err := a.lister.APIRegistrationV1().APIServiceLister().Get(svcName)
+		if err != nil {
+			return fmt.Errorf("apiservice %q not found: %s", svcName, err.Error())
+		}
 		namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
+		key := registry.APIKey{Group: owned.Group, Version: owned.Version, Kind: owned.Kind}
 		for suffix, verbs := range VerbsForSuffix {
-			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, owned.Group, owned.Name, nil); err != nil {
+			if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, owned.Group, owned.Name, nil, svc, key); err != nil {
 				return err
 			}
 		}
@@ -853,38 +881,60 @@ func (a *Operator) updateNamespaceList(op *v1.OperatorGroup) ([]string, error) {
 	return namespaceList, nil
 }
 
-func (a *Operator) ensureOpGroupClusterRole(op *v1.OperatorGroup, suffix string) error {
+func (a *Operator) ensureOpGroupClusterRole(op *v1.OperatorGroup, suffix string, apis resolver.APISet) error {
 	clusterRole := &rbacv1.ClusterRole{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: strings.Join([]string{op.GetName(), suffix}, "-"),
 		},
-		AggregationRule: &rbacv1.AggregationRule{
-			ClusterRoleSelectors: []metav1.LabelSelector{
-				{
-					MatchLabels: map[string]string{
-						operatorGroupAggregrationKeyPrefix + suffix: op.GetName(),
-					},
-				},
+	}
+	var selectors []metav1.LabelSelector
+	for api := range apis {
+		aggregationLabel, err := aggregationLabelFromAPIKey(api, suffix)
+		if err != nil {
+			return err
+		}
+		selectors = append(selectors, metav1.LabelSelector{
+			MatchLabels: map[string]string{
+				aggregationLabel: "true",
 			},
-		},
+		})
+	}
+	if len(selectors) > 0 {
+		clusterRole.AggregationRule = &rbacv1.AggregationRule{
+			ClusterRoleSelectors: selectors,
+		}
 	}
 	err := ownerutil.AddOwnerLabels(clusterRole, op)
 	if err != nil {
 		return err
 	}
-	_, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
-	if k8serrors.IsAlreadyExists(err) {
+
+	existingRole, err := a.lister.RbacV1().ClusterRoleLister().Get(clusterRole.Name)
+	if existingRole == nil {
+		existingRole, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
+		if err == nil {
+			return nil
+		}
+		if !k8serrors.IsAlreadyExists(err) {
+			a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
+			return err
+		}
+	}
+
+	if existingRole != nil && labels.Equals(existingRole.Labels, clusterRole.Labels) && reflect.DeepEqual(existingRole.AggregationRule, clusterRole.AggregationRule) {
 		return nil
-	} else if err != nil {
-		a.logger.WithError(err).Errorf("Create cluster role failed: %v", clusterRole)
+	}
+
+	if _, err := a.opClient.UpdateClusterRole(clusterRole); err != nil {
+		a.logger.WithError(err).Errorf("Update existing cluster role failed: %v", clusterRole)
 		return err
 	}
 	return nil
 }
 
-func (a *Operator) ensureOpGroupClusterRoles(op *v1.OperatorGroup) error {
+func (a *Operator) ensureOpGroupClusterRoles(op *v1.OperatorGroup, apis resolver.APISet) error {
 	for _, suffix := range Suffices {
-		if err := a.ensureOpGroupClusterRole(op, suffix); err != nil {
+		if err := a.ensureOpGroupClusterRole(op, suffix, apis); err != nil {
 			return err
 		}
 	}

pkg/lib/ownerutil/util.go

@@ -9,10 +9,13 @@ import (
 	log "github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	rbac "k8s.io/api/rbac/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 )
 
 const (
@@ -365,6 +368,24 @@ func InferGroupVersionKind(obj runtime.Object) error {
 			Version: v1.GroupVersion,
 			Kind:    "OperatorGroup",
 		})
+	case *apiregistrationv1.APIService:
+		objectKind.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apiregistrationv1.GroupName,
+			Version: apiregistrationv1.SchemeGroupVersion.Version,
+			Kind:    "APIService",
+		})
+	case *apiextensionsv1beta1.CustomResourceDefinition:
+		objectKind.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apiextensionsv1beta1.GroupName,
+			Version: apiextensionsv1beta1.SchemeGroupVersion.Version,
+			Kind:    "CustomResourceDefinition",
+		})
+	case *apiextensionsv1.CustomResourceDefinition:
+		objectKind.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   apiextensionsv1.GroupName,
+			Version: apiextensionsv1.SchemeGroupVersion.Version,
+			Kind:    "CustomResourceDefinition",
+		})
 	default:
 		return fmt.Errorf("could not infer GVK for object: %#v, %#v", obj, objectKind)
 	}