Merge pull request kubernetes#23909 from danwinship/aws-nodeaddress-s…

…orting-4.1

Bug 1758159: aws: sort addresses of multiple interfaces correctly [4.1]

Origin-commit: 7d3de0535007829711e9ac5f0a9845e09e231f29

pkg/cloudprovider/providers/aws/aws.go

@@ -1315,14 +1315,35 @@ func (c *Cloud) NodeAddresses(ctx context.Context, name types.NodeName) ([]v1.No
 			return nil, fmt.Errorf("error querying AWS metadata for %q: %q", "network/interfaces/macs", err)
 		}
 
+		// We want the IPs to end up in order by interface (in particular, we want eth0's
+		// IPs first), but macs isn't necessarily sorted in that order so we have to
+		// explicitly order by device-number (device-number == the "0" in "eth0").
+		macIPs := make(map[int]string)
 		for _, macID := range strings.Split(macs, "\n") {
 			if macID == "" {
 				continue
 			}
-			macPath := path.Join("network/interfaces/macs/", macID, "local-ipv4s")
-			internalIPs, err := c.metadata.GetMetadata(macPath)
+			numPath := path.Join("network/interfaces/macs/", macID, "device-number")
+			numStr, err := c.metadata.GetMetadata(numPath)
 			if err != nil {
-				return nil, fmt.Errorf("error querying AWS metadata for %q: %q", macPath, err)
+				return nil, fmt.Errorf("error querying AWS metadata for %q: %q", numPath, err)
+			}
+			num, err := strconv.Atoi(strings.TrimSpace(numStr))
+			if err != nil {
+				klog.Warningf("Bad device-number %q for interface %s\n", numStr, macID)
+				continue
+			}
+			ipPath := path.Join("network/interfaces/macs/", macID, "local-ipv4s")
+			macIPs[num], err = c.metadata.GetMetadata(ipPath)
+			if err != nil {
+				return nil, fmt.Errorf("error querying AWS metadata for %q: %q", ipPath, err)
+			}
+		}
+
+		for i := 0; i < len(macIPs); i++ {
+			internalIPs := macIPs[i]
+			if internalIPs == "" {
+				continue
 			}
 			for _, internalIP := range strings.Split(internalIPs, "\n") {
 				if internalIP == "" {

pkg/cloudprovider/providers/aws/aws_fakes.go

@@ -17,6 +17,8 @@ limitations under the License.
 package aws
 
 import (
+	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -333,7 +335,13 @@ func (m *FakeMetadata) GetMetadata(key string) (string, error) {
 		return aws.StringValue(i.PublicIpAddress), nil
 	} else if strings.HasPrefix(key, networkInterfacesPrefix) {
 		if key == networkInterfacesPrefix {
-			return strings.Join(m.aws.networkInterfacesMacs, "/\n") + "/\n", nil
+			// Return the MACs sorted lexically rather than in device-number
+			// order; this matches AWS's observed behavior and lets us test
+			// that we fix up the ordering correctly in NodeAddresses().
+			macs := make([]string, len(m.aws.networkInterfacesMacs))
+			copy(macs, m.aws.networkInterfacesMacs)
+			sort.Strings(macs)
+			return strings.Join(macs, "/\n") + "/\n", nil
 		}
 
 		keySplit := strings.Split(key, "/")
@@ -345,6 +353,13 @@ func (m *FakeMetadata) GetMetadata(key string) (string, error) {
 				}
 			}
 		}
+		if len(keySplit) == 5 && keySplit[4] == "device-number" {
+			for i, macElem := range m.aws.networkInterfacesMacs {
+				if macParam == macElem {
+					return fmt.Sprintf("%d\n", i), nil
+				}
+			}
+		}
 		if len(keySplit) == 5 && keySplit[4] == "local-ipv4s" {
 			for i, macElem := range m.aws.networkInterfacesMacs {
 				if macParam == macElem {

pkg/cloudprovider/providers/aws/aws_test.go

@@ -574,7 +574,7 @@ func testHasNodeAddress(t *testing.T, addrs []v1.NodeAddress, addressType v1.Nod
 }
 
 func TestNodeAddresses(t *testing.T) {
-	// Note these instances have the same name
+	// Note instance0 and instance1 have the same name
 	// (we test that this produces an error)
 	var instance0 ec2.Instance
 	var instance1 ec2.Instance
@@ -690,7 +690,7 @@ func TestNodeAddressesWithMetadata(t *testing.T) {
 	instances := []*ec2.Instance{&instance}
 	awsCloud, awsServices := mockInstancesResp(&instance, instances)
 
-	awsServices.networkInterfacesMacs = []string{"0a:26:89:f3:9c:f6", "0a:77:64:c4:6a:48"}
+	awsServices.networkInterfacesMacs = []string{"0a:77:89:f3:9c:f6", "0a:26:64:c4:6a:48"}
 	awsServices.networkInterfacesPrivateIPs = [][]string{{"192.168.0.1"}, {"192.168.0.2"}}
 	addrs, err := awsCloud.NodeAddresses(context.TODO(), "")
 	if err != nil {
@@ -699,6 +699,17 @@ func TestNodeAddressesWithMetadata(t *testing.T) {
 	testHasNodeAddress(t, addrs, v1.NodeInternalIP, "192.168.0.1")
 	testHasNodeAddress(t, addrs, v1.NodeInternalIP, "192.168.0.2")
 	testHasNodeAddress(t, addrs, v1.NodeExternalIP, "2.3.4.5")
+	var index1, index2 int
+	for i, addr := range addrs {
+		if addr.Type == v1.NodeInternalIP && addr.Address == "192.168.0.1" {
+			index1 = i
+		} else if addr.Type == v1.NodeInternalIP && addr.Address == "192.168.0.2" {
+			index2 = i
+		}
+	}
+	if index1 > index2 {
+		t.Errorf("Addresses in incorrect order: %v", addrs)
+	}
 }
 
 func TestGetRegion(t *testing.T) {