Introduce protectedCopiedCSVNamespaces flag

awgreene · awgreene · commit 2da2b81d8b0b · 2022-07-15T13:35:44.000-06:00
Problem: Users rely on Copied CSVs in order to understand which
operators are available in a given namespace. When installing All
Namespace operators, a Copied CSV is created in every namespace which
can place a huge performance strain on clusters with many namespaces.
OLM introduced the ability to disable Copied CSVs for All Namespace mode
operators  in an effort to resolve the performance issues on large clusters,
unfortunately removing the ability for users to identify which operators are
available in a given namespace.

Solution: The protectedCopiedCSVNamespaces runtime flag can be used to
prevent Copied CSVs from being deleted even when Copied CSVs are
disabled. An admin can then provide users with the proper RBAC to view
which operators are running in All Namespace mode.

Signed-off-by: Alexander Greene &lt;greene.al1991@gmail.com&gt;
diff --git a/cmd/olm/main.go b/cmd/olm/main.go
@@ -60,6 +60,9 @@ var (
 	tlsKeyPath = pflag.String(
 		"tls-key", "", "Path to use for private key (requires tls-cert)")
 
+	protectedCopiedCSVNamespaces = pflag.String("protectedCopiedCSVNamespaces",
+		"", "A comma-delimited set of namespaces where global Copied CSVs will always appear, even if Copied CSVs are disabled")
+
 	tlsCertPath = pflag.String(
 		"tls-cert", "", "Path to use for certificate key (requires tls-key)")
 
@@ -162,6 +165,7 @@ func main() {
 		olm.WithOperatorClient(opClient),
 		olm.WithRestConfig(config),
 		olm.WithConfigClient(versionedConfigClient),
+		olm.WithProtectedCopiedCSVNamespaces(protectedCopiedCSVNamespaces),
 	)
 	if err != nil {
 		logger.WithError(err).Fatal("error configuring operator")
diff --git a/pkg/controller/operators/olm/config.go b/pkg/controller/operators/olm/config.go
@@ -1,6 +1,7 @@
 package olm
 
 import (
+	"strings"
 	"time"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer"
@@ -21,18 +22,19 @@ import (
 type OperatorOption func(*operatorConfig)
 
 type operatorConfig struct {
-	resyncPeriod      func() time.Duration
-	operatorNamespace string
-	watchedNamespaces []string
-	clock             utilclock.Clock
-	logger            *logrus.Logger
-	operatorClient    operatorclient.ClientInterface
-	externalClient    versioned.Interface
-	strategyResolver  install.StrategyResolverInterface
-	apiReconciler     APIIntersectionReconciler
-	apiLabeler        labeler.Labeler
-	restConfig        *rest.Config
-	configClient      configv1client.Interface
+	protectedCopiedCSVNamespaces map[string]struct{}
+	resyncPeriod                 func() time.Duration
+	operatorNamespace            string
+	watchedNamespaces            []string
+	clock                        utilclock.Clock
+	logger                       *logrus.Logger
+	operatorClient               operatorclient.ClientInterface
+	externalClient               versioned.Interface
+	strategyResolver             install.StrategyResolverInterface
+	apiReconciler                APIIntersectionReconciler
+	apiLabeler                   labeler.Labeler
+	restConfig                   *rest.Config
+	configClient                 configv1client.Interface
 }
 
 func (o *operatorConfig) apply(options []OperatorOption) {
@@ -112,6 +114,19 @@ func WithLogger(logger *logrus.Logger) OperatorOption {
 	}
 }
 
+func WithProtectedCopiedCSVNamespaces(namespaces *string) OperatorOption {
+	return func(config *operatorConfig) {
+		config.protectedCopiedCSVNamespaces = map[string]struct{}{}
+		if namespaces == nil {
+			return
+		}
+
+		for _, ns := range strings.Split(*namespaces, ",") {
+			config.protectedCopiedCSVNamespaces[ns] = struct{}{}
+		}
+	}
+}
+
 func WithClock(clock utilclock.Clock) OperatorOption {
 	return func(config *operatorConfig) {
 		config.clock = clock
diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go
@@ -63,32 +63,33 @@ var (
 type Operator struct {
 	queueinformer.Operator
 
-	clock                 utilclock.Clock
-	logger                *logrus.Logger
-	opClient              operatorclient.ClientInterface
-	client                versioned.Interface
-	lister                operatorlister.OperatorLister
-	copiedCSVLister       operatorsv1alpha1listers.ClusterServiceVersionLister
-	ogQueueSet            *queueinformer.ResourceQueueSet
-	csvQueueSet           *queueinformer.ResourceQueueSet
-	olmConfigQueue        workqueue.RateLimitingInterface
-	csvCopyQueueSet       *queueinformer.ResourceQueueSet
-	copiedCSVGCQueueSet   *queueinformer.ResourceQueueSet
-	objGCQueueSet         *queueinformer.ResourceQueueSet
-	nsQueueSet            workqueue.RateLimitingInterface
-	apiServiceQueue       workqueue.RateLimitingInterface
-	csvIndexers           map[string]cache.Indexer
-	recorder              record.EventRecorder
-	resolver              install.StrategyResolverInterface
-	apiReconciler         APIIntersectionReconciler
-	apiLabeler            labeler.Labeler
-	csvSetGenerator       csvutility.SetGenerator
-	csvReplaceFinder      csvutility.ReplaceFinder
-	csvNotification       csvutility.WatchNotification
-	serviceAccountSyncer  *scoped.UserDefinedServiceAccountSyncer
-	clientAttenuator      *scoped.ClientAttenuator
-	serviceAccountQuerier *scoped.UserDefinedServiceAccountQuerier
-	clientFactory         clients.Factory
+	clock                        utilclock.Clock
+	logger                       *logrus.Logger
+	opClient                     operatorclient.ClientInterface
+	client                       versioned.Interface
+	lister                       operatorlister.OperatorLister
+	protectedCopiedCSVNamespaces map[string]struct{}
+	copiedCSVLister              operatorsv1alpha1listers.ClusterServiceVersionLister
+	ogQueueSet                   *queueinformer.ResourceQueueSet
+	csvQueueSet                  *queueinformer.ResourceQueueSet
+	olmConfigQueue               workqueue.RateLimitingInterface
+	csvCopyQueueSet              *queueinformer.ResourceQueueSet
+	copiedCSVGCQueueSet          *queueinformer.ResourceQueueSet
+	objGCQueueSet                *queueinformer.ResourceQueueSet
+	nsQueueSet                   workqueue.RateLimitingInterface
+	apiServiceQueue              workqueue.RateLimitingInterface
+	csvIndexers                  map[string]cache.Indexer
+	recorder                     record.EventRecorder
+	resolver                     install.StrategyResolverInterface
+	apiReconciler                APIIntersectionReconciler
+	apiLabeler                   labeler.Labeler
+	csvSetGenerator              csvutility.SetGenerator
+	csvReplaceFinder             csvutility.ReplaceFinder
+	csvNotification              csvutility.WatchNotification
+	serviceAccountSyncer         *scoped.UserDefinedServiceAccountSyncer
+	clientAttenuator             *scoped.ClientAttenuator
+	serviceAccountQuerier        *scoped.UserDefinedServiceAccountQuerier
+	clientFactory                clients.Factory
 }
 
 func NewOperator(ctx context.Context, options ...OperatorOption) (*Operator, error) {
@@ -121,30 +122,31 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 	}
 
 	op := &Operator{
-		Operator:              queueOperator,
-		clock:                 config.clock,
-		logger:                config.logger,
-		opClient:              config.operatorClient,
-		client:                config.externalClient,
-		ogQueueSet:            queueinformer.NewEmptyResourceQueueSet(),
-		csvQueueSet:           queueinformer.NewEmptyResourceQueueSet(),
-		olmConfigQueue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "olmConfig"),
-		csvCopyQueueSet:       queueinformer.NewEmptyResourceQueueSet(),
-		copiedCSVGCQueueSet:   queueinformer.NewEmptyResourceQueueSet(),
-		objGCQueueSet:         queueinformer.NewEmptyResourceQueueSet(),
-		apiServiceQueue:       workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "apiservice"),
-		resolver:              config.strategyResolver,
-		apiReconciler:         config.apiReconciler,
-		lister:                lister,
-		recorder:              eventRecorder,
-		apiLabeler:            config.apiLabeler,
-		csvIndexers:           map[string]cache.Indexer{},
-		csvSetGenerator:       csvutility.NewSetGenerator(config.logger, lister),
-		csvReplaceFinder:      csvutility.NewReplaceFinder(config.logger, config.externalClient),
-		serviceAccountSyncer:  scoped.NewUserDefinedServiceAccountSyncer(config.logger, scheme, config.operatorClient, config.externalClient),
-		clientAttenuator:      scoped.NewClientAttenuator(config.logger, config.restConfig, config.operatorClient),
-		serviceAccountQuerier: scoped.NewUserDefinedServiceAccountQuerier(config.logger, config.externalClient),
-		clientFactory:         clients.NewFactory(config.restConfig),
+		Operator:                     queueOperator,
+		clock:                        config.clock,
+		logger:                       config.logger,
+		opClient:                     config.operatorClient,
+		client:                       config.externalClient,
+		ogQueueSet:                   queueinformer.NewEmptyResourceQueueSet(),
+		csvQueueSet:                  queueinformer.NewEmptyResourceQueueSet(),
+		olmConfigQueue:               workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "olmConfig"),
+		csvCopyQueueSet:              queueinformer.NewEmptyResourceQueueSet(),
+		copiedCSVGCQueueSet:          queueinformer.NewEmptyResourceQueueSet(),
+		objGCQueueSet:                queueinformer.NewEmptyResourceQueueSet(),
+		apiServiceQueue:              workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "apiservice"),
+		resolver:                     config.strategyResolver,
+		apiReconciler:                config.apiReconciler,
+		lister:                       lister,
+		recorder:                     eventRecorder,
+		apiLabeler:                   config.apiLabeler,
+		csvIndexers:                  map[string]cache.Indexer{},
+		csvSetGenerator:              csvutility.NewSetGenerator(config.logger, lister),
+		csvReplaceFinder:             csvutility.NewReplaceFinder(config.logger, config.externalClient),
+		serviceAccountSyncer:         scoped.NewUserDefinedServiceAccountSyncer(config.logger, scheme, config.operatorClient, config.externalClient),
+		clientAttenuator:             scoped.NewClientAttenuator(config.logger, config.restConfig, config.operatorClient),
+		serviceAccountQuerier:        scoped.NewUserDefinedServiceAccountQuerier(config.logger, config.externalClient),
+		clientFactory:                clients.NewFactory(config.restConfig),
+		protectedCopiedCSVNamespaces: config.protectedCopiedCSVNamespaces,
 	}
 
 	// Set up syncing for namespace-scoped resources
@@ -1277,20 +1279,29 @@ func (a *Operator) syncOLMConfig(obj interface{}) (syncError error) {
 			return err
 		}
 
-		// Filter to unique copies
-		uniqueCopiedCSVs := map[string]struct{}{}
+		copiedCSVCount := map[string]int{}
 		for _, copiedCSV := range copiedCSVs {
-			uniqueCopiedCSVs[copiedCSV.GetName()] = struct{}{}
+			copiedCSVCount[copiedCSV.GetName()] = copiedCSVCount[copiedCSV.GetName()] + 1
 		}
 
 		csvs, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(og.GetNamespace()).List(labels.NewSelector().Add(*nonCopiedCSVRequirement))
 		if err != nil {
 			return err
 		}
 
+		namespaces, err := a.lister.CoreV1().NamespaceLister().List(labels.Everything())
+		if err != nil {
+			return err
+		}
+
 		for _, csv := range csvs {
-			// If the correct number of copied CSVs were found, continue
-			if _, ok := uniqueCopiedCSVs[csv.GetName()]; ok == olmConfig.CopiedCSVsAreEnabled() {
+			numberOfCopiedCSVs := copiedCSVCount[csv.GetName()]
+			// Ignore NS where actual CSV is installed
+			if olmConfig.CopiedCSVsAreEnabled() && numberOfCopiedCSVs == len(namespaces)-1 {
+				continue
+			}
+
+			if !olmConfig.CopiedCSVsAreEnabled() && numberOfCopiedCSVs == len(a.protectedCopiedCSVNamespaces) {
 				continue
 			}
 
@@ -1302,7 +1313,7 @@ func (a *Operator) syncOLMConfig(obj interface{}) (syncError error) {
 	}
 
 	// Update the olmConfig status if it has changed.
-	condition := getCopiedCSVsCondition(!olmConfig.CopiedCSVsAreEnabled(), csvIsRequeued)
+	condition := getCopiedCSVsCondition(olmConfig.CopiedCSVsAreEnabled(), csvIsRequeued)
 	if !isStatusConditionPresentAndAreTypeReasonMessageStatusEqual(olmConfig.Status.Conditions, condition) {
 		meta.SetStatusCondition(&olmConfig.Status.Conditions, condition)
 		if _, err := a.client.OperatorsV1().OLMConfigs().UpdateStatus(context.TODO(), olmConfig, metav1.UpdateOptions{}); err != nil {
@@ -1324,13 +1335,13 @@ func isStatusConditionPresentAndAreTypeReasonMessageStatusEqual(conditions []met
 		foundCondition.Status == condition.Status
 }
 
-func getCopiedCSVsCondition(isDisabled, csvIsRequeued bool) metav1.Condition {
+func getCopiedCSVsCondition(enabled, csvIsRequeued bool) metav1.Condition {
 	condition := metav1.Condition{
 		Type:               operatorsv1.DisabledCopiedCSVsConditionType,
 		LastTransitionTime: metav1.Now(),
 		Status:             metav1.ConditionFalse,
 	}
-	if !isDisabled {
+	if enabled {
 		condition.Reason = "CopiedCSVsEnabled"
 		condition.Message = "Copied CSVs are enabled and present across the cluster"
 		if csvIsRequeued {
@@ -1339,15 +1350,14 @@ func getCopiedCSVsCondition(isDisabled, csvIsRequeued bool) metav1.Condition {
 		return condition
 	}
 
+	condition.Reason = "CopiedCSVsDisabled"
 	if csvIsRequeued {
-		condition.Reason = "CopiedCSVsFound"
-		condition.Message = "Copied CSVs are disabled and at least one copied CSV was found for an operator installed in AllNamespace mode"
+		condition.Message = "Copied CSVs are disabled and at least one unexpected copied CSV was found for an operator installed in AllNamespace mode"
 		return condition
 	}
 
 	condition.Status = metav1.ConditionTrue
-	condition.Reason = "NoCopiedCSVsFound"
-	condition.Message = "Copied CSVs are disabled and none were found for operators installed in AllNamespace mode"
+	condition.Message = "Copied CSVs are disabled and no unexpected copied CSVs were found for operators installed in AllNamespace mode"
 
 	return condition
 }
@@ -1422,7 +1432,25 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
 		return err
 	}
 
+	// Ensure that the Copied CSVs exist in the protected namespaces.
+	protectedNamespaces := []string{}
+	for ns := range a.protectedCopiedCSVNamespaces {
+		if ns == clusterServiceVersion.GetNamespace() {
+			continue
+		}
+		protectedNamespaces = append(protectedNamespaces, ns)
+	}
+
+	if err := a.ensureCSVsInNamespaces(clusterServiceVersion, operatorGroup, NewNamespaceSet(protectedNamespaces)); err != nil {
+		logger.WithError(err).Info("couldn't copy CSV to protected Copied CSV namespaces")
+		syncError = err
+	}
+
+	// Delete Copied CSVs in namespaces that are not protected.
 	for _, copiedCSV := range copiedCSVs {
+		if _, ok := a.protectedCopiedCSVNamespaces[copiedCSV.Namespace]; ok {
+			continue
+		}
 		err := a.client.OperatorsV1alpha1().ClusterServiceVersions(copiedCSV.Namespace).Delete(context.TODO(), copiedCSV.Name, metav1.DeleteOptions{})
 		if err != nil && !apierrors.IsNotFound(err) {
 			return err
diff --git a/test/e2e/disabling_copied_csv_e2e_test.go b/test/e2e/disabling_copied_csv_e2e_test.go
