Merge pull request #679 from openshift-bot/synchronize-upstream

OCPBUGS-24587,OCPBUGS-28744: Synchronize From Upstream Repositories
openshift · Feb 5, 2024 · 5748166 · 5748166
2 parents ebfa3c1 + 249da52
commit 5748166
Show file tree

Hide file tree

Showing 12 changed files with 315 additions and 69 deletions.
diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go
@@ -186,7 +186,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo
 		return nil, err
 	}
 
-	canFilter, err := labeller.Validate(ctx, logger, metadataClient, crClient)
+	canFilter, err := labeller.Validate(ctx, logger, metadataClient, crClient, labeller.IdentityCatalogOperator)
 	if err != nil {
 		return nil, err
 	}
@@ -541,6 +541,49 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo
 		return nil, err
 	}
 
+	{
+		gvr := servicesgvk
+		informer := serviceInformer.Informer()
+
+		logger := op.logger.WithFields(logrus.Fields{"gvr": gvr.String()})
+		logger.Info("registering owner reference fixer")
+
+		queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{
+			Name: gvr.String(),
+		})
+		queueInformer, err := queueinformer.NewQueueInformer(
+			ctx,
+			queueinformer.WithQueue(queue),
+			queueinformer.WithLogger(op.logger),
+			queueinformer.WithInformer(informer),
+			queueinformer.WithSyncer(queueinformer.LegacySyncHandler(func(obj interface{}) error {
+				service, ok := obj.(*corev1.Service)
+				if !ok {
+					err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(*corev1.Service), obj)
+					logger.WithError(err).Error("casting failed")
+					return fmt.Errorf("casting failed: %w", err)
+				}
+
+				deduped := deduplicateOwnerReferences(service.OwnerReferences)
+				if len(deduped) != len(service.OwnerReferences) {
+					localCopy := service.DeepCopy()
+					localCopy.OwnerReferences = deduped
+					if _, err := op.opClient.KubernetesInterface().CoreV1().Services(service.Namespace).Update(ctx, localCopy, metav1.UpdateOptions{}); err != nil {
+						return err
+					}
+				}
+				return nil
+			}).ToSyncer()),
+		)
+		if err != nil {
+			return nil, err
+		}
+
+		if err := op.RegisterQueueInformer(queueInformer); err != nil {
+			return nil, err
+		}
+	}
+
 	// Wire Pods for CatalogSource
 	catsrcReq, err := labels.NewRequirement(reconciler.CatalogSourceLabelKey, selection.Exists, nil)
 	if err != nil {
@@ -1393,16 +1436,14 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 	}
 
 	// Make sure that we no longer indicate unpacking progress
-	subs = o.setSubsCond(subs, v1alpha1.SubscriptionCondition{
-		Type:   v1alpha1.SubscriptionBundleUnpacking,
-		Status: corev1.ConditionFalse,
-	})
+	o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking)
 
 	// Remove BundleUnpackFailed condition from subscriptions
 	o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpackFailed)
 
 	// Remove resolutionfailed condition from subscriptions
 	o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)
+
 	newSub := true
 	for _, updatedSub := range updatedSubs {
 		updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed)
@@ -1679,13 +1720,9 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.Subs
 	return subList
 }
 
-// removeSubsCond will remove the condition to the subscription if it exists
-// Only return the list of updated subscriptions
-func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription {
-	var (
-		lastUpdated = o.now()
-	)
-	var subList []*v1alpha1.Subscription
+// removeSubsCond removes the given condition from all of the subscriptions in the input
+func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) {
+	lastUpdated := o.now()
 	for _, sub := range subs {
 		cond := sub.Status.GetCondition(condType)
 		// if status is ConditionUnknown, the condition doesn't exist. Just skip
@@ -1694,9 +1731,7 @@ func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alph
 		}
 		sub.Status.LastUpdated = lastUpdated
 		sub.Status.RemoveConditions(condType)
-		subList = append(subList, sub)
 	}
-	return subList
 }
 
 func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) {
@@ -2860,3 +2895,7 @@ func (o *Operator) apiresourceFromGVK(gvk schema.GroupVersionKind) (metav1.APIRe
 	logger.Info("couldn't find GVK in api discovery")
 	return metav1.APIResource{}, olmerrors.GroupVersionKindNotFoundError{Group: gvk.Group, Version: gvk.Version, Kind: gvk.Kind}
 }
+
+func deduplicateOwnerReferences(refs []metav1.OwnerReference) []metav1.OwnerReference {
+	return sets.New(refs...).UnsortedList()
+}
diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go
@@ -1261,13 +1261,6 @@ func TestSyncResolvingNamespace(t *testing.T) {
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "",
 						State:      "",
-						Conditions: []v1alpha1.SubscriptionCondition{
-							{
-								Type:   v1alpha1.SubscriptionBundleUnpacking,
-								Status: corev1.ConditionFalse,
-							},
-						},
-						LastUpdated: now,
 					},
 				},
 			},
@@ -1399,6 +1392,62 @@ func TestSyncResolvingNamespace(t *testing.T) {
 			},
 			wantErr: fmt.Errorf("some error"),
 		},
+		{
+			name: "HadErrorShouldClearError",
+			fields: fields{
+				clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)},
+				existingOLMObjs: []runtime.Object{
+					&v1alpha1.Subscription{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SchemeGroupVersion.String(),
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "sub",
+							Namespace: testNamespace,
+						},
+						Spec: &v1alpha1.SubscriptionSpec{
+							CatalogSource:          "src",
+							CatalogSourceNamespace: testNamespace,
+						},
+						Status: v1alpha1.SubscriptionStatus{
+							InstalledCSV: "sub-csv",
+							State:        "AtLatestKnown",
+							Conditions: []v1alpha1.SubscriptionCondition{
+								{
+									Type:    v1alpha1.SubscriptionResolutionFailed,
+									Reason:  "ConstraintsNotSatisfiable",
+									Message: "constraints not satisfiable: no operators found from catalog src in namespace testNamespace referenced by subscrition sub, subscription sub exists",
+									Status:  corev1.ConditionTrue,
+								},
+							},
+						},
+					},
+				},
+				resolveErr: nil,
+			},
+			wantSubscriptions: []*v1alpha1.Subscription{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       v1alpha1.SubscriptionKind,
+						APIVersion: v1alpha1.SchemeGroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: testNamespace,
+					},
+					Spec: &v1alpha1.SubscriptionSpec{
+						CatalogSource:          "src",
+						CatalogSourceNamespace: testNamespace,
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						InstalledCSV: "sub-csv",
+						State:        "AtLatestKnown",
+						LastUpdated:  now,
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/ownerrefs_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/ownerrefs_test.go
@@ -0,0 +1,42 @@
+package catalog
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestDedupe(t *testing.T) {
+	yes := true
+	refs := []metav1.OwnerReference{
+		{
+			APIVersion:         "apiversion",
+			Kind:               "kind",
+			Name:               "name",
+			UID:                "uid",
+			Controller:         &yes,
+			BlockOwnerDeletion: &yes,
+		},
+		{
+			APIVersion:         "apiversion",
+			Kind:               "kind",
+			Name:               "name",
+			UID:                "uid",
+			Controller:         &yes,
+			BlockOwnerDeletion: &yes,
+		},
+		{
+			APIVersion:         "apiversion",
+			Kind:               "kind",
+			Name:               "name",
+			UID:                "uid",
+			Controller:         &yes,
+			BlockOwnerDeletion: &yes,
+		},
+	}
+	deduped := deduplicateOwnerReferences(refs)
+	t.Logf("got %d deduped from %d", len(deduped), len(refs))
+	if len(deduped) == len(refs) {
+		t.Errorf("didn't dedupe: %#v", deduped)
+	}
+}
diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/labeller/filters.go b/staging/operator-lifecycle-manager/pkg/controller/operators/labeller/filters.go
@@ -80,7 +80,39 @@ var filters = map[schema.GroupVersionResource]func(metav1.Object) bool{
 	},
 }
 
-func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface, operatorClient operators.Interface) (bool, error) {
+type Identity string
+
+const (
+	IdentityCatalogOperator = "catalog-operator"
+	IdentityOLMOperator     = "olm-operator"
+)
+
+func gvrRelevant(identity Identity) func(schema.GroupVersionResource) bool {
+	switch identity {
+	case IdentityCatalogOperator:
+		return sets.New(
+			rbacv1.SchemeGroupVersion.WithResource("roles"),
+			rbacv1.SchemeGroupVersion.WithResource("rolebindings"),
+			corev1.SchemeGroupVersion.WithResource("serviceaccounts"),
+			corev1.SchemeGroupVersion.WithResource("services"),
+			corev1.SchemeGroupVersion.WithResource("pods"),
+			corev1.SchemeGroupVersion.WithResource("configmaps"),
+			batchv1.SchemeGroupVersion.WithResource("jobs"),
+			apiextensionsv1.SchemeGroupVersion.WithResource("customresourcedefinitions"),
+		).Has
+	case IdentityOLMOperator:
+		return sets.New(
+			appsv1.SchemeGroupVersion.WithResource("deployments"),
+			rbacv1.SchemeGroupVersion.WithResource("clusterroles"),
+			rbacv1.SchemeGroupVersion.WithResource("clusterrolebindings"),
+			apiextensionsv1.SchemeGroupVersion.WithResource("customresourcedefinitions"),
+		).Has
+	default:
+		panic("programmer error: invalid identity")
+	}
+}
+
+func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface, operatorClient operators.Interface, identity Identity) (bool, error) {
 	okLock := sync.Mutex{}
 	ok := true
 	g, ctx := errgroup.WithContext(ctx)
@@ -125,6 +157,10 @@ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadat
 	})
 
 	for gvr, filter := range allFilters {
+		if !gvrRelevant(identity)(gvr) {
+			logger.WithField("gvr", gvr.String()).Info("skipping irrelevant gvr")
+			continue
+		}
 		gvr, filter := gvr, filter
 		g.Go(func() error {
 			list, err := metadataClient.Resource(gvr).List(ctx, metav1.ListOptions{})

diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go
@@ -183,7 +183,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		return nil, err
 	}
 
-	canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient, config.externalClient)
+	canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient, config.externalClient, labeller.IdentityOLMOperator)
 	if err != nil {
 		return nil, err
 	}

diff --git a/staging/operator-lifecycle-manager/pkg/lib/ownerutil/util.go b/staging/operator-lifecycle-manager/pkg/lib/ownerutil/util.go
@@ -167,20 +167,13 @@ func AddNonBlockingOwner(object metav1.Object, owner Owner) {
 		ownerRefs = []metav1.OwnerReference{}
 	}
 
-	// Infer TypeMeta for the target
-	if err := InferGroupVersionKind(owner); err != nil {
-		log.Warn(err.Error())
-	}
-	gvk := owner.GetObjectKind().GroupVersionKind()
-
+	nonBlockingOwner := NonBlockingOwner(owner)
 	for _, item := range ownerRefs {
-		if item.Kind == gvk.Kind {
-			if item.Name == owner.GetName() && item.UID == owner.GetUID() {
-				return
-			}
+		if item.Kind == nonBlockingOwner.Kind && item.Name == nonBlockingOwner.Name && item.UID == nonBlockingOwner.UID {
+			return
 		}
 	}
-	ownerRefs = append(ownerRefs, NonBlockingOwner(owner))
+	ownerRefs = append(ownerRefs, nonBlockingOwner)
 	object.SetOwnerReferences(ownerRefs)
 }
 
@@ -284,14 +277,20 @@ func AddOwner(object metav1.Object, owner Owner, blockOwnerDeletion, isControlle
 	}
 	gvk := owner.GetObjectKind().GroupVersionKind()
 	apiVersion, kind := gvk.ToAPIVersionAndKind()
-	ownerRefs = append(ownerRefs, metav1.OwnerReference{
+	ownerRef := metav1.OwnerReference{
 		APIVersion:         apiVersion,
 		Kind:               kind,
 		Name:               owner.GetName(),
 		UID:                owner.GetUID(),
 		BlockOwnerDeletion: &blockOwnerDeletion,
 		Controller:         &isController,
-	})
+	}
+	for _, ref := range ownerRefs {
+		if ref.Kind == ownerRef.Kind && ref.Name == ownerRef.Name && ref.UID == ownerRef.UID {
+			return
+		}
+	}
+	ownerRefs = append(ownerRefs, ownerRef)
 	object.SetOwnerReferences(ownerRefs)
 }
 

diff --git a/staging/operator-lifecycle-manager/pkg/lib/testobj/runtime.go b/staging/operator-lifecycle-manager/pkg/lib/testobj/runtime.go
@@ -135,14 +135,21 @@ func WithOwner(owner, obj RuntimeMetaObject) RuntimeMetaObject {
 	out := obj.DeepCopyObject().(RuntimeMetaObject)
 	gvk := owner.GetObjectKind().GroupVersionKind()
 	apiVersion, kind := gvk.ToAPIVersionAndKind()
-	refs := append(out.GetOwnerReferences(), metav1.OwnerReference{
+
+	nonBlockingOwner := metav1.OwnerReference{
 		APIVersion:         apiVersion,
 		Kind:               kind,
 		Name:               owner.GetName(),
 		UID:                owner.GetUID(),
 		BlockOwnerDeletion: &dontBlockOwnerDeletion,
 		Controller:         &notController,
-	})
+	}
+	for _, item := range out.GetOwnerReferences() {
+		if item.Kind == nonBlockingOwner.Kind && item.Name == nonBlockingOwner.Name && item.UID == nonBlockingOwner.UID {
+			return out
+		}
+	}
+	refs := append(out.GetOwnerReferences(), nonBlockingOwner)
 	out.SetOwnerReferences(refs)
 
 	return out

diff --git a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go
@@ -2445,7 +2445,7 @@ var _ = Describe("Subscription", func() {
 				},
 				5*time.Minute,
 				interval,
-			).Should(Equal(corev1.ConditionFalse))
+			).Should(Equal(corev1.ConditionUnknown))
 
 			By("verifying that the subscription is not reporting unpacking errors")
 			Eventually(
@@ -2806,7 +2806,7 @@ properties:
 				if err != nil {
 					return err
 				}
-				if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionFalse {
+				if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionUnknown {
 					return fmt.Errorf("subscription condition %s has unexpected value %s, expected %s", v1alpha1.SubscriptionBundleUnpacking, cond.Status, corev1.ConditionFalse)
 				}
 				if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionUnknown {