Merge pull request #1408 from awgreene/reconcile-namespace-og

Reconcile Namespace OG Labels in Namespace Syncer

Author: openshift-merge-robot

pkg/api/apis/operators/v1/operatorgroup_types.go

@@ -1,6 +1,7 @@
 package v1
 
 import (
+	"fmt"
 	"sort"
 	"strings"
 
@@ -15,6 +16,9 @@ const (
 	OperatorGroupProvidedAPIsAnnotationKey = "olm.providedAPIs"
 
 	OperatorGroupKind = "OperatorGroup"
+
+	operatorGroupLabelPrefix   = "olm.operatorgroup/"
+	operatorGroupLabelTemplate = operatorGroupLabelPrefix + "%s.%s"
 )
 
 // OperatorGroupSpec is the spec for an OperatorGroup resource.
@@ -99,3 +103,14 @@ func (o *OperatorGroup) HasServiceAccountSynced() bool {
 
 	return false
 }
+
+// getOperatorGroupLabel returns a label that is applied to Namespaces to signify that the
+// namespace is a part of the OperatorGroup using selectors.
+func (o *OperatorGroup) GetLabel() string {
+	return fmt.Sprintf(operatorGroupLabelTemplate, o.GetNamespace(), o.GetName())
+}
+
+// IsOperatorGroupLabel returns true if the label is an OperatorGroup label.
+func IsOperatorGroupLabel(label string) bool {
+	return strings.HasPrefix(label, operatorGroupLabelPrefix)
+}

pkg/controller/operators/olm/operator.go

@@ -68,6 +68,7 @@ type Operator struct {
 	csvCopyQueueSet       *queueinformer.ResourceQueueSet
 	csvGCQueueSet         *queueinformer.ResourceQueueSet
 	objGCQueueSet         *queueinformer.ResourceQueueSet
+	nsQueueSet            workqueue.RateLimitingInterface
 	apiServiceQueue       workqueue.RateLimitingInterface
 	csvIndexers           map[string]cache.Indexer
 	recorder              record.EventRecorder
@@ -394,6 +395,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 	// register namespace queueinformer
 	namespaceInformer := k8sInformerFactory.Core().V1().Namespaces()
 	op.lister.CoreV1().RegisterNamespaceLister(namespaceInformer.Lister())
+	op.nsQueueSet = workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "resolver")
 	namespaceInformer.Informer().AddEventHandler(
 		&cache.ResourceEventHandlerFuncs{
 			DeleteFunc: op.namespaceAddedOrRemoved,
@@ -403,8 +405,9 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 	namespaceQueueInformer, err := queueinformer.NewQueueInformer(
 		ctx,
 		queueinformer.WithLogger(op.logger),
+		queueinformer.WithQueue(op.nsQueueSet),
 		queueinformer.WithInformer(namespaceInformer.Informer()),
-		queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncObject).ToSyncer()),
+		queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncNamespace).ToSyncer()),
 	)
 	if err != nil {
 		return nil, err
@@ -736,6 +739,49 @@ func (a *Operator) namespaceAddedOrRemoved(obj interface{}) {
 	return
 }
 
+func (a *Operator) syncNamespace(obj interface{}) error {
+	// Check to see if any operator groups are associated with this namespace
+	namespace, ok := obj.(*corev1.Namespace)
+	if !ok {
+		a.logger.Debugf("wrong type: %#v\n", obj)
+		return fmt.Errorf("casting Namespace failed")
+	}
+
+	logger := a.logger.WithFields(logrus.Fields{
+		"name": namespace.GetName(),
+	})
+
+	// Remove existing OperatorGroup labels
+	for label := range namespace.GetLabels() {
+		if v1.IsOperatorGroupLabel(label) {
+			delete(namespace.Labels, label)
+		}
+	}
+
+	operatorGroupList, err := a.lister.OperatorsV1().OperatorGroupLister().List(labels.Everything())
+	if err != nil {
+		logger.WithError(err).Warn("lister failed")
+		return err
+	}
+
+	for _, group := range operatorGroupList {
+		namespaceSet := resolver.NewNamespaceSet(group.Status.Namespaces)
+
+		// Apply the label if not an All Namespaces OperatorGroup.
+		if namespaceSet.Contains(namespace.GetName()) && !namespaceSet.IsAllNamespaces() {
+			if namespace.Labels == nil {
+				namespace.Labels = make(map[string]string, 1)
+			}
+			namespace.Labels[group.GetLabel()] = ""
+		}
+	}
+
+	// Update the Namespace
+	_, err = a.opClient.KubernetesInterface().CoreV1().Namespaces().Update(namespace)
+
+	return err
+}
+
 func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 	clusterServiceVersion, ok := obj.(*v1alpha1.ClusterServiceVersion)
 	if !ok {
@@ -1893,3 +1939,12 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets ..
 	out, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(out)
 	return out, err
 }
+
+func containsString(list []string, item string) bool {
+	for i := range list {
+		if list[i] == item {
+			return true
+		}
+	}
+	return false
+}

pkg/controller/operators/olm/operatorgroup.go

@@ -27,8 +27,6 @@ const (
 	AdminSuffix = "admin"
 	EditSuffix  = "edit"
 	ViewSuffix  = "view"
-
-	operatorGroupLabelTemplate = "olm.operatorgroup/%s.%s"
 )
 
 var (
@@ -100,9 +98,8 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	logger.WithField("targetNamespaces", targetNamespaces).Debug("updated target namespaces")
 
 	if namespacesChanged(targetNamespaces, op.Status.Namespaces) {
-		// Labels are only applied to namespaces that are not empty strings.
-		prunedStatusNamespaces := pruneEmptyStrings(op.Status.Namespaces)
-		prunedTargetNamespaces := pruneEmptyStrings(targetNamespaces)
+		logger.Debug("OperatorGroup namespaces change detected")
+		outOfSyncNamespaces := namespacesAddedOrRemoved(op.Spec.TargetNamespaces, op.Status.Namespaces)
 
 		// Update operatorgroup target namespace selection
 		logger.WithField("targets", targetNamespaces).Debug("namespace change detected")
@@ -116,25 +113,15 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 			return err
 		}
 
-		// Remove labels from targetNamespaces that were dropped.
-		ogLabel := getOperatorGroupLabel(*op)
-		for _, namespaceName := range setDifference(prunedStatusNamespaces, prunedTargetNamespaces) {
-			err = a.removeNamespaceLabel(namespaceName, ogLabel)
-			if err != nil {
-				logger.WithError(err).Warnf("failed to remove operatorgroup label from namespace %s", namespaceName)
-				return err
-			}
-		}
+		logger.Debug("operatorgroup status updated")
 
-		// Add labels to targetNamespaces that were added.
-		for _, namespaceName := range setDifference(prunedTargetNamespaces, prunedStatusNamespaces) {
-			err = a.addNamespaceLabel(namespaceName, ogLabel, "")
-			if err != nil {
-				logger.WithError(err).Warnf("failed to add operatorgroup to from namespace %s", namespaceName)
-				return err
-			}
+		// Requeueing out of sync namespaces
+		logger.Debug("Requeueing out of sync namespaces")
+		for _, ns := range outOfSyncNamespaces {
+			logger.WithField("namespace", ns).Debug("requeueing")
+			a.nsQueueSet.Add(ns)
 		}
-		logger.Debug("namespace change detected and operatorgroup status updated")
+
 		// CSV requeue is handled by the succeeding sync in `annotateCSVs`
 		return nil
 	}
@@ -185,43 +172,6 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 	return nil
 }
 
-// pruneEmptyStrings removes any items from a list that are empty
-func pruneEmptyStrings(strings []string) []string {
-	prunedStrings := []string{}
-	for _, str := range strings {
-		if str != "" {
-			prunedStrings = append(prunedStrings, str)
-		}
-	}
-	return prunedStrings
-}
-
-// setDifference implements Set Difference: A - B
-// https://en.wikipedia.org/wiki/Complement_(set_theory)#In_programming_languages
-func setDifference(a, b []string) (diff []string) {
-	if len(a) == 0 {
-		return diff
-	}
-
-	if len(b) == 0 {
-		return a
-	}
-
-	// Find the Set Difference.
-	m := make(map[string]bool)
-
-	for _, item := range b {
-		m[item] = true
-	}
-
-	for _, item := range a {
-		if _, ok := m[item]; !ok {
-			diff = append(diff, item)
-		}
-	}
-	return diff
-}
-
 func (a *Operator) operatorGroupDeleted(obj interface{}) {
 	op, ok := obj.(*v1.OperatorGroup)
 	if !ok {
@@ -246,52 +196,12 @@ func (a *Operator) operatorGroupDeleted(obj interface{}) {
 		}
 	}
 
-	// Remove OperatorGroup label from targeNamespaces.
-	ogLabel := getOperatorGroupLabel(*op)
-	for _, namespaceName := range op.Spec.TargetNamespaces {
-		a.removeNamespaceLabel(namespaceName, ogLabel)
-		if err != nil {
-			logger.WithError(err).Error("failed to remove OperatorGroup Label from Namespace during garbage collection")
-		}
-	}
-}
-
-func (a *Operator) updateNamespace(namespaceName string, namespaceUpdateFunc func(*corev1.Namespace)) error {
-	namespace, err := a.opClient.KubernetesInterface().CoreV1().Namespaces().Get(namespaceName, metav1.GetOptions{})
-	if err != nil {
-		return err
-	}
-
-	namespaceUpdateFunc(namespace)
-
-	_, err = a.opClient.KubernetesInterface().CoreV1().Namespaces().Update(namespace)
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func (a *Operator) removeNamespaceLabel(namespaceName, key string) error {
-	namespaceUpdateFunc := func(namespace *corev1.Namespace) {
-		delete(namespace.Labels, key)
-	}
-	return a.updateNamespace(namespaceName, namespaceUpdateFunc)
-}
-
-func (a *Operator) addNamespaceLabel(namespaceName string, key, value string) error {
-	namespaceUpdateFunc := func(namespace *corev1.Namespace) {
-		if namespace.Labels == nil {
-			namespace.Labels = make(map[string]string, 1)
-		}
-		namespace.Labels[key] = value
+	// Trigger a sync on namespaces
+	logger.Debug("OperatorGroup deleted, requeueing out of sync namespaces")
+	for _, ns := range op.Status.Namespaces {
+		logger.WithField("namespace", ns).Debug("requeueing")
+		a.nsQueueSet.Add(ns)
 	}
-	return a.updateNamespace(namespaceName, namespaceUpdateFunc)
-}
-
-// getOperatorGroupLabel returns a label that is applied to Namespaces to signify that the
-// namespace is a part of the OperatorGroup using selectors.
-func getOperatorGroupLabel(og v1.OperatorGroup) string {
-	return fmt.Sprintf(operatorGroupLabelTemplate, og.GetNamespace(), og.GetName())
 }
 
 func (a *Operator) annotateCSVs(group *v1.OperatorGroup, targetNamespaces []string, logger *logrus.Entry) error {
@@ -1075,3 +985,32 @@ func (a *Operator) findCSVsThatProvideAnyOf(provide resolver.APISet) ([]*v1alpha
 
 	return providers, nil
 }
+
+// namespacesAddedOrRemoved returns the union of:
+// - the set of elements in A but not in B
+// - the set of elements in B but not in A
+func namespacesAddedOrRemoved(a, b []string) []string {
+	check := make(map[string]struct{})
+
+	for _, namespace := range a {
+		check[namespace] = struct{}{}
+	}
+
+	for _, namespace := range b {
+		if _, ok := check[namespace]; !ok {
+			check[namespace] = struct{}{}
+		} else {
+			delete(check, namespace)
+		}
+	}
+
+	// Remove global namespace name if added
+	delete(check, "")
+
+	var keys []string
+	for key := range check {
+		keys = append(keys, key)
+	}
+
+	return keys
+}