From db8774391309cefa185668e5799c5f7f0b3d723a Mon Sep 17 00:00:00 2001 From: Nicole Han Date: Tue, 6 Oct 2020 15:58:13 -0700 Subject: [PATCH] cloud node controller: handle empty providerID from getProviderID --- .../controllers/node/node_controller.go | 13 +- .../controllers/node/node_controller_test.go | 201 +++++++++++++++++- 2 files changed, 208 insertions(+), 6 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 91780fa5871..99990a3e257 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 @@ -474,8 +474,12 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider( ) ([]nodeModifier, error) { var nodeModifiers []nodeModifier - if node.Spec.ProviderID == "" && providerID != "" { - nodeModifiers = append(nodeModifiers, func(n *v1.Node) { n.Spec.ProviderID = providerID }) + if node.Spec.ProviderID == "" { + if providerID != "" { + nodeModifiers = append(nodeModifiers, func(n *v1.Node) { n.Spec.ProviderID = providerID }) + } else if instanceMeta.ProviderID != "" { + nodeModifiers = append(nodeModifiers, func(n *v1.Node) { n.Spec.ProviderID = instanceMeta.ProviderID }) + } } // If user provided an IP address, ensure that IP address is found @@ -527,6 +531,11 @@ func (cnc *CloudNodeController) getProviderID(ctx context.Context, node *v1.Node return node.Spec.ProviderID, nil } + if _, ok := cnc.cloud.InstancesV2(); ok { + // We don't need providerID when we call InstanceMetadata for InstancesV2 + return "", nil + } + providerID, err := cloudprovider.GetInstanceProviderID(ctx, cnc.cloud, types.NodeName(node.Name)) if err == cloudprovider.NotImplemented { // if the cloud provider being used does not support provider IDs, 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 827f33c8c72..410aa6e3eb4 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 @@ -764,6 +764,7 @@ func Test_syncNode(t *testing.T) { Effect: v1.TaintEffectNoSchedule, }, }, + ProviderID: "fake://12345", }, }, updatedNode: &v1.Node{ @@ -924,9 +925,6 @@ func Test_syncNode(t *testing.T) { }, }, }, - Spec: v1.NodeSpec{ - ProviderID: "aws://12345", - }, }, }, { @@ -991,7 +989,6 @@ func Test_syncNode(t *testing.T) { CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), }, Spec: v1.NodeSpec{ - ProviderID: "fake://12345", Taints: []v1.Taint{ { Key: "ImproveCoverageTaint", @@ -1882,3 +1879,199 @@ func TestNodeAddressesNotUpdate(t *testing.T) { t.Errorf("Node addresses should not be updated") } } + +func TestGetProviderID(t *testing.T) { + tests := []struct { + name string + fakeCloud *fakecloud.Cloud + existingNode *v1.Node + expectedProviderID string + }{ + { + name: "node initialized with provider ID", + fakeCloud: &fakecloud.Cloud{ + EnableInstancesV2: false, + InstanceTypes: map[types.NodeName]string{ + types.NodeName("node0"): "t1.micro", + }, + ExtID: map[types.NodeName]string{ + types.NodeName("node0"): "12345", + }, + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeHostName, + Address: "node0.cloud.internal", + }, + { + Type: v1.NodeInternalIP, + Address: "10.0.0.1", + }, + { + Type: v1.NodeExternalIP, + Address: "132.143.154.163", + }, + }, + ErrByProviderID: nil, + Err: nil, + }, + existingNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + { + Key: cloudproviderapi.TaintExternalCloudProvider, + Value: "true", + Effect: v1.TaintEffectNoSchedule, + }, + }, + ProviderID: "fake://12345", + }, + }, + expectedProviderID: "fake://12345", + }, + { + name: "cloud implemented with Instances (without providerID)", + fakeCloud: &fakecloud.Cloud{ + EnableInstancesV2: false, + InstanceTypes: map[types.NodeName]string{ + types.NodeName("node0"): "t1.micro", + types.NodeName("fake://12345"): "t1.micro", + }, + ExtID: map[types.NodeName]string{ + types.NodeName("node0"): "12345", + }, + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeHostName, + Address: "node0.cloud.internal", + }, + { + Type: v1.NodeInternalIP, + Address: "10.0.0.1", + }, + { + Type: v1.NodeExternalIP, + Address: "132.143.154.163", + }, + }, + Err: nil, + }, + existingNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + }, + }, + expectedProviderID: "fake://12345", + }, + { + name: "cloud implemented with InstancesV2 (with providerID)", + fakeCloud: &fakecloud.Cloud{ + EnableInstancesV2: true, + InstanceTypes: map[types.NodeName]string{ + types.NodeName("node0"): "t1.micro", + types.NodeName("fake://12345"): "t1.micro", + }, + ExtID: map[types.NodeName]string{ + types.NodeName("node0"): "12345", + }, + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeHostName, + Address: "node0.cloud.internal", + }, + { + Type: v1.NodeInternalIP, + Address: "10.0.0.1", + }, + { + Type: v1.NodeExternalIP, + Address: "132.143.154.163", + }, + }, + Err: nil, + }, + existingNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + { + Key: cloudproviderapi.TaintExternalCloudProvider, + Value: "true", + Effect: v1.TaintEffectNoSchedule, + }, + }, + ProviderID: "fake://12345", + }, + }, + expectedProviderID: "fake://12345", + }, + { + name: "cloud implemented with InstancesV2 (without providerID)", + fakeCloud: &fakecloud.Cloud{ + EnableInstancesV2: true, + InstanceTypes: map[types.NodeName]string{ + types.NodeName("node0"): "t1.micro", + types.NodeName("fake://12345"): "t1.micro", + }, + ExtID: map[types.NodeName]string{ + types.NodeName("node0"): "12345", + }, + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeHostName, + Address: "node0.cloud.internal", + }, + { + Type: v1.NodeInternalIP, + Address: "10.0.0.1", + }, + { + Type: v1.NodeExternalIP, + Address: "132.143.154.163", + }, + }, + Err: nil, + }, + existingNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + }, + Spec: v1.NodeSpec{ + Taints: []v1.Taint{ + { + Key: cloudproviderapi.TaintExternalCloudProvider, + Value: "true", + Effect: v1.TaintEffectNoSchedule, + }, + }, + }, + }, + expectedProviderID: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cloudNodeController := &CloudNodeController{ + cloud: test.fakeCloud, + } + + providerID, err := cloudNodeController.getProviderID(context.TODO(), test.existingNode) + if err != nil { + t.Fatalf("error getting provider ID: %v", err) + } + + if !cmp.Equal(providerID, test.expectedProviderID) { + t.Errorf("unexpected providerID %s", cmp.Diff(providerID, test.expectedProviderID)) + } + }) + } +}