From 27a77e0ef3c8415ad85c1a64a5ba59a08b34af4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Fri, 1 Sep 2023 15:58:39 +0200 Subject: [PATCH 1/5] Encapsulate KCM controllers with their metadata - These metadata can be used to handle controllers in a generic way. - This enables showing feature gated controllers in kube-controller-manager's help. - It is possible to obtain a controllerName in the InitFunc so it can be passed down to and used by the controller. metadata about a controller: - name - requiredFeatureGates - isDisabledByDefault - isCloudProviderController --- cmd/kube-controller-manager/app/apps.go | 35 +- .../app/autoscaling.go | 10 +- cmd/kube-controller-manager/app/batch.go | 19 +- cmd/kube-controller-manager/app/bootstrap.go | 19 +- .../app/certificates.go | 35 +- .../app/controllermanager.go | 308 +++++++++++------- .../app/controllermanager_test.go | 1 + cmd/kube-controller-manager/app/core.go | 235 +++++++++++-- cmd/kube-controller-manager/app/core_test.go | 21 +- cmd/kube-controller-manager/app/discovery.go | 19 +- cmd/kube-controller-manager/app/policy.go | 10 +- cmd/kube-controller-manager/app/rbac.go | 10 +- .../app/testing/testserver.go | 2 +- .../app/validatingadmissionpolicystatus.go | 14 +- .../names/controller_names.go | 25 +- 15 files changed, 566 insertions(+), 197 deletions(-) diff --git a/cmd/kube-controller-manager/app/apps.go b/cmd/kube-controller-manager/app/apps.go index 7c5826878c538..a4e1e886420ae 100644 --- a/cmd/kube-controller-manager/app/apps.go +++ b/cmd/kube-controller-manager/app/apps.go @@ -27,13 +27,20 @@ import ( "k8s.io/client-go/util/flowcontrol" "k8s.io/controller-manager/controller" "k8s.io/klog/v2" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/daemon" "k8s.io/kubernetes/pkg/controller/deployment" "k8s.io/kubernetes/pkg/controller/replicaset" "k8s.io/kubernetes/pkg/controller/statefulset" ) -func startDaemonSetController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newDaemonSetControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.DaemonSetController, + initFunc: startDaemonSetController, + } +} +func startDaemonSetController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { dsc, err := daemon.NewDaemonSetsController( ctx, controllerContext.InformerFactory.Apps().V1().DaemonSets(), @@ -50,7 +57,13 @@ func startDaemonSetController(ctx context.Context, controllerContext ControllerC return nil, true, nil } -func startStatefulSetController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newStatefulSetControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.StatefulSetController, + initFunc: startStatefulSetController, + } +} +func startStatefulSetController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go statefulset.NewStatefulSetController( ctx, controllerContext.InformerFactory.Core().V1().Pods(), @@ -62,7 +75,14 @@ func startStatefulSetController(ctx context.Context, controllerContext Controlle return nil, true, nil } -func startReplicaSetController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newReplicaSetControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ReplicaSetController, + initFunc: startReplicaSetController, + } +} + +func startReplicaSetController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go replicaset.NewReplicaSetController( klog.FromContext(ctx), controllerContext.InformerFactory.Apps().V1().ReplicaSets(), @@ -73,7 +93,14 @@ func startReplicaSetController(ctx context.Context, controllerContext Controller return nil, true, nil } -func startDeploymentController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newDeploymentControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.DeploymentController, + initFunc: startDeploymentController, + } +} + +func startDeploymentController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { dc, err := deployment.NewDeploymentController( ctx, controllerContext.InformerFactory.Apps().V1().Deployments(), diff --git a/cmd/kube-controller-manager/app/autoscaling.go b/cmd/kube-controller-manager/app/autoscaling.go index aabfaa03dc979..a781ed2ffa993 100644 --- a/cmd/kube-controller-manager/app/autoscaling.go +++ b/cmd/kube-controller-manager/app/autoscaling.go @@ -26,6 +26,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/client-go/scale" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/podautoscaler" "k8s.io/kubernetes/pkg/controller/podautoscaler/metrics" "k8s.io/kubernetes/pkg/features" @@ -35,11 +36,14 @@ import ( "k8s.io/metrics/pkg/client/external_metrics" ) -func startHPAController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { - return startHPAControllerWithRESTClient(ctx, controllerContext) +func newHorizontalPodAutoscalerControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.HorizontalPodAutoscalerController, + initFunc: startHorizontalPodAutoscalerControllerWithRESTClient, + } } -func startHPAControllerWithRESTClient(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func startHorizontalPodAutoscalerControllerWithRESTClient(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { clientConfig := controllerContext.ClientBuilder.ConfigOrDie("horizontal-pod-autoscaler") hpaClient := controllerContext.ClientBuilder.ClientOrDie("horizontal-pod-autoscaler") diff --git a/cmd/kube-controller-manager/app/batch.go b/cmd/kube-controller-manager/app/batch.go index ecee824a199eb..ebebf9155dfa0 100644 --- a/cmd/kube-controller-manager/app/batch.go +++ b/cmd/kube-controller-manager/app/batch.go @@ -24,11 +24,19 @@ import ( "fmt" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/cronjob" "k8s.io/kubernetes/pkg/controller/job" ) -func startJobController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newJobControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.JobController, + initFunc: startJobController, + } +} + +func startJobController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { jobController, err := job.NewController( ctx, controllerContext.InformerFactory.Core().V1().Pods(), @@ -42,7 +50,14 @@ func startJobController(ctx context.Context, controllerContext ControllerContext return nil, true, nil } -func startCronJobController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newCronJobControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.CronJobController, + initFunc: startCronJobController, + } +} + +func startCronJobController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { cj2c, err := cronjob.NewControllerV2(ctx, controllerContext.InformerFactory.Batch().V1().Jobs(), controllerContext.InformerFactory.Batch().V1().CronJobs(), controllerContext.ClientBuilder.ClientOrDie("cronjob-controller"), diff --git a/cmd/kube-controller-manager/app/bootstrap.go b/cmd/kube-controller-manager/app/bootstrap.go index 02b2ed0229fbd..f8929c97532bc 100644 --- a/cmd/kube-controller-manager/app/bootstrap.go +++ b/cmd/kube-controller-manager/app/bootstrap.go @@ -21,10 +21,18 @@ import ( "fmt" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/bootstrap" ) -func startBootstrapSignerController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newBootstrapSignerControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.BootstrapSignerController, + initFunc: startBootstrapSignerController, + isDisabledByDefault: true, + } +} +func startBootstrapSignerController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { bsc, err := bootstrap.NewSigner( controllerContext.ClientBuilder.ClientOrDie("bootstrap-signer"), controllerContext.InformerFactory.Core().V1().Secrets(), @@ -38,7 +46,14 @@ func startBootstrapSignerController(ctx context.Context, controllerContext Contr return nil, true, nil } -func startTokenCleanerController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newTokenCleanerControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.TokenCleanerController, + initFunc: startTokenCleanerController, + isDisabledByDefault: true, + } +} +func startTokenCleanerController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { tcc, err := bootstrap.NewTokenCleaner( controllerContext.ClientBuilder.ClientOrDie("token-cleaner"), controllerContext.InformerFactory.Core().V1().Secrets(), diff --git a/cmd/kube-controller-manager/app/certificates.go b/cmd/kube-controller-manager/app/certificates.go index 5d716f85d03b6..073025f46a912 100644 --- a/cmd/kube-controller-manager/app/certificates.go +++ b/cmd/kube-controller-manager/app/certificates.go @@ -25,6 +25,7 @@ import ( "k8s.io/controller-manager/controller" "k8s.io/klog/v2" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/certificates/approver" "k8s.io/kubernetes/pkg/controller/certificates/cleaner" "k8s.io/kubernetes/pkg/controller/certificates/rootcacertpublisher" @@ -32,7 +33,14 @@ import ( csrsigningconfig "k8s.io/kubernetes/pkg/controller/certificates/signer/config" ) -func startCSRSigningController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newCertificateSigningRequestSigningControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.CertificateSigningRequestSigningController, + initFunc: startCertificateSigningRequestSigningController, + } +} + +func startCertificateSigningRequestSigningController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) missingSingleSigningFile := controllerContext.ComponentConfig.CSRSigningController.ClusterSigningCertFile == "" || controllerContext.ComponentConfig.CSRSigningController.ClusterSigningKeyFile == "" if missingSingleSigningFile && !anySpecificFilesSet(controllerContext.ComponentConfig.CSRSigningController) { @@ -148,7 +156,13 @@ func getLegacyUnknownSignerFiles(config csrsigningconfig.CSRSigningControllerCon return config.ClusterSigningCertFile, config.ClusterSigningKeyFile } -func startCSRApprovingController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newCertificateSigningRequestApprovingControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.CertificateSigningRequestApprovingController, + initFunc: startCertificateSigningRequestApprovingController, + } +} +func startCertificateSigningRequestApprovingController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { approver := approver.NewCSRApprovingController( ctx, controllerContext.ClientBuilder.ClientOrDie("certificate-controller"), @@ -159,7 +173,13 @@ func startCSRApprovingController(ctx context.Context, controllerContext Controll return nil, true, nil } -func startCSRCleanerController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newCertificateSigningRequestCleanerControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.CertificateSigningRequestCleanerController, + initFunc: startCertificateSigningRequestCleanerController, + } +} +func startCertificateSigningRequestCleanerController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { cleaner := cleaner.NewCSRCleanerController( controllerContext.ClientBuilder.ClientOrDie("certificate-controller").CertificatesV1().CertificateSigningRequests(), controllerContext.InformerFactory.Certificates().V1().CertificateSigningRequests(), @@ -168,7 +188,14 @@ func startCSRCleanerController(ctx context.Context, controllerContext Controller return nil, true, nil } -func startRootCACertPublisher(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newRootCACertificatePublisherControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.RootCACertificatePublisherController, + initFunc: startRootCACertificatePublisherController, + } +} + +func startRootCACertificatePublisherController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { var ( rootCA []byte err error diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index 11742b6adcf32..70df86173e8b6 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -25,6 +25,7 @@ import ( "math/rand" "net/http" "os" + "sort" "time" "github.com/spf13/cobra" @@ -35,7 +36,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" - genericfeatures "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/server/healthz" "k8s.io/apiserver/pkg/server/mux" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -51,10 +51,10 @@ import ( certutil "k8s.io/client-go/util/cert" "k8s.io/client-go/util/keyutil" cloudprovider "k8s.io/cloud-provider" - cpnames "k8s.io/cloud-provider/names" cliflag "k8s.io/component-base/cli/flag" "k8s.io/component-base/cli/globalflag" "k8s.io/component-base/configz" + "k8s.io/component-base/featuregate" "k8s.io/component-base/logs" logsapi "k8s.io/component-base/logs/api/v1" "k8s.io/component-base/metrics/features" @@ -70,8 +70,6 @@ import ( "k8s.io/controller-manager/pkg/informerfactory" "k8s.io/controller-manager/pkg/leadermigration" "k8s.io/klog/v2" - kubefeatures "k8s.io/kubernetes/pkg/features" - "k8s.io/kubernetes/cmd/kube-controller-manager/app/config" "k8s.io/kubernetes/cmd/kube-controller-manager/app/options" "k8s.io/kubernetes/cmd/kube-controller-manager/names" @@ -137,7 +135,7 @@ controller, and serviceaccounts controller.`, } cliflag.PrintFlags(cmd.Flags()) - c, err := s.Config(KnownControllers(), ControllersDisabledByDefault.List(), names.KCMControllerAliases()) + c, err := s.Config(KnownControllers(), ControllersDisabledByDefault(), names.KCMControllerAliases()) if err != nil { return err } @@ -156,7 +154,7 @@ controller, and serviceaccounts controller.`, } fs := cmd.Flags() - namedFlagSets := s.Flags(KnownControllers(), ControllersDisabledByDefault.List(), names.KCMControllerAliases()) + namedFlagSets := s.Flags(KnownControllers(), ControllersDisabledByDefault(), names.KCMControllerAliases()) verflag.AddFlags(namedFlagSets.FlagSet("global")) globalflag.AddGlobalFlags(namedFlagSets.FlagSet("global"), cmd.Name(), logs.SkipLoggingConfigurationFlags()) registerLegacyGlobalFlags(namedFlagSets) @@ -226,16 +224,16 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { clientBuilder, rootClientBuilder := createClientBuilders(logger, c) - saTokenControllerInitFunc := serviceAccountTokenControllerStarter{rootClientBuilder: rootClientBuilder}.startServiceAccountTokenController + saTokenControllerDescriptor := newServiceAccountTokenControllerDescriptor(rootClientBuilder) - run := func(ctx context.Context, startSATokenController InitFunc, initializersFunc ControllerInitializersFunc) { + run := func(ctx context.Context, startSATokenControllerDescriptor *ControllerDescriptor, controllerDescriptorsFunc ControllerDescriptorsFunc) { controllerContext, err := CreateControllerContext(logger, c, rootClientBuilder, clientBuilder, ctx.Done()) if err != nil { logger.Error(err, "Error building controller context") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } - controllerInitializers := initializersFunc(controllerContext.LoopMode) - if err := StartControllers(ctx, controllerContext, startSATokenController, controllerInitializers, unsecuredMux, healthzHandler); err != nil { + controllerDescriptors := controllerDescriptorsFunc() + if err := StartControllers(ctx, controllerContext, startSATokenControllerDescriptor, controllerDescriptors, unsecuredMux, healthzHandler); err != nil { logger.Error(err, "Error starting controllers") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } @@ -249,7 +247,7 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { // No leader election, run directly if !c.ComponentConfig.Generic.LeaderElection.LeaderElect { - run(ctx, saTokenControllerInitFunc, NewControllerInitializers) + run(ctx, saTokenControllerDescriptor, NewControllerDescriptors) return nil } @@ -264,9 +262,6 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { // leaderMigrator will be non-nil if and only if Leader Migration is enabled. var leaderMigrator *leadermigration.LeaderMigrator = nil - // startSATokenController will be original saTokenControllerInitFunc if leader migration is not enabled. - startSATokenController := saTokenControllerInitFunc - // If leader migration is enabled, create the LeaderMigrator and prepare for migration if leadermigration.Enabled(&c.ComponentConfig.Generic) { logger.Info("starting leader migration") @@ -274,11 +269,14 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { leaderMigrator = leadermigration.NewLeaderMigrator(&c.ComponentConfig.Generic.LeaderMigration, "kube-controller-manager") - // Wrap saTokenControllerInitFunc to signal readiness for migration after starting + // startSATokenControllerInit is the original InitFunc. + startSATokenControllerInit := saTokenControllerDescriptor.GetInitFunc() + + // Wrap saTokenControllerDescriptor to signal readiness for migration after starting // the controller. - startSATokenController = func(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { + saTokenControllerDescriptor.initFunc = func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { defer close(leaderMigrator.MigrationReady) - return saTokenControllerInitFunc(ctx, controllerContext) + return startSATokenControllerInit(ctx, controllerContext, controllerName) } } @@ -288,14 +286,14 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { c.ComponentConfig.Generic.LeaderElection.ResourceName, leaderelection.LeaderCallbacks{ OnStartedLeading: func(ctx context.Context) { - initializersFunc := NewControllerInitializers + initializersFunc := NewControllerDescriptors if leaderMigrator != nil { // If leader migration is enabled, we should start only non-migrated controllers // for the main lock. - initializersFunc = createInitializersFunc(leaderMigrator.FilterFunc, leadermigration.ControllerNonMigrated) + initializersFunc = createFilteredControllerDescriptorsFunc(leaderMigrator.FilterFunc, leadermigration.ControllerNonMigrated) logger.Info("leader migration: starting main controllers.") } - run(ctx, startSATokenController, initializersFunc) + run(ctx, saTokenControllerDescriptor, initializersFunc) }, OnStoppedLeading: func() { logger.Error(nil, "leaderelection lost") @@ -319,7 +317,7 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { OnStartedLeading: func(ctx context.Context) { logger.Info("leader migration: starting migrated controllers.") // DO NOT start saTokenController under migration lock - run(ctx, nil, createInitializersFunc(leaderMigrator.FilterFunc, leadermigration.ControllerMigrated)) + run(ctx, nil, createFilteredControllerDescriptorsFunc(leaderMigrator.FilterFunc, leadermigration.ControllerMigrated)) }, OnStoppedLeading: func() { logger.Error(nil, "migration leaderelection lost") @@ -377,8 +375,12 @@ type ControllerContext struct { } // IsControllerEnabled checks if the context's controllers enabled or not -func (c ControllerContext) IsControllerEnabled(name string) bool { - return genericcontrollermanager.IsControllerEnabled(name, ControllersDisabledByDefault, c.ComponentConfig.Generic.Controllers) +func (c ControllerContext) IsControllerEnabled(controllerDescriptor *ControllerDescriptor) bool { + controllersDisabledByDefault := sets.NewString() + if controllerDescriptor.IsDisabledByDefault() { + controllersDisabledByDefault.Insert(controllerDescriptor.Name()) + } + return genericcontrollermanager.IsControllerEnabled(controllerDescriptor.Name(), controllersDisabledByDefault, c.ComponentConfig.Generic.Controllers) } // InitFunc is used to launch a particular controller. It returns a controller @@ -388,101 +390,141 @@ func (c ControllerContext) IsControllerEnabled(name string) bool { // that requests no additional features from the controller manager. // Any error returned will cause the controller process to `Fatal` // The bool indicates whether the controller was enabled. -type InitFunc func(ctx context.Context, controllerCtx ControllerContext) (controller controller.Interface, enabled bool, err error) +type InitFunc func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller controller.Interface, enabled bool, err error) + +type ControllerDescriptor struct { + name string + initFunc InitFunc + requiredFeatureGates []featuregate.Feature + isDisabledByDefault bool + isCloudProviderController bool +} + +func (r *ControllerDescriptor) Name() string { + return r.name +} + +func (r *ControllerDescriptor) GetInitFunc() InitFunc { + return r.initFunc +} + +func (r *ControllerDescriptor) GetRequiredFeatureGates() []featuregate.Feature { + return append([]featuregate.Feature(nil), r.requiredFeatureGates...) +} -// ControllerInitializersFunc is used to create a collection of initializers +func (r *ControllerDescriptor) IsDisabledByDefault() bool { + return r.isDisabledByDefault +} + +func (r *ControllerDescriptor) IsCloudProviderController() bool { + return r.isCloudProviderController +} + +// ControllerDescriptorsFunc is used to create a collection of controller descriptors // given the loopMode. -type ControllerInitializersFunc func(loopMode ControllerLoopMode) (initializers map[string]InitFunc) +type ControllerDescriptorsFunc func() (initializers map[string]*ControllerDescriptor) -var _ ControllerInitializersFunc = NewControllerInitializers +var _ ControllerDescriptorsFunc = NewControllerDescriptors // KnownControllers returns all known controllers's name func KnownControllers() []string { - ret := sets.StringKeySet(NewControllerInitializers(IncludeCloudLoops)) + ret := sets.StringKeySet(NewControllerDescriptors()) // add "special" controllers that aren't initialized normally. These controllers cannot be initialized // using a normal function. The only known special case is the SA token controller which *must* be started // first to ensure that the SA tokens for future controllers will exist. Think very carefully before adding // to this list. ret.Insert( - names.ServiceAccountTokenController, + newServiceAccountTokenControllerDescriptor(nil).Name(), ) return ret.List() } -// ControllersDisabledByDefault is the set of controllers which is disabled by default -var ControllersDisabledByDefault = sets.NewString( - names.BootstrapSignerController, - names.TokenCleanerController, -) +func ControllersDisabledByDefault() []string { + var controllersDisabledByDefault []string + + for name, c := range NewControllerDescriptors() { + if c.IsDisabledByDefault() { + controllersDisabledByDefault = append(controllersDisabledByDefault, name) + } + } + + sort.Strings(controllersDisabledByDefault) + + return controllersDisabledByDefault +} -// NewControllerInitializers is a public map of named controller groups (you can start more than one in an init func) -// paired to their InitFunc. This allows for structured downstream composition and subdivision. -func NewControllerInitializers(loopMode ControllerLoopMode) map[string]InitFunc { - controllers := map[string]InitFunc{} +// NewControllerDescriptors is a public map of named controller groups (you can start more than one in an init func) +// paired to their ControllerDescriptor wrapper object that includes InitFunc. +// This allows for structured downstream composition and subdivision. +func NewControllerDescriptors() map[string]*ControllerDescriptor { + controllers := map[string]*ControllerDescriptor{} // All of the controllers must have unique names, or else we will explode. - register := func(name string, fn InitFunc) { + register := func(controllerDesc *ControllerDescriptor) { + if controllerDesc == nil { + panic("received nil controller for a registration") + } + name := controllerDesc.Name() + if len(name) == 0 { + panic("received controller without a name for a registration") + } if _, found := controllers[name]; found { panic(fmt.Sprintf("controller name %q was registered twice", name)) } - controllers[name] = fn - } - - register(names.EndpointsController, startEndpointController) - register(names.EndpointSliceController, startEndpointSliceController) - register(names.EndpointSliceMirroringController, startEndpointSliceMirroringController) - register(names.ReplicationControllerController, startReplicationController) - register(names.PodGarbageCollectorController, startPodGCController) - register(names.ResourceQuotaController, startResourceQuotaController) - register(names.NamespaceController, startNamespaceController) - register(names.ServiceAccountController, startServiceAccountController) - register(names.GarbageCollectorController, startGarbageCollectorController) - register(names.DaemonSetController, startDaemonSetController) - register(names.JobController, startJobController) - register(names.DeploymentController, startDeploymentController) - register(names.ReplicaSetController, startReplicaSetController) - register(names.HorizontalPodAutoscalerController, startHPAController) - register(names.DisruptionController, startDisruptionController) - register(names.StatefulSetController, startStatefulSetController) - register(names.CronJobController, startCronJobController) - register(names.CertificateSigningRequestSigningController, startCSRSigningController) - register(names.CertificateSigningRequestApprovingController, startCSRApprovingController) - register(names.CertificateSigningRequestCleanerController, startCSRCleanerController) - register(names.TTLController, startTTLController) - register(names.BootstrapSignerController, startBootstrapSignerController) - register(names.TokenCleanerController, startTokenCleanerController) - register(names.NodeIpamController, startNodeIpamController) - register(names.NodeLifecycleController, startNodeLifecycleController) - if loopMode == IncludeCloudLoops { - register(cpnames.ServiceLBController, startServiceController) - register(cpnames.NodeRouteController, startRouteController) - register(cpnames.CloudNodeLifecycleController, startCloudNodeLifecycleController) - // TODO: persistent volume controllers into the IncludeCloudLoops only set. - } - register(names.PersistentVolumeBinderController, startPersistentVolumeBinderController) - register(names.PersistentVolumeAttachDetachController, startAttachDetachController) - register(names.PersistentVolumeExpanderController, startVolumeExpandController) - register(names.ClusterRoleAggregationController, startClusterRoleAggregrationController) - register(names.PersistentVolumeClaimProtectionController, startPVCProtectionController) - register(names.PersistentVolumeProtectionController, startPVProtectionController) - register(names.TTLAfterFinishedController, startTTLAfterFinishedController) - register(names.RootCACertificatePublisherController, startRootCACertPublisher) - register(names.EphemeralVolumeController, startEphemeralVolumeController) - if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.APIServerIdentity) && - utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StorageVersionAPI) { - register(names.StorageVersionGarbageCollectorController, startStorageVersionGCController) - } - if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.DynamicResourceAllocation) { - register(names.ResourceClaimController, startResourceClaimController) - } - if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.LegacyServiceAccountTokenCleanUp) { - register(names.LegacyServiceAccountTokenCleanerController, startLegacySATokenCleaner) - } - if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.ValidatingAdmissionPolicy) { - register(names.ValidatingAdmissionPolicyStatusController, startValidatingAdmissionPolicyStatusController) - } + if controllerDesc.GetInitFunc() == nil { + panic("received controller without an init function for a registration") + } + controllers[name] = controllerDesc + } + + register(newEndpointsControllerDescriptor()) + register(newEndpointSliceControllerDescriptor()) + register(newEndpointSliceMirroringControllerDescriptor()) + register(newReplicationControllerDescriptor()) + register(newPodGarbageCollectorControllerDescriptor()) + register(newResourceQuotaControllerDescriptor()) + register(newNamespaceControllerDescriptor()) + register(newServiceAccountControllerDescriptor()) + register(newGarbageCollectorControllerDescriptor()) + register(newDaemonSetControllerDescriptor()) + register(newJobControllerDescriptor()) + register(newDeploymentControllerDescriptor()) + register(newReplicaSetControllerDescriptor()) + register(newHorizontalPodAutoscalerControllerDescriptor()) + register(newDisruptionControllerDescriptor()) + register(newStatefulSetControllerDescriptor()) + register(newCronJobControllerDescriptor()) + register(newCertificateSigningRequestSigningControllerDescriptor()) + register(newCertificateSigningRequestApprovingControllerDescriptor()) + register(newCertificateSigningRequestCleanerControllerDescriptor()) + register(newTTLControllerDescriptor()) + register(newBootstrapSignerControllerDescriptor()) + register(newTokenCleanerControllerDescriptor()) + register(newNodeIpamControllerDescriptor()) + register(newNodeLifecycleControllerDescriptor()) + + register(newServiceLBControllerDescriptor()) // cloud provider controller + register(newNodeRouteControllerDescriptor()) // cloud provider controller + register(newCloudNodeLifecycleControllerDescriptor()) // cloud provider controller + // TODO: persistent volume controllers into the IncludeCloudLoops only set as a cloud provider controller. + + register(newPersistentVolumeBinderControllerDescriptor()) + register(newPersistentVolumeAttachDetachControllerDescriptor()) + register(newPersistentVolumeExpanderControllerDescriptor()) + register(newClusterRoleAggregrationControllerDescriptor()) + register(newPersistentVolumeClaimProtectionControllerDescriptor()) + register(newPersistentVolumeProtectionControllerDescriptor()) + register(newTTLAfterFinishedControllerDescriptor()) + register(newRootCACertificatePublisherControllerDescriptor()) + register(newEphemeralVolumeControllerDescriptor()) + + // feature gated + register(newStorageVersionGarbageCollectorControllerDescriptor()) + register(newResourceClaimControllerDescriptor()) + register(newLegacyServiceAccountTokenCleanerControllerDescriptor()) + register(newValidatingAdmissionPolicyStatusControllerDescriptor()) return controllers } @@ -542,15 +584,20 @@ func CreateControllerContext(logger klog.Logger, s *config.CompletedConfig, root } // StartControllers starts a set of controllers with a specified ControllerContext -func StartControllers(ctx context.Context, controllerCtx ControllerContext, startSATokenController InitFunc, controllers map[string]InitFunc, +func StartControllers(ctx context.Context, controllerCtx ControllerContext, startSATokenControllerDescriptor *ControllerDescriptor, controllerDescriptors map[string]*ControllerDescriptor, unsecuredMux *mux.PathRecorderMux, healthzHandler *controllerhealthz.MutableHealthzHandler) error { logger := klog.FromContext(ctx) // Always start the SA token controller first using a full-power client, since it needs to mint tokens for the rest // If this fails, just return here and fail since other controllers won't be able to get credentials. - if startSATokenController != nil { - if _, _, err := startSATokenController(ctx, controllerCtx); err != nil { - return err + if startSATokenControllerDescriptor != nil { + if !controllerCtx.IsControllerEnabled(startSATokenControllerDescriptor) { + logger.Info("Warning: controller is disabled", "controller", startSATokenControllerDescriptor.Name()) + } else { + initFunc := startSATokenControllerDescriptor.GetInitFunc() + if _, _, err := initFunc(ctx, controllerCtx, startSATokenControllerDescriptor.Name()); err != nil { + return err + } } } @@ -571,8 +618,25 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, star // so we cannot rely on it yet to add the name // - it allows distinguishing between log entries emitted by the controller // and those emitted for it - this is a bit debatable and could be revised. - for controllerName, initFn := range controllers { - if !controllerCtx.IsControllerEnabled(controllerName) { + for controllerName, controllerDesc := range controllerDescriptors { + disabledByFeatureGate := false + for _, featureGate := range controllerDesc.GetRequiredFeatureGates() { + if !utilfeature.DefaultFeatureGate.Enabled(featureGate) { + disabledByFeatureGate = true + break + } + } + if disabledByFeatureGate { + logger.Info("Controller is disabled by a feature gate", "controller", controllerName, "requiredFeatureGates", controllerDesc.GetRequiredFeatureGates()) + continue + } + + if controllerDesc.IsCloudProviderController() && controllerCtx.LoopMode != IncludeCloudLoops { + logger.Info("Skipping a cloud provider controller", "controller", controllerName, "loopMode", controllerCtx.LoopMode) + continue + } + + if !controllerCtx.IsControllerEnabled(controllerDesc) { logger.Info("Warning: controller is disabled", "controller", controllerName) continue } @@ -580,7 +644,9 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, star time.Sleep(wait.Jitter(controllerCtx.ComponentConfig.Generic.ControllerStartInterval.Duration, ControllerStartJitter)) logger.V(1).Info("Starting controller", "controller", controllerName) - ctrl, started, err := initFn(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx) + + initFunc := controllerDesc.GetInitFunc() + ctrl, started, err := initFunc(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx, controllerName) if err != nil { logger.Error(err, "Error starting controller", "controller", controllerName) return err @@ -618,20 +684,20 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, star // serviceAccountTokenControllerStarter is special because it must run first to set up permissions for other controllers. // It cannot use the "normal" client builder, so it tracks its own. It must also avoid being included in the "normal" -// init map so that it can always run first. -type serviceAccountTokenControllerStarter struct { - rootClientBuilder clientbuilder.ControllerClientBuilder +// ControllerDescriptor map so that it can always run first. +func newServiceAccountTokenControllerDescriptor(rootClientBuilder clientbuilder.ControllerClientBuilder) *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ServiceAccountTokenController, + initFunc: func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { + return startServiceAccountTokenController(ctx, controllerContext, controllerName, rootClientBuilder) + }, + } } -func (c serviceAccountTokenControllerStarter) startServiceAccountTokenController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func startServiceAccountTokenController(ctx context.Context, controllerContext ControllerContext, controllerName string, rootClientBuilder clientbuilder.ControllerClientBuilder) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) - if !controllerContext.IsControllerEnabled(names.ServiceAccountTokenController) { - logger.Info("Warning: controller is disabled", "controller", names.ServiceAccountTokenController) - return nil, false, nil - } - if len(controllerContext.ComponentConfig.SAController.ServiceAccountKeyFile) == 0 { - logger.Info("Controller is disabled because there is no private key", "controller", names.ServiceAccountTokenController) + logger.Info("Controller is disabled because there is no private key", "controller", controllerName) return nil, false, nil } privateKey, err := keyutil.PrivateKeyFromFile(controllerContext.ComponentConfig.SAController.ServiceAccountKeyFile) @@ -645,7 +711,7 @@ func (c serviceAccountTokenControllerStarter) startServiceAccountTokenController return nil, true, fmt.Errorf("error parsing root-ca-file at %s: %v", controllerContext.ComponentConfig.SAController.RootCAFile, err) } } else { - rootCA = c.rootClientBuilder.ConfigOrDie("tokens-controller").CAData + rootCA = rootClientBuilder.ConfigOrDie("tokens-controller").CAData } tokenGenerator, err := serviceaccount.JWTTokenGenerator(serviceaccount.LegacyIssuer, privateKey) @@ -655,7 +721,7 @@ func (c serviceAccountTokenControllerStarter) startServiceAccountTokenController tokenController, err := serviceaccountcontroller.NewTokensController( controllerContext.InformerFactory.Core().V1().ServiceAccounts(), controllerContext.InformerFactory.Core().V1().Secrets(), - c.rootClientBuilder.ClientOrDie("tokens-controller"), + rootClientBuilder.ClientOrDie("tokens-controller"), serviceaccountcontroller.TokensControllerOptions{ TokenGenerator: tokenGenerator, RootCA: rootCA, @@ -737,16 +803,16 @@ func leaderElectAndRun(ctx context.Context, c *config.CompletedConfig, lockIdent panic("unreachable") } -// createInitializersFunc creates a initializersFunc that returns all initializer +// createFilteredControllerDescriptorsFunc creates a controllerDescriptorsFunc that returns all controllerDescriptors // with expected as the result after filtering through filterFunc. -func createInitializersFunc(filterFunc leadermigration.FilterFunc, expected leadermigration.FilterResult) ControllerInitializersFunc { - return func(loopMode ControllerLoopMode) map[string]InitFunc { - initializers := make(map[string]InitFunc) - for name, initializer := range NewControllerInitializers(loopMode) { +func createFilteredControllerDescriptorsFunc(filterFunc leadermigration.FilterFunc, expected leadermigration.FilterResult) ControllerDescriptorsFunc { + return func() map[string]*ControllerDescriptor { + controllerDescriptors := make(map[string]*ControllerDescriptor) + for name, controllerDesc := range NewControllerDescriptors() { if filterFunc(name) == expected { - initializers[name] = initializer + controllerDescriptors[name] = controllerDesc } } - return initializers + return controllerDescriptors } } diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index d71823e37964a..50e56c4552890 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -83,6 +83,7 @@ func TestControllerNamesDeclaration(t *testing.T) { names.StorageVersionGarbageCollectorController, names.ResourceClaimController, names.LegacyServiceAccountTokenCleanerController, + names.ValidatingAdmissionPolicyStatusController, ) for _, name := range KnownControllers() { diff --git a/cmd/kube-controller-manager/app/core.go b/cmd/kube-controller-manager/app/core.go index bbeb7a360ccf8..709eb27300b4d 100644 --- a/cmd/kube-controller-manager/app/core.go +++ b/cmd/kube-controller-manager/app/core.go @@ -31,6 +31,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime/schema" + genericfeatures "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/quota/v1/generic" utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" @@ -39,8 +40,11 @@ import ( cloudnodelifecyclecontroller "k8s.io/cloud-provider/controllers/nodelifecycle" routecontroller "k8s.io/cloud-provider/controllers/route" servicecontroller "k8s.io/cloud-provider/controllers/service" + cpnames "k8s.io/cloud-provider/names" + "k8s.io/component-base/featuregate" "k8s.io/controller-manager/controller" csitrans "k8s.io/csi-translation-lib" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" pkgcontroller "k8s.io/kubernetes/pkg/controller" endpointcontroller "k8s.io/kubernetes/pkg/controller/endpoint" "k8s.io/kubernetes/pkg/controller/garbagecollector" @@ -63,6 +67,7 @@ import ( persistentvolumecontroller "k8s.io/kubernetes/pkg/controller/volume/persistentvolume" "k8s.io/kubernetes/pkg/controller/volume/pvcprotection" "k8s.io/kubernetes/pkg/controller/volume/pvprotection" + "k8s.io/kubernetes/pkg/features" quotainstall "k8s.io/kubernetes/pkg/quota/v1/install" "k8s.io/kubernetes/pkg/volume/csimigration" "k8s.io/utils/clock" @@ -76,7 +81,15 @@ const ( defaultNodeMaskCIDRIPv6 = 64 ) -func startServiceController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newServiceLBControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: cpnames.ServiceLBController, + initFunc: startServiceLBController, + isCloudProviderController: true, + } +} + +func startServiceLBController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { serviceController, err := servicecontroller.New( controllerContext.Cloud, controllerContext.ClientBuilder.ClientOrDie("service-controller"), @@ -93,8 +106,14 @@ func startServiceController(ctx context.Context, controllerContext ControllerCon go serviceController.Run(ctx, int(controllerContext.ComponentConfig.ServiceController.ConcurrentServiceSyncs), controllerContext.ControllerManagerMetrics) return nil, true, nil } +func newNodeIpamControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.NodeIpamController, + initFunc: startNodeIpamController, + } +} -func startNodeIpamController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func startNodeIpamController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { var serviceCIDR *net.IPNet var secondaryServiceCIDR *net.IPNet logger := klog.FromContext(ctx) @@ -166,7 +185,14 @@ func startNodeIpamController(ctx context.Context, controllerContext ControllerCo return nil, true, nil } -func startNodeLifecycleController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newNodeLifecycleControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.NodeLifecycleController, + initFunc: startNodeLifecycleController, + } +} + +func startNodeLifecycleController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { lifecycleController, err := lifecyclecontroller.NewNodeLifecycleController( ctx, controllerContext.InformerFactory.Coordination().V1().Leases(), @@ -190,7 +216,15 @@ func startNodeLifecycleController(ctx context.Context, controllerContext Control return nil, true, nil } -func startCloudNodeLifecycleController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newCloudNodeLifecycleControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: cpnames.CloudNodeLifecycleController, + initFunc: startCloudNodeLifecycleController, + isCloudProviderController: true, + } +} + +func startCloudNodeLifecycleController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) cloudNodeLifecycleController, err := cloudnodelifecyclecontroller.NewCloudNodeLifecycleController( controllerContext.InformerFactory.Core().V1().Nodes(), @@ -210,7 +244,15 @@ func startCloudNodeLifecycleController(ctx context.Context, controllerContext Co return nil, true, nil } -func startRouteController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newNodeRouteControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: cpnames.NodeRouteController, + initFunc: startNodeRouteController, + isCloudProviderController: true, + } +} + +func startNodeRouteController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) if !controllerContext.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs || !controllerContext.ComponentConfig.KubeCloudShared.ConfigureCloudRoutes { logger.Info("Will not configure cloud provider routes for allocate-node-cidrs", "CIDRs", controllerContext.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs, "routes", controllerContext.ComponentConfig.KubeCloudShared.ConfigureCloudRoutes) @@ -240,7 +282,14 @@ func startRouteController(ctx context.Context, controllerContext ControllerConte return nil, true, nil } -func startPersistentVolumeBinderController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPersistentVolumeBinderControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PersistentVolumeBinderController, + initFunc: startPersistentVolumeBinderController, + } +} + +func startPersistentVolumeBinderController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) plugins, err := ProbeControllerVolumePlugins(logger, controllerContext.Cloud, controllerContext.ComponentConfig.PersistentVolumeBinderController.VolumeConfiguration) if err != nil { @@ -268,7 +317,14 @@ func startPersistentVolumeBinderController(ctx context.Context, controllerContex return nil, true, nil } -func startAttachDetachController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPersistentVolumeAttachDetachControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PersistentVolumeAttachDetachController, + initFunc: startPersistentVolumeAttachDetachController, + } +} + +func startPersistentVolumeAttachDetachController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) csiNodeInformer := controllerContext.InformerFactory.Storage().V1().CSINodes() csiDriverInformer := controllerContext.InformerFactory.Storage().V1().CSIDrivers() @@ -304,7 +360,14 @@ func startAttachDetachController(ctx context.Context, controllerContext Controll return nil, true, nil } -func startVolumeExpandController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPersistentVolumeExpanderControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PersistentVolumeExpanderController, + initFunc: startPersistentVolumeExpanderController, + } +} + +func startPersistentVolumeExpanderController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) plugins, err := ProbeExpandableVolumePlugins(logger, controllerContext.ComponentConfig.PersistentVolumeBinderController.VolumeConfiguration) if err != nil { @@ -326,10 +389,16 @@ func startVolumeExpandController(ctx context.Context, controllerContext Controll } go expandController.Run(ctx) return nil, true, nil +} +func newEphemeralVolumeControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.EphemeralVolumeController, + initFunc: startEphemeralVolumeController, + } } -func startEphemeralVolumeController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func startEphemeralVolumeController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { ephemeralController, err := ephemeral.NewController( controllerContext.ClientBuilder.ClientOrDie("ephemeral-volume-controller"), controllerContext.InformerFactory.Core().V1().Pods(), @@ -343,7 +412,17 @@ func startEphemeralVolumeController(ctx context.Context, controllerContext Contr const defaultResourceClaimControllerWorkers = 10 -func startResourceClaimController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newResourceClaimControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ResourceClaimController, + initFunc: startResourceClaimController, + requiredFeatureGates: []featuregate.Feature{ + features.DynamicResourceAllocation, + }, + } +} + +func startResourceClaimController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { ephemeralController, err := resourceclaim.NewController( klog.FromContext(ctx), controllerContext.ClientBuilder.ClientOrDie("resource-claim-controller"), @@ -358,18 +437,32 @@ func startResourceClaimController(ctx context.Context, controllerContext Control return nil, true, nil } -func startEndpointController(ctx context.Context, controllerCtx ControllerContext) (controller.Interface, bool, error) { +func newEndpointsControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.EndpointsController, + initFunc: startEndpointsController, + } +} + +func startEndpointsController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go endpointcontroller.NewEndpointController( - controllerCtx.InformerFactory.Core().V1().Pods(), - controllerCtx.InformerFactory.Core().V1().Services(), - controllerCtx.InformerFactory.Core().V1().Endpoints(), - controllerCtx.ClientBuilder.ClientOrDie("endpoint-controller"), - controllerCtx.ComponentConfig.EndpointController.EndpointUpdatesBatchPeriod.Duration, - ).Run(ctx, int(controllerCtx.ComponentConfig.EndpointController.ConcurrentEndpointSyncs)) + controllerContext.InformerFactory.Core().V1().Pods(), + controllerContext.InformerFactory.Core().V1().Services(), + controllerContext.InformerFactory.Core().V1().Endpoints(), + controllerContext.ClientBuilder.ClientOrDie("endpoint-controller"), + controllerContext.ComponentConfig.EndpointController.EndpointUpdatesBatchPeriod.Duration, + ).Run(ctx, int(controllerContext.ComponentConfig.EndpointController.ConcurrentEndpointSyncs)) return nil, true, nil } -func startReplicationController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newReplicationControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ReplicationControllerController, + initFunc: startReplicationController, + } +} + +func startReplicationController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go replicationcontroller.NewReplicationManager( klog.FromContext(ctx), controllerContext.InformerFactory.Core().V1().Pods(), @@ -380,7 +473,14 @@ func startReplicationController(ctx context.Context, controllerContext Controlle return nil, true, nil } -func startPodGCController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPodGarbageCollectorControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PodGarbageCollectorController, + initFunc: startPodGarbageCollectorController, + } +} + +func startPodGarbageCollectorController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go podgc.NewPodGC( ctx, controllerContext.ClientBuilder.ClientOrDie("pod-garbage-collector"), @@ -391,7 +491,14 @@ func startPodGCController(ctx context.Context, controllerContext ControllerConte return nil, true, nil } -func startResourceQuotaController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newResourceQuotaControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ResourceQuotaController, + initFunc: startResourceQuotaController, + } +} + +func startResourceQuotaController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { resourceQuotaControllerClient := controllerContext.ClientBuilder.ClientOrDie("resourcequota-controller") resourceQuotaControllerDiscoveryClient := controllerContext.ClientBuilder.DiscoveryClientOrDie("resourcequota-controller") discoveryFunc := resourceQuotaControllerDiscoveryClient.ServerPreferredNamespacedResources @@ -422,7 +529,14 @@ func startResourceQuotaController(ctx context.Context, controllerContext Control return nil, true, nil } -func startNamespaceController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newNamespaceControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.NamespaceController, + initFunc: startNamespaceController, + } +} + +func startNamespaceController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { // the namespace cleanup controller is very chatty. It makes lots of discovery calls and then it makes lots of delete calls // the ratelimiter negatively affects its speed. Deleting 100 total items in a namespace (that's only a few of each resource // including events), takes ~10 seconds by default. @@ -456,7 +570,14 @@ func startModifiedNamespaceController(ctx context.Context, controllerContext Con return nil, true, nil } -func startServiceAccountController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newServiceAccountControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ServiceAccountController, + initFunc: startServiceAccountController, + } +} + +func startServiceAccountController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { sac, err := serviceaccountcontroller.NewServiceAccountsController( controllerContext.InformerFactory.Core().V1().ServiceAccounts(), controllerContext.InformerFactory.Core().V1().Namespaces(), @@ -470,7 +591,14 @@ func startServiceAccountController(ctx context.Context, controllerContext Contro return nil, true, nil } -func startTTLController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newTTLControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.TTLController, + initFunc: startTTLController, + } +} + +func startTTLController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go ttlcontroller.NewTTLController( ctx, controllerContext.InformerFactory.Core().V1().Nodes(), @@ -479,7 +607,14 @@ func startTTLController(ctx context.Context, controllerContext ControllerContext return nil, true, nil } -func startGarbageCollectorController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newGarbageCollectorControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.GarbageCollectorController, + initFunc: startGarbageCollectorController, + } +} + +func startGarbageCollectorController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { if !controllerContext.ComponentConfig.GarbageCollectorController.EnableGarbageCollector { return nil, false, nil } @@ -523,7 +658,14 @@ func startGarbageCollectorController(ctx context.Context, controllerContext Cont return garbageCollector, true, nil } -func startPVCProtectionController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPersistentVolumeClaimProtectionControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PersistentVolumeClaimProtectionController, + initFunc: startPersistentVolumeClaimProtectionController, + } +} + +func startPersistentVolumeClaimProtectionController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { pvcProtectionController, err := pvcprotection.NewPVCProtectionController( klog.FromContext(ctx), controllerContext.InformerFactory.Core().V1().PersistentVolumeClaims(), @@ -537,7 +679,14 @@ func startPVCProtectionController(ctx context.Context, controllerContext Control return nil, true, nil } -func startPVProtectionController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newPersistentVolumeProtectionControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.PersistentVolumeProtectionController, + initFunc: startPersistentVolumeProtectionController, + } +} + +func startPersistentVolumeProtectionController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go pvprotection.NewPVProtectionController( klog.FromContext(ctx), controllerContext.InformerFactory.Core().V1().PersistentVolumes(), @@ -546,7 +695,14 @@ func startPVProtectionController(ctx context.Context, controllerContext Controll return nil, true, nil } -func startTTLAfterFinishedController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newTTLAfterFinishedControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.TTLAfterFinishedController, + initFunc: startTTLAfterFinishedController, + } +} + +func startTTLAfterFinishedController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go ttlafterfinished.New( ctx, controllerContext.InformerFactory.Batch().V1().Jobs(), @@ -555,7 +711,17 @@ func startTTLAfterFinishedController(ctx context.Context, controllerContext Cont return nil, true, nil } -func startLegacySATokenCleaner(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newLegacyServiceAccountTokenCleanerControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.LegacyServiceAccountTokenCleanerController, + initFunc: startLegacyServiceAccountTokenCleanerController, + requiredFeatureGates: []featuregate.Feature{ + features.LegacyServiceAccountTokenCleanUp, + }, + } +} + +func startLegacyServiceAccountTokenCleanerController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { cleanUpPeriod := controllerContext.ComponentConfig.LegacySATokenCleaner.CleanUpPeriod.Duration legacySATokenCleaner, err := serviceaccountcontroller.NewLegacySATokenCleaner( controllerContext.InformerFactory.Core().V1().ServiceAccounts(), @@ -690,7 +856,18 @@ func setNodeCIDRMaskSizes(cfg nodeipamconfig.NodeIPAMControllerConfiguration, cl return sortedSizes(ipv4Mask, ipv6Mask), nil } -func startStorageVersionGCController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newStorageVersionGarbageCollectorControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.StorageVersionGarbageCollectorController, + initFunc: startStorageVersionGarbageCollectorController, + requiredFeatureGates: []featuregate.Feature{ + genericfeatures.APIServerIdentity, + genericfeatures.StorageVersionAPI, + }, + } +} + +func startStorageVersionGarbageCollectorController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go storageversiongc.NewStorageVersionGC( ctx, controllerContext.ClientBuilder.ClientOrDie("storage-version-garbage-collector"), diff --git a/cmd/kube-controller-manager/app/core_test.go b/cmd/kube-controller-manager/app/core_test.go index 1e7513121d543..d66a768df8940 100644 --- a/cmd/kube-controller-manager/app/core_test.go +++ b/cmd/kube-controller-manager/app/core_test.go @@ -28,7 +28,6 @@ import ( clientset "k8s.io/client-go/kubernetes" fakeclientset "k8s.io/client-go/kubernetes/fake" restclient "k8s.io/client-go/rest" - "k8s.io/controller-manager/controller" ) // TestClientBuilder inherits ClientBuilder and can accept a given fake clientset. @@ -105,15 +104,13 @@ func possibleDiscoveryResource() []*metav1.APIResourceList { } } -type controllerInitFunc func(context.Context, ControllerContext) (controller.Interface, bool, error) - func TestController_DiscoveryError(t *testing.T) { - controllerInitFuncMap := map[string]controllerInitFunc{ - "ResourceQuotaController": startResourceQuotaController, - "GarbageCollectorController": startGarbageCollectorController, - "EndpointSliceController": startEndpointSliceController, - "EndpointSliceMirroringController": startEndpointSliceMirroringController, - "PodDisruptionBudgetController": startDisruptionController, + controllerDescriptorMap := map[string]*ControllerDescriptor{ + "ResourceQuotaController": newResourceQuotaControllerDescriptor(), + "GarbageCollectorController": newGarbageCollectorControllerDescriptor(), + "EndpointSliceController": newEndpointSliceControllerDescriptor(), + "EndpointSliceMirroringController": newEndpointSliceMirroringControllerDescriptor(), + "PodDisruptionBudgetController": newDisruptionControllerDescriptor(), } tcs := map[string]struct { @@ -143,10 +140,10 @@ func TestController_DiscoveryError(t *testing.T) { ObjectOrMetadataInformerFactory: testInformerFactory, InformersStarted: make(chan struct{}), } - for funcName, controllerInit := range controllerInitFuncMap { - _, _, err := controllerInit(context.TODO(), ctx) + for controllerName, controllerDesc := range controllerDescriptorMap { + _, _, err := controllerDesc.GetInitFunc()(context.TODO(), ctx, controllerName) if test.expectedErr != (err != nil) { - t.Errorf("%v test failed for use case: %v", funcName, name) + t.Errorf("%v test failed for use case: %v", controllerName, name) } } _, _, err := startModifiedNamespaceController( diff --git a/cmd/kube-controller-manager/app/discovery.go b/cmd/kube-controller-manager/app/discovery.go index 7ed711a8d7303..6a31b1f0d1b10 100644 --- a/cmd/kube-controller-manager/app/discovery.go +++ b/cmd/kube-controller-manager/app/discovery.go @@ -23,11 +23,19 @@ import ( "context" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" endpointslicecontroller "k8s.io/kubernetes/pkg/controller/endpointslice" endpointslicemirroringcontroller "k8s.io/kubernetes/pkg/controller/endpointslicemirroring" ) -func startEndpointSliceController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newEndpointSliceControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.EndpointSliceController, + initFunc: startEndpointSliceController, + } +} + +func startEndpointSliceController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go endpointslicecontroller.NewController( ctx, controllerContext.InformerFactory.Core().V1().Pods(), @@ -41,7 +49,14 @@ func startEndpointSliceController(ctx context.Context, controllerContext Control return nil, true, nil } -func startEndpointSliceMirroringController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newEndpointSliceMirroringControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.EndpointSliceMirroringController, + initFunc: startEndpointSliceMirroringController, + } +} + +func startEndpointSliceMirroringController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go endpointslicemirroringcontroller.NewController( ctx, controllerContext.InformerFactory.Core().V1().Endpoints(), diff --git a/cmd/kube-controller-manager/app/policy.go b/cmd/kube-controller-manager/app/policy.go index 001086cb8d82d..2fead2bc16a07 100644 --- a/cmd/kube-controller-manager/app/policy.go +++ b/cmd/kube-controller-manager/app/policy.go @@ -25,10 +25,18 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/client-go/scale" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/disruption" ) -func startDisruptionController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newDisruptionControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.DisruptionController, + initFunc: startDisruptionController, + } +} + +func startDisruptionController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { client := controllerContext.ClientBuilder.ClientOrDie("disruption-controller") config := controllerContext.ClientBuilder.ConfigOrDie("disruption-controller") scaleKindResolver := scale.NewDiscoveryScaleKindResolver(client.Discovery()) diff --git a/cmd/kube-controller-manager/app/rbac.go b/cmd/kube-controller-manager/app/rbac.go index 334dc74c01203..594a3817b6f40 100644 --- a/cmd/kube-controller-manager/app/rbac.go +++ b/cmd/kube-controller-manager/app/rbac.go @@ -20,10 +20,18 @@ import ( "context" "k8s.io/controller-manager/controller" + "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/clusterroleaggregation" ) -func startClusterRoleAggregrationController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newClusterRoleAggregrationControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ClusterRoleAggregationController, + initFunc: startClusterRoleAggregationController, + } +} + +func startClusterRoleAggregationController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { go clusterroleaggregation.NewClusterRoleAggregation( controllerContext.InformerFactory.Rbac().V1().ClusterRoles(), controllerContext.ClientBuilder.ClientOrDie("clusterrole-aggregation-controller").RbacV1(), diff --git a/cmd/kube-controller-manager/app/testing/testserver.go b/cmd/kube-controller-manager/app/testing/testserver.go index 92dcd6d593c37..76abca6c30e9d 100644 --- a/cmd/kube-controller-manager/app/testing/testserver.go +++ b/cmd/kube-controller-manager/app/testing/testserver.go @@ -97,7 +97,7 @@ func StartTestServer(ctx context.Context, customFlags []string) (result TestServ if err != nil { return TestServer{}, err } - all, disabled, aliases := app.KnownControllers(), app.ControllersDisabledByDefault.List(), names.KCMControllerAliases() + all, disabled, aliases := app.KnownControllers(), app.ControllersDisabledByDefault(), names.KCMControllerAliases() namedFlagSets := s.Flags(all, disabled, aliases) for _, f := range namedFlagSets.FlagSets { fs.AddFlagSet(f) diff --git a/cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go b/cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go index ca5ceeb2b0e9f..9f4c78c52babe 100644 --- a/cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go +++ b/cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go @@ -21,14 +21,26 @@ import ( pluginvalidatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy" "k8s.io/apiserver/pkg/cel/openapi/resolver" + genericfeatures "k8s.io/apiserver/pkg/features" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/component-base/featuregate" "k8s.io/controller-manager/controller" "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/validatingadmissionpolicystatus" "k8s.io/kubernetes/pkg/generated/openapi" ) -func startValidatingAdmissionPolicyStatusController(ctx context.Context, controllerContext ControllerContext) (controller.Interface, bool, error) { +func newValidatingAdmissionPolicyStatusControllerDescriptor() *ControllerDescriptor { + return &ControllerDescriptor{ + name: names.ValidatingAdmissionPolicyStatusController, + initFunc: startValidatingAdmissionPolicyStatusController, + requiredFeatureGates: []featuregate.Feature{ + genericfeatures.ValidatingAdmissionPolicy, + }, + } +} + +func startValidatingAdmissionPolicyStatusController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { // KCM won't start the controller without the feature gate set. typeChecker := &pluginvalidatingadmissionpolicy.TypeChecker{ SchemaResolver: resolver.NewDefinitionsSchemaResolver(scheme.Scheme, openapi.GetOpenAPIDefinitions), diff --git a/cmd/kube-controller-manager/names/controller_names.go b/cmd/kube-controller-manager/names/controller_names.go index 9004514fd3c00..42244373110db 100644 --- a/cmd/kube-controller-manager/names/controller_names.go +++ b/cmd/kube-controller-manager/names/controller_names.go @@ -33,20 +33,17 @@ import cpnames "k8s.io/cloud-provider/names" // // USE CASES // The following places should use the controller name constants, when: -// 1. registering a controller in app.NewControllerInitializers or app.KnownControllers: -// 1.1. disabling a controller by default in app.ControllersDisabledByDefault -// 1.2. checking if IsControllerEnabled -// 1.3. defining an alias in KCMControllerAliases (for backwards compatibility only) -// 2. used anywhere inside the controller itself: -// 2.1. [TODO] logger component should be configured with the controller name by calling LoggerWithName -// 2.2. [TODO] logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X) -// 2.3. [TODO] emitted events should have an EventSource.Component set to the controller name (usually when initializing an EventRecorder) -// 2.4. [TODO] registering ControllerManagerMetrics with ControllerStarted and ControllerStopped -// 2.5. [TODO] calling WaitForNamedCacheSync -// 3. defining controller options for "--help" command or generated documentation -// 3.1. controller name should be used to create a pflag.FlagSet when registering controller options (the name is rendered in a controller flag group header) -// 3.2. when defined flag's help mentions a controller name -// 4. defining a new service account for a new controller (old controllers may have inconsistent service accounts to stay backwards compatible) +// 1. defining a new app.ControllerDescriptor so it can be used in app.NewControllerDescriptors or app.KnownControllers: +// 2. defining an alias in KCMControllerAliases (for backwards compatibility only) +// 3. used anywhere inside the controller itself: +// 3.1. [TODO] logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X) +// 3.2. [TODO] emitted events should have an EventSource.Component set to the controller name (usually when initializing an EventRecorder) +// 3.3. [TODO] registering ControllerManagerMetrics with ControllerStarted and ControllerStopped +// 3.4. [TODO] calling WaitForNamedCacheSync +// 4. defining controller options for "--help" command or generated documentation +// 1.1. controller name should be used to create a pflag.FlagSet when registering controller options (the name is rendered in a controller flag group header) in options.KubeControllerManagerOptions +// 1.2. when defined flag's help mentions a controller name +// 5. defining a new service account for a new controller (old controllers may have inconsistent service accounts to stay backwards compatible) const ( ServiceAccountTokenController = "serviceaccount-token-controller" EndpointsController = "endpoints-controller" From a85779b4dfe4c71a2def2f2385fb4d8f896522ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Fri, 1 Sep 2023 20:04:08 +0200 Subject: [PATCH 2/5] include ServiceAccountTokenController in the NewControllerDescriptors to make it more generic - pass a map of controllerDescriptors instead of a function --- .../app/controllermanager.go | 92 ++++++++++--------- 1 file changed, 50 insertions(+), 42 deletions(-) diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index 70df86173e8b6..06a91d85bdc54 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -226,14 +226,14 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { saTokenControllerDescriptor := newServiceAccountTokenControllerDescriptor(rootClientBuilder) - run := func(ctx context.Context, startSATokenControllerDescriptor *ControllerDescriptor, controllerDescriptorsFunc ControllerDescriptorsFunc) { + run := func(ctx context.Context, controllerDescriptors map[string]*ControllerDescriptor) { controllerContext, err := CreateControllerContext(logger, c, rootClientBuilder, clientBuilder, ctx.Done()) if err != nil { logger.Error(err, "Error building controller context") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } - controllerDescriptors := controllerDescriptorsFunc() - if err := StartControllers(ctx, controllerContext, startSATokenControllerDescriptor, controllerDescriptors, unsecuredMux, healthzHandler); err != nil { + + if err := StartControllers(ctx, controllerContext, controllerDescriptors, unsecuredMux, healthzHandler); err != nil { logger.Error(err, "Error starting controllers") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } @@ -247,7 +247,9 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { // No leader election, run directly if !c.ComponentConfig.Generic.LeaderElection.LeaderElect { - run(ctx, saTokenControllerDescriptor, NewControllerDescriptors) + controllerDescriptors := NewControllerDescriptors() + controllerDescriptors[names.ServiceAccountTokenController] = saTokenControllerDescriptor + run(ctx, controllerDescriptors) return nil } @@ -286,14 +288,15 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { c.ComponentConfig.Generic.LeaderElection.ResourceName, leaderelection.LeaderCallbacks{ OnStartedLeading: func(ctx context.Context) { - initializersFunc := NewControllerDescriptors + controllerDescriptors := NewControllerDescriptors() if leaderMigrator != nil { // If leader migration is enabled, we should start only non-migrated controllers // for the main lock. - initializersFunc = createFilteredControllerDescriptorsFunc(leaderMigrator.FilterFunc, leadermigration.ControllerNonMigrated) + controllerDescriptors = filteredControllerDescriptors(controllerDescriptors, leaderMigrator.FilterFunc, leadermigration.ControllerNonMigrated) logger.Info("leader migration: starting main controllers.") } - run(ctx, saTokenControllerDescriptor, initializersFunc) + controllerDescriptors[names.ServiceAccountTokenController] = saTokenControllerDescriptor + run(ctx, controllerDescriptors) }, OnStoppedLeading: func() { logger.Error(nil, "leaderelection lost") @@ -316,8 +319,11 @@ func Run(ctx context.Context, c *config.CompletedConfig) error { leaderelection.LeaderCallbacks{ OnStartedLeading: func(ctx context.Context) { logger.Info("leader migration: starting migrated controllers.") + controllerDescriptors := NewControllerDescriptors() + controllerDescriptors = filteredControllerDescriptors(controllerDescriptors, leaderMigrator.FilterFunc, leadermigration.ControllerMigrated) // DO NOT start saTokenController under migration lock - run(ctx, nil, createFilteredControllerDescriptorsFunc(leaderMigrator.FilterFunc, leadermigration.ControllerMigrated)) + delete(controllerDescriptors, names.ServiceAccountTokenController) + run(ctx, controllerDescriptors) }, OnStoppedLeading: func() { logger.Error(nil, "migration leaderelection lost") @@ -398,6 +404,7 @@ type ControllerDescriptor struct { requiredFeatureGates []featuregate.Feature isDisabledByDefault bool isCloudProviderController bool + requiresSpecialHandling bool } func (r *ControllerDescriptor) Name() string { @@ -420,25 +427,14 @@ func (r *ControllerDescriptor) IsCloudProviderController() bool { return r.isCloudProviderController } -// ControllerDescriptorsFunc is used to create a collection of controller descriptors -// given the loopMode. -type ControllerDescriptorsFunc func() (initializers map[string]*ControllerDescriptor) - -var _ ControllerDescriptorsFunc = NewControllerDescriptors +// RequiresSpecialHandling should return true only in a special non-generic controllers like ServiceAccountTokenController +func (r *ControllerDescriptor) RequiresSpecialHandling() bool { + return r.requiresSpecialHandling +} // KnownControllers returns all known controllers's name func KnownControllers() []string { - ret := sets.StringKeySet(NewControllerDescriptors()) - - // add "special" controllers that aren't initialized normally. These controllers cannot be initialized - // using a normal function. The only known special case is the SA token controller which *must* be started - // first to ensure that the SA tokens for future controllers will exist. Think very carefully before adding - // to this list. - ret.Insert( - newServiceAccountTokenControllerDescriptor(nil).Name(), - ) - - return ret.List() + return sets.StringKeySet(NewControllerDescriptors()).List() } func ControllersDisabledByDefault() []string { @@ -479,6 +475,14 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor { controllers[name] = controllerDesc } + // First add "special" controllers that aren't initialized normally. These controllers cannot be initialized + // in the main controller loop initialization, so we add them here only for the metadata and duplication detection. + // app.ControllerDescriptor#RequiresSpecialHandling should return true for such controllers + // The only known special case is the ServiceAccountTokenController which *must* be started + // first to ensure that the SA tokens for future controllers will exist. Think very carefully before adding new + // special controllers. + register(newServiceAccountTokenControllerDescriptor(nil)) + register(newEndpointsControllerDescriptor()) register(newEndpointSliceControllerDescriptor()) register(newEndpointSliceMirroringControllerDescriptor()) @@ -584,18 +588,20 @@ func CreateControllerContext(logger klog.Logger, s *config.CompletedConfig, root } // StartControllers starts a set of controllers with a specified ControllerContext -func StartControllers(ctx context.Context, controllerCtx ControllerContext, startSATokenControllerDescriptor *ControllerDescriptor, controllerDescriptors map[string]*ControllerDescriptor, +func StartControllers(ctx context.Context, controllerCtx ControllerContext, controllerDescriptors map[string]*ControllerDescriptor, unsecuredMux *mux.PathRecorderMux, healthzHandler *controllerhealthz.MutableHealthzHandler) error { logger := klog.FromContext(ctx) // Always start the SA token controller first using a full-power client, since it needs to mint tokens for the rest // If this fails, just return here and fail since other controllers won't be able to get credentials. - if startSATokenControllerDescriptor != nil { - if !controllerCtx.IsControllerEnabled(startSATokenControllerDescriptor) { - logger.Info("Warning: controller is disabled", "controller", startSATokenControllerDescriptor.Name()) + if serviceAccountTokenControllerDescriptor, ok := controllerDescriptors[names.ServiceAccountTokenController]; ok { + controllerName := serviceAccountTokenControllerDescriptor.Name() + if !controllerCtx.IsControllerEnabled(serviceAccountTokenControllerDescriptor) { + logger.Info("Warning: controller is disabled", "controller", controllerName) } else { - initFunc := startSATokenControllerDescriptor.GetInitFunc() - if _, _, err := initFunc(ctx, controllerCtx, startSATokenControllerDescriptor.Name()); err != nil { + logger.V(1).Info("Starting controller", "controller", controllerName) + initFunc := serviceAccountTokenControllerDescriptor.GetInitFunc() + if _, _, err := initFunc(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx, controllerName); err != nil { return err } } @@ -619,6 +625,10 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, star // - it allows distinguishing between log entries emitted by the controller // and those emitted for it - this is a bit debatable and could be revised. for controllerName, controllerDesc := range controllerDescriptors { + if controllerDesc.RequiresSpecialHandling() { + continue + } + disabledByFeatureGate := false for _, featureGate := range controllerDesc.GetRequiredFeatureGates() { if !utilfeature.DefaultFeatureGate.Enabled(featureGate) { @@ -683,14 +693,15 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, star } // serviceAccountTokenControllerStarter is special because it must run first to set up permissions for other controllers. -// It cannot use the "normal" client builder, so it tracks its own. It must also avoid being included in the "normal" -// ControllerDescriptor map so that it can always run first. +// It cannot use the "normal" client builder, so it tracks its own. func newServiceAccountTokenControllerDescriptor(rootClientBuilder clientbuilder.ControllerClientBuilder) *ControllerDescriptor { return &ControllerDescriptor{ name: names.ServiceAccountTokenController, initFunc: func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { return startServiceAccountTokenController(ctx, controllerContext, controllerName, rootClientBuilder) }, + // will make sure it runs first before other controllers + requiresSpecialHandling: true, } } @@ -803,16 +814,13 @@ func leaderElectAndRun(ctx context.Context, c *config.CompletedConfig, lockIdent panic("unreachable") } -// createFilteredControllerDescriptorsFunc creates a controllerDescriptorsFunc that returns all controllerDescriptors -// with expected as the result after filtering through filterFunc. -func createFilteredControllerDescriptorsFunc(filterFunc leadermigration.FilterFunc, expected leadermigration.FilterResult) ControllerDescriptorsFunc { - return func() map[string]*ControllerDescriptor { - controllerDescriptors := make(map[string]*ControllerDescriptor) - for name, controllerDesc := range NewControllerDescriptors() { - if filterFunc(name) == expected { - controllerDescriptors[name] = controllerDesc - } +// filteredControllerDescriptors returns all controllerDescriptors after filtering through filterFunc. +func filteredControllerDescriptors(controllerDescriptors map[string]*ControllerDescriptor, filterFunc leadermigration.FilterFunc, expected leadermigration.FilterResult) map[string]*ControllerDescriptor { + resultControllers := make(map[string]*ControllerDescriptor) + for name, controllerDesc := range controllerDescriptors { + if filterFunc(name) == expected { + resultControllers[name] = controllerDesc } - return controllerDescriptors } + return resultControllers } From b76896728025ce7f04fcec0cd199d81a21bc53fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Fri, 1 Sep 2023 21:11:11 +0200 Subject: [PATCH 3/5] move aliases into each registrable controller --- cmd/kube-controller-manager/app/apps.go | 4 + .../app/autoscaling.go | 1 + cmd/kube-controller-manager/app/batch.go | 2 + cmd/kube-controller-manager/app/bootstrap.go | 2 + .../app/certificates.go | 4 + .../app/controllermanager.go | 44 +++++++++-- .../app/controllermanager_test.go | 6 +- cmd/kube-controller-manager/app/core.go | 23 ++++++ cmd/kube-controller-manager/app/discovery.go | 2 + cmd/kube-controller-manager/app/policy.go | 1 + cmd/kube-controller-manager/app/rbac.go | 1 + .../app/testing/testserver.go | 3 +- .../names/controller_names.go | 74 +++---------------- 13 files changed, 95 insertions(+), 72 deletions(-) diff --git a/cmd/kube-controller-manager/app/apps.go b/cmd/kube-controller-manager/app/apps.go index a4e1e886420ae..7d9a2dc132084 100644 --- a/cmd/kube-controller-manager/app/apps.go +++ b/cmd/kube-controller-manager/app/apps.go @@ -37,6 +37,7 @@ import ( func newDaemonSetControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.DaemonSetController, + aliases: []string{"daemonset"}, initFunc: startDaemonSetController, } } @@ -60,6 +61,7 @@ func startDaemonSetController(ctx context.Context, controllerContext ControllerC func newStatefulSetControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.StatefulSetController, + aliases: []string{"statefulset"}, initFunc: startStatefulSetController, } } @@ -78,6 +80,7 @@ func startStatefulSetController(ctx context.Context, controllerContext Controlle func newReplicaSetControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ReplicaSetController, + aliases: []string{"replicaset"}, initFunc: startReplicaSetController, } } @@ -96,6 +99,7 @@ func startReplicaSetController(ctx context.Context, controllerContext Controller func newDeploymentControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.DeploymentController, + aliases: []string{"deployment"}, initFunc: startDeploymentController, } } diff --git a/cmd/kube-controller-manager/app/autoscaling.go b/cmd/kube-controller-manager/app/autoscaling.go index a781ed2ffa993..8e12f16ad9f7b 100644 --- a/cmd/kube-controller-manager/app/autoscaling.go +++ b/cmd/kube-controller-manager/app/autoscaling.go @@ -39,6 +39,7 @@ import ( func newHorizontalPodAutoscalerControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.HorizontalPodAutoscalerController, + aliases: []string{"horizontalpodautoscaling"}, initFunc: startHorizontalPodAutoscalerControllerWithRESTClient, } } diff --git a/cmd/kube-controller-manager/app/batch.go b/cmd/kube-controller-manager/app/batch.go index ebebf9155dfa0..159aebd82848f 100644 --- a/cmd/kube-controller-manager/app/batch.go +++ b/cmd/kube-controller-manager/app/batch.go @@ -32,6 +32,7 @@ import ( func newJobControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.JobController, + aliases: []string{"job"}, initFunc: startJobController, } } @@ -53,6 +54,7 @@ func startJobController(ctx context.Context, controllerContext ControllerContext func newCronJobControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.CronJobController, + aliases: []string{"cronjob"}, initFunc: startCronJobController, } } diff --git a/cmd/kube-controller-manager/app/bootstrap.go b/cmd/kube-controller-manager/app/bootstrap.go index f8929c97532bc..aedaaf65aa03f 100644 --- a/cmd/kube-controller-manager/app/bootstrap.go +++ b/cmd/kube-controller-manager/app/bootstrap.go @@ -28,6 +28,7 @@ import ( func newBootstrapSignerControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.BootstrapSignerController, + aliases: []string{"bootstrapsigner"}, initFunc: startBootstrapSignerController, isDisabledByDefault: true, } @@ -49,6 +50,7 @@ func startBootstrapSignerController(ctx context.Context, controllerContext Contr func newTokenCleanerControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.TokenCleanerController, + aliases: []string{"tokencleaner"}, initFunc: startTokenCleanerController, isDisabledByDefault: true, } diff --git a/cmd/kube-controller-manager/app/certificates.go b/cmd/kube-controller-manager/app/certificates.go index 073025f46a912..c2b3628429655 100644 --- a/cmd/kube-controller-manager/app/certificates.go +++ b/cmd/kube-controller-manager/app/certificates.go @@ -36,6 +36,7 @@ import ( func newCertificateSigningRequestSigningControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.CertificateSigningRequestSigningController, + aliases: []string{"csrsigning"}, initFunc: startCertificateSigningRequestSigningController, } } @@ -159,6 +160,7 @@ func getLegacyUnknownSignerFiles(config csrsigningconfig.CSRSigningControllerCon func newCertificateSigningRequestApprovingControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.CertificateSigningRequestApprovingController, + aliases: []string{"csrapproving"}, initFunc: startCertificateSigningRequestApprovingController, } } @@ -176,6 +178,7 @@ func startCertificateSigningRequestApprovingController(ctx context.Context, cont func newCertificateSigningRequestCleanerControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.CertificateSigningRequestCleanerController, + aliases: []string{"csrcleaner"}, initFunc: startCertificateSigningRequestCleanerController, } } @@ -191,6 +194,7 @@ func startCertificateSigningRequestCleanerController(ctx context.Context, contro func newRootCACertificatePublisherControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.RootCACertificatePublisherController, + aliases: []string{"root-ca-cert-publisher"}, initFunc: startRootCACertificatePublisherController, } } diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index 06a91d85bdc54..51ac74aa6d8e6 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -135,7 +135,7 @@ controller, and serviceaccounts controller.`, } cliflag.PrintFlags(cmd.Flags()) - c, err := s.Config(KnownControllers(), ControllersDisabledByDefault(), names.KCMControllerAliases()) + c, err := s.Config(KnownControllers(), ControllersDisabledByDefault(), ControllerAliases()) if err != nil { return err } @@ -154,7 +154,7 @@ controller, and serviceaccounts controller.`, } fs := cmd.Flags() - namedFlagSets := s.Flags(KnownControllers(), ControllersDisabledByDefault(), names.KCMControllerAliases()) + namedFlagSets := s.Flags(KnownControllers(), ControllersDisabledByDefault(), ControllerAliases()) verflag.AddFlags(namedFlagSets.FlagSet("global")) globalflag.AddGlobalFlags(namedFlagSets.FlagSet("global"), cmd.Name(), logs.SkipLoggingConfigurationFlags()) registerLegacyGlobalFlags(namedFlagSets) @@ -402,6 +402,7 @@ type ControllerDescriptor struct { name string initFunc InitFunc requiredFeatureGates []featuregate.Feature + aliases []string isDisabledByDefault bool isCloudProviderController bool requiresSpecialHandling bool @@ -419,6 +420,12 @@ func (r *ControllerDescriptor) GetRequiredFeatureGates() []featuregate.Feature { return append([]featuregate.Feature(nil), r.requiredFeatureGates...) } +// GetAliases returns aliases to ensure backwards compatibility and should never be removed! +// Only addition of new aliases is allowed, and only when a canonical name is changed (please see CHANGE POLICY of controller names) +func (r *ControllerDescriptor) GetAliases() []string { + return append([]string(nil), r.aliases...) +} + func (r *ControllerDescriptor) IsDisabledByDefault() bool { return r.isDisabledByDefault } @@ -437,6 +444,17 @@ func KnownControllers() []string { return sets.StringKeySet(NewControllerDescriptors()).List() } +// ControllerAliases returns a mapping of aliases to canonical controller names +func ControllerAliases() map[string]string { + aliases := map[string]string{} + for name, c := range NewControllerDescriptors() { + for _, alias := range c.GetAliases() { + aliases[alias] = name + } + } + return aliases +} + func ControllersDisabledByDefault() []string { var controllersDisabledByDefault []string @@ -456,8 +474,9 @@ func ControllersDisabledByDefault() []string { // This allows for structured downstream composition and subdivision. func NewControllerDescriptors() map[string]*ControllerDescriptor { controllers := map[string]*ControllerDescriptor{} + aliases := sets.NewString() - // All of the controllers must have unique names, or else we will explode. + // All the controllers must fulfil common constraints, or else we will explode. register := func(controllerDesc *ControllerDescriptor) { if controllerDesc == nil { panic("received nil controller for a registration") @@ -470,8 +489,16 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor { panic(fmt.Sprintf("controller name %q was registered twice", name)) } if controllerDesc.GetInitFunc() == nil { - panic("received controller without an init function for a registration") + panic(fmt.Sprintf("controller %q does not have an init function", name)) } + + for _, alias := range controllerDesc.GetAliases() { + if aliases.Has(alias) { + panic(fmt.Sprintf("controller %q has a duplicate alias %q", name, alias)) + } + aliases.Insert(alias) + } + controllers[name] = controllerDesc } @@ -530,6 +557,12 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor { register(newLegacyServiceAccountTokenCleanerControllerDescriptor()) register(newValidatingAdmissionPolicyStatusControllerDescriptor()) + for _, alias := range aliases.UnsortedList() { + if _, ok := controllers[alias]; ok { + panic(fmt.Sprintf("alias %q conflicts with a controller name", alias)) + } + } + return controllers } @@ -696,7 +729,8 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, cont // It cannot use the "normal" client builder, so it tracks its own. func newServiceAccountTokenControllerDescriptor(rootClientBuilder clientbuilder.ControllerClientBuilder) *ControllerDescriptor { return &ControllerDescriptor{ - name: names.ServiceAccountTokenController, + name: names.ServiceAccountTokenController, + aliases: []string{"serviceaccount-token"}, initFunc: func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { return startServiceAccountTokenController(ctx, controllerContext, controllerName, rootClientBuilder) }, diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index 50e56c4552890..40125b687f026 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -41,7 +41,7 @@ func TestControllerNamesConsistency(t *testing.T) { } func TestControllerNamesDeclaration(t *testing.T) { - declaredControllers := sets.New( + declaredControllers := sets.NewString( names.ServiceAccountTokenController, names.EndpointsController, names.EndpointSliceController, @@ -92,3 +92,7 @@ func TestControllerNamesDeclaration(t *testing.T) { } } } + +func TestNewControllerDescriptorsShouldNotPanic(t *testing.T) { + NewControllerDescriptors() +} diff --git a/cmd/kube-controller-manager/app/core.go b/cmd/kube-controller-manager/app/core.go index 709eb27300b4d..47e01cddc2ada 100644 --- a/cmd/kube-controller-manager/app/core.go +++ b/cmd/kube-controller-manager/app/core.go @@ -84,6 +84,7 @@ const ( func newServiceLBControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: cpnames.ServiceLBController, + aliases: []string{"service"}, initFunc: startServiceLBController, isCloudProviderController: true, } @@ -109,6 +110,7 @@ func startServiceLBController(ctx context.Context, controllerContext ControllerC func newNodeIpamControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.NodeIpamController, + aliases: []string{"nodeipam"}, initFunc: startNodeIpamController, } } @@ -188,6 +190,7 @@ func startNodeIpamController(ctx context.Context, controllerContext ControllerCo func newNodeLifecycleControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.NodeLifecycleController, + aliases: []string{"nodelifecycle"}, initFunc: startNodeLifecycleController, } } @@ -219,6 +222,7 @@ func startNodeLifecycleController(ctx context.Context, controllerContext Control func newCloudNodeLifecycleControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: cpnames.CloudNodeLifecycleController, + aliases: []string{"cloud-node-lifecycle"}, initFunc: startCloudNodeLifecycleController, isCloudProviderController: true, } @@ -247,6 +251,7 @@ func startCloudNodeLifecycleController(ctx context.Context, controllerContext Co func newNodeRouteControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: cpnames.NodeRouteController, + aliases: []string{"route"}, initFunc: startNodeRouteController, isCloudProviderController: true, } @@ -285,6 +290,7 @@ func startNodeRouteController(ctx context.Context, controllerContext ControllerC func newPersistentVolumeBinderControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PersistentVolumeBinderController, + aliases: []string{"persistentvolume-binder"}, initFunc: startPersistentVolumeBinderController, } } @@ -320,6 +326,7 @@ func startPersistentVolumeBinderController(ctx context.Context, controllerContex func newPersistentVolumeAttachDetachControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PersistentVolumeAttachDetachController, + aliases: []string{"attachdetach"}, initFunc: startPersistentVolumeAttachDetachController, } } @@ -363,6 +370,7 @@ func startPersistentVolumeAttachDetachController(ctx context.Context, controller func newPersistentVolumeExpanderControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PersistentVolumeExpanderController, + aliases: []string{"persistentvolume-expander"}, initFunc: startPersistentVolumeExpanderController, } } @@ -394,6 +402,7 @@ func startPersistentVolumeExpanderController(ctx context.Context, controllerCont func newEphemeralVolumeControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.EphemeralVolumeController, + aliases: []string{"ephemeral-volume"}, initFunc: startEphemeralVolumeController, } } @@ -415,6 +424,7 @@ const defaultResourceClaimControllerWorkers = 10 func newResourceClaimControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ResourceClaimController, + aliases: []string{"resource-claim-controller"}, initFunc: startResourceClaimController, requiredFeatureGates: []featuregate.Feature{ features.DynamicResourceAllocation, @@ -440,6 +450,7 @@ func startResourceClaimController(ctx context.Context, controllerContext Control func newEndpointsControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.EndpointsController, + aliases: []string{"endpoint"}, initFunc: startEndpointsController, } } @@ -458,6 +469,7 @@ func startEndpointsController(ctx context.Context, controllerContext ControllerC func newReplicationControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ReplicationControllerController, + aliases: []string{"replicationcontroller"}, initFunc: startReplicationController, } } @@ -476,6 +488,7 @@ func startReplicationController(ctx context.Context, controllerContext Controlle func newPodGarbageCollectorControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PodGarbageCollectorController, + aliases: []string{"podgc"}, initFunc: startPodGarbageCollectorController, } } @@ -494,6 +507,7 @@ func startPodGarbageCollectorController(ctx context.Context, controllerContext C func newResourceQuotaControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ResourceQuotaController, + aliases: []string{"resourcequota"}, initFunc: startResourceQuotaController, } } @@ -532,6 +546,7 @@ func startResourceQuotaController(ctx context.Context, controllerContext Control func newNamespaceControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.NamespaceController, + aliases: []string{"namespace"}, initFunc: startNamespaceController, } } @@ -573,6 +588,7 @@ func startModifiedNamespaceController(ctx context.Context, controllerContext Con func newServiceAccountControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ServiceAccountController, + aliases: []string{"serviceaccount"}, initFunc: startServiceAccountController, } } @@ -594,6 +610,7 @@ func startServiceAccountController(ctx context.Context, controllerContext Contro func newTTLControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.TTLController, + aliases: []string{"ttl"}, initFunc: startTTLController, } } @@ -610,6 +627,7 @@ func startTTLController(ctx context.Context, controllerContext ControllerContext func newGarbageCollectorControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.GarbageCollectorController, + aliases: []string{"garbagecollector"}, initFunc: startGarbageCollectorController, } } @@ -661,6 +679,7 @@ func startGarbageCollectorController(ctx context.Context, controllerContext Cont func newPersistentVolumeClaimProtectionControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PersistentVolumeClaimProtectionController, + aliases: []string{"pvc-protection"}, initFunc: startPersistentVolumeClaimProtectionController, } } @@ -682,6 +701,7 @@ func startPersistentVolumeClaimProtectionController(ctx context.Context, control func newPersistentVolumeProtectionControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.PersistentVolumeProtectionController, + aliases: []string{"pv-protection"}, initFunc: startPersistentVolumeProtectionController, } } @@ -698,6 +718,7 @@ func startPersistentVolumeProtectionController(ctx context.Context, controllerCo func newTTLAfterFinishedControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.TTLAfterFinishedController, + aliases: []string{"ttl-after-finished"}, initFunc: startTTLAfterFinishedController, } } @@ -714,6 +735,7 @@ func startTTLAfterFinishedController(ctx context.Context, controllerContext Cont func newLegacyServiceAccountTokenCleanerControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.LegacyServiceAccountTokenCleanerController, + aliases: []string{"legacy-service-account-token-cleaner"}, initFunc: startLegacyServiceAccountTokenCleanerController, requiredFeatureGates: []featuregate.Feature{ features.LegacyServiceAccountTokenCleanUp, @@ -859,6 +881,7 @@ func setNodeCIDRMaskSizes(cfg nodeipamconfig.NodeIPAMControllerConfiguration, cl func newStorageVersionGarbageCollectorControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.StorageVersionGarbageCollectorController, + aliases: []string{"storage-version-gc"}, initFunc: startStorageVersionGarbageCollectorController, requiredFeatureGates: []featuregate.Feature{ genericfeatures.APIServerIdentity, diff --git a/cmd/kube-controller-manager/app/discovery.go b/cmd/kube-controller-manager/app/discovery.go index 6a31b1f0d1b10..e79fc9b2b932c 100644 --- a/cmd/kube-controller-manager/app/discovery.go +++ b/cmd/kube-controller-manager/app/discovery.go @@ -31,6 +31,7 @@ import ( func newEndpointSliceControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.EndpointSliceController, + aliases: []string{"endpointslice"}, initFunc: startEndpointSliceController, } } @@ -52,6 +53,7 @@ func startEndpointSliceController(ctx context.Context, controllerContext Control func newEndpointSliceMirroringControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.EndpointSliceMirroringController, + aliases: []string{"endpointslicemirroring"}, initFunc: startEndpointSliceMirroringController, } } diff --git a/cmd/kube-controller-manager/app/policy.go b/cmd/kube-controller-manager/app/policy.go index 2fead2bc16a07..0db44ba6e4aca 100644 --- a/cmd/kube-controller-manager/app/policy.go +++ b/cmd/kube-controller-manager/app/policy.go @@ -32,6 +32,7 @@ import ( func newDisruptionControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.DisruptionController, + aliases: []string{"disruption"}, initFunc: startDisruptionController, } } diff --git a/cmd/kube-controller-manager/app/rbac.go b/cmd/kube-controller-manager/app/rbac.go index 594a3817b6f40..c63c61987b4fe 100644 --- a/cmd/kube-controller-manager/app/rbac.go +++ b/cmd/kube-controller-manager/app/rbac.go @@ -27,6 +27,7 @@ import ( func newClusterRoleAggregrationControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ name: names.ClusterRoleAggregationController, + aliases: []string{"clusterrole-aggregation"}, initFunc: startClusterRoleAggregationController, } } diff --git a/cmd/kube-controller-manager/app/testing/testserver.go b/cmd/kube-controller-manager/app/testing/testserver.go index 76abca6c30e9d..51471ed8a60b8 100644 --- a/cmd/kube-controller-manager/app/testing/testserver.go +++ b/cmd/kube-controller-manager/app/testing/testserver.go @@ -33,7 +33,6 @@ import ( "k8s.io/kubernetes/cmd/kube-controller-manager/app" kubecontrollerconfig "k8s.io/kubernetes/cmd/kube-controller-manager/app/config" "k8s.io/kubernetes/cmd/kube-controller-manager/app/options" - "k8s.io/kubernetes/cmd/kube-controller-manager/names" ) func init() { @@ -97,7 +96,7 @@ func StartTestServer(ctx context.Context, customFlags []string) (result TestServ if err != nil { return TestServer{}, err } - all, disabled, aliases := app.KnownControllers(), app.ControllersDisabledByDefault(), names.KCMControllerAliases() + all, disabled, aliases := app.KnownControllers(), app.ControllersDisabledByDefault(), app.ControllerAliases() namedFlagSets := s.Flags(all, disabled, aliases) for _, f := range namedFlagSets.FlagSets { fs.AddFlagSet(f) diff --git a/cmd/kube-controller-manager/names/controller_names.go b/cmd/kube-controller-manager/names/controller_names.go index 42244373110db..cfda67d775f97 100644 --- a/cmd/kube-controller-manager/names/controller_names.go +++ b/cmd/kube-controller-manager/names/controller_names.go @@ -16,8 +16,6 @@ limitations under the License. package names -import cpnames "k8s.io/cloud-provider/names" - // Canonical controller names // // NAMING CONVENTIONS @@ -28,22 +26,21 @@ import cpnames "k8s.io/cloud-provider/names" // CHANGE POLICY // The controller names should be treated as IDs. // They can only be changed if absolutely necessary. For example if an inappropriate name was chosen in the past, or if the scope of the controller changes. -// When a name is changed, the old name should be aliased in KCMControllerAliases, while preserving all old aliases. +// When a name is changed, the old name should be aliased in app.ControllerDescriptor#GetAliases, while preserving all old aliases. // This is done to achieve backwards compatibility // // USE CASES // The following places should use the controller name constants, when: // 1. defining a new app.ControllerDescriptor so it can be used in app.NewControllerDescriptors or app.KnownControllers: -// 2. defining an alias in KCMControllerAliases (for backwards compatibility only) -// 3. used anywhere inside the controller itself: -// 3.1. [TODO] logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X) -// 3.2. [TODO] emitted events should have an EventSource.Component set to the controller name (usually when initializing an EventRecorder) -// 3.3. [TODO] registering ControllerManagerMetrics with ControllerStarted and ControllerStopped -// 3.4. [TODO] calling WaitForNamedCacheSync -// 4. defining controller options for "--help" command or generated documentation -// 1.1. controller name should be used to create a pflag.FlagSet when registering controller options (the name is rendered in a controller flag group header) in options.KubeControllerManagerOptions -// 1.2. when defined flag's help mentions a controller name -// 5. defining a new service account for a new controller (old controllers may have inconsistent service accounts to stay backwards compatible) +// 2. used anywhere inside the controller itself: +// 2.1. [TODO] logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X) +// 2.2. [TODO] emitted events should have an EventSource.Component set to the controller name (usually when initializing an EventRecorder) +// 2.3. [TODO] registering ControllerManagerMetrics with ControllerStarted and ControllerStopped +// 2.4. [TODO] calling WaitForNamedCacheSync +// 3. defining controller options for "--help" command or generated documentation +// 3.1. controller name should be used to create a pflag.FlagSet when registering controller options (the name is rendered in a controller flag group header) in options.KubeControllerManagerOptions +// 3.2. when defined flag's help mentions a controller name +// 4. defining a new service account for a new controller (old controllers may have inconsistent service accounts to stay backwards compatible) const ( ServiceAccountTokenController = "serviceaccount-token-controller" EndpointsController = "endpoints-controller" @@ -85,54 +82,3 @@ const ( LegacyServiceAccountTokenCleanerController = "legacy-serviceaccount-token-cleaner-controller" ValidatingAdmissionPolicyStatusController = "validatingadmissionpolicy-status-controller" ) - -// KCMControllerAliases returns a mapping of aliases to canonical controller names -// -// These aliases ensure backwards compatibility and should never be removed! -// Only addition of new aliases is allowed, and only when a canonical name is changed (please see CHANGE POLICY of controller names) -func KCMControllerAliases() map[string]string { - // return a new reference to achieve immutability of the mapping - return map[string]string{ - "serviceaccount-token": ServiceAccountTokenController, - "endpoint": EndpointsController, - "endpointslice": EndpointSliceController, - "endpointslicemirroring": EndpointSliceMirroringController, - "replicationcontroller": ReplicationControllerController, - "podgc": PodGarbageCollectorController, - "resourcequota": ResourceQuotaController, - "namespace": NamespaceController, - "serviceaccount": ServiceAccountController, - "garbagecollector": GarbageCollectorController, - "daemonset": DaemonSetController, - "job": JobController, - "deployment": DeploymentController, - "replicaset": ReplicaSetController, - "horizontalpodautoscaling": HorizontalPodAutoscalerController, - "disruption": DisruptionController, - "statefulset": StatefulSetController, - "cronjob": CronJobController, - "csrsigning": CertificateSigningRequestSigningController, - "csrapproving": CertificateSigningRequestApprovingController, - "csrcleaner": CertificateSigningRequestCleanerController, - "ttl": TTLController, - "bootstrapsigner": BootstrapSignerController, - "tokencleaner": TokenCleanerController, - "nodeipam": NodeIpamController, - "nodelifecycle": NodeLifecycleController, - "service": cpnames.ServiceLBController, - "route": cpnames.NodeRouteController, - "cloud-node-lifecycle": cpnames.CloudNodeLifecycleController, - "persistentvolume-binder": PersistentVolumeBinderController, - "attachdetach": PersistentVolumeAttachDetachController, - "persistentvolume-expander": PersistentVolumeExpanderController, - "clusterrole-aggregation": ClusterRoleAggregationController, - "pvc-protection": PersistentVolumeClaimProtectionController, - "pv-protection": PersistentVolumeProtectionController, - "ttl-after-finished": TTLAfterFinishedController, - "root-ca-cert-publisher": RootCACertificatePublisherController, - "ephemeral-volume": EphemeralVolumeController, - "storage-version-gc": StorageVersionGarbageCollectorController, - "resource-claim-controller": ResourceClaimController, - "legacy-service-account-token-cleaner": LegacyServiceAccountTokenCleanerController, - } -} From 44cac266673f7aaee5219a3945b2f937bfdd128d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Mon, 18 Sep 2023 23:16:15 +0200 Subject: [PATCH 4/5] move start controller pre- and post- checks/actions out of StartControllers into StartController function the function is reused by ServiceAccountTokenController --- .../app/controllermanager.go | 133 ++++++++++-------- 1 file changed, 71 insertions(+), 62 deletions(-) diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index 51ac74aa6d8e6..302a75250ee6d 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -623,20 +623,18 @@ func CreateControllerContext(logger klog.Logger, s *config.CompletedConfig, root // StartControllers starts a set of controllers with a specified ControllerContext func StartControllers(ctx context.Context, controllerCtx ControllerContext, controllerDescriptors map[string]*ControllerDescriptor, unsecuredMux *mux.PathRecorderMux, healthzHandler *controllerhealthz.MutableHealthzHandler) error { - logger := klog.FromContext(ctx) + var controllerChecks []healthz.HealthChecker // Always start the SA token controller first using a full-power client, since it needs to mint tokens for the rest // If this fails, just return here and fail since other controllers won't be able to get credentials. if serviceAccountTokenControllerDescriptor, ok := controllerDescriptors[names.ServiceAccountTokenController]; ok { - controllerName := serviceAccountTokenControllerDescriptor.Name() - if !controllerCtx.IsControllerEnabled(serviceAccountTokenControllerDescriptor) { - logger.Info("Warning: controller is disabled", "controller", controllerName) - } else { - logger.V(1).Info("Starting controller", "controller", controllerName) - initFunc := serviceAccountTokenControllerDescriptor.GetInitFunc() - if _, _, err := initFunc(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx, controllerName); err != nil { - return err - } + check, err := StartController(ctx, controllerCtx, serviceAccountTokenControllerDescriptor, unsecuredMux) + if err != nil { + return err + } + if check != nil { + // HealthChecker should be present when controller has started + controllerChecks = append(controllerChecks, check) } } @@ -646,83 +644,94 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, cont controllerCtx.Cloud.Initialize(controllerCtx.ClientBuilder, ctx.Done()) } - var controllerChecks []healthz.HealthChecker - // Each controller is passed a context where the logger has the name of // the controller set through WithName. That name then becomes the prefix of // of all log messages emitted by that controller. // - // In this loop, an explicit "controller" key is used instead, for two reasons: + // In StartController, an explicit "controller" key is used instead, for two reasons: // - while contextual logging is alpha, klog.LoggerWithName is still a no-op, // so we cannot rely on it yet to add the name // - it allows distinguishing between log entries emitted by the controller // and those emitted for it - this is a bit debatable and could be revised. - for controllerName, controllerDesc := range controllerDescriptors { + for _, controllerDesc := range controllerDescriptors { if controllerDesc.RequiresSpecialHandling() { continue } - disabledByFeatureGate := false - for _, featureGate := range controllerDesc.GetRequiredFeatureGates() { - if !utilfeature.DefaultFeatureGate.Enabled(featureGate) { - disabledByFeatureGate = true - break - } + check, err := StartController(ctx, controllerCtx, controllerDesc, unsecuredMux) + if err != nil { + return err } - if disabledByFeatureGate { - logger.Info("Controller is disabled by a feature gate", "controller", controllerName, "requiredFeatureGates", controllerDesc.GetRequiredFeatureGates()) - continue + if check != nil { + // HealthChecker should be present when controller has started + controllerChecks = append(controllerChecks, check) } + } - if controllerDesc.IsCloudProviderController() && controllerCtx.LoopMode != IncludeCloudLoops { - logger.Info("Skipping a cloud provider controller", "controller", controllerName, "loopMode", controllerCtx.LoopMode) - continue - } + healthzHandler.AddHealthChecker(controllerChecks...) - if !controllerCtx.IsControllerEnabled(controllerDesc) { - logger.Info("Warning: controller is disabled", "controller", controllerName) - continue + return nil +} + +// StartController starts a controller with a specified ControllerContext +// and performs required pre- and post- checks/actions +func StartController(ctx context.Context, controllerCtx ControllerContext, controllerDescriptor *ControllerDescriptor, + unsecuredMux *mux.PathRecorderMux) (healthz.HealthChecker, error) { + logger := klog.FromContext(ctx) + controllerName := controllerDescriptor.Name() + + for _, featureGate := range controllerDescriptor.GetRequiredFeatureGates() { + if !utilfeature.DefaultFeatureGate.Enabled(featureGate) { + logger.Info("Controller is disabled by a feature gate", "controller", controllerName, "requiredFeatureGates", controllerDescriptor.GetRequiredFeatureGates()) + return nil, nil } + } - time.Sleep(wait.Jitter(controllerCtx.ComponentConfig.Generic.ControllerStartInterval.Duration, ControllerStartJitter)) + if controllerDescriptor.IsCloudProviderController() && controllerCtx.LoopMode != IncludeCloudLoops { + logger.Info("Skipping a cloud provider controller", "controller", controllerName, "loopMode", controllerCtx.LoopMode) + return nil, nil + } - logger.V(1).Info("Starting controller", "controller", controllerName) + if !controllerCtx.IsControllerEnabled(controllerDescriptor) { + logger.Info("Warning: controller is disabled", "controller", controllerName) + return nil, nil + } - initFunc := controllerDesc.GetInitFunc() - ctrl, started, err := initFunc(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx, controllerName) - if err != nil { - logger.Error(err, "Error starting controller", "controller", controllerName) - return err - } - if !started { - logger.Info("Warning: skipping controller", "controller", controllerName) - continue - } - check := controllerhealthz.NamedPingChecker(controllerName) - if ctrl != nil { - // check if the controller supports and requests a debugHandler - // and it needs the unsecuredMux to mount the handler onto. - if debuggable, ok := ctrl.(controller.Debuggable); ok && unsecuredMux != nil { - if debugHandler := debuggable.DebuggingHandler(); debugHandler != nil { - basePath := "/debug/controllers/" + controllerName - unsecuredMux.UnlistedHandle(basePath, http.StripPrefix(basePath, debugHandler)) - unsecuredMux.UnlistedHandlePrefix(basePath+"/", http.StripPrefix(basePath, debugHandler)) - } + time.Sleep(wait.Jitter(controllerCtx.ComponentConfig.Generic.ControllerStartInterval.Duration, ControllerStartJitter)) + + logger.V(1).Info("Starting controller", "controller", controllerName) + + initFunc := controllerDescriptor.GetInitFunc() + ctrl, started, err := initFunc(klog.NewContext(ctx, klog.LoggerWithName(logger, controllerName)), controllerCtx, controllerName) + if err != nil { + logger.Error(err, "Error starting controller", "controller", controllerName) + return nil, err + } + if !started { + logger.Info("Warning: skipping controller", "controller", controllerName) + return nil, nil + } + + check := controllerhealthz.NamedPingChecker(controllerName) + if ctrl != nil { + // check if the controller supports and requests a debugHandler + // and it needs the unsecuredMux to mount the handler onto. + if debuggable, ok := ctrl.(controller.Debuggable); ok && unsecuredMux != nil { + if debugHandler := debuggable.DebuggingHandler(); debugHandler != nil { + basePath := "/debug/controllers/" + controllerName + unsecuredMux.UnlistedHandle(basePath, http.StripPrefix(basePath, debugHandler)) + unsecuredMux.UnlistedHandlePrefix(basePath+"/", http.StripPrefix(basePath, debugHandler)) } - if healthCheckable, ok := ctrl.(controller.HealthCheckable); ok { - if realCheck := healthCheckable.HealthChecker(); realCheck != nil { - check = controllerhealthz.NamedHealthChecker(controllerName, realCheck) - } + } + if healthCheckable, ok := ctrl.(controller.HealthCheckable); ok { + if realCheck := healthCheckable.HealthChecker(); realCheck != nil { + check = controllerhealthz.NamedHealthChecker(controllerName, realCheck) } } - controllerChecks = append(controllerChecks, check) - - logger.Info("Started controller", "controller", controllerName) } - healthzHandler.AddHealthChecker(controllerChecks...) - - return nil + logger.Info("Started controller", "controller", controllerName) + return check, nil } // serviceAccountTokenControllerStarter is special because it must run first to set up permissions for other controllers. From 1591a0e132cba58e92dd620249e1f37992bac837 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Tue, 19 Sep 2023 13:31:02 +0200 Subject: [PATCH 5/5] add unit tests for NewControllerDescriptors - controller descriptors should not be feature gated - aliases should not be defined for new controllers and have only a canonical name --- .../app/controllermanager_test.go | 60 +++++++++++++++++++ cmd/kube-controller-manager/app/core.go | 4 +- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index 40125b687f026..8cd85d899edde 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -21,8 +21,13 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/util/sets" + utilfeature "k8s.io/apiserver/pkg/util/feature" cpnames "k8s.io/cloud-provider/names" + "k8s.io/component-base/featuregate" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/cmd/kube-controller-manager/names" ) @@ -96,3 +101,58 @@ func TestControllerNamesDeclaration(t *testing.T) { func TestNewControllerDescriptorsShouldNotPanic(t *testing.T) { NewControllerDescriptors() } + +func TestNewControllerDescriptorsAlwaysReturnsDescriptorsForAllControllers(t *testing.T) { + controllersWithoutFeatureGates := KnownControllers() + + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, "AllAlpha", true)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, "AllBeta", true)() + + controllersWithFeatureGates := KnownControllers() + + if diff := cmp.Diff(controllersWithoutFeatureGates, controllersWithFeatureGates); diff != "" { + t.Errorf("unexpected controllers after enabling feature gates, NewControllerDescriptors should always return all controller descriptors. Controllers should define required feature gates in ControllerDescriptor.requiredFeatureGates. Diff of returned controllers:\n%s", diff) + } +} + +func TestFeatureGatedControllersShouldNotDefineAliases(t *testing.T) { + featureGateRegex := regexp.MustCompile("^([a-zA-Z0-9]+)") + + alphaFeatures := sets.NewString() + for _, featureText := range utilfeature.DefaultFeatureGate.KnownFeatures() { + // we have to parse this from KnownFeatures, because usage of mutable FeatureGate is not allowed in unit tests + feature := featureGateRegex.FindString(featureText) + if strings.Contains(featureText, string(featuregate.Alpha)) && feature != "AllAlpha" { + alphaFeatures.Insert(feature) + } + + } + + for name, controller := range NewControllerDescriptors() { + if len(controller.GetAliases()) == 0 { + continue + } + + requiredFeatureGates := controller.GetRequiredFeatureGates() + if len(requiredFeatureGates) == 0 { + continue + } + + // DO NOT ADD any new controllers here. These two controllers are an exception, because they were added before this test was introduced + if name == names.LegacyServiceAccountTokenCleanerController || name == names.ResourceClaimController { + continue + } + + areAllRequiredFeaturesAlpha := true + for _, feature := range requiredFeatureGates { + if !alphaFeatures.Has(string(feature)) { + areAllRequiredFeaturesAlpha = false + break + } + } + + if areAllRequiredFeaturesAlpha { + t.Errorf("alias check failed: controller name %q should not be aliased as it is still guarded by alpha feature gates (%v) and thus should have only a canonical name", name, requiredFeatureGates) + } + } +} diff --git a/cmd/kube-controller-manager/app/core.go b/cmd/kube-controller-manager/app/core.go index 47e01cddc2ada..1dc4ea400d4ca 100644 --- a/cmd/kube-controller-manager/app/core.go +++ b/cmd/kube-controller-manager/app/core.go @@ -427,7 +427,7 @@ func newResourceClaimControllerDescriptor() *ControllerDescriptor { aliases: []string{"resource-claim-controller"}, initFunc: startResourceClaimController, requiredFeatureGates: []featuregate.Feature{ - features.DynamicResourceAllocation, + features.DynamicResourceAllocation, // TODO update app.TestFeatureGatedControllersShouldNotDefineAliases when removing this feature }, } } @@ -738,7 +738,7 @@ func newLegacyServiceAccountTokenCleanerControllerDescriptor() *ControllerDescri aliases: []string{"legacy-service-account-token-cleaner"}, initFunc: startLegacyServiceAccountTokenCleanerController, requiredFeatureGates: []featuregate.Feature{ - features.LegacyServiceAccountTokenCleanUp, + features.LegacyServiceAccountTokenCleanUp, // TODO update app.TestFeatureGatedControllersShouldNotDefineAliases when removing this feature }, } }