Merge pull request #1377 from openshift-cherrypick-robot/cherry-pick-1348-to-release-4.4

[release-4.4] Bug 1809568: Make provided API ClusterRoles be owned by the corresponding API.

Author: openshift-merge-robot

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)
 	}