Merge pull request #49202 from cbonte/node-addresses

Automatic merge from submit-queue (batch tested with PRs 51728, 49202)

Fix setNodeAddress when a node IP and a cloud provider are set

**What this PR does / why we need it**:
When a node IP is set and a cloud provider returns the same address with
several types, only the first address was accepted. With the changes made
in PR #45201, the vSphere cloud provider returned the ExternalIP first,
which led to a node without any InternalIP.

The behaviour is modified to return all the address types for the
specified node IP.

**Which issue this PR fixes**: fixes #48760

**Special notes for your reviewer**:
* I'm not a golang expert, is it possible to mock `kubelet.validateNodeIP()` to avoid the need of real host interface addresses in the test ?
* It would be great to have it backported for a next 1.6.8 release.

**Release note**:
```release-note
NONE
```

pkg/kubelet/BUILD

@@ -170,6 +170,7 @@ go_test(
         "//pkg/api:go_default_library",
         "//pkg/api/install:go_default_library",
         "//pkg/capabilities:go_default_library",
+        "//pkg/cloudprovider/providers/fake:go_default_library",
         "//pkg/kubelet/apis:go_default_library",
         "//pkg/kubelet/apis/cri/v1alpha1/runtime:go_default_library",
         "//pkg/kubelet/apis/kubeletconfig:go_default_library",

pkg/kubelet/kubelet_node_status.go

@@ -469,15 +469,17 @@ func (kl *Kubelet) setNodeAddress(node *v1.Node) error {
 			return fmt.Errorf("failed to get node address from cloud provider: %v", err)
 		}
 		if kl.nodeIP != nil {
+			enforcedNodeAddresses := []v1.NodeAddress{}
 			for _, nodeAddress := range nodeAddresses {
 				if nodeAddress.Address == kl.nodeIP.String() {
-					node.Status.Addresses = []v1.NodeAddress{
-						{Type: nodeAddress.Type, Address: nodeAddress.Address},
-						{Type: v1.NodeHostName, Address: kl.GetHostname()},
-					}
-					return nil
+					enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
 				}
 			}
+			if len(enforcedNodeAddresses) > 0 {
+				enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: v1.NodeHostName, Address: kl.GetHostname()})
+				node.Status.Addresses = enforcedNodeAddresses
+				return nil
+			}
 			return fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", kl.nodeIP)
 		}
 

pkg/kubelet/kubelet_node_status_test.go

@@ -19,6 +19,7 @@ package kubelet
 import (
 	"encoding/json"
 	"fmt"
+	"net"
 	goruntime "runtime"
 	"sort"
 	"strconv"
@@ -43,6 +44,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
+	fakecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/fake"
 	kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis"
 	"k8s.io/kubernetes/pkg/kubelet/cm"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
@@ -127,6 +129,85 @@ func (lcm *localCM) GetCapacity() v1.ResourceList {
 	return lcm.capacity
 }
 
+func TestNodeStatusWithCloudProviderNodeIP(t *testing.T) {
+	testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */)
+	defer testKubelet.Cleanup()
+	kubelet := testKubelet.kubelet
+	kubelet.hostname = testKubeletHostname
+
+	existingNode := v1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: testKubeletHostname, Annotations: make(map[string]string)},
+		Spec:       v1.NodeSpec{},
+	}
+
+	// TODO : is it possible to mock kubelet.validateNodeIP() to avoid relying on the host interface addresses ?
+	addrs, err := net.InterfaceAddrs()
+	assert.NoError(t, err)
+	for _, addr := range addrs {
+		var ip net.IP
+		switch v := addr.(type) {
+		case *net.IPNet:
+			ip = v.IP
+		case *net.IPAddr:
+			ip = v.IP
+		}
+		if ip != nil && !ip.IsLoopback() && ip.To4() != nil {
+			kubelet.nodeIP = ip
+			break
+		}
+	}
+	assert.NotNil(t, kubelet.nodeIP)
+
+	fakeCloud := &fakecloud.FakeCloud{
+		Addresses: []v1.NodeAddress{
+			{
+				Type:    v1.NodeExternalIP,
+				Address: "132.143.154.163",
+			},
+			{
+				Type:    v1.NodeExternalIP,
+				Address: kubelet.nodeIP.String(),
+			},
+			{
+				Type:    v1.NodeInternalIP,
+				Address: "132.143.154.164",
+			},
+			{
+				Type:    v1.NodeInternalIP,
+				Address: kubelet.nodeIP.String(),
+			},
+			{
+				Type:    v1.NodeInternalIP,
+				Address: "132.143.154.165",
+			},
+			{
+				Type:    v1.NodeHostName,
+				Address: testKubeletHostname,
+			},
+		},
+		Err: nil,
+	}
+	kubelet.cloud = fakeCloud
+
+	kubelet.setNodeAddress(&existingNode)
+
+	expectedAddresses := []v1.NodeAddress{
+		{
+			Type:    v1.NodeExternalIP,
+			Address: kubelet.nodeIP.String(),
+		},
+		{
+			Type:    v1.NodeInternalIP,
+			Address: kubelet.nodeIP.String(),
+		},
+		{
+			Type:    v1.NodeHostName,
+			Address: testKubeletHostname,
+		},
+	}
+	assert.True(t, apiequality.Semantic.DeepEqual(expectedAddresses, existingNode.Status.Addresses), "%s", diff.ObjectDiff(expectedAddresses, existingNode.Status.Addresses))
+}
+
 func TestUpdateNewNodeStatus(t *testing.T) {
 	// generate one more than maxImagesInNodeStatus in inputImageList
 	inputImageList, expectedImageList := generateTestingImageList(maxImagesInNodeStatus + 1)