Merge pull request #63170 from micahhausler/node-ip-fix

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Report node DNS info with --node-ip

**What this PR does / why we need it**:
This PR adds `ExternalDNS`, `InternalDNS`, and `ExternalIP` info for kubelets with the `--nodeip` flag enabled. 

**Which issue(s) this PR fixes** 
Fixes #63158

**Special notes for your reviewer**:

I added a field to the Kubelet to make IP validation more testable (`validateNodeIP` relies on the `net` package and the IP address of the host that is executing the test.) I also converted the test to use a table so new cases could be added more easily.

**Release Notes**
```release-note
Report node DNS info with --node-ip flag
```

@andrewsykim
@nckturner 

/sig node
/sig network

pkg/controller/cloud/node_controller.go

@@ -191,7 +191,6 @@ func (cnc *CloudNodeController) updateNodeAddress(node *v1.Node, instances cloud
 			glog.Errorf("Specified Node IP not found in cloudprovider")
 			return
 		}
-		nodeAddresses = []v1.NodeAddress{*nodeIP}
 	}
 	newNode := node.DeepCopy()
 	newNode.Status.Addresses = nodeAddresses

pkg/controller/cloud/node_controller_test.go

@@ -870,7 +870,7 @@ func TestNodeProvidedIPAddresses(t *testing.T) {
 
 	assert.Equal(t, 1, len(fnh.UpdatedNodes), "Node was not updated")
 	assert.Equal(t, "node0", fnh.UpdatedNodes[0].Name, "Node was not updated")
-	assert.Equal(t, 1, len(fnh.UpdatedNodes[0].Status.Addresses), "Node status unexpectedly updated")
+	assert.Equal(t, 3, len(fnh.UpdatedNodes[0].Status.Addresses), "Node status unexpectedly updated")
 
 	cloudNodeController.Run()
 
@@ -878,7 +878,7 @@ func TestNodeProvidedIPAddresses(t *testing.T) {
 
 	updatedNodes := fnh.GetUpdatedNodesCopy()
 
-	assert.Equal(t, 1, len(updatedNodes[0].Status.Addresses), 1, "Node Addresses not correctly updated")
+	assert.Equal(t, 3, len(updatedNodes[0].Status.Addresses), "Node Addresses not correctly updated")
 	assert.Equal(t, "10.0.0.1", updatedNodes[0].Status.Addresses[0].Address, "Node Addresses not correctly updated")
 }
 

pkg/kubelet/apis/well_known_annotations.go

@@ -19,7 +19,7 @@ package apis
 const (
 	// When kubelet is started with the "external" cloud provider, then
 	// it sets this annotation on the node to denote an ip address set from the
-	// cmd line flag. This ip is verified with the cloudprovider as valid by
+	// cmd line flag (--node-ip). This ip is verified with the cloudprovider as valid by
 	// the cloud-controller-manager
 	AnnotationProvidedIPAddr = "alpha.kubernetes.io/provided-node-ip"
 )

pkg/kubelet/kubelet.go

@@ -520,6 +520,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 		containerManager:     kubeDeps.ContainerManager,
 		containerRuntimeName: containerRuntime,
 		nodeIP:               parsedNodeIP,
+		nodeIPValidator:      validateNodeIP,
 		clock:                clock.RealClock{},
 		enableControllerAttachDetach:            kubeCfg.EnableControllerAttachDetach,
 		iptClient:                               utilipt.New(utilexec.New(), utildbus.New(), utilipt.ProtocolIpv4),
@@ -1069,6 +1070,9 @@ type Kubelet struct {
 	// If non-nil, use this IP address for the node
 	nodeIP net.IP
 
+	// use this function to validate the kubelet nodeIP
+	nodeIPValidator func(net.IP) error
+
 	// If non-nil, this is a unique identifier for the node in an external database, eg. cloudprovider
 	providerID string
 

pkg/kubelet/kubelet_node_status.go

@@ -407,7 +407,7 @@ func (kl *Kubelet) recordNodeStatusEvent(eventType, event string) {
 // Set IP and hostname addresses for the node.
 func (kl *Kubelet) setNodeAddress(node *v1.Node) error {
 	if kl.nodeIP != nil {
-		if err := validateNodeIP(kl.nodeIP); err != nil {
+		if err := kl.nodeIPValidator(kl.nodeIP); err != nil {
 			return fmt.Errorf("failed to validate nodeIP: %v", err)
 		}
 		glog.V(2).Infof("Using node IP: %q", kl.nodeIP.String())
@@ -467,12 +467,22 @@ func (kl *Kubelet) setNodeAddress(node *v1.Node) error {
 		}
 		if kl.nodeIP != nil {
 			enforcedNodeAddresses := []v1.NodeAddress{}
+
+			var nodeIPType v1.NodeAddressType
 			for _, nodeAddress := range nodeAddresses {
 				if nodeAddress.Address == kl.nodeIP.String() {
 					enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
+					nodeIPType = nodeAddress.Type
+					break
 				}
 			}
 			if len(enforcedNodeAddresses) > 0 {
+				for _, nodeAddress := range nodeAddresses {
+					if nodeAddress.Type != nodeIPType && nodeAddress.Type != v1.NodeHostName {
+						enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
+					}
+				}
+
 				enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: v1.NodeHostName, Address: kl.GetHostname()})
 				node.Status.Addresses = enforcedNodeAddresses
 				return nil
@@ -513,7 +523,7 @@ func (kl *Kubelet) setNodeAddress(node *v1.Node) error {
 			var addrs []net.IP
 			addrs, _ = net.LookupIP(node.Name)
 			for _, addr := range addrs {
-				if err = validateNodeIP(addr); err == nil {
+				if err = kl.nodeIPValidator(addr); err == nil {
 					if addr.To4() != nil {
 						ipAddr = addr
 						break

pkg/kubelet/kubelet_node_status_test.go

@@ -139,80 +139,163 @@ func TestNodeStatusWithCloudProviderNodeIP(t *testing.T) {
 	kubelet.kubeClient = nil // ensure only the heartbeat client is used
 	kubelet.hostname = testKubeletHostname
 
+	cases := []struct {
+		name              string
+		nodeIP            net.IP
+		nodeAddresses     []v1.NodeAddress
+		expectedAddresses []v1.NodeAddress
+		shouldError       bool
+	}{
+		{
+			name:   "A single InternalIP",
+			nodeIP: net.ParseIP("10.1.1.1"),
+			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},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "NodeIP is external",
+			nodeIP: net.ParseIP("55.55.55.55"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			// Accommodating #45201 and #49202
+			name:   "InternalIP and ExternalIP are the same",
+			nodeIP: net.ParseIP("55.55.55.55"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An Internal/ExternalIP, an Internal/ExternalDNS",
+			nodeIP: net.ParseIP("10.1.1.1"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalDNS, Address: "ip-10-1-1-1.us-west-2.compute.internal"},
+				{Type: v1.NodeExternalDNS, Address: "ec2-55-55-55-55.us-west-2.compute.amazonaws.com"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalDNS, Address: "ip-10-1-1-1.us-west-2.compute.internal"},
+				{Type: v1.NodeExternalDNS, Address: "ec2-55-55-55-55.us-west-2.compute.amazonaws.com"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An Internal with multiple internal IPs",
+			nodeIP: net.ParseIP("10.1.1.1"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "10.2.2.2"},
+				{Type: v1.NodeInternalIP, Address: "10.3.3.3"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An InternalIP that isn't valid: should error",
+			nodeIP: net.ParseIP("10.2.2.2"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: nil,
+			shouldError:       true,
+		},
+	}
+	for _, testCase := range cases {
+		// testCase setup
 		existingNode := v1.Node{
 			ObjectMeta: metav1.ObjectMeta{Name: testKubeletHostname, Annotations: make(map[string]string)},
 			Spec:       v1.NodeSpec{},
 		}
 
-	// TODO : is it possible to mock 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)
+		kubelet.nodeIP = testCase.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,
-			},
-		},
+			Addresses: testCase.nodeAddresses,
 			Err:       nil,
 		}
 		kubelet.cloud = fakeCloud
 		kubelet.cloudproviderRequestParallelism = make(chan int, 1)
 		kubelet.cloudproviderRequestSync = make(chan int)
 		kubelet.cloudproviderRequestTimeout = 10 * time.Second
-
-	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,
-		},
+		kubelet.nodeIPValidator = func(nodeIP net.IP) error {
+			return nil
 		}
-	assert.True(t, apiequality.Semantic.DeepEqual(expectedAddresses, existingNode.Status.Addresses), "%s", diff.ObjectDiff(expectedAddresses, existingNode.Status.Addresses))
+
+		// execute method
+		err := kubelet.setNodeAddress(&existingNode)
+		if err != nil && !testCase.shouldError {
+			t.Errorf("Unexpected error for test %s: %q", testCase.name, err)
+			continue
+		} else if err != nil && testCase.shouldError {
+			// expected an error
+			continue
+		}
+
+		// Sort both sets for consistent equality
+		sortNodeAddresses(testCase.expectedAddresses)
+		sortNodeAddresses(existingNode.Status.Addresses)
+
+		assert.True(
+			t,
+			apiequality.Semantic.DeepEqual(
+				testCase.expectedAddresses,
+				existingNode.Status.Addresses,
+			),
+			fmt.Sprintf("Test %s failed %%s", testCase.name),
+			diff.ObjectDiff(testCase.expectedAddresses, existingNode.Status.Addresses),
+		)
+	}
+}
+
+// sortableNodeAddress is a type for sorting []v1.NodeAddress
+type sortableNodeAddress []v1.NodeAddress
+
+func (s sortableNodeAddress) Len() int { return len(s) }
+func (s sortableNodeAddress) Less(i, j int) bool {
+	return (string(s[i].Type) + s[i].Address) < (string(s[j].Type) + s[j].Address)
+}
+func (s sortableNodeAddress) Swap(i, j int) { s[j], s[i] = s[i], s[j] }
+
+func sortNodeAddresses(addrs sortableNodeAddress) {
+	sort.Sort(addrs)
 }
 
 func TestUpdateNewNodeStatus(t *testing.T) {