diff --git a/staging/src/k8s.io/controller-manager/options/generic.go b/staging/src/k8s.io/controller-manager/options/generic.go index 45c086b11f2..35b7cc2322d 100644 --- a/staging/src/k8s.io/controller-manager/options/generic.go +++ b/staging/src/k8s.io/controller-manager/options/generic.go @@ -49,7 +49,7 @@ func NewGenericControllerManagerConfigurationOptions(cfg *cmconfig.GenericContro } // AddFlags adds flags related to generic for controller manager to the specified FlagSet. -func (o *GenericControllerManagerConfigurationOptions) AddFlags(fss *cliflag.NamedFlagSets, allControllers, disabledByDefaultControllers []string) { +func (o *GenericControllerManagerConfigurationOptions) AddFlags(fss *cliflag.NamedFlagSets, allControllers, disabledByDefaultControllers []string, controllerAliases map[string]string) { if o == nil { return } @@ -71,7 +71,7 @@ func (o *GenericControllerManagerConfigurationOptions) AddFlags(fss *cliflag.Nam } // ApplyTo fills up generic config with options. -func (o *GenericControllerManagerConfigurationOptions) ApplyTo(cfg *cmconfig.GenericControllerManagerConfiguration) error { +func (o *GenericControllerManagerConfigurationOptions) ApplyTo(cfg *cmconfig.GenericControllerManagerConfiguration, allControllers []string, disabledByDefaultControllers []string, controllerAliases map[string]string) error { if o == nil { return nil } @@ -88,13 +88,26 @@ func (o *GenericControllerManagerConfigurationOptions) ApplyTo(cfg *cmconfig.Gen cfg.ClientConnection = o.ClientConnection cfg.ControllerStartInterval = o.ControllerStartInterval cfg.LeaderElection = o.LeaderElection - cfg.Controllers = o.Controllers + + // copy controller names and replace aliases with canonical names + cfg.Controllers = make([]string, len(o.Controllers)) + for i, initialName := range o.Controllers { + initialNameWithoutPrefix := strings.TrimPrefix(initialName, "-") + controllerName := initialNameWithoutPrefix + if canonicalName, ok := controllerAliases[controllerName]; ok { + controllerName = canonicalName + } + if strings.HasPrefix(initialName, "-") { + controllerName = fmt.Sprintf("-%s", controllerName) + } + cfg.Controllers[i] = controllerName + } return nil } // Validate checks validation of GenericOptions. -func (o *GenericControllerManagerConfigurationOptions) Validate(allControllers []string, disabledByDefaultControllers []string) []error { +func (o *GenericControllerManagerConfigurationOptions) Validate(allControllers []string, disabledByDefaultControllers []string, controllerAliases map[string]string) []error { if o == nil { return nil } @@ -109,13 +122,17 @@ func (o *GenericControllerManagerConfigurationOptions) Validate(allControllers [ } allControllersSet := sets.NewString(allControllers...) - for _, controller := range o.Controllers { - if controller == "*" { + for _, initialName := range o.Controllers { + if initialName == "*" { continue } - controller = strings.TrimPrefix(controller, "-") - if !allControllersSet.Has(controller) { - errs = append(errs, fmt.Errorf("%q is not in the list of known controllers", controller)) + initialNameWithoutPrefix := strings.TrimPrefix(initialName, "-") + controllerName := initialNameWithoutPrefix + if canonicalName, ok := controllerAliases[controllerName]; ok { + controllerName = canonicalName + } + if !allControllersSet.Has(controllerName) { + errs = append(errs, fmt.Errorf("%q is not in the list of known controllers", initialNameWithoutPrefix)) } } diff --git a/staging/src/k8s.io/controller-manager/options/generic_test.go b/staging/src/k8s.io/controller-manager/options/generic_test.go new file mode 100644 index 00000000000..d41cc217586 --- /dev/null +++ b/staging/src/k8s.io/controller-manager/options/generic_test.go @@ -0,0 +1,260 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package options + +import ( + "reflect" + "strings" + "testing" + + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/component-base/config" + cmconfig "k8s.io/controller-manager/config" +) + +func TestValidateGenericControllerManagerConfigurationOptions(t *testing.T) { + testCases := []struct { + name string + allControllers []string + controllerAliases map[string]string + options *GenericControllerManagerConfigurationOptions + expectErrors bool + expectedErrorSubString string + }{ + { + name: "no controllers defined", + allControllers: nil, + controllerAliases: nil, + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "*", + }, + }), + }, + { + name: "recognizes empty controllers", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{}), + }, + { + name: "recognizes controllers without any aliases", + allControllers: getAllControllers(), + controllerAliases: nil, + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "blue-controller", + }, + }), + }, + { + name: "recognizes valid controllers", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "*", + "-red-controller", + "blue-controller", + }, + }), + }, + { + name: "recognizes disabled controller", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "green-controller", + }, + }), + }, + { + name: "recognized aliased controller", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "ultramarine-controller", + "-pink-controller", + }, + }), + }, + { + name: "does not recognize controller", + allControllers: nil, + controllerAliases: nil, + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "red-controller", + }, + }), + expectErrors: true, + expectedErrorSubString: "\"red-controller\" is not in the list of known controllers", + }, + { + name: "does not recognize controller with aliases", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "crimson-controller", + "grey-controller", + }, + }), + expectErrors: true, + expectedErrorSubString: "\"grey-controller\" is not in the list of known controllers", + }, + { + name: "leader election accepts only leases", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + LeaderElection: config.LeaderElectionConfiguration{ + LeaderElect: true, + ResourceLock: "configmapsleases", + }, + }), + expectErrors: true, + expectedErrorSubString: "resourceLock value must be \"leases\"", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + errs := tc.options.Validate(tc.allControllers, []string{"green-controller"}, tc.controllerAliases) + if len(errs) > 0 && !tc.expectErrors { + t.Errorf("expected no errors, errors found %+v", errs) + } + + if len(errs) == 0 && tc.expectErrors { + t.Errorf("expected errors, no errors found") + } + + if len(errs) > 0 && tc.expectErrors { + gotErr := utilerrors.NewAggregate(errs).Error() + if !strings.Contains(gotErr, tc.expectedErrorSubString) { + t.Errorf("expected error: %s, got err: %v", tc.expectedErrorSubString, gotErr) + } + } + }) + } +} + +func TestApplyToGenericControllerManagerConfigurationOptions(t *testing.T) { + testCases := []struct { + name string + allControllers []string + controllerAliases map[string]string + options *GenericControllerManagerConfigurationOptions + expectedControllers []string + }{ + { + name: "no controllers defined", + allControllers: nil, + controllerAliases: nil, + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "*", + }, + }), + expectedControllers: []string{ + "*", + }, + }, + { + name: "empty aliases", + allControllers: getAllControllers(), + controllerAliases: nil, + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "-blue-controller", + }, + }), + expectedControllers: []string{ + "-blue-controller", + }, + }, + { + name: "applies valid controllers", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "*", + "green-controller", + "-red-controller", + "blue-controller", + }, + }), + expectedControllers: []string{ + "*", + "green-controller", + "-red-controller", + "blue-controller", + }, + }, + { + name: "resolves aliases", + allControllers: getAllControllers(), + controllerAliases: getControllerAliases(), + options: NewGenericControllerManagerConfigurationOptions(&cmconfig.GenericControllerManagerConfiguration{ + Controllers: []string{ + "green-controller", + "-crimson-controller", + "ultramarine-controller", + "-pink-controller", + }, + }), + expectedControllers: []string{ + "green-controller", + "-red-controller", + "blue-controller", + "-red-controller", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cfg := &cmconfig.GenericControllerManagerConfiguration{} + err := tc.options.ApplyTo(cfg, tc.allControllers, []string{"green-controller"}, tc.controllerAliases) + if err != nil { + t.Errorf("expected no errors, error found: %v", err) + } + if !reflect.DeepEqual(cfg.Controllers, tc.expectedControllers) { + t.Errorf("applyTo failed, expected controllers %q, got controllers %q", strings.Join(cfg.Controllers, ","), strings.Join(tc.expectedControllers, ",")) + } + }) + } +} + +func getAllControllers() []string { + return []string{ + "red-controller", + "green-controller", + "blue-controller", + } +} + +func getControllerAliases() map[string]string { + return map[string]string{ + "crimson-controller": "red-controller", + "pink-controller": "red-controller", + "ultramarine-controller": "blue-controller", + } +} diff --git a/staging/src/k8s.io/controller-manager/pkg/leadermigration/config/default.go b/staging/src/k8s.io/controller-manager/pkg/leadermigration/config/default.go index 362893b4071..995f48ac4c6 100644 --- a/staging/src/k8s.io/controller-manager/pkg/leadermigration/config/default.go +++ b/staging/src/k8s.io/controller-manager/pkg/leadermigration/config/default.go @@ -26,13 +26,13 @@ func DefaultLeaderMigrationConfiguration() *internal.LeaderMigrationConfiguratio ResourceLock: ResourceLockLeases, ControllerLeaders: []internal.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "*", }, { - Name: "service", + Name: "service-controller", Component: "*", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "*", }, }, diff --git a/staging/src/k8s.io/controller-manager/pkg/leadermigration/migrator_test.go b/staging/src/k8s.io/controller-manager/pkg/leadermigration/migrator_test.go index ddeb6a0943d..e1b788862e5 100644 --- a/staging/src/k8s.io/controller-manager/pkg/leadermigration/migrator_test.go +++ b/staging/src/k8s.io/controller-manager/pkg/leadermigration/migrator_test.go @@ -28,13 +28,13 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { LeaderName: "cloud-provider-extraction-migration", ControllerLeaders: []internal.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "kube-controller-manager", }, { - Name: "service", + Name: "service-controller", Component: "kube-controller-manager", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "kube-controller-manager", }, }, @@ -44,13 +44,13 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { LeaderName: "cloud-provider-extraction-migration", ControllerLeaders: []internal.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "cloud-controller-manager", }, { - Name: "service", + Name: "service-controller", Component: "cloud-controller-manager", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "cloud-controller-manager", }, }, @@ -60,13 +60,13 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { LeaderName: "cloud-provider-extraction-migration", ControllerLeaders: []internal.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "*", }, { - Name: "service", + Name: "service-controller", Component: "*", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "*", }, }, @@ -83,10 +83,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: fromConfig, component: "kube-controller-manager", expectResult: map[string]FilterResult{ - "deployment": ControllerNonMigrated, - "route": ControllerMigrated, - "service": ControllerMigrated, - "cloud-node-lifecycle": ControllerMigrated, + "deployment-controller": ControllerNonMigrated, + "route-controller": ControllerMigrated, + "service-controller": ControllerMigrated, + "cloud-node-lifecycle-controller": ControllerMigrated, }, }, { @@ -94,10 +94,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: fromConfig, component: "cloud-controller-manager", expectResult: map[string]FilterResult{ - "cloud-node": ControllerNonMigrated, - "route": ControllerUnowned, - "service": ControllerUnowned, - "cloud-node-lifecycle": ControllerUnowned, + "cloud-node": ControllerNonMigrated, + "route-controller": ControllerUnowned, + "service-controller": ControllerUnowned, + "cloud-node-lifecycle-controller": ControllerUnowned, }, }, { @@ -105,10 +105,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: toConfig, component: "kube-controller-manager", expectResult: map[string]FilterResult{ - "deployment": ControllerNonMigrated, - "route": ControllerUnowned, - "service": ControllerUnowned, - "cloud-node-lifecycle": ControllerUnowned, + "deployment-controller": ControllerNonMigrated, + "route-controller": ControllerUnowned, + "service-controller": ControllerUnowned, + "cloud-node-lifecycle-controller": ControllerUnowned, }, }, { @@ -116,10 +116,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: toConfig, component: "cloud-controller-manager", expectResult: map[string]FilterResult{ - "cloud-node": ControllerNonMigrated, - "route": ControllerMigrated, - "service": ControllerMigrated, - "cloud-node-lifecycle": ControllerMigrated, + "cloud-node-controller": ControllerNonMigrated, + "route-controller": ControllerMigrated, + "service-controller": ControllerMigrated, + "cloud-node-lifecycle-controller": ControllerMigrated, }, }, { @@ -127,10 +127,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: wildcardConfig, component: "kube-controller-manager", expectResult: map[string]FilterResult{ - "deployment": ControllerNonMigrated, // KCM only - "route": ControllerMigrated, - "service": ControllerMigrated, - "cloud-node-lifecycle": ControllerMigrated, + "deployment-controller": ControllerNonMigrated, // KCM only + "route-controller": ControllerMigrated, + "service-controller": ControllerMigrated, + "cloud-node-lifecycle-controller": ControllerMigrated, }, }, { @@ -138,10 +138,10 @@ func TestLeaderMigratorFilterFunc(t *testing.T) { config: wildcardConfig, component: "cloud-controller-manager", expectResult: map[string]FilterResult{ - "cloud-node": ControllerNonMigrated, // CCM only - "route": ControllerMigrated, - "service": ControllerMigrated, - "cloud-node-lifecycle": ControllerMigrated, + "cloud-node-controller": ControllerNonMigrated, // CCM only + "route-controller": ControllerMigrated, + "service-controller": ControllerMigrated, + "cloud-node-lifecycle-controller": ControllerMigrated, }, }, } { diff --git a/staging/src/k8s.io/controller-manager/pkg/leadermigration/options/options_test.go b/staging/src/k8s.io/controller-manager/pkg/leadermigration/options/options_test.go index ebd57a5b402..596083c1ae6 100644 --- a/staging/src/k8s.io/controller-manager/pkg/leadermigration/options/options_test.go +++ b/staging/src/k8s.io/controller-manager/pkg/leadermigration/options/options_test.go @@ -107,13 +107,13 @@ apiVersion: controllermanager.config.k8s.io/v1 kind: LeaderMigrationConfiguration leaderName: test-leader-migration controllerLeaders: - - name: route + - name: route-controller component: "*" - - name: service + - name: service-controller component: "*" - - name: cloud-node-lifecycle + - name: cloud-node-lifecycle-controller component: "*" - - name: nodeipam + - name: node-ipam-controller component: "*" `, expectErr: false, @@ -122,19 +122,19 @@ controllerLeaders: ResourceLock: "leases", ControllerLeaders: []config.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "*", }, { - Name: "service", + Name: "service-controller", Component: "*", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "*", }, { - Name: "nodeipam", + Name: "node-ipam-controller", Component: "*", }, }, @@ -148,13 +148,13 @@ apiVersion: controllermanager.config.k8s.io/v1 kind: LeaderMigrationConfiguration leaderName: test-leader-migration controllerLeaders: - - name: route + - name: route-controller component: "cloud-controller-manager" - - name: service + - name: service-controller component: "cloud-controller-manager" - - name: cloud-node-lifecycle + - name: cloud-node-lifecycle-controller component: "cloud-controller-manager" - - name: nodeipam + - name: node-ipam-controller component: "kube-controller-manager" `, expectErr: false, @@ -163,19 +163,19 @@ controllerLeaders: ResourceLock: "leases", ControllerLeaders: []config.ControllerLeaderConfiguration{ { - Name: "route", + Name: "route-controller", Component: "cloud-controller-manager", }, { - Name: "service", + Name: "service-controller", Component: "cloud-controller-manager", }, { - Name: "cloud-node-lifecycle", + Name: "cloud-node-lifecycle-controller", Component: "cloud-controller-manager", }, { - Name: "nodeipam", + Name: "node-ipam-controller", Component: "kube-controller-manager", }, },