Merge pull request #109794 from shiftstack/always_annotate_nodeip

Make kubelet set alpha.kubernetes.io/provided-node-ip unconditionally

pkg/kubelet/nodestatus/setters.go

@@ -94,14 +94,28 @@ func NodeAddress(nodeIPs []net.IP, // typically Kubelet.nodeIPs
 			klog.V(4).InfoS("Using secondary node IP", "IP", secondaryNodeIP.String())
 		}
 
-		if externalCloudProvider {
+		if externalCloudProvider || cloud != nil {
+			// Annotate the Node object with nodeIP for external cloud provider.
+			//
+			// We do this even when external CCM is not configured to cover a situation
+			// during migration from legacy to external CCM: when CCM is running the
+			// node controller in the cluster but kubelet is still running the in-tree
+			// provider. Adding this annotation in all cases ensures that while
+			// Addresses flap between the competing controllers, they at least flap
+			// consistently.
+			//
+			// We do not add the annotation in the case where there is no cloud
+			// controller at all, as we don't expect to migrate these clusters to use an
+			// external CCM.
 			if nodeIPSpecified {
 				if node.ObjectMeta.Annotations == nil {
 					node.ObjectMeta.Annotations = make(map[string]string)
 				}
 				node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr] = nodeIP.String()
 			}
+		}
 
+		if externalCloudProvider {
 			// If --cloud-provider=external and node address is already set,
 			// then we return early because provider set addresses should take precedence.
 			// Otherwise, we try to look up the node IP and let the cloud provider override it later

pkg/kubelet/nodestatus/setters_test.go

@@ -56,14 +56,21 @@ const (
 
 // TODO(mtaufen): below is ported from the old kubelet_node_status_test.go code, potentially add more test coverage for NodeAddress setter in future
 func TestNodeAddress(t *testing.T) {
+	type cloudProviderType int
+	const (
+		cloudProviderLegacy cloudProviderType = iota
+		cloudProviderExternal
+		cloudProviderNone
+	)
 	cases := []struct {
-		name                  string
+		name                string
-		hostnameOverride      bool
+		hostnameOverride    bool
-		nodeIP                net.IP
+		nodeIP              net.IP
-		externalCloudProvider bool
+		cloudProviderType   cloudProviderType
-		nodeAddresses         []v1.NodeAddress
+		nodeAddresses       []v1.NodeAddress
-		expectedAddresses     []v1.NodeAddress
+		expectedAddresses   []v1.NodeAddress
-		shouldError           bool
+		expectedAnnotations map[string]string
+		shouldError         bool
 	}{
 		{
 			name:   "A single InternalIP",
@@ -211,10 +218,10 @@ func TestNodeAddress(t *testing.T) {
 			shouldError:      false,
 		},
 		{
-			name:                  "cloud provider is external",
+			name:              "cloud provider is external",
-			nodeIP:                netutils.ParseIPSloppy("10.0.0.1"),
+			nodeIP:            netutils.ParseIPSloppy("10.0.0.1"),
-			nodeAddresses:         []v1.NodeAddress{},
+			nodeAddresses:     []v1.NodeAddress{},
-			externalCloudProvider: true,
+			cloudProviderType: cloudProviderExternal,
 			expectedAddresses: []v1.NodeAddress{
 				{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
 				{Type: v1.NodeHostName, Address: testKubeletHostname},
@@ -396,6 +403,55 @@ func TestNodeAddress(t *testing.T) {
 			},
 			shouldError: false,
 		},
+		{
+			name:              "Legacy cloud provider gets nodeIP annotation",
+			nodeIP:            netutils.ParseIPSloppy("10.1.1.1"),
+			cloudProviderType: cloudProviderLegacy,
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAnnotations: map[string]string{
+				"alpha.kubernetes.io/provided-node-ip": "10.1.1.1",
+			},
+			shouldError: false,
+		},
+		{
+			name:              "External cloud provider gets nodeIP annotation",
+			nodeIP:            netutils.ParseIPSloppy("10.1.1.1"),
+			cloudProviderType: cloudProviderExternal,
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAnnotations: map[string]string{
+				"alpha.kubernetes.io/provided-node-ip": "10.1.1.1",
+			},
+			shouldError: false,
+		},
+		{
+			name:              "No cloud provider does not get nodeIP annotation",
+			nodeIP:            netutils.ParseIPSloppy("10.1.1.1"),
+			cloudProviderType: cloudProviderNone,
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAnnotations: map[string]string{},
+			shouldError:         false,
+		},
 	}
 	for _, testCase := range cases {
 		t.Run(testCase.name, func(t *testing.T) {
@@ -418,16 +474,13 @@ func TestNodeAddress(t *testing.T) {
 				return testCase.nodeAddresses, nil
 			}
 
-			// cloud provider is expected to be nil if external provider is set
+			// cloud provider is expected to be nil if external provider is set or there is no cloud provider
 			var cloud cloudprovider.Interface
-			if testCase.externalCloudProvider {
+			if testCase.cloudProviderType == cloudProviderLegacy {
-				cloud = nil
-			} else {
 				cloud = &fakecloud.Cloud{
 					Addresses: testCase.nodeAddresses,
 					Err:       nil,
 				}
-
 			}
 
 			// construct setter
@@ -435,7 +488,7 @@ func TestNodeAddress(t *testing.T) {
 				nodeIPValidator,
 				hostname,
 				testCase.hostnameOverride,
-				testCase.externalCloudProvider,
+				testCase.cloudProviderType == cloudProviderExternal,
 				cloud,
 				nodeAddressesFunc)
 
@@ -450,6 +503,10 @@ func TestNodeAddress(t *testing.T) {
 
 			assert.True(t, apiequality.Semantic.DeepEqual(testCase.expectedAddresses, existingNode.Status.Addresses),
 				"Diff: %s", diff.ObjectDiff(testCase.expectedAddresses, existingNode.Status.Addresses))
+			if testCase.expectedAnnotations != nil {
+				assert.True(t, apiequality.Semantic.DeepEqual(testCase.expectedAnnotations, existingNode.Annotations),
+					"Diff: %s", diff.ObjectDiff(testCase.expectedAnnotations, existingNode.Annotations))
+			}
 		})
 	}
 }