Skip to content

Commit

Permalink
Fix incorrect order of Node internal address issue
Browse files Browse the repository at this point in the history 
getLocalInstanceNodeAddresses() always returns IPv4 then IPv6
if Node is dual-stack. The order of 2 internal IPs should be
consistent as when first IP is added to the Node.

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
  • Loading branch information
@lzhecheng
lzhecheng committed Mar 24, 2023
1 parent e75e419 commit a2758f6
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 2 deletions.
41 changes: 40 additions & 1 deletion pkg/nodemanager/nodemanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1.
}

newNode := node.DeepCopy()
newNode.Status.Addresses = nodeAddresses
newNode.Status.Addresses = ensureNodeInternalAddressesOrder(node.Status.Addresses, nodeAddresses)
_, _, err = PatchNodeStatus(cnc.kubeClient.CoreV1(), types.NodeName(node.Name), node, newNode)
if err != nil {
return fmt.Errorf("patching node with cloud ip addresses: %w", err)
Expand All @@ -289,6 +289,45 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1.
return nil
}

// ensureNodeInternalAddressesOrder ensures Node internal addresses are consistent in terms of order.
func ensureNodeInternalAddressesOrder(old, new []v1.NodeAddress) []v1.NodeAddress {
ordered := []v1.NodeAddress{}
ipv6First := false
for _, addr := range old {
if addr.Type != v1.NodeInternalIP {
continue
}
if net.ParseIP(addr.Address).To4() == nil {
ipv6First = true
}
break
}
newInternalIPs := []v1.NodeAddress{}
for _, addr := range new {
if addr.Type != v1.NodeInternalIP {
ordered = append(ordered, addr)
} else {
newInternalIPs = append(newInternalIPs, addr)
}
}

if len(newInternalIPs) == 1 {
ordered = append(ordered, newInternalIPs[0])
} else if len(newInternalIPs) == 2 {
if (ipv6First && net.ParseIP(newInternalIPs[0].Address).To4() == nil) ||
(!ipv6First && net.ParseIP(newInternalIPs[0].Address).To4() != nil) {
ordered = append(ordered, newInternalIPs[0], newInternalIPs[1])
} else {
ordered = append(ordered, newInternalIPs[1], newInternalIPs[0])
}
} else {
klog.Warningf("incorrect number of internal IPs to update: %v", newInternalIPs)
ordered = append(ordered, newInternalIPs...)
}

return ordered
}

// nodeModifier is used to carry changes to node objects across multiple attempts to update them
// in a retry-if-conflict loop.
type nodeModifier func(*v1.Node)
Expand Down
136 changes: 135 additions & 1 deletion pkg/nodemanager/nodemanager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,8 @@ func TestNodeProvidedIPAddresses(t *testing.T) {
cloudNodeController.UpdateNodeStatus(context.TODO())
updatedNodes := fnh.GetUpdatedNodesCopy()
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")
// Because of order issue #3401, internal addresses are at the end.
assert.Equal(t, "10.0.0.1", updatedNodes[0].Status.Addresses[2].Address, "Node Addresses not correctly updated")
}

// Tests that node address changes are detected correctly
Expand Down Expand Up @@ -880,6 +881,139 @@ func TestNodeAddressesNotUpdate(t *testing.T) {
}
}

// TestEnsureNodeAddressesOrder verifies if ensureNodeAddressesOrder() can ensure
// the order of Node internal addresses is correct.
func TestEnsureNodeAddressesOrder(t *testing.T) {
testcases := []struct {
desc string
oldAddrs []v1.NodeAddress
newAddrs []v1.NodeAddress
expectedAddrs []v1.NodeAddress
}{
{
"IPv4 no internal IPs",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
},
{
"IPv6 no internal IPs",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
},
{
"IPv6 add second IPv4 internal IP",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
},
{
"IPv6 add second IPv4 internal IP with reverse order",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "fe::1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
},
{
"Dual-stack add second IPv6 internal IP",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
},
{
"Dual-stack add second IPv6 internal IP with reverse order",
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
},
[]v1.NodeAddress{
{Type: v1.NodeHostName, Address: "hostname0"},
{Type: v1.NodeExternalIP, Address: "20.0.0.1"},
{Type: v1.NodeInternalIP, Address: "10.0.0.1"},
{Type: v1.NodeInternalIP, Address: "2001::1"},
},
},
}

for _, tc := range testcases {
t.Run(tc.desc, func(t *testing.T) {
res := ensureNodeInternalAddressesOrder(tc.oldAddrs, tc.newAddrs)
assert.Equal(t, tc.expectedAddrs, res)
})
}
}

// This test checks that a node is set with the correct providerID
func TestNodeProviderID(t *testing.T) {
ctrl := gomock.NewController(t)
Expand Down
2 changes: 2 additions & 0 deletions pkg/provider/azure_instances.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ func (az *Cloud) getLocalInstanceNodeAddresses(netInterfaces []NetworkInterface,
addresses := []v1.NodeAddress{
{Type: v1.NodeHostName, Address: nodeName},
}
// Notice: IPv4 addresses are always before IPv6 ones regardless of the
// current addresses of the Node.
if len(netInterface.IPV4.IPAddress) > 0 && len(netInterface.IPV4.IPAddress[0].PrivateIP) > 0 {
address := netInterface.IPV4.IPAddress[0]
addresses = append(addresses, v1.NodeAddress{
Expand Down

0 comments on commit a2758f6

Please sign in to comment.