From a4bac275faa56fc44ba1564b9954c0f24d288428 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 30 Jul 2019 08:32:55 -0400 Subject: [PATCH] aws: sort addresses of multiple interfaces correctly GetMetadata("network/interfaces/macs/") returns the interface MAC addresses in lexicographic order, not in device order, so we need to sort the results. --- .../k8s.io/legacy-cloud-providers/aws/aws.go | 27 ++++++++++++++++--- .../legacy-cloud-providers/aws/aws_fakes.go | 17 +++++++++++- .../legacy-cloud-providers/aws/aws_test.go | 15 +++++++++-- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go index 513d9c2d47c..df3ea241b25 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go @@ -1402,14 +1402,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 == "" { diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_fakes.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_fakes.go index 844a3a0c0af..a1be898ee23 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_fakes.go +++ b/staging/src/k8s.io/legacy-cloud-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 { diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go index a3cf55f2930..70813c1537b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go @@ -578,7 +578,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 @@ -694,7 +694,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 { @@ -703,6 +703,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 TestParseMetadataLocalHostname(t *testing.T) {