Skip to content

Commit 15260dc

Browse files
ecordellecordell
committed
fix(operatorgroups): fixes rbac checking for multinamespace groups
this was causing the operatorgroup test to hang
1 parent 56dbdf9 commit 15260dc

File tree

5 files changed

+74
-44
lines changed

5 files changed

+74
-44
lines changed

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

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,12 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
551551
})
552552
logger.Debug("syncing CSV")
553553

554+
if clusterServiceVersion.IsCopied() {
555+
logger.Debug("skipping copied csv transition, schedule for gc check")
556+
a.gcQueueIndexer.Enqueue(clusterServiceVersion)
557+
return
558+
}
559+
554560
outCSV, syncError := a.transitionCSVState(*clusterServiceVersion)
555561

556562
if outCSV == nil {
@@ -569,9 +575,10 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
569575
updateErr := errors.New("error updating ClusterServiceVersion status: " + err.Error())
570576
if syncError == nil {
571577
logger.Info(updateErr)
572-
return updateErr
578+
syncError = updateErr
579+
} else {
580+
syncError = fmt.Errorf("error transitioning ClusterServiceVersion: %s and error updating CSV status: %s", syncError, updateErr)
573581
}
574-
syncError = fmt.Errorf("error transitioning ClusterServiceVersion: %s and error updating CSV status: %s", syncError, updateErr)
575582
}
576583
}
577584

@@ -598,11 +605,6 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
598605

599606
logger.Debug("copying CSV")
600607

601-
if clusterServiceVersion.IsUncopiable() {
602-
logger.Debug("CSV uncopiable")
603-
return
604-
}
605-
606608
operatorGroup := a.operatorGroupFromAnnotations(logger, clusterServiceVersion)
607609
if operatorGroup == nil {
608610
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping CSV resource copy to target namespaces")
@@ -743,12 +745,6 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
743745
out = in.DeepCopy()
744746
now := timeNow()
745747

746-
if out.IsCopied() {
747-
logger.Debug("skipping copied csv transition, schedule for gc check")
748-
a.gcQueueIndexer.Enqueue(out)
749-
return
750-
}
751-
752748
operatorSurface, err := resolver.NewOperatorFromV1Alpha1CSV(out)
753749
if err != nil {
754750
// TODO: Add failure status to CSV

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

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2466,8 +2466,8 @@ func TestTransitionCSV(t *testing.T) {
24662466
},
24672467
expected: expected{
24682468
csvStates: map[string]csvState{
2469-
"csv1": {exists: true, phase: v1alpha1.CSVPhaseDeleting},
2470-
"csv2": {exists: true, phase: v1alpha1.CSVPhaseDeleting},
2469+
"csv1": {exists: true, phase: v1alpha1.CSVPhaseReplacing},
2470+
"csv2": {exists: true, phase: v1alpha1.CSVPhaseReplacing},
24712471
"csv3": {exists: true, phase: v1alpha1.CSVPhaseSucceeded},
24722472
},
24732473
},
@@ -3549,8 +3549,9 @@ func TestSyncOperatorGroups(t *testing.T) {
35493549
APIVersion: rbacv1.GroupName,
35503550
},
35513551
ObjectMeta: metav1.ObjectMeta{
3552-
Name: "csv-role",
3553-
Namespace: targetNamespace,
3552+
ResourceVersion: "0",
3553+
Name: "csv-role",
3554+
Namespace: targetNamespace,
35543555
Labels: map[string]string{
35553556
"olm.copiedFrom": "operator-ns",
35563557
"olm.owner": "csv1",
@@ -3569,8 +3570,9 @@ func TestSyncOperatorGroups(t *testing.T) {
35693570
APIVersion: rbacv1.GroupName,
35703571
},
35713572
ObjectMeta: metav1.ObjectMeta{
3572-
Name: "csv-rolebinding",
3573-
Namespace: targetNamespace,
3573+
ResourceVersion: "0",
3574+
Name: "csv-rolebinding",
3575+
Namespace: targetNamespace,
35743576
Labels: map[string]string{
35753577
"olm.copiedFrom": "operator-ns",
35763578
"olm.owner": "csv1",
@@ -3649,8 +3651,9 @@ func TestSyncOperatorGroups(t *testing.T) {
36493651
APIVersion: rbacv1.GroupName,
36503652
},
36513653
ObjectMeta: metav1.ObjectMeta{
3652-
Name: "csv-role",
3653-
Namespace: targetNamespace,
3654+
ResourceVersion: "0",
3655+
Name: "csv-role",
3656+
Namespace: targetNamespace,
36543657
Labels: map[string]string{
36553658
"olm.copiedFrom": "operator-ns",
36563659
"olm.owner": "csv1",
@@ -3669,8 +3672,9 @@ func TestSyncOperatorGroups(t *testing.T) {
36693672
APIVersion: rbacv1.GroupName,
36703673
},
36713674
ObjectMeta: metav1.ObjectMeta{
3672-
Name: "csv-rolebinding",
3673-
Namespace: targetNamespace,
3675+
ResourceVersion: "0",
3676+
Name: "csv-rolebinding",
3677+
Namespace: targetNamespace,
36743678
Labels: map[string]string{
36753679
"olm.copiedFrom": "operator-ns",
36763680
"olm.owner": "csv1",

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

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi
297297
strategyDetailsDeployment.ClusterPermissions = append(strategyDetailsDeployment.ClusterPermissions, p)
298298
}
299299
strategyDetailsDeployment.Permissions = nil
300-
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, corev1.NamespaceAll)
300+
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, corev1.NamespaceAll, csv.GetNamespace())
301301
if err != nil {
302302
return err
303303
}
@@ -321,15 +321,21 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi
321321
continue
322322
}
323323

324-
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns)
324+
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv.GetNamespace())
325325
if err != nil {
326+
logger.WithError(err).Debug("permission status")
326327
return err
327328
}
329+
logger.WithField("target", ns).WithField("permMet", permMet).Debug("permission status")
330+
328331
// operator already has access in the target namespace
329332
if permMet {
330-
return nil
333+
logger.Debug("operator has access")
334+
continue
331335
}
336+
332337
if err := a.ensureTenantRBAC(operatorGroup.GetNamespace(), ns, csv); err != nil {
338+
logger.WithError(err).Debug("ensuring tenant rbac")
333339
return err
334340
}
335341
}
@@ -408,6 +414,10 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
408414
}
409415

410416
func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, csv *v1alpha1.ClusterServiceVersion) error {
417+
if operatorNamespace == targetNamespace {
418+
return nil
419+
}
420+
411421
targetCSV, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(targetNamespace).Get(csv.GetName())
412422
if err != nil {
413423
return err
@@ -418,6 +428,10 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
418428
return err
419429
}
420430

431+
if len(ownedRoles) == 0 {
432+
return fmt.Errorf("owned roles not found in cache")
433+
}
434+
421435
targetRoles, err := a.lister.RbacV1().RoleLister().Roles(targetNamespace).List(ownerutil.CSVOwnerSelector(targetCSV))
422436
if err != nil {
423437
return err
@@ -448,13 +462,15 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
448462

449463
// role doesn't exist, create it
450464
// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
451-
ownedRole.SetNamespace(targetNamespace)
452-
ownedRole.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
453-
if err := ownerutil.AddOwnerLabels(ownedRole, targetCSV); err != nil {
465+
targetRole := ownedRole.DeepCopy()
466+
targetRole.SetResourceVersion("0")
467+
targetRole.SetNamespace(targetNamespace)
468+
targetRole.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
469+
if err := ownerutil.AddOwnerLabels(targetRole, targetCSV); err != nil {
454470
return err
455471
}
456-
ownedRole.SetLabels(utillabels.AddLabel(ownedRole.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
457-
if _, err := a.OpClient.CreateRole(ownedRole); err != nil {
472+
targetRole.SetLabels(utillabels.AddLabel(targetRole.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace))
473+
if _, err := a.OpClient.CreateRole(targetRole); err != nil {
458474
return err
459475
}
460476
}
@@ -491,6 +507,7 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
491507
// role binding doesn't exist
492508
// TODO: we can work around error cases here; if there's an un-owned role with a matching name we should generate instead
493509
ownedRoleBinding.SetNamespace(targetNamespace)
510+
ownedRoleBinding.SetResourceVersion("0")
494511
ownedRoleBinding.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)})
495512
if err := ownerutil.AddOwnerLabels(ownedRoleBinding, targetCSV); err != nil {
496513
return err

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
242242
}
243243

244244
// permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace
245-
func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus, error) {
245+
func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, targetNamespace, serviceAccountNamespace string) (bool, []v1alpha1.RequirementStatus, error) {
246246
statusesSet := map[string]v1alpha1.RequirementStatus{}
247247

248248
checkPermissions := func(permissions []install.StrategyDeploymentPermissions, namespace string) (bool, error) {
@@ -266,7 +266,7 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
266266
}
267267

268268
// Ensure the ServiceAccount exists
269-
sa, err := a.OpClient.GetServiceAccount(csvNamespace, perm.ServiceAccountName)
269+
sa, err := a.OpClient.GetServiceAccount(serviceAccountNamespace, perm.ServiceAccountName)
270270
if err != nil {
271271
met = false
272272
status.Status = v1alpha1.RequirementStatusReasonNotPresent
@@ -320,7 +320,7 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
320320
return met, nil
321321
}
322322

323-
permMet, err := checkPermissions(strategyDetailsDeployment.Permissions, csvNamespace)
323+
permMet, err := checkPermissions(strategyDetailsDeployment.Permissions, targetNamespace)
324324
if err != nil {
325325
return false, nil, err
326326
}
@@ -331,7 +331,7 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
331331

332332
statuses := []v1alpha1.RequirementStatus{}
333333
for key, status := range statusesSet {
334-
a.Log.Debugf("appending permission status: %s", key)
334+
a.Log.WithField("key", key).WithField("status", status).Debugf("appending permission status")
335335
statuses = append(statuses, status)
336336
}
337337

@@ -365,7 +365,7 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe
365365
clusterRoleBindingLister := rbacLister.ClusterRoleBindingLister()
366366

367367
ruleChecker := install.NewCSVRuleChecker(roleLister, roleBindingLister, clusterRoleLister, clusterRoleBindingLister, csv)
368-
permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace())
368+
permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace(), csv.GetNamespace())
369369
if err != nil {
370370
return false, nil, err
371371
}

‎test/e2e/operator_groups_e2e_test.go

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ func TestOperatorGroup(t *testing.T) {
163163
require.NoError(t, err)
164164

165165
log("Creating CSV")
166+
166167
// Generate permissions
167168
serviceAccountName := genName("nginx-sa")
168169
permissions := []install.StrategyDeploymentPermissions{
@@ -178,19 +179,35 @@ func TestOperatorGroup(t *testing.T) {
178179
},
179180
}
180181

182+
// Create a new NamedInstallStrategy
183+
deploymentName := genName("operator-deployment")
184+
namedStrategy := newNginxInstallStrategy(deploymentName, permissions, nil)
185+
186+
aCSV := newCSV(csvName, opGroupNamespace, "", semver.MustParse("0.0.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, namedStrategy)
187+
createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Create(&aCSV)
188+
require.NoError(t, err)
189+
181190
serviceAccount := &corev1.ServiceAccount{
182191
ObjectMeta: metav1.ObjectMeta{
183192
Namespace: opGroupNamespace,
184193
Name: serviceAccountName,
185194
},
186195
}
196+
ownerutil.AddNonBlockingOwner(serviceAccount, createdCSV)
197+
err = ownerutil.AddOwnerLabels(serviceAccount, createdCSV)
198+
require.NoError(t, err)
199+
187200
role := &rbacv1.Role{
188201
ObjectMeta: metav1.ObjectMeta{
189202
Namespace: opGroupNamespace,
190203
Name: serviceAccountName + "-role",
191204
},
192205
Rules: permissions[0].Rules,
193206
}
207+
ownerutil.AddNonBlockingOwner(role, createdCSV)
208+
err = ownerutil.AddOwnerLabels(role, createdCSV)
209+
require.NoError(t, err)
210+
194211
roleBinding := &rbacv1.RoleBinding{
195212
ObjectMeta: metav1.ObjectMeta{
196213
Namespace: opGroupNamespace,
@@ -208,21 +225,17 @@ func TestOperatorGroup(t *testing.T) {
208225
Name: role.GetName(),
209226
},
210227
}
228+
ownerutil.AddNonBlockingOwner(roleBinding, createdCSV)
229+
err = ownerutil.AddOwnerLabels(roleBinding, createdCSV)
230+
require.NoError(t, err)
231+
211232
_, err = c.CreateServiceAccount(serviceAccount)
212233
require.NoError(t, err)
213234
_, err = c.CreateRole(role)
214235
require.NoError(t, err)
215236
_, err = c.CreateRoleBinding(roleBinding)
216237
require.NoError(t, err)
217238

218-
// Create a new NamedInstallStrategy
219-
deploymentName := genName("operator-deployment")
220-
namedStrategy := newNginxInstallStrategy(deploymentName, permissions, nil)
221-
222-
aCSV := newCSV(csvName, opGroupNamespace, "", semver.MustParse("0.0.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, namedStrategy)
223-
createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Create(&aCSV)
224-
require.NoError(t, err)
225-
226239
log("wait for CSV to succeed")
227240
err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
228241
fetched, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Get(createdCSV.GetName(), metav1.GetOptions{})

0 commit comments

Comments
 (0)