From 8d6769f62c32090c47caf29579ee8831e16c1676 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Tue, 22 Oct 2024 14:04:48 +0000 Subject: [PATCH] disable cloud-provider code from kube-controller-manager Since 1.31 the core component cloud provider logic should not exist, this disables the existing code in the kube-controller-manager that still expects to work with the cloud-provider logic to avoid having time bombs in the code base that can break the component. The code can not be completely removed because this will impact existing users that may be using some of the flags breaking their deployments, so this just removes the code that is no longer to be used becuase it depends on options that no longer are exposed to users. It also adds validation on the configuration/flag level to ensure that the --cloud-provider flag can only be set to external or empty. --- cluster/gce/gci/configure-helper.sh | 7 -- .../app/cloudproviders.go | 72 ---------------- .../app/controllermanager.go | 38 +-------- .../app/controllermanager_test.go | 5 +- cmd/kube-controller-manager/app/core.go | 84 ++----------------- .../app/options/options.go | 12 +++ .../app/options/options_test.go | 70 ++++++++++++++++ 7 files changed, 93 insertions(+), 195 deletions(-) delete mode 100644 cmd/kube-controller-manager/app/cloudproviders.go diff --git a/cluster/gce/gci/configure-helper.sh b/cluster/gce/gci/configure-helper.sh index 2fe81511997..ad8aa3764de 100755 --- a/cluster/gce/gci/configure-helper.sh +++ b/cluster/gce/gci/configure-helper.sh @@ -2223,9 +2223,6 @@ function start-kube-controller-manager { if [[ -n "${SERVICE_CLUSTER_IP_RANGE:-}" ]]; then params+=("--service-cluster-ip-range=${SERVICE_CLUSTER_IP_RANGE}") fi - if [[ -n "${CONCURRENT_SERVICE_SYNCS:-}" ]]; then - params+=("--concurrent-service-syncs=${CONCURRENT_SERVICE_SYNCS}") - fi if [[ "${NETWORK_PROVIDER:-}" == "kubenet" ]]; then params+=("--allocate-node-cidrs=true") elif [[ -n "${ALLOCATE_NODE_CIDRS:-}" ]]; then @@ -2234,10 +2231,6 @@ function start-kube-controller-manager { if [[ -n "${TERMINATED_POD_GC_THRESHOLD:-}" ]]; then params+=("--terminated-pod-gc-threshold=${TERMINATED_POD_GC_THRESHOLD}") fi - if [[ "${ENABLE_IP_ALIASES:-}" == 'true' ]]; then - params+=("--cidr-allocator-type=${NODE_IPAM_MODE}") - params+=("--configure-cloud-routes=false") - fi if [[ -n "${FEATURE_GATES:-}" ]]; then params+=("--feature-gates=${FEATURE_GATES}") fi diff --git a/cmd/kube-controller-manager/app/cloudproviders.go b/cmd/kube-controller-manager/app/cloudproviders.go deleted file mode 100644 index ab259f5b17f..00000000000 --- a/cmd/kube-controller-manager/app/cloudproviders.go +++ /dev/null @@ -1,72 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package app - -import ( - "fmt" - - "k8s.io/klog/v2" - - "k8s.io/client-go/informers" - cloudprovider "k8s.io/cloud-provider" -) - -// createCloudProvider helps consolidate what is needed for cloud providers, we explicitly list the things -// that the cloud providers need as parameters, so we can control -func createCloudProvider(logger klog.Logger, cloudProvider string, externalCloudVolumePlugin string, cloudConfigFile string, - allowUntaggedCloud bool, sharedInformers informers.SharedInformerFactory) (cloudprovider.Interface, ControllerLoopMode, error) { - var cloud cloudprovider.Interface - var err error - loopMode := ExternalLoops - - if cloudprovider.IsExternal(cloudProvider) { - if externalCloudVolumePlugin == "" { - // externalCloudVolumePlugin is temporary until we split all cloud providers out. - // So we just tell the caller that we need to run ExternalLoops without any cloud provider. - return nil, loopMode, nil - } - cloud, err = cloudprovider.InitCloudProvider(externalCloudVolumePlugin, cloudConfigFile) - } else { - // in the case where the cloudProvider is not set, we need to inform the caller that there - // is no cloud provider and that the default loops should be used, and there is no error. - // this will cause the kube-controller-manager to start the default controller contexts - // without being attached to a specific cloud. - if len(cloudProvider) == 0 { - loopMode = IncludeCloudLoops - } else { - // for all other cloudProvider values the internal cloud loops are disabled - cloudprovider.DisableWarningForProvider(cloudProvider) - err = cloudprovider.ErrorForDisabledProvider(cloudProvider) - } - } - if err != nil { - return nil, loopMode, fmt.Errorf("cloud provider could not be initialized: %v", err) - } - - if cloud != nil && !cloud.HasClusterID() { - if allowUntaggedCloud { - logger.Info("Warning: detected a cluster without a ClusterID. A ClusterID will be required in the future. Please tag your cluster to avoid any future issues") - } else { - return nil, loopMode, fmt.Errorf("no ClusterID Found. A ClusterID is required for the cloud provider to function properly. This check can be bypassed by setting the allow-untagged-cloud option") - } - } - - if informerUserCloud, ok := cloud.(cloudprovider.InformerUser); ok { - informerUserCloud.SetInformers(sharedInformers) - } - return cloud, loopMode, err -} diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index 99332e92b9f..593ed03028b 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -53,7 +53,6 @@ import ( "k8s.io/client-go/tools/leaderelection/resourcelock" certutil "k8s.io/client-go/util/cert" "k8s.io/client-go/util/keyutil" - cloudprovider "k8s.io/cloud-provider" cliflag "k8s.io/component-base/cli/flag" "k8s.io/component-base/cli/globalflag" "k8s.io/component-base/configz" @@ -95,16 +94,6 @@ const ( ConfigzName = "kubecontrollermanager.config.k8s.io" ) -// ControllerLoopMode is the kube-controller-manager's mode of running controller loops that are cloud provider dependent -type ControllerLoopMode int - -const ( - // IncludeCloudLoops means the kube-controller-manager include the controller loops that are cloud provider dependent - IncludeCloudLoops ControllerLoopMode = iota - // ExternalLoops means the kube-controller-manager exclude the controller loops that are cloud provider dependent - ExternalLoops -) - // NewControllerManagerCommand creates a *cobra.Command object with default parameters func NewControllerManagerCommand() *cobra.Command { _, _ = utilversion.DefaultComponentGlobalsRegistry.ComponentGlobalsOrRegister( @@ -396,15 +385,6 @@ type ControllerContext struct { // requested. RESTMapper *restmapper.DeferredDiscoveryRESTMapper - // Cloud is the cloud provider interface for the controllers to use. - // It must be initialized and ready to use. - Cloud cloudprovider.Interface - - // Control for which control loops to be run - // IncludeCloudLoops is for a kube-controller-manager running all loops - // ExternalLoops is for a kube-controller-manager running with a cloud-controller-manager - LoopMode ControllerLoopMode - // InformersStarted is closed after all of the controllers have been initialized and are running. After this point it is safe, // for an individual controller to start the shared informers. Before it is closed, they should not. InformersStarted chan struct{} @@ -644,20 +624,12 @@ func CreateControllerContext(ctx context.Context, s *config.CompletedConfig, roo restMapper.Reset() }, 30*time.Second, ctx.Done()) - cloud, loopMode, err := createCloudProvider(klog.FromContext(ctx), s.ComponentConfig.KubeCloudShared.CloudProvider.Name, s.ComponentConfig.KubeCloudShared.ExternalCloudVolumePlugin, - s.ComponentConfig.KubeCloudShared.CloudProvider.CloudConfigFile, s.ComponentConfig.KubeCloudShared.AllowUntaggedCloud, sharedInformers) - if err != nil { - return ControllerContext{}, err - } - controllerContext := ControllerContext{ ClientBuilder: clientBuilder, InformerFactory: sharedInformers, ObjectOrMetadataInformerFactory: informerfactory.NewInformerFactory(sharedInformers, metadataInformers), ComponentConfig: s.ComponentConfig, RESTMapper: restMapper, - Cloud: cloud, - LoopMode: loopMode, InformersStarted: make(chan struct{}), ResyncPeriod: ResyncPeriod(s), ControllerManagerMetrics: controllersmetrics.NewControllerManagerMetrics("kube-controller-manager"), @@ -702,12 +674,6 @@ func StartControllers(ctx context.Context, controllerCtx ControllerContext, cont } } - // Initialize the cloud provider with a reference to the clientBuilder only after token controller - // has started in case the cloud provider uses the client builder. - if controllerCtx.Cloud != nil { - controllerCtx.Cloud.Initialize(controllerCtx.ClientBuilder, ctx.Done()) - } - // 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. @@ -751,8 +717,8 @@ func StartController(ctx context.Context, controllerCtx ControllerContext, contr } } - if controllerDescriptor.IsCloudProviderController() && controllerCtx.LoopMode != IncludeCloudLoops { - logger.Info("Skipping a cloud provider controller", "controller", controllerName, "loopMode", controllerCtx.LoopMode) + if controllerDescriptor.IsCloudProviderController() { + logger.Info("Skipping a cloud provider controller", "controller", controllerName) return nil, nil } diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index be77788b4c9..c20cd65f424 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -225,10 +225,7 @@ func TestNoCloudProviderControllerStarted(t *testing.T) { ctx, cancel := context.WithCancel(ctx) defer cancel() - controllerCtx := ControllerContext{ - Cloud: nil, - LoopMode: IncludeCloudLoops, - } + controllerCtx := ControllerContext{} controllerCtx.ComponentConfig.Generic.Controllers = []string{"*"} for _, controller := range NewControllerDescriptors() { if !controller.IsCloudProviderController() { diff --git a/cmd/kube-controller-manager/app/core.go b/cmd/kube-controller-manager/app/core.go index 6d348317ee3..3c517479781 100644 --- a/cmd/kube-controller-manager/app/core.go +++ b/cmd/kube-controller-manager/app/core.go @@ -37,9 +37,6 @@ import ( clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/metadata" restclient "k8s.io/client-go/rest" - 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" @@ -93,26 +90,8 @@ func newServiceLBControllerDescriptor() *ControllerDescriptor { func startServiceLBController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) - if controllerContext.Cloud == nil { - logger.Info("Warning: service-controller is set, but no cloud provider specified. Will not configure service controller.") - return nil, false, nil - } - - serviceController, err := servicecontroller.New( - controllerContext.Cloud, - controllerContext.ClientBuilder.ClientOrDie("service-controller"), - controllerContext.InformerFactory.Core().V1().Services(), - controllerContext.InformerFactory.Core().V1().Nodes(), - controllerContext.ComponentConfig.KubeCloudShared.ClusterName, - utilfeature.DefaultFeatureGate, - ) - if err != nil { - // This error shouldn't fail. It lives like this as a legacy. - logger.Error(err, "Failed to start service controller.") - return nil, false, nil - } - go serviceController.Run(ctx, int(controllerContext.ComponentConfig.ServiceController.ConcurrentServiceSyncs), controllerContext.ControllerManagerMetrics) - return nil, true, nil + logger.Info("Warning: service-controller is set, but no cloud provider functionality is available in kube-controller-manger (KEP-2395). Will not configure service controller.") + return nil, false, nil } func newNodeIpamControllerDescriptor() *ControllerDescriptor { return &ControllerDescriptor{ @@ -134,11 +113,7 @@ func startNodeIpamController(ctx context.Context, controllerContext ControllerCo if controllerContext.ComponentConfig.KubeCloudShared.CIDRAllocatorType == string(ipam.CloudAllocatorType) { // Cannot run cloud ipam controller if cloud provider is nil (--cloud-provider not set or set to 'external') - if controllerContext.Cloud == nil { - return nil, false, errors.New("--cidr-allocator-type is set to 'CloudAllocator' but cloud provider is not configured") - } - // As part of the removal of all the cloud providers from kubernetes, this support will be removed as well - klog.Warningf("DEPRECATED: 'CloudAllocator' bas been deprecated and will be removed in a future release.") + return nil, false, errors.New("--cidr-allocator-type is set to 'CloudAllocator' but cloud provider is not configured") } clusterCIDRs, err := validateCIDRs(controllerContext.ComponentConfig.KubeCloudShared.ClusterCIDR) @@ -183,7 +158,7 @@ func startNodeIpamController(ctx context.Context, controllerContext ControllerCo nodeIpamController, err := nodeipamcontroller.NewNodeIpamController( ctx, controllerContext.InformerFactory.Core().V1().Nodes(), - controllerContext.Cloud, + nil, // no cloud provider on kube-controller-manager since v1.31 (KEP-2395) controllerContext.ClientBuilder.ClientOrDie("node-controller"), clusterCIDRs, serviceCIDR, @@ -267,27 +242,8 @@ func newCloudNodeLifecycleControllerDescriptor() *ControllerDescriptor { func startCloudNodeLifecycleController(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller.Interface, bool, error) { logger := klog.FromContext(ctx) - if controllerContext.Cloud == nil { - logger.Info("Warning: node-controller is set, but no cloud provider specified. Will not configure node lifecyle controller.") - return nil, false, nil - } - - cloudNodeLifecycleController, err := cloudnodelifecyclecontroller.NewCloudNodeLifecycleController( - controllerContext.InformerFactory.Core().V1().Nodes(), - // cloud node lifecycle controller uses existing cluster role from node-controller - controllerContext.ClientBuilder.ClientOrDie("node-controller"), - controllerContext.Cloud, - controllerContext.ComponentConfig.KubeCloudShared.NodeMonitorPeriod.Duration, - ) - if err != nil { - // the controller manager should continue to run if the "Instances" interface is not - // supported, though it's unlikely for a cloud provider to not support it - logger.Error(err, "Failed to start cloud node lifecycle controller") - return nil, false, nil - } - - go cloudNodeLifecycleController.Run(ctx, controllerContext.ControllerManagerMetrics) - return nil, true, nil + logger.Info("Warning: node-controller is set, but no cloud provider functionality is available in kube-controller-manger (KEP-2395). Will not configure node lifecyle controller.") + return nil, false, nil } func newNodeRouteControllerDescriptor() *ControllerDescriptor { @@ -301,32 +257,8 @@ func newNodeRouteControllerDescriptor() *ControllerDescriptor { 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) - return nil, false, nil - } - if controllerContext.Cloud == nil { - logger.Info("Warning: configure-cloud-routes is set, but no cloud provider specified. Will not configure cloud provider routes.") - return nil, false, nil - } - routes, ok := controllerContext.Cloud.Routes() - if !ok { - logger.Info("Warning: configure-cloud-routes is set, but cloud provider does not support routes. Will not configure cloud provider routes.") - return nil, false, nil - } - - clusterCIDRs, err := validateCIDRs(controllerContext.ComponentConfig.KubeCloudShared.ClusterCIDR) - if err != nil { - return nil, false, err - } - - routeController := routecontroller.New(routes, - controllerContext.ClientBuilder.ClientOrDie("route-controller"), - controllerContext.InformerFactory.Core().V1().Nodes(), - controllerContext.ComponentConfig.KubeCloudShared.ClusterName, - clusterCIDRs) - go routeController.Run(ctx, controllerContext.ComponentConfig.KubeCloudShared.RouteReconciliationPeriod.Duration, controllerContext.ControllerManagerMetrics) - return nil, true, nil + logger.Info("Warning: configure-cloud-routes is set, but no cloud provider functionality is available in kube-controller-manger (KEP-2395). Will not configure cloud provider routes.") + return nil, false, nil } func newPersistentVolumeBinderControllerDescriptor() *ControllerDescriptor { diff --git a/cmd/kube-controller-manager/app/options/options.go b/cmd/kube-controller-manager/app/options/options.go index 9497fa1f27f..851dac6c10e 100644 --- a/cmd/kube-controller-manager/app/options/options.go +++ b/cmd/kube-controller-manager/app/options/options.go @@ -33,6 +33,7 @@ import ( restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" "k8s.io/client-go/tools/record" + cloudprovider "k8s.io/cloud-provider" cpnames "k8s.io/cloud-provider/names" cpoptions "k8s.io/cloud-provider/options" cliflag "k8s.io/component-base/cli/flag" @@ -50,6 +51,7 @@ import ( kubectrlmgrconfigscheme "k8s.io/kubernetes/pkg/controller/apis/config/scheme" "k8s.io/kubernetes/pkg/controller/garbagecollector" garbagecollectorconfig "k8s.io/kubernetes/pkg/controller/garbagecollector/config" + "k8s.io/kubernetes/pkg/controller/nodeipam/ipam" netutils "k8s.io/utils/net" // add the kubernetes feature gates @@ -452,6 +454,16 @@ func (s *KubeControllerManagerOptions) Validate(allControllers []string, disable errs = append(errs, s.Metrics.Validate()...) errs = append(errs, utilversion.ValidateKubeEffectiveVersion(s.ComponentGlobalsRegistry.EffectiveVersionFor(utilversion.DefaultKubeComponent))) + // in-tree cloud providers are disabled since v1.31 (KEP-2395) + if len(s.KubeCloudShared.CloudProvider.Name) > 0 && !cloudprovider.IsExternal(s.KubeCloudShared.CloudProvider.Name) { + cloudprovider.DisableWarningForProvider(s.KubeCloudShared.CloudProvider.Name) + errs = append(errs, cloudprovider.ErrorForDisabledProvider(s.KubeCloudShared.CloudProvider.Name)) + } + + if len(s.KubeCloudShared.CIDRAllocatorType) > 0 && s.KubeCloudShared.CIDRAllocatorType != string(ipam.RangeAllocatorType) { + errs = append(errs, fmt.Errorf("built-in cloud providers are disabled. The ipam %s is not available", s.KubeCloudShared.CIDRAllocatorType)) + } + // TODO: validate component config, master and kubeconfig return utilerrors.NewAggregate(errs) diff --git a/cmd/kube-controller-manager/app/options/options_test.go b/cmd/kube-controller-manager/app/options/options_test.go index 82638720ca1..08b600f9b2c 100644 --- a/cmd/kube-controller-manager/app/options/options_test.go +++ b/cmd/kube-controller-manager/app/options/options_test.go @@ -461,6 +461,76 @@ func TestAddFlags(t *testing.T) { } } +func TestValidateFlags(t *testing.T) { + testcases := []struct { + name string + flags []string // not a good place to test flagParse error + wantErr bool // this won't apply to flagParse, it only apply to KubeControllerManagerOptions.Validate + }{ + { + name: "empty flags", + flags: []string{}, + wantErr: false, + }, + { + name: "cloud provider empty flag", + flags: []string{ + "--cloud-provider", "", + }, + wantErr: false, + }, + { + name: "cloud provider set but not external", + flags: []string{ + "--cloud-provider=gce", + }, + wantErr: true, + }, + { + name: "cloud provider set to external", + flags: []string{ + "--cloud-provider=external", + }, + wantErr: false, + }, + { + name: "nodeipam to cloudAllocator", + flags: []string{ + "--cidr-allocator-type=CloudAllocator", + }, + wantErr: true, + }, + { + name: "nodeipam to rangeAllocator", + flags: []string{ + "--cidr-allocator-type=RangeAllocator", + }, + wantErr: false, + }, + { + name: "nodeipam to IPAMFromCloud", + flags: []string{ + "--cidr-allocator-type=IPAMFromCloud", + }, + wantErr: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + fs, s := setupControllerManagerFlagSet(t) + err := fs.Parse(tc.flags) + checkTestError(t, err, false, "") + err = s.Validate([]string{""}, []string{""}, nil) + if !tc.wantErr && err != nil { + t.Fatal(fmt.Errorf("expected no error, got %w", err)) + } else if tc.wantErr && err == nil { + t.Fatal("expected error, got nil") + } + }) + } +} + func TestApplyTo(t *testing.T) { fs, s := setupControllerManagerFlagSet(t)