From d1d05d3eba7d5627e9d010a5d3cce750ef974c99 Mon Sep 17 00:00:00 2001 From: elmiko Date: Thu, 26 Sep 2024 14:55:16 -0400 Subject: [PATCH 1/2] remove IsDeprecatedInternal from cloudprovider.plugins The internal cloud controller loops are disabled at this point, this function should not be used as it does not return accurate information. In its place we check for the presence of the external cloud provider as that is the only acceptable value. --- cmd/kube-controller-manager/app/cloudproviders.go | 2 +- pkg/kubeapiserver/options/cloudprovider.go | 2 +- pkg/kubelet/kubelet.go | 2 +- staging/src/k8s.io/cloud-provider/plugins.go | 12 ------------ 4 files changed, 3 insertions(+), 15 deletions(-) diff --git a/cmd/kube-controller-manager/app/cloudproviders.go b/cmd/kube-controller-manager/app/cloudproviders.go index f93890d24c4..562e478f2d5 100644 --- a/cmd/kube-controller-manager/app/cloudproviders.go +++ b/cmd/kube-controller-manager/app/cloudproviders.go @@ -35,7 +35,7 @@ func createCloudProvider(logger klog.Logger, cloudProvider string, externalCloud var loopMode ControllerLoopMode var err error - if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && cloudprovider.IsDeprecatedInternal(cloudProvider) { + if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && !cloudprovider.IsExternal(cloudProvider) { cloudprovider.DisableWarningForProvider(cloudProvider) return nil, ExternalLoops, fmt.Errorf( "cloud provider %q was specified, but built-in cloud providers are disabled. Please set --cloud-provider=external and migrate to an external cloud provider", diff --git a/pkg/kubeapiserver/options/cloudprovider.go b/pkg/kubeapiserver/options/cloudprovider.go index 6977e8dd16b..716e0651e6b 100644 --- a/pkg/kubeapiserver/options/cloudprovider.go +++ b/pkg/kubeapiserver/options/cloudprovider.go @@ -51,7 +51,7 @@ func (opts *CloudProviderOptions) Validate() []error { errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ "please set DisableKubeletCloudCredentialProviders feature to true", opts.CloudProvider)) } - case cloudprovider.IsDeprecatedInternal(opts.CloudProvider): + case !cloudprovider.IsExternal(opts.CloudProvider): if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) { errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ "please set DisableCloudProviders feature to false", opts.CloudProvider)) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index df9a401b5c0..516f9d5014d 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -386,7 +386,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration, return nil, fmt.Errorf("invalid sync frequency %d", kubeCfg.SyncFrequency.Duration) } - if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && cloudprovider.IsDeprecatedInternal(cloudProvider) { + if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && !cloudprovider.IsExternal(cloudProvider) { cloudprovider.DisableWarningForProvider(cloudProvider) return nil, fmt.Errorf("cloud provider %q was specified, but built-in cloud providers are disabled. Please set --cloud-provider=external and migrate to an external cloud provider", cloudProvider) } diff --git a/staging/src/k8s.io/cloud-provider/plugins.go b/staging/src/k8s.io/cloud-provider/plugins.go index 458fff69a83..718016a2c87 100644 --- a/staging/src/k8s.io/cloud-provider/plugins.go +++ b/staging/src/k8s.io/cloud-provider/plugins.go @@ -85,18 +85,6 @@ func IsExternal(name string) bool { return name == externalCloudProvider } -// IsDeprecatedInternal is responsible for preventing cloud.Interface -// from being initialized in kubelet, kube-controller-manager or kube-api-server -func IsDeprecatedInternal(name string) bool { - for _, provider := range deprecatedCloudProviders { - if provider.name == name { - return true - } - } - - return false -} - // DisableWarningForProvider logs information about disabled cloud provider state func DisableWarningForProvider(providerName string) { for _, provider := range deprecatedCloudProviders { From 38fe239ac44050c2ed83ffb8ea7e514db71f903f Mon Sep 17 00:00:00 2001 From: elmiko Date: Fri, 27 Sep 2024 10:03:18 -0400 Subject: [PATCH 2/2] factor our cloudprovider.DeprecationWarningForProvider this change removes the deprecation warning function in favor of using the `cloudprovider.DisableWarningForProvider`. it also fixes some of the logic to ensure that non-external providers are properly detected and warned about. --- .../app/cloudproviders.go | 27 ++++++------ cmd/kubelet/app/server.go | 14 ++----- pkg/kubeapiserver/options/cloudprovider.go | 11 +---- pkg/kubelet/kubelet.go | 2 +- staging/src/k8s.io/cloud-provider/plugins.go | 41 +++++-------------- 5 files changed, 28 insertions(+), 67 deletions(-) diff --git a/cmd/kube-controller-manager/app/cloudproviders.go b/cmd/kube-controller-manager/app/cloudproviders.go index 562e478f2d5..ab259f5b17f 100644 --- a/cmd/kube-controller-manager/app/cloudproviders.go +++ b/cmd/kube-controller-manager/app/cloudproviders.go @@ -19,9 +19,7 @@ package app import ( "fmt" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog/v2" - "k8s.io/kubernetes/pkg/features" "k8s.io/client-go/informers" cloudprovider "k8s.io/cloud-provider" @@ -32,18 +30,10 @@ import ( 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 loopMode ControllerLoopMode var err error - - if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && !cloudprovider.IsExternal(cloudProvider) { - cloudprovider.DisableWarningForProvider(cloudProvider) - return nil, ExternalLoops, fmt.Errorf( - "cloud provider %q was specified, but built-in cloud providers are disabled. Please set --cloud-provider=external and migrate to an external cloud provider", - cloudProvider) - } + loopMode := ExternalLoops if cloudprovider.IsExternal(cloudProvider) { - loopMode = ExternalLoops 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. @@ -51,10 +41,17 @@ func createCloudProvider(logger klog.Logger, cloudProvider string, externalCloud } cloud, err = cloudprovider.InitCloudProvider(externalCloudVolumePlugin, cloudConfigFile) } else { - cloudprovider.DeprecationWarningForProvider(cloudProvider) - - loopMode = IncludeCloudLoops - cloud, err = cloudprovider.InitCloudProvider(cloudProvider, cloudConfigFile) + // 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) diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go index 5e2300ddbb8..11bcb069a53 100644 --- a/cmd/kubelet/app/server.go +++ b/cmd/kubelet/app/server.go @@ -651,16 +651,10 @@ func run(ctx context.Context, s *options.KubeletServer, kubeDeps *kubelet.Depend } if kubeDeps.Cloud == nil { - if !cloudprovider.IsExternal(s.CloudProvider) { - cloudprovider.DeprecationWarningForProvider(s.CloudProvider) - cloud, err := cloudprovider.InitCloudProvider(s.CloudProvider, s.CloudConfigFile) - if err != nil { - return err - } - if cloud != nil { - klog.V(2).InfoS("Successfully initialized cloud provider", "cloudProvider", s.CloudProvider, "cloudConfigFile", s.CloudConfigFile) - } - kubeDeps.Cloud = cloud + if !cloudprovider.IsExternal(s.CloudProvider) && len(s.CloudProvider) != 0 { + // internal cloud provider loops are disabled + cloudprovider.DisableWarningForProvider(s.CloudProvider) + return cloudprovider.ErrorForDisabledProvider(s.CloudProvider) } } diff --git a/pkg/kubeapiserver/options/cloudprovider.go b/pkg/kubeapiserver/options/cloudprovider.go index 716e0651e6b..5fd96aa1dbb 100644 --- a/pkg/kubeapiserver/options/cloudprovider.go +++ b/pkg/kubeapiserver/options/cloudprovider.go @@ -42,7 +42,7 @@ func (opts *CloudProviderOptions) Validate() []error { switch { case opts.CloudProvider == "": - case opts.CloudProvider == "external": + case cloudprovider.IsExternal(opts.CloudProvider): if !utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) { errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ "please set DisableCloudProviders feature to true", opts.CloudProvider)) @@ -51,15 +51,6 @@ func (opts *CloudProviderOptions) Validate() []error { errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ "please set DisableKubeletCloudCredentialProviders feature to true", opts.CloudProvider)) } - case !cloudprovider.IsExternal(opts.CloudProvider): - if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) { - errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ - "please set DisableCloudProviders feature to false", opts.CloudProvider)) - } - if utilfeature.DefaultFeatureGate.Enabled(features.DisableKubeletCloudCredentialProviders) { - errs = append(errs, fmt.Errorf("when using --cloud-provider set to '%s', "+ - "please set DisableKubeletCloudCredentialProviders feature to false", opts.CloudProvider)) - } default: errs = append(errs, fmt.Errorf("unknown --cloud-provider: %s", opts.CloudProvider)) } diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 516f9d5014d..81e9ce4c4cd 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -386,7 +386,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration, return nil, fmt.Errorf("invalid sync frequency %d", kubeCfg.SyncFrequency.Duration) } - if utilfeature.DefaultFeatureGate.Enabled(features.DisableCloudProviders) && !cloudprovider.IsExternal(cloudProvider) { + if !cloudprovider.IsExternal(cloudProvider) && len(cloudProvider) != 0 { cloudprovider.DisableWarningForProvider(cloudProvider) return nil, fmt.Errorf("cloud provider %q was specified, but built-in cloud providers are disabled. Please set --cloud-provider=external and migrate to an external cloud provider", cloudProvider) } diff --git a/staging/src/k8s.io/cloud-provider/plugins.go b/staging/src/k8s.io/cloud-provider/plugins.go index 718016a2c87..8ef778b1785 100644 --- a/staging/src/k8s.io/cloud-provider/plugins.go +++ b/staging/src/k8s.io/cloud-provider/plugins.go @@ -33,13 +33,8 @@ type Factory func(config io.Reader) (Interface, error) // All registered cloud providers. var ( - providersMutex sync.Mutex - providers = make(map[string]Factory) - deprecatedCloudProviders = []struct { - name string - external bool - detail string - }{} + providersMutex sync.Mutex + providers = make(map[string]Factory) ) const externalCloudProvider = "external" @@ -87,33 +82,17 @@ func IsExternal(name string) bool { // DisableWarningForProvider logs information about disabled cloud provider state func DisableWarningForProvider(providerName string) { - for _, provider := range deprecatedCloudProviders { - if provider.name == providerName { - klog.Infof("INFO: Please make sure you are running external cloud controller manager binary for provider %q."+ - "In-tree cloud providers are currently disabled. Refer to https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/cloud-provider/sample"+ - "for example implementation.", providerName) - detail := fmt.Sprintf("Please reach to sig-cloud-provider and use 'external' cloud provider for %q: %s", providerName, provider.detail) - klog.Warningf("WARNING: %q built-in cloud provider is now disabled. %s", providerName, detail) - break - } + if !IsExternal(providerName) { + klog.Infof("INFO: Please make sure you are running an external cloud controller manager binary for provider %q."+ + "In-tree cloud providers are disabled. Refer to https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/cloud-provider/sample "+ + "for an example implementation.", providerName) + klog.Warningf("WARNING: built-in cloud providers are disabled. Please set \"--cloud-provider=external\" and migrate to an external cloud controller manager for provider %q", providerName) } } -// DeprecationWarningForProvider logs information about deprecated cloud provider state -func DeprecationWarningForProvider(providerName string) { - for _, provider := range deprecatedCloudProviders { - if provider.name != providerName { - continue - } - - detail := provider.detail - if provider.external { - detail = fmt.Sprintf("Please use 'external' cloud provider for %s: %s", providerName, provider.detail) - } - - klog.Warningf("WARNING: %s built-in cloud provider is now deprecated. %s", providerName, detail) - break - } +// ErrorForDisabledProvider returns an error formatted with the supplied provider name +func ErrorForDisabledProvider(providerName string) error { + return fmt.Errorf("cloud provider %q was specified, but built-in cloud providers are disabled. Please set --cloud-provider=external and migrate to an external cloud provider", providerName) } // InitCloudProvider creates an instance of the named cloud provider.