Revert "[cloud-provider] require providerID to initialize node"

This reverts commit 03bd3e25b16666a6c89d8875782d1132819eeb6e. Kubernetes-commit: 6b08919473a5dc4224239cebc983bea9308ffce7
kubernetes · Mar 1, 2024 · 51373f4 · 51373f4
1 parent 123fab8
commit 51373f4
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 104 deletions.
diff --git a/controllers/node/node_controller.go b/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/controllers/node/node_controller_test.go b/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 {