Fix cloud reported hostname being overridden if nodeIP set

Author: Matthew Wong

pkg/kubelet/nodestatus/setters.go

@@ -84,40 +84,52 @@ func NodeAddress(nodeIP net.IP, // typically Kubelet.nodeIP
 			return nil
 		}
 		if cloud != nil {
-			nodeAddresses, err := nodeAddressesFunc()
+			cloudNodeAddresses, err := nodeAddressesFunc()
 			if err != nil {
 				return err
 			}
+
+			var nodeAddresses []v1.NodeAddress
+
+			// For every address supplied by the cloud provider that matches nodeIP, nodeIP is the enforced node address for
+			// that address Type (like InternalIP and ExternalIP), meaning other addresses of the same Type are discarded.
+			// See #61921 for more information: some cloud providers may supply secondary IPs, so nodeIP serves as a way to
+			// ensure that the correct IPs show up on a Node object.
 			if nodeIP != nil {
 				enforcedNodeAddresses := []v1.NodeAddress{}
 
 				nodeIPTypes := make(map[v1.NodeAddressType]bool)
-				for _, nodeAddress := range nodeAddresses {
+				for _, nodeAddress := range cloudNodeAddresses {
 					if nodeAddress.Address == nodeIP.String() {
 						enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
 						nodeIPTypes[nodeAddress.Type] = true
 					}
 				}
-				if len(enforcedNodeAddresses) > 0 {
-					for _, nodeAddress := range nodeAddresses {
-						if !nodeIPTypes[nodeAddress.Type] && nodeAddress.Type != v1.NodeHostName {
-							enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
-						}
-					}
 
-					enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: v1.NodeHostName, Address: hostname})
-					node.Status.Addresses = enforcedNodeAddresses
-					return nil
+				// nodeIP must be among the addresses supplied by the cloud provider
+				if len(enforcedNodeAddresses) == 0 {
+					return fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP)
+				}
+
+				// nodeIP was found, now use all other addresses supplied by the cloud provider NOT of the same Type as nodeIP.
+				for _, nodeAddress := range cloudNodeAddresses {
+					if !nodeIPTypes[nodeAddress.Type] {
+						enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
+					}
 				}
-				return fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP)
+
+				nodeAddresses = enforcedNodeAddresses
+			} else {
+				// If nodeIP is unset, just use the addresses provided by the cloud provider as-is
+				nodeAddresses = cloudNodeAddresses
 			}
 
 			switch {
-			case len(nodeAddresses) == 0:
+			case len(cloudNodeAddresses) == 0:
 				// the cloud provider didn't specify any addresses
 				nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeHostName, Address: hostname})
 
-			case !hasAddressType(nodeAddresses, v1.NodeHostName) && hasAddressValue(nodeAddresses, hostname):
+			case !hasAddressType(cloudNodeAddresses, v1.NodeHostName) && hasAddressValue(cloudNodeAddresses, hostname):
 				// the cloud provider didn't specify an address of type Hostname,
 				// but the auto-detected hostname matched an address reported by the cloud provider,
 				// so we can add it and count on the value being verifiable via cloud provider metadata

pkg/kubelet/nodestatus/setters_test.go

@@ -177,6 +177,21 @@ func TestNodeAddress(t *testing.T) {
 			},
 			shouldError: false,
 		},
+		{
+			name:   "cloud reports hostname, nodeIP is set, no override",
+			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.NodeHostName, Address: "cloud-host"},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: "cloud-host"}, // cloud-reported hostname wins over detected hostname
+			},
+			shouldError: false,
+		},
 		{
 			name: "cloud reports hostname, overridden",
 			nodeAddresses: []v1.NodeAddress{
@@ -220,6 +235,37 @@ func TestNodeAddress(t *testing.T) {
 			},
 			shouldError: false,
 		},
+		{
+			name:   "cloud doesn't report hostname, nodeIP is set, no override, detected hostname match",
+			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.NodeExternalDNS, Address: testKubeletHostname}, // cloud-reported address value matches detected hostname
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalDNS, Address: testKubeletHostname},
+				{Type: v1.NodeHostName, Address: testKubeletHostname}, // detected hostname gets auto-added
+			},
+			shouldError: false,
+		},
+		{
+			name:   "cloud doesn't report hostname, nodeIP is set, no override, detected hostname match with same type as nodeIP",
+			nodeIP: net.ParseIP("10.1.1.1"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: testKubeletHostname}, // cloud-reported address value matches detected hostname
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname}, // detected hostname gets auto-added
+			},
+			shouldError: false,
+		},
 		{
 			name: "cloud doesn't report hostname, hostname override, hostname mismatch",
 			nodeAddresses: []v1.NodeAddress{