Skip to content

Commit b6ac382

Browse files
ecordellecordell
committed
fix(catalog): kind was omitted from ownerlabels
ownerlabel for roles/rolebindings were missing the kind label, which is used in the selector to determine which roles/bindings are owned by the operator. this fixes a bug where roles/rolebindings would not be lifted to clusterroles/bindings when installed into a global operatorgroup
1 parent d848b97 commit b6ac382

File tree

10 files changed

+135
-47
lines changed

10 files changed

+135
-47
lines changed

‎Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ container:
9898

9999
clean-e2e:
100100
kubectl delete crds --all
101+
kubectl delete apiservices.apiregistration.k8s.io v1alpha1.packages.apps.redhat.com || true
101102
kubectl delete -f test/e2e/resources/0000_50_olm_00-namespace.yaml
102103

103104
clean:

‎pkg/controller/install/deployment.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,9 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeployme
7575
dep.Spec.Template.SetAnnotations(annotations)
7676

7777
ownerutil.AddNonBlockingOwner(dep, i.owner)
78-
ownerutil.AddOwnerLabels(dep, i.owner)
78+
if err := ownerutil.AddOwnerLabels(dep, i.owner); err != nil {
79+
return err
80+
}
7981
if _, err := i.strategyClient.CreateOrUpdateDeployment(dep); err != nil {
8082
return err
8183
}

‎pkg/controller/operators/catalog/operator.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
388388

389389
return nil
390390
}
391-
391+
392392
logger.Debug("catsrc configmap state good, checking registry pod")
393393
}
394394

@@ -398,7 +398,6 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
398398
return fmt.Errorf("no reconciler for source type %s", catsrc.Spec.SourceType)
399399
}
400400

401-
402401
// if registry pod hasn't been created or hasn't been updated since the last configmap update, recreate it
403402
if catsrc.Status.RegistryServiceStatus == nil || catsrc.Status.RegistryServiceStatus.CreatedAt.Before(&catsrc.Status.LastSync) {
404403
logger.Debug("registry server scheduled recheck")
@@ -545,7 +544,7 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
545544

546545
subs, err := o.lister.OperatorsV1alpha1().SubscriptionLister().Subscriptions(namespace).List(labels.Everything())
547546
if err != nil {
548-
logger.WithError(err).Debug("couldn't list subscriptions")
547+
logger.WithError(err).Debug("couldn't list subscriptions")
549548
return err
550549
}
551550

@@ -752,7 +751,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
752751
} else {
753752
out.Status.State = v1alpha1.SubscriptionStateAtLatest
754753
}
755-
754+
756755
out.Status.InstalledCSV = sub.Status.CurrentCSV
757756
}
758757

‎pkg/controller/operators/olm/apiservices.go

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
9393

9494
// Check if the APIService is adoptable
9595
if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
96+
logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
9697
err := olmerrors.NewUnadoptableError("", apiServiceName)
9798
logger.WithError(err).Warn("found unadoptable apiservice")
9899
errs = append(errs, err)
@@ -554,7 +555,10 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
554555
if err == nil {
555556
// Check if the only owners are this CSV or in this CSV's replacement chain.
556557
if ownerutil.AdoptableLabels(existingAuthDelegatorClusterRoleBinding.GetLabels(), true, csv) {
557-
ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv)
558+
logger.WithFields(log.Fields{"obj": "authDelegatorCRB", "labels": existingAuthDelegatorClusterRoleBinding.GetLabels()}).Debug("adopting")
559+
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv); err != nil {
560+
return nil, err
561+
}
558562
}
559563

560564
// Attempt an update.
@@ -564,7 +568,9 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
564568
}
565569
} else if k8serrors.IsNotFound(err) {
566570
// Create the role.
567-
ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv)
571+
if err := ownerutil.AddOwnerLabels(authDelegatorClusterRoleBinding, csv); err != nil {
572+
return nil, err
573+
}
568574
_, err = a.OpClient.CreateClusterRoleBinding(authDelegatorClusterRoleBinding)
569575
if err != nil {
570576
log.Warnf("could not create auth delegator clusterrolebinding %s", authDelegatorClusterRoleBinding.GetName())
@@ -597,7 +603,10 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
597603
if err == nil {
598604
// Check if the only owners are this CSV or in this CSV's replacement chain.
599605
if ownerutil.AdoptableLabels(existingAuthReaderRoleBinding.GetLabels(), true, csv) {
600-
ownerutil.AddOwnerLabels(authReaderRoleBinding, csv)
606+
logger.WithFields(log.Fields{"obj": "existingAuthReaderRB", "labels": existingAuthReaderRoleBinding.GetLabels()}).Debug("adopting")
607+
if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, csv); err != nil {
608+
return nil, err
609+
}
601610
}
602611
// Attempt an update.
603612
if _, err := a.OpClient.UpdateRoleBinding(authReaderRoleBinding); err != nil {
@@ -606,7 +615,9 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
606615
}
607616
} else if k8serrors.IsNotFound(err) {
608617
// Create the role.
609-
ownerutil.AddOwnerLabels(authReaderRoleBinding, csv)
618+
if err := ownerutil.AddOwnerLabels(authReaderRoleBinding, csv); err != nil {
619+
return nil, err
620+
}
610621
_, err = a.OpClient.CreateRoleBinding(authReaderRoleBinding)
611622
if err != nil {
612623
log.Warnf("could not create auth reader role binding %s", authReaderRoleBinding.GetName())
@@ -706,12 +717,15 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
706717

707718
// check if the APIService is adoptable
708719
if !ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {
720+
logger.WithFields(log.Fields{"obj": "apiService", "labels": apiService.GetLabels()}).Debug("adoption failed")
709721
return nil, fmt.Errorf("pre-existing APIService %s is not adoptable", apiServiceName)
710722
}
711723
}
712724

713725
// Add the CSV as an owner
714-
ownerutil.AddOwnerLabels(apiService, csv)
726+
if err := ownerutil.AddOwnerLabels(apiService, csv); err != nil {
727+
return nil, err
728+
}
715729

716730
// update the ServiceReference
717731
apiService.Spec.Service = &apiregistrationv1.ServiceReference{

‎pkg/controller/operators/olm/operator_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2987,7 +2987,7 @@ func TestSyncOperatorGroups(t *testing.T) {
29872987
ObjectMeta: metav1.ObjectMeta{
29882988
Name: "csv-role",
29892989
Namespace: operatorNamespace,
2990-
Labels: ownerutil.OwnerLabel(operatorCSV),
2990+
Labels: ownerutil.OwnerLabel(operatorCSV, v1alpha1.ClusterServiceVersionKind),
29912991
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(operatorCSV)},
29922992
},
29932993
Rules: permissions[0].Rules,
@@ -3001,7 +3001,7 @@ func TestSyncOperatorGroups(t *testing.T) {
30013001
ObjectMeta: metav1.ObjectMeta{
30023002
Name: "csv-rolebinding",
30033003
Namespace: operatorNamespace,
3004-
Labels: ownerutil.OwnerLabel(operatorCSV),
3004+
Labels: ownerutil.OwnerLabel(operatorCSV, v1alpha1.ClusterServiceVersionKind),
30053005
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(operatorCSV)},
30063006
},
30073007
Subjects: []rbacv1.Subject{

‎pkg/controller/operators/olm/operatorgroup.go

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -249,30 +249,21 @@ func (a *Operator) ensureClusterRolesForCSV(csv *v1alpha1.ClusterServiceVersion,
249249
group := nameGroupPair[1]
250250
namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
251251

252-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, AdminSuffix, VerbsForSuffix[AdminSuffix], group, plural, nil); err != nil {
253-
return err
254-
}
255-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, EditSuffix, VerbsForSuffix[EditSuffix], group, plural, nil); err != nil {
256-
return err
257-
}
258-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, ViewSuffix, VerbsForSuffix[ViewSuffix], group, plural, nil); err != nil {
259-
return err
252+
for suffix, verbs := range VerbsForSuffix {
253+
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, group, plural, nil); err != nil {
254+
return err
255+
}
260256
}
261-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"-crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
257+
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
262258
return err
263259
}
264260
}
265261
for _, owned := range csv.Spec.APIServiceDefinitions.Owned {
266262
namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)
267-
268-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, AdminSuffix, VerbsForSuffix[AdminSuffix], owned.Group, owned.Name, nil); err != nil {
269-
return err
270-
}
271-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, EditSuffix, VerbsForSuffix[EditSuffix], owned.Group, owned.Name, nil); err != nil {
272-
return err
273-
}
274-
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, ViewSuffix, VerbsForSuffix[ViewSuffix], owned.Group, owned.Name, nil); err != nil {
275-
return err
263+
for suffix, verbs := range VerbsForSuffix {
264+
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, suffix, verbs, owned.Group, owned.Name, nil); err != nil {
265+
return err
266+
}
276267
}
277268
}
278269
return nil
@@ -351,6 +342,9 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
351342
if err != nil {
352343
return err
353344
}
345+
if len(ownedRoles) == 0 {
346+
return fmt.Errorf("no owned roles found")
347+
}
354348

355349
for _, r := range ownedRoles {
356350
a.Log.Debug("processing role")
@@ -363,7 +357,7 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
363357
},
364358
ObjectMeta: metav1.ObjectMeta{
365359
Name: r.GetName(),
366-
Labels: ownerutil.OwnerLabel(csv),
360+
Labels: r.GetLabels(),
367361
},
368362
Rules: r.Rules,
369363
}
@@ -378,6 +372,9 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
378372
if err != nil {
379373
return err
380374
}
375+
if len(ownedRoleBindings) == 0 {
376+
return fmt.Errorf("no owned rolebindings found")
377+
}
381378

382379
for _, r := range ownedRoleBindings {
383380
_, err := a.lister.RbacV1().ClusterRoleBindingLister().Get(r.GetName())
@@ -389,7 +386,7 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
389386
},
390387
ObjectMeta: metav1.ObjectMeta{
391388
Name: r.GetName(),
392-
Labels: ownerutil.OwnerLabel(csv),
389+
Labels: r.GetLabels(),
393390
},
394391
Subjects: r.Subjects,
395392
RoleRef: rbacv1.RoleRef{
@@ -449,7 +446,9 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
449446
// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
450447
ownedRole.SetNamespace(targetNamespace)
451448
ownedRole.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
452-
ownerutil.AddOwnerLabels(ownedRole, targetCSV)
449+
if err := ownerutil.AddOwnerLabels(ownedRole, targetCSV); err != nil {
450+
return err
451+
}
453452
ownedRole.SetLabels(utillabels.AddLabel(ownedRole.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
454453
if _, err := a.OpClient.CreateRole(ownedRole); err != nil {
455454
return err
@@ -489,7 +488,9 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
489488
// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
490489
ownedRoleBinding.SetNamespace(targetNamespace)
491490
ownedRoleBinding.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
492-
ownerutil.AddOwnerLabels(ownedRoleBinding, targetCSV)
491+
if err := ownerutil.AddOwnerLabels(ownedRoleBinding, targetCSV); err != nil {
492+
return err
493+
}
493494
ownedRoleBinding.SetLabels(utillabels.AddLabel(ownedRoleBinding.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
494495
if _, err := a.OpClient.CreateRoleBinding(ownedRoleBinding); err != nil {
495496
return err

‎pkg/controller/registry/resolver/rbac.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
8585
Name: generateName(csv.GetName()),
8686
Namespace: csv.GetNamespace(),
8787
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
88-
Labels: ownerutil.OwnerLabel(csv),
88+
Labels: ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
8989
},
9090
Rules: permission.Rules,
9191
}
@@ -97,7 +97,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
9797
Name: generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
9898
Namespace: csv.GetNamespace(),
9999
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
100-
Labels: ownerutil.OwnerLabel(csv),
100+
Labels: ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
101101
},
102102
RoleRef: rbacv1.RoleRef{
103103
Kind: "Role",
@@ -128,7 +128,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
128128
ObjectMeta: metav1.ObjectMeta{
129129
Name: generateName(csv.GetName()),
130130
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
131-
Labels: ownerutil.OwnerLabel(csv),
131+
Labels: ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
132132
},
133133
Rules: permission.Rules,
134134
}
@@ -140,7 +140,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
140140
Name: generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
141141
Namespace: csv.GetNamespace(),
142142
OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
143-
Labels: ownerutil.OwnerLabel(csv),
143+
Labels: ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
144144
},
145145
RoleRef: rbacv1.RoleRef{
146146
Kind: "ClusterRole",

‎pkg/lib/ownerutil/util.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -187,25 +187,29 @@ func NonBlockingOwner(owner Owner) metav1.OwnerReference {
187187
}
188188

189189
// OwnerLabel returns a label added to generated objects for later querying
190-
func OwnerLabel(owner Owner) map[string]string {
190+
func OwnerLabel(owner Owner, kind string) map[string]string {
191191
return map[string]string{
192192
OwnerKey: owner.GetName(),
193193
OwnerNamespaceKey: owner.GetNamespace(),
194-
OwnerKind: owner.GetObjectKind().GroupVersionKind().Kind,
194+
OwnerKind: kind,
195195
}
196196
}
197197

198198
// AddOwnerLabels adds ownerref-like labels to an object
199-
func AddOwnerLabels(object metav1.Object, owner Owner) {
199+
func AddOwnerLabels(object metav1.Object, owner Owner) error {
200+
err := InferGroupVersionKind(owner)
201+
if err != nil {
202+
return err
203+
}
200204
labels := object.GetLabels()
201205
if labels == nil {
202206
labels = map[string]string{}
203207
}
204-
for key, val := range OwnerLabel(owner) {
208+
for key, val := range OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) {
205209
labels[key] = val
206210
}
207211
object.SetLabels(labels)
208-
return
212+
return nil
209213
}
210214

211215
// IsOwnedByKindLabel returns whether or not a label exists on the object pointing to an owner of a particular kind
@@ -241,7 +245,7 @@ func AdoptableLabels(labels map[string]string, checkName bool, targets ...Owner)
241245

242246
// CSVOwnerSelector returns a label selector to find generated objects owned by owner
243247
func CSVOwnerSelector(owner *v1alpha1.ClusterServiceVersion) labels.Selector {
244-
return labels.SelectorFromSet(OwnerLabel(owner))
248+
return labels.SelectorFromSet(OwnerLabel(owner, v1alpha1.ClusterServiceVersionKind))
245249
}
246250

247251
// AddOwner adds an owner to the ownerref list.

‎test/e2e/installplan_e2e_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
corev1 "k8s.io/api/core/v1"
1313
rbacv1 "k8s.io/api/rbac/v1"
1414
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
15+
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1516
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1617
"k8s.io/apimachinery/pkg/util/wait"
1718
"k8s.io/kubernetes/pkg/apis/rbac"
@@ -711,11 +712,36 @@ func TestCreateInstallPlanWithPermissions(t *testing.T) {
711712
t.Logf("%v, %v: %v && %v", key, expected, strings.HasPrefix(key.Name, expected.Name), key.Kind == expected.Kind)
712713
}
713714
}
715+
716+
// This operator was installed into a global operator group, so the roles should have been lifted to clusterroles
717+
if step.Resource.Kind == "Role" {
718+
err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
719+
_, err = c.GetClusterRole(step.Resource.Name)
720+
if err != nil {
721+
if k8serrors.IsNotFound(err) {
722+
return false, nil
723+
}
724+
return false, err
725+
}
726+
return true, nil
727+
})
728+
}
729+
if step.Resource.Kind == "RoleBinding" {
730+
err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
731+
_, err = c.GetClusterRoleBinding(step.Resource.Name)
732+
if err != nil {
733+
if k8serrors.IsNotFound(err) {
734+
return false, nil
735+
}
736+
return false, err
737+
}
738+
return true, nil
739+
})
740+
}
714741
}
715742

716743
// Should have removed every matching step
717744
require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected: %#v", expectedSteps)
718-
719745
}
720746

721747
func TestInstallPlanCRDValidation(t *testing.T) {

0 commit comments

Comments
 (0)