cvo: refactor always enabled caps processing

Refactor the code to be consistent with how the other options are processed. The `NewControllerContext` is a method on `Options` struct, so it does not make sense to pass it some options through the struct, and some options through method parameters. I went further and separated the validation (performed with all other validation) from conversion, resulting in two smaller methods instead one larger one. The result is slightly less efficient but more readable.

Author: petr-muller

pkg/start/start.go

@@ -19,6 +19,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
@@ -159,6 +160,10 @@ func (o *Options) ValidateAndComplete() error {
 		o.PromQLTarget.URL = parsed
 	}
 
+	if err := validateCapabilities(o.AlwaysEnableCapabilities); err != nil {
+		return fmt.Errorf("--always-enable-capabilities: %w", err)
+	}
+
 	return nil
 }
 
@@ -169,10 +174,6 @@ func (o *Options) Run(ctx context.Context) error {
 	if len(o.Exclude) > 0 {
 		klog.Infof("Excluding manifests for %q", o.Exclude)
 	}
-	alwaysEnableCaps, unknownCaps := parseAlwaysEnableCapabilities(o.AlwaysEnableCapabilities)
-	if len(unknownCaps) > 0 {
-		return fmt.Errorf("--always-enable-capabilities was set with unknown capabilities: %v", unknownCaps)
-	}
 
 	// Inject the cluster ID into PromQL queries in HyperShift
 	o.InjectClusterIdIntoPromQL = o.HyperShift
@@ -189,7 +190,7 @@ func (o *Options) Run(ctx context.Context) error {
 	}
 
 	// initialize the controllers and attempt to load the payload information
-	controllerCtx, err := o.NewControllerContext(cb, alwaysEnableCaps)
+	controllerCtx, err := o.NewControllerContext(cb)
 	if err != nil {
 		return err
 	}
@@ -472,9 +473,17 @@ type Context struct {
 	fgLister configlistersv1.FeatureGateLister
 }
 
+func stringsToCapabilities(caps []string) []configv1.ClusterVersionCapability {
+	alwaysEnableCapabilities := make([]configv1.ClusterVersionCapability, 0, len(caps))
+	for _, c := range caps {
+		alwaysEnableCapabilities = append(alwaysEnableCapabilities, configv1.ClusterVersionCapability(c))
+	}
+	return alwaysEnableCapabilities
+}
+
 // NewControllerContext initializes the default Context for the current Options. It does
 // not start any background processes.
-func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabilities []configv1.ClusterVersionCapability) (*Context, error) {
+func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) {
 	client := cb.ClientOrDie("shared-informer")
 	kubeClient := cb.KubeClientOrDie(internal.ConfigNamespace, useProtobuf)
 	operatorClient := cb.OperatorClientOrDie("operator-client")
@@ -494,6 +503,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti
 
 	cvoKubeClient := cb.KubeClientOrDie(o.Namespace, useProtobuf)
 	o.PromQLTarget.KubeClient = cvoKubeClient
+
 	cvo, err := cvo.New(
 		o.NodeName,
 		o.Namespace, o.Name,
@@ -515,7 +525,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti
 		o.PromQLTarget,
 		o.InjectClusterIdIntoPromQL,
 		o.UpdateService,
-		alwaysEnableCapabilities,
+		stringsToCapabilities(o.AlwaysEnableCapabilities),
 	)
 	if err != nil {
 		return nil, err
@@ -622,25 +632,14 @@ func (c *Context) InitializeFromPayload(ctx context.Context, restConfig *rest.Co
 	return nil
 }
 
-// parseAlwaysEnableCapabilities parses the string list of capabilities
-// into two lists of configv1.ClusterVersionCapability: known and unknown.
-func parseAlwaysEnableCapabilities(caps []string) ([]configv1.ClusterVersionCapability, []configv1.ClusterVersionCapability) {
-	var (
-		knownCaps   []configv1.ClusterVersionCapability
-		unknownCaps []configv1.ClusterVersionCapability
-	)
-	for _, c := range caps {
-		known := false
-		for _, kc := range configv1.KnownClusterVersionCapabilities {
-			if configv1.ClusterVersionCapability(c) == kc {
-				knownCaps = append(knownCaps, kc)
-				known = true
-				break
-			}
-		}
-		if !known {
-			unknownCaps = append(unknownCaps, configv1.ClusterVersionCapability(c))
-		}
+func validateCapabilities(caps []string) error {
+	unknown := sets.New(caps...)
+	for _, kc := range configv1.KnownClusterVersionCapabilities {
+		unknown.Delete(string(kc))
+	}
+
+	if len(unknown) > 0 {
+		return fmt.Errorf("unknown capabilities: %s", sets.List(unknown))
 	}
-	return knownCaps, unknownCaps
+	return nil
 }

pkg/start/start_integration_test.go

@@ -184,15 +184,13 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) {
 	options.ReleaseImage = payloadImage1
 	options.PayloadOverride = filepath.Join(dir, "0.0.1")
 	options.leaderElection = getLeaderElectionConfig(ctx, cfg)
-	alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
-		configv1.ClusterVersionCapabilityIngress,
-	}
-	controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
+	options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
+	controllers, err := options.NewControllerContext(cb)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
+	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
 	controllers.CVO.SetSyncWorkerForTesting(worker)
 
 	lock, err := createResourceLock(cb, options.Namespace, options.Name)
@@ -318,15 +316,13 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) {
 	options.ReleaseImage = payloadImage1
 	options.PayloadOverride = filepath.Join(dir, "0.0.1")
 	options.leaderElection = getLeaderElectionConfig(ctx, cfg)
-	alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
-		configv1.ClusterVersionCapabilityIngress,
-	}
-	controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
+	options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
+	controllers, err := options.NewControllerContext(cb)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
+	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
 	controllers.CVO.SetSyncWorkerForTesting(worker)
 
 	lock, err := createResourceLock(cb, options.Namespace, options.Name)
@@ -514,15 +510,13 @@ metadata:
 	options.ReleaseImage = payloadImage1
 	options.PayloadOverride = payloadDir
 	options.leaderElection = getLeaderElectionConfig(ctx, cfg)
-	alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
-		configv1.ClusterVersionCapabilityIngress,
-	}
-	controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
+	options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
+	controllers, err := options.NewControllerContext(cb)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
+	worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
 	controllers.CVO.SetSyncWorkerForTesting(worker)
 
 	lock, err := createResourceLock(cb, options.Namespace, options.Name)