diff --git a/cmd/cluster-version-operator/start.go b/cmd/cluster-version-operator/start.go index f8cf70236..cfc73bc32 100644 --- a/cmd/cluster-version-operator/start.go +++ b/cmd/cluster-version-operator/start.go @@ -2,6 +2,7 @@ package main import ( "context" + "github.com/spf13/cobra" "k8s.io/klog/v2" @@ -15,10 +16,12 @@ func init() { Use: "start", Short: "Starts Cluster Version Operator", Long: "", - Run: func(cmd *cobra.Command, args []string) { + PreRunE: func(_ *cobra.Command, _ []string) error { // To help debugging, immediately log version klog.Info(version.String) - + return opts.ValidateAndComplete() + }, + Run: func(_ *cobra.Command, _ []string) { if err := opts.Run(context.Background()); err != nil { klog.Fatalf("error: %v", err) } diff --git a/pkg/start/start.go b/pkg/start/start.go index daee3c77f..6fb92c684 100644 --- a/pkg/start/start.go +++ b/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" @@ -132,7 +133,7 @@ func NewOptions() *Options { } } -func (o *Options) Run(ctx context.Context) error { +func (o *Options) ValidateAndComplete() error { if o.NodeName == "" { return fmt.Errorf("node-name is required") } @@ -152,25 +153,29 @@ func (o *Options) Run(ctx context.Context) error { (o.PromQLTarget.KubeSvc.Namespace == "" || o.PromQLTarget.KubeSvc.Name == "") { return fmt.Errorf("--use-dns-for-services is disabled, so --metrics-service and --metrics-namespace must be set") } - if len(o.PayloadOverride) > 0 { - klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride) - } - 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) + + if parsed, err := url.Parse(o.PrometheusURLString); err != nil { + return fmt.Errorf("error parsing promql url: %v", err) + } else { + o.PromQLTarget.URL = parsed } // Inject the cluster ID into PromQL queries in HyperShift o.InjectClusterIdIntoPromQL = o.HyperShift - // parse the prometheus url - var err error - o.PromQLTarget.URL, err = url.Parse(o.PrometheusURLString) - if err != nil { - return fmt.Errorf("error parsing promql url: %v", err) + if err := validateCapabilities(o.AlwaysEnableCapabilities); err != nil { + return fmt.Errorf("--always-enable-capabilities: %w", err) + } + + return nil +} + +func (o *Options) Run(ctx context.Context) error { + if len(o.PayloadOverride) > 0 { + klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride) + } + if len(o.Exclude) > 0 { + klog.Infof("Excluding manifests for %q", o.Exclude) } // initialize the core objects @@ -185,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 } @@ -470,7 +475,7 @@ type Context struct { // 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") @@ -490,6 +495,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, @@ -511,7 +517,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti o.PromQLTarget, o.InjectClusterIdIntoPromQL, o.UpdateService, - alwaysEnableCapabilities, + stringsToCapabilities(o.AlwaysEnableCapabilities), ) if err != nil { return nil, err @@ -618,25 +624,22 @@ 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 stringsToCapabilities(names []string) []configv1.ClusterVersionCapability { + caps := make([]configv1.ClusterVersionCapability, len(names)) + for i, c := range names { + caps[i] = configv1.ClusterVersionCapability(c) + } + return caps +} + +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 } diff --git a/pkg/start/start_integration_test.go b/pkg/start/start_integration_test.go index 86704f1a8..d3f063d24 100644 --- a/pkg/start/start_integration_test.go +++ b/pkg/start/start_integration_test.go @@ -184,15 +184,16 @@ 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, + options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)} + if err := options.ValidateAndComplete(); err != nil { + t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities) + 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 +319,16 @@ 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, + options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)} + if err := options.ValidateAndComplete(); err != nil { + t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities) + 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 +516,16 @@ metadata: options.ReleaseImage = payloadImage1 options.PayloadOverride = payloadDir options.leaderElection = getLeaderElectionConfig(ctx, cfg) - alwaysEnableCapabilities := []configv1.ClusterVersionCapability{ - configv1.ClusterVersionCapabilityIngress, + options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)} + if err := options.ValidateAndComplete(); err != nil { + t.Fatalf("incorrectly initialized options: %v", err) } - controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities) + 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)