Skip to content

Commit ae5ef05

Browse files
author
Jeff Peeler
committed
fix(olm): unit test flakes
This refactors the implementation to no longer race itself with reading from the cache, which sometimes would fail. Essentially, the code has been modified to not create a CSV and then later within the same sync loop try to look up the same CSV in the lister. Changes: -ensureRBACInTargetNamespace was moved from syncCopyCSV to syncClusterServiceVersion -ensureRBACInTargetNamespace changed to no longer create role/rolebindings, just cluster level RBAC -ensureTenantRBAC was moved into ensureCSVsInNamespaces -ensureTenantRBAC was changed to utilize newly created CSV of target namespace instead of retrieving from the cache
1 parent 66cc9dd commit ae5ef05

File tree

2 files changed

+83
-45
lines changed

2 files changed

+83
-45
lines changed

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

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,23 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
582582
}
583583
}
584584

585+
operatorGroup := a.operatorGroupFromAnnotations(logger, clusterServiceVersion)
586+
if operatorGroup == nil {
587+
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping potential RBAC creation in target namespaces")
588+
return
589+
}
590+
591+
if len(operatorGroup.Status.Namespaces) == 1 && operatorGroup.Status.Namespaces[0] == operatorGroup.GetNamespace() {
592+
logger.Debug("skipping copy for OwnNamespace operatorgroup")
593+
return
594+
}
595+
// Ensure operator has access to targetnamespaces with cluster RBAC
596+
// (roles/rolebindings are checked for each target namespace in syncCopyCSV)
597+
if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {
598+
logger.WithError(err).Info("couldn't ensure RBAC in target namespaces")
599+
syncError = err
600+
}
601+
585602
if !outCSV.IsUncopiable() {
586603
a.copyQueueIndexer.Enqueue(outCSV)
587604
}
@@ -607,7 +624,9 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
607624

608625
operatorGroup := a.operatorGroupFromAnnotations(logger, clusterServiceVersion)
609626
if operatorGroup == nil {
610-
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping CSV resource copy to target namespaces")
627+
// since syncClusterServiceVersion is the only enqueuer, annotations should be present
628+
logger.WithField("reason", "no operatorgroup found for active CSV").Error("operatorgroup should have annotations")
629+
syncError = fmt.Errorf("operatorGroup for csv '%v' should have annotations", clusterServiceVersion.GetName())
611630
return
612631
}
613632

@@ -621,12 +640,6 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
621640
syncError = err
622641
}
623642

624-
// Ensure operator has access to targetnamespaces
625-
if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {
626-
logger.WithError(err).Info("couldn't ensure RBAC in target namespaces")
627-
syncError = err
628-
}
629-
630643
return
631644
}
632645

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

Lines changed: 63 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -315,30 +315,6 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi
315315
return nil
316316
}
317317

318-
// otherwise, create roles/rolebindings for each target namespace
319-
for _, ns := range targetNamespaces {
320-
if ns == operatorGroup.GetNamespace() {
321-
continue
322-
}
323-
324-
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv.GetNamespace())
325-
if err != nil {
326-
logger.WithError(err).Debug("permission status")
327-
return err
328-
}
329-
logger.WithField("target", ns).WithField("permMet", permMet).Debug("permission status")
330-
331-
// operator already has access in the target namespace
332-
if permMet {
333-
logger.Debug("operator has access")
334-
continue
335-
}
336-
337-
if err := a.ensureTenantRBAC(operatorGroup.GetNamespace(), ns, csv); err != nil {
338-
logger.WithError(err).Debug("ensuring tenant rbac")
339-
return err
340-
}
341-
}
342318
return nil
343319
}
344320

@@ -413,15 +389,11 @@ func (a *Operator) ensureSingletonRBAC(operatorNamespace string, csv *v1alpha1.C
413389
return nil
414390
}
415391

416-
func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, csv *v1alpha1.ClusterServiceVersion) error {
392+
func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, csv *v1alpha1.ClusterServiceVersion, targetCSV *v1alpha1.ClusterServiceVersion) error {
417393
if operatorNamespace == targetNamespace {
418394
return nil
419395
}
420396

421-
targetCSV, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(targetNamespace).Get(csv.GetName())
422-
if err != nil {
423-
return err
424-
}
425397
ownerSelector := ownerutil.CSVOwnerSelector(csv)
426398
ownedRoles, err := a.lister.RbacV1().RoleLister().Roles(operatorNamespace).List(ownerSelector)
427399
if err != nil {
@@ -525,27 +497,74 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o
525497
if err != nil {
526498
return err
527499
}
500+
501+
strategyResolver := install.StrategyResolver{}
502+
strategy, err := strategyResolver.UnmarshalStrategy(csv.Spec.InstallStrategy)
503+
if err != nil {
504+
return err
505+
}
506+
strategyDetailsDeployment, ok := strategy.(*install.StrategyDetailsDeployment)
507+
if !ok {
508+
return fmt.Errorf("could not cast install strategy as type %T", strategyDetailsDeployment)
509+
}
510+
ruleChecker := install.NewCSVRuleChecker(a.lister.RbacV1().RoleLister(), a.lister.RbacV1().RoleBindingLister(), a.lister.RbacV1().ClusterRoleLister(), a.lister.RbacV1().ClusterRoleBindingLister(), csv)
511+
512+
logger := a.Log.WithField("opgroup", operatorGroup.GetName()).WithField("csv", csv.GetName())
513+
514+
targetCSVs := make(map[string]*v1alpha1.ClusterServiceVersion)
528515
for _, ns := range namespaces {
529516
if ns.GetName() == operatorGroup.Namespace {
530517
continue
531518
}
532519
if targets.Contains(ns.GetName()) {
533-
if err := a.copyToNamespace(csv, ns.GetName()); err != nil {
520+
var targetCSV *v1alpha1.ClusterServiceVersion
521+
if targetCSV, err = a.copyToNamespace(csv, ns.GetName()); err != nil {
534522
a.Log.WithError(err).Debug("error copying to target")
535523
}
524+
targetCSVs[ns.GetName()] = targetCSV
536525
} else {
537526
if err := a.pruneFromNamespace(operatorGroup.GetName(), ns.GetName()); err != nil {
538527
a.Log.WithError(err).Debug("error pruning from old target")
539528
}
540529
}
541530
}
542531

532+
targetNamespaces := operatorGroup.Status.Namespaces
533+
if targetNamespaces == nil {
534+
a.Log.Errorf("operatorgroup '%v' should have non-nil status", operatorGroup.GetName())
535+
return nil
536+
}
537+
for _, ns := range targetNamespaces {
538+
// create roles/rolebindings for each target namespace
539+
permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv.GetNamespace())
540+
if err != nil {
541+
logger.WithError(err).Debug("permission status")
542+
return err
543+
}
544+
logger.WithField("target", ns).WithField("permMet", permMet).Debug("permission status")
545+
546+
// operator already has access in the target namespace
547+
if permMet {
548+
logger.Debug("operator has access")
549+
continue
550+
}
551+
552+
targetCSV, ok := targetCSVs[ns]
553+
if !ok {
554+
return fmt.Errorf("bug: no target CSV for namespace %v", ns)
555+
}
556+
if err := a.ensureTenantRBAC(operatorGroup.GetNamespace(), ns, csv, targetCSV); err != nil {
557+
logger.WithError(err).Debug("ensuring tenant rbac")
558+
return err
559+
}
560+
}
561+
543562
return nil
544563
}
545564

546-
func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespace string) error {
565+
func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespace string) (*v1alpha1.ClusterServiceVersion, error) {
547566
if csv.GetNamespace() == namespace {
548-
return nil
567+
return nil, fmt.Errorf("bug: can not copy to active namespace %v", csv.GetNamespace())
549568
}
550569

551570
logger := a.Log.WithField("operator-ns", csv.GetNamespace()).WithField("target-ns", namespace)
@@ -566,7 +585,7 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
566585
logger.Debug("updating target CSV")
567586
if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(fetchedCSV); err != nil {
568587
logger.WithError(err).Error("update target CSV failed")
569-
return err
588+
return nil, err
570589
}
571590
}
572591

@@ -582,10 +601,12 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
582601
fetchedCSV.Status.LastUpdateTime = timeNow()
583602
if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(fetchedCSV); err != nil {
584603
logger.WithError(err).Error("status update for target CSV failed")
585-
return err
604+
return nil, err
586605
}
587606
}
588607

608+
return fetchedCSV, nil
609+
589610
} else if k8serrors.IsNotFound(err) {
590611
newCSV.SetNamespace(namespace)
591612
newCSV.SetResourceVersion("")
@@ -595,21 +616,25 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
595616
createdCSV, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Create(newCSV)
596617
if err != nil {
597618
a.Log.Errorf("Create for new CSV failed: %v", err)
598-
return err
619+
return nil, err
599620
}
600621
createdCSV.Status.Reason = v1alpha1.CSVReasonCopied
601622
createdCSV.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", csv.GetNamespace())
602623
createdCSV.Status.LastUpdateTime = timeNow()
603624
if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(createdCSV); err != nil {
604625
a.Log.Errorf("Status update for CSV failed: %v", err)
605-
return err
626+
return nil, err
606627
}
607628

629+
return createdCSV, nil
630+
608631
} else if err != nil {
609632
logger.WithError(err).Error("couldn't get CSV")
610-
return err
633+
return nil, err
611634
}
612-
return nil
635+
636+
// this return shouldn't be hit
637+
return nil, fmt.Errorf("unhandled code path")
613638
}
614639

615640
func (a *Operator) pruneFromNamespace(operatorGroupName, namespace string) error {

0 commit comments

Comments
 (0)