Skip to content

Commit

Permalink
Make provided API ClusterRoles be owned by the corresponding API.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@benluddy
benluddy committed Mar 9, 2020
1 parent 42df653 commit d921b44
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 56 deletions.
6 changes: 4 additions & 2 deletions pkg/api/apis/operators/operatorgroup_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, ",")
}
6 changes: 4 additions & 2 deletions pkg/api/apis/operators/v1/operatorgroup_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
19 changes: 11 additions & 8 deletions pkg/controller/operators/olm/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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...); {
Expand Down
138 changes: 94 additions & 44 deletions pkg/controller/operators/olm/operatorgroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down
21 changes: 21 additions & 0 deletions pkg/lib/ownerutil/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit d921b44

Please sign in to comment.