From 6b08919473a5dc4224239cebc983bea9308ffce7 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Fri, 1 Mar 2024 07:09:33 +0000 Subject: [PATCH] Revert "[cloud-provider] require providerID to initialize node" This reverts commit 03bd3e25b16666a6c89d8875782d1132819eeb6e. --- .../controllers/node/node_controller.go | 15 --- .../controllers/node/node_controller_test.go | 99 ++----------------- .../pkg/features/kube_features.go | 8 -- 3 files changed, 10 insertions(+), 112 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go index 4f0f4d31871..b31c04d4ecf 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" - utilfeature "k8s.io/apiserver/pkg/util/feature" coreinformers "k8s.io/client-go/informers/core/v1" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" @@ -45,7 +44,6 @@ import ( cloudnodeutil "k8s.io/cloud-provider/node/helpers" controllersmetrics "k8s.io/component-base/metrics/prometheus/controllers" nodeutil "k8s.io/component-helpers/node/util" - "k8s.io/controller-manager/pkg/features" "k8s.io/klog/v2" ) @@ -483,19 +481,6 @@ func (cnc *CloudNodeController) syncNode(ctx context.Context, nodeName string) e modify(newNode) } - // spec.ProviderID is required for multiple controllers, like loadbalancers, so we should not - // untaint the node until is set. Once it is set, the field is immutable, so no need to reconcile. - // We only set this value during initialization and is never reconciled, so if for some reason - // we are not able to set it, the instance will never be able to acquire it. - // Before external cloud providers were enabled by default, the field was set by the kubelet, and the - // node was created with the value. - // xref: https://issues.k8s.io/123024 - if !utilfeature.DefaultFeatureGate.Enabled(features.OptionalProviderID) { - if newNode.Spec.ProviderID == "" { - return fmt.Errorf("failed to get provider ID for node %s at cloudprovider", nodeName) - } - } - _, err = cnc.kubeClient.CoreV1().Nodes().Update(context.TODO(), newNode, metav1.UpdateOptions{}) if err != nil { return err diff --git a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go index af8aa55a786..7f5e95e0a9d 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go +++ b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go @@ -32,15 +32,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/informers" clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" cloudprovider "k8s.io/cloud-provider" cloudproviderapi "k8s.io/cloud-provider/api" fakecloud "k8s.io/cloud-provider/fake" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/controller-manager/pkg/features" _ "k8s.io/controller-manager/pkg/features/register" "github.com/google/go-cmp/cmp" @@ -49,12 +46,10 @@ import ( func Test_syncNode(t *testing.T) { tests := []struct { - name string - fakeCloud *fakecloud.Cloud - existingNode *v1.Node - updatedNode *v1.Node - optionalProviderID bool - expectedErr bool + name string + fakeCloud *fakecloud.Cloud + existingNode *v1.Node + updatedNode *v1.Node }{ { name: "node initialized with provider ID", @@ -550,8 +545,7 @@ func Test_syncNode(t *testing.T) { }, }, { - name: "provided node IP address is not valid", - expectedErr: true, + name: "provided node IP address is not valid", fakeCloud: &fakecloud.Cloud{ EnableInstancesV2: false, Addresses: []v1.NodeAddress{ @@ -649,8 +643,7 @@ func Test_syncNode(t *testing.T) { }, }, { - name: "provided node IP address is not present", - expectedErr: true, + name: "provided node IP address is not present", fakeCloud: &fakecloud.Cloud{ EnableInstancesV2: false, Addresses: []v1.NodeAddress{ @@ -843,8 +836,7 @@ func Test_syncNode(t *testing.T) { }, }, { - name: "provider ID not implemented and optional", - optionalProviderID: true, + name: "provider ID not implemented", fakeCloud: &fakecloud.Cloud{ EnableInstancesV2: false, InstanceTypes: map[types.NodeName]string{}, @@ -900,71 +892,6 @@ func Test_syncNode(t *testing.T) { }, }, }, - { - name: "provider ID not implemented and required", - optionalProviderID: false, - expectedErr: true, - fakeCloud: &fakecloud.Cloud{ - EnableInstancesV2: false, - InstanceTypes: map[types.NodeName]string{}, - Provider: "test", - ExtID: map[types.NodeName]string{}, - ExtIDErr: map[types.NodeName]error{ - types.NodeName("node0"): cloudprovider.NotImplemented, - }, - Err: nil, - }, - existingNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionUnknown, - LastHeartbeatTime: metav1.Date(2015, 1, 1, 12, 0, 0, 0, time.UTC), - LastTransitionTime: metav1.Date(2015, 1, 1, 12, 0, 0, 0, time.UTC), - }, - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: cloudproviderapi.TaintExternalCloudProvider, - Value: "true", - Effect: v1.TaintEffectNoSchedule, - }, - }, - }, - }, - updatedNode: &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node0", - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), - }, - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: v1.ConditionUnknown, - LastHeartbeatTime: metav1.Date(2015, 1, 1, 12, 0, 0, 0, time.UTC), - LastTransitionTime: metav1.Date(2015, 1, 1, 12, 0, 0, 0, time.UTC), - }, - }, - }, - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: cloudproviderapi.TaintExternalCloudProvider, - Value: "true", - Effect: v1.TaintEffectNoSchedule, - }, - }, - }, - }, - }, { name: "[instanceV2] node initialized with provider ID", fakeCloud: &fakecloud.Cloud{ @@ -1714,8 +1641,7 @@ func Test_syncNode(t *testing.T) { }, }, { - name: "[instanceV2] provider ID not implemented", - optionalProviderID: true, + name: "[instanceV2] provider ID not implemented", fakeCloud: &fakecloud.Cloud{ EnableInstancesV2: true, InstanceTypes: map[types.NodeName]string{}, @@ -1772,8 +1698,7 @@ func Test_syncNode(t *testing.T) { }, }, { - name: "[instanceV2] error getting InstanceMetadata", - expectedErr: true, + name: "[instanceV2] error getting InstanceMetadata", fakeCloud: &fakecloud.Cloud{ EnableInstancesV2: true, InstanceTypes: map[types.NodeName]string{}, @@ -1839,7 +1764,6 @@ func Test_syncNode(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.OptionalProviderID, test.optionalProviderID)() clientset := fake.NewSimpleClientset(test.existingNode) factory := informers.NewSharedInformerFactory(clientset, 0) @@ -1862,10 +1786,7 @@ func Test_syncNode(t *testing.T) { w := eventBroadcaster.StartLogging(klog.Infof) defer w.Stop() - err := cloudNodeController.syncNode(context.TODO(), test.existingNode.Name) - if (err != nil) != test.expectedErr { - t.Fatalf("error got: %v expected: %v", err, test.expectedErr) - } + cloudNodeController.syncNode(context.TODO(), test.existingNode.Name) updatedNode, err := clientset.CoreV1().Nodes().Get(context.TODO(), test.existingNode.Name, metav1.GetOptions{}) if err != nil { diff --git a/staging/src/k8s.io/controller-manager/pkg/features/kube_features.go b/staging/src/k8s.io/controller-manager/pkg/features/kube_features.go index 0eb04463909..65950dd8f57 100644 --- a/staging/src/k8s.io/controller-manager/pkg/features/kube_features.go +++ b/staging/src/k8s.io/controller-manager/pkg/features/kube_features.go @@ -47,13 +47,6 @@ const ( // `alpha.kubernetes.io/provided-node-ip` annotation CloudDualStackNodeIPs featuregate.Feature = "CloudDualStackNodeIPs" - // owner: @aojea - // Deprecated: v1.30 - // issue: https://issues.k8s.io/123024 - // - // If enabled, the ProviderID field is not required for the node initialization. - OptionalProviderID featuregate.Feature = "OptionalProviderID" - // owner: @alexanderConstantinescu // kep: http://kep.k8s.io/3458 // beta: v1.27 @@ -73,6 +66,5 @@ func SetupCurrentKubernetesSpecificFeatureGates(featuregates featuregate.Mutable var cloudPublicFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ CloudControllerManagerWebhook: {Default: false, PreRelease: featuregate.Alpha}, CloudDualStackNodeIPs: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 - OptionalProviderID: {Default: false, PreRelease: featuregate.Deprecated}, // remove after 1.31 StableLoadBalancerNodeSet: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.30, remove in 1.31 }