Skip to content

Commit a651111

Browse files
ecordellecordell
committed
fix(opgroup): incorrectly detecting CSVs as "copyable"
We were treating CSVs that had empty phases as "copyable", but that happens before the checks to see if operatorgroups conflict. This was causing a race where CSVs could be copied to target namespaces incorrectly
1 parent 58f3e62 commit a651111

File tree

5 files changed

+48
-30
lines changed

5 files changed

+48
-30
lines changed

‎pkg/api/apis/operators/v1alpha1/clusterserviceversion.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,9 @@ func (c *ClusterServiceVersion) IsCopied() bool {
103103
}
104104

105105
func (c *ClusterServiceVersion) IsUncopiable() bool {
106+
if c.Status.Phase == CSVPhaseNone {
107+
return true
108+
}
106109
_, ok := uncopiableReasons[c.Status.Reason]
107110
return ok
108111
}

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

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -460,23 +460,32 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
460460

461461
func (a *Operator) removeDanglingChildCSVs(csv *v1alpha1.ClusterServiceVersion) error {
462462
logger := a.Log.WithFields(logrus.Fields{
463-
"id": queueinformer.NewLoopID(),
464-
"csv": csv.GetName(),
465-
"namespace": csv.GetNamespace(),
466-
"phase": csv.Status.Phase,
463+
"id": queueinformer.NewLoopID(),
464+
"csv": csv.GetName(),
465+
"namespace": csv.GetNamespace(),
466+
"phase": csv.Status.Phase,
467+
"labels": csv.GetLabels(),
468+
"annotations": csv.GetAnnotations(),
467469
})
468470

471+
if !csv.IsCopied() {
472+
logger.Debug("removeDanglingChild called on a parent. this is a no-op but should be avoided.")
473+
return nil
474+
}
475+
469476
operatorNamespace, ok := csv.Annotations[v1alpha2.OperatorGroupNamespaceAnnotationKey]
470477
if !ok {
471478
logger.Debug("missing operator namespace annotation on copied CSV")
472479
return a.deleteChild(csv)
473480
}
474481

482+
logger = logger.WithField("parentNamespace", operatorNamespace)
475483
parent, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(operatorNamespace).Get(csv.GetName())
476484
if k8serrors.IsNotFound(err) || k8serrors.IsGone(err) || parent == nil {
477485
logger.Debug("deleting copied CSV since parent is missing")
478486
return a.deleteChild(csv)
479487
}
488+
480489
if parent.Status.Phase == v1alpha1.CSVPhaseFailed && parent.Status.Reason == v1alpha1.CSVReasonInterOperatorGroupOwnerConflict {
481490
logger.Debug("deleting copied CSV since parent has intersecting operatorgroup conflict")
482491
return a.deleteChild(csv)
@@ -537,11 +546,6 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
537546
}
538547
}
539548

540-
if outCSV.IsUncopiable() {
541-
logger.WithField("reason", outCSV.Status.Reason).Debug("skipping CSV resource copy to target namespaces")
542-
return
543-
}
544-
545549
a.copyQueueIndexer.Enqueue(outCSV)
546550

547551
return
@@ -563,6 +567,11 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
563567

564568
logger.Debug("copying CSV")
565569

570+
if clusterServiceVersion.IsUncopiable() {
571+
logger.Debug("CSV uncopiable")
572+
return
573+
}
574+
566575
operatorGroup := a.operatorGroupForActiveCSV(logger, clusterServiceVersion)
567576
if operatorGroup == nil {
568577
logger.WithField("reason", "no operatorgroup found for active CSV").Debug("skipping CSV resource copy to target namespaces")
@@ -574,6 +583,10 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
574583
return
575584
}
576585

586+
logger.WithFields(logrus.Fields{
587+
"targetNamespaces": strings.Join(operatorGroup.Status.Namespaces, ","),
588+
}).Debug("copying csv to targets")
589+
577590
// Check if we need to do any copying / annotation for the operatorgroup
578591
if err := a.ensureCSVsInNamespaces(clusterServiceVersion, operatorGroup, resolver.NewNamespaceSet(operatorGroup.Status.Namespaces)); err != nil {
579592
logger.WithError(err).Info("couldn't copy CSV to target namespaces")

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,6 @@ func (a *Operator) ensureClusterRolesForCSV(csv *v1alpha1.ClusterServiceVersion,
244244
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix, ViewSuffix, VerbsForSuffix[ViewSuffix], group, plural, nil); err != nil {
245245
return err
246246
}
247-
248247
if err := a.ensureProvidedAPIClusterRole(operatorGroup, csv, namePrefix+"-crd", ViewSuffix, []string{"get"}, "apiextensions.k8s.io", "customresourcedefinitions", []string{owned.Name}); err != nil {
249248
return err
250249
}

‎test/e2e/operator_groups_e2e_test.go

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -696,9 +696,9 @@ func TestOperatorGroupIntersection(t *testing.T) {
696696
// Wait for csvB to have a CSV with a copied status in namespace C
697697

698698
// Create a catalog for csvA, csvB, and csvD
699-
pkgA := genName("a")
700-
pkgB := genName("b")
701-
pkgD := genName("d")
699+
pkgA := genName("a-")
700+
pkgB := genName("b-")
701+
pkgD := genName("d-")
702702
pkgAStable := pkgA + "-stable"
703703
pkgBStable := pkgB + "-stable"
704704
pkgDStable := pkgD + "-stable"
@@ -717,7 +717,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
717717
csvD := newCSV(pkgDStable, testNamespace, "", *semver.New("0.1.0"), []apiextensions.CustomResourceDefinition{crdD}, nil, strategyD)
718718

719719
// Create namespaces
720-
nsA, nsB, nsC, nsD, nsE := genName("a"), genName("b"), genName("c"), genName("d"), genName("e")
720+
nsA, nsB, nsC, nsD, nsE := genName("a-"), genName("b-"), genName("c-"), genName("d-"), genName("e-")
721721
c := newKubeClient(t)
722722
crc := newCRClient(t)
723723
for _, ns := range []string{nsA, nsB, nsC, nsD, nsE} {
@@ -773,9 +773,9 @@ func TestOperatorGroupIntersection(t *testing.T) {
773773
require.NoError(t, err)
774774

775775
// Create operatorgroups
776-
groupA := newOperatorGroup(nsA, genName("a"), nil, nil, nil, false)
777-
groupB := newOperatorGroup(nsB, genName("b"), nil, nil, []string{nsC}, false)
778-
groupD := newOperatorGroup(nsD, genName("d"), nil, nil, []string{nsD, nsE}, false)
776+
groupA := newOperatorGroup(nsA, genName("a-"), nil, nil, nil, false)
777+
groupB := newOperatorGroup(nsB, genName("b-"), nil, nil, []string{nsC}, false)
778+
groupD := newOperatorGroup(nsD, genName("d-"), nil, nil, []string{nsD, nsE}, false)
779779
for _, group := range []*v1alpha2.OperatorGroup{groupA, groupB, groupD} {
780780
_, err := crc.OperatorsV1alpha2().OperatorGroups(group.GetNamespace()).Create(group)
781781
require.NoError(t, err)
@@ -785,7 +785,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
785785
}
786786

787787
// Create subscription for csvD in namespaceD
788-
subDName := genName("d")
788+
subDName := genName("d-")
789789
cleanupSubD := createSubscriptionForCatalog(t, crc, nsD, subDName, catalog, pkgD, stableChannel, pkgDStable, v1alpha1.ApprovalAutomatic)
790790
defer cleanupSubD()
791791
subD, err := fetchSubscription(t, crc, nsD, subDName, subscriptionHasInstallPlanChecker)
@@ -808,7 +808,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
808808
require.NoError(t, awaitAnnotations(t, q, map[string]string{v1alpha2.OperatorGroupProvidedAPIsAnnotationKey: kvgD}))
809809

810810
// Create subscription for csvD2 in namespaceA
811-
subD2Name := genName("d")
811+
subD2Name := genName("d2-")
812812
cleanupSubD2 := createSubscriptionForCatalog(t, crc, nsA, subD2Name, catalog, pkgD, stableChannel, pkgDStable, v1alpha1.ApprovalAutomatic)
813813
defer cleanupSubD2()
814814
subD2, err := fetchSubscription(t, crc, nsA, subD2Name, subscriptionHasInstallPlanChecker)
@@ -832,7 +832,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
832832
require.NoError(t, err)
833833

834834
// Create subscription for csvA in namespaceA
835-
subAName := genName("a")
835+
subAName := genName("a-")
836836
cleanupSubA := createSubscriptionForCatalog(t, crc, nsA, subAName, catalog, pkgA, stableChannel, pkgAStable, v1alpha1.ApprovalAutomatic)
837837
defer cleanupSubA()
838838
subA, err := fetchSubscription(t, crc, nsA, subAName, subscriptionHasInstallPlanChecker)
@@ -855,7 +855,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
855855
require.NoError(t, err)
856856

857857
// Create subscription for csvB in namespaceB
858-
subBName := genName("b")
858+
subBName := genName("b-")
859859
cleanupSubB := createSubscriptionForCatalog(t, crc, nsB, subBName, catalog, pkgB, stableChannel, pkgBStable, v1alpha1.ApprovalAutomatic)
860860
defer cleanupSubB()
861861
subB, err := fetchSubscription(t, crc, nsB, subBName, subscriptionHasInstallPlanChecker)
@@ -928,8 +928,8 @@ func TestStaticProviderOperatorGroup(t *testing.T) {
928928
// Ensure KindA.version.group providedAPI annotation on operatorGroupA
929929

930930
// Create a catalog for csvA, csvB
931-
pkgA := genName("a")
932-
pkgB := genName("b")
931+
pkgA := genName("a-")
932+
pkgB := genName("b-")
933933
pkgAStable := pkgA + "-stable"
934934
pkgBStable := pkgB + "-stable"
935935
stableChannel := "stable"
@@ -943,7 +943,7 @@ func TestStaticProviderOperatorGroup(t *testing.T) {
943943
csvB := newCSV(pkgBStable, testNamespace, "", *semver.New("0.1.0"), []apiextensions.CustomResourceDefinition{crdB}, nil, strategyB)
944944

945945
// Create namespaces
946-
nsA, nsB, nsC, nsD := genName("a"), genName("b"), genName("c"), genName("d")
946+
nsA, nsB, nsC, nsD := genName("a-"), genName("b-"), genName("c-"), genName("d-")
947947
c := newKubeClient(t)
948948
crc := newCRClient(t)
949949
for _, ns := range []string{nsA, nsB, nsC, nsD} {
@@ -989,9 +989,9 @@ func TestStaticProviderOperatorGroup(t *testing.T) {
989989
require.NoError(t, err)
990990

991991
// Create OperatorGroups
992-
groupA := newOperatorGroup(nsA, genName("a"), map[string]string{v1alpha2.OperatorGroupProvidedAPIsAnnotationKey: kvgA}, nil, []string{nsD}, true)
993-
groupB := newOperatorGroup(nsB, genName("b"), nil, nil, nil, false)
994-
groupC := newOperatorGroup(nsC, genName("d"), nil, nil, []string{nsC}, false)
992+
groupA := newOperatorGroup(nsA, genName("a-"), map[string]string{v1alpha2.OperatorGroupProvidedAPIsAnnotationKey: kvgA}, nil, []string{nsD}, true)
993+
groupB := newOperatorGroup(nsB, genName("b-"), nil, nil, nil, false)
994+
groupC := newOperatorGroup(nsC, genName("d-"), nil, nil, []string{nsC}, false)
995995
for _, group := range []*v1alpha2.OperatorGroup{groupA, groupB, groupC} {
996996
_, err := crc.OperatorsV1alpha2().OperatorGroups(group.GetNamespace()).Create(group)
997997
require.NoError(t, err)
@@ -1001,7 +1001,8 @@ func TestStaticProviderOperatorGroup(t *testing.T) {
10011001
}
10021002

10031003
// Create subscription for csvA in namespaceB
1004-
subAName := genName("a")
1004+
subAName := genName("a-" +
1005+
"")
10051006
cleanupSubA := createSubscriptionForCatalog(t, crc, nsB, subAName, catalog, pkgA, stableChannel, pkgAStable, v1alpha1.ApprovalAutomatic)
10061007
defer cleanupSubA()
10071008
subA, err := fetchSubscription(t, crc, nsB, subAName, subscriptionHasInstallPlanChecker)
@@ -1053,7 +1054,7 @@ func TestStaticProviderOperatorGroup(t *testing.T) {
10531054
require.NoError(t, awaitAnnotations(t, q, map[string]string{v1alpha2.OperatorGroupProvidedAPIsAnnotationKey: kvgA}))
10541055

10551056
// Create subscription for csvB in namespaceB
1056-
subBName := genName("b")
1057+
subBName := genName("b-")
10571058
cleanupSubB := createSubscriptionForCatalog(t, crc, nsB, subBName, catalog, pkgB, stableChannel, pkgBStable, v1alpha1.ApprovalAutomatic)
10581059
defer cleanupSubB()
10591060
subB, err := fetchSubscription(t, crc, nsB, subBName, subscriptionHasInstallPlanChecker)
@@ -1126,7 +1127,7 @@ func TestCSVCopyWatchingAllNamespaces(t *testing.T) {
11261127
otherNamespaceName := genName(opGroupNamespace + "-")
11271128

11281129
t.Log("Creating CRD")
1129-
mainCRDPlural := genName("opgroup")
1130+
mainCRDPlural := genName("opgroup-")
11301131
mainCRD := newCRD(mainCRDPlural)
11311132
cleanupCRD, err := createCRD(c, mainCRD)
11321133
require.NoError(t, err)

‎test/e2e/setup_bare_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ func TestMain(m *testing.M) {
7474
}
7575
defer olmLog.Close()
7676
olmlogger := logrus.New()
77+
olmlogger.SetLevel(logrus.DebugLevel)
7778
mw := io.MultiWriter(os.Stderr, olmLog)
7879
olmlogger.SetOutput(mw)
7980
olmlogger.SetFormatter(&logrus.TextFormatter{
@@ -88,6 +89,7 @@ func TestMain(m *testing.M) {
8889
}
8990
defer catLog.Close()
9091
catlogger := logrus.New()
92+
catlogger.SetLevel(logrus.DebugLevel)
9193
cmw := io.MultiWriter(os.Stderr, catLog)
9294
catlogger.SetOutput(cmw)
9395
catlogger.SetFormatter(&logrus.TextFormatter{

0 commit comments

Comments
 (0)