From 681bb78f783d0fc9071f48c2658c66c9f1e8c226 Mon Sep 17 00:00:00 2001
From: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Date: Mon, 20 Nov 2023 16:41:22 +0100
Subject: [PATCH] aws: log when multiple subnets found for the same
 availability zone

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
---
 aws/adapter.go      | 54 +++++++++++++++++++++------------------------
 aws/adapter_test.go | 45 +++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 29 deletions(-)

diff --git a/aws/adapter.go b/aws/adapter.go
index 19df8977..7326c337 100644
--- a/aws/adapter.go
+++ b/aws/adapter.go
@@ -3,6 +3,7 @@ package aws
 import (
 	"errors"
 	"fmt"
+	"sort"
 	"strings"
 	"time"
 
@@ -915,47 +916,42 @@ func (a *Adapter) FindLBSubnets(scheme string) []string {
 		internal = true
 	}
 
-	subnetsByAZ := make(map[string]*subnetDetails)
+	subnetsByAZ := make(map[string][]*subnetDetails)
 	for _, subnet := range a.manifest.subnets {
 		// ignore private subnet for public LB
 		if !internal && !subnet.public {
 			continue
 		}
+		subnetsByAZ[subnet.availabilityZone] = append(subnetsByAZ[subnet.availabilityZone], subnet)
+	}
 
-		existing, ok := subnetsByAZ[subnet.availabilityZone]
-		if !ok {
-			subnetsByAZ[subnet.availabilityZone] = subnet
-			continue
-		}
+	subnetIDs := make([]string, 0, len(subnetsByAZ))
+	for az, subnets := range subnetsByAZ {
+		if len(subnets) > 1 {
+			sort.Slice(subnets, func(a, b int) bool {
+				subnetA, subnetB := subnets[a], subnets[b]
 
-		// prefer subnet with an elb role tag
-		var tagName string
-		if internal {
-			tagName = internalELBRoleTagName
-		} else {
-			tagName = elbRoleTagName
-		}
+				// prefer subnet with an elb role tag
+				tagName := elbRoleTagName
+				if internal {
+					tagName = internalELBRoleTagName
+				}
 
-		_, existingHasTag := existing.tags[tagName]
-		_, subnetHasTag := subnet.tags[tagName]
+				_, subnetAHasTag := subnetA.tags[tagName]
+				_, subnetBHasTag := subnetB.tags[tagName]
 
-		if existingHasTag != subnetHasTag {
-			if subnetHasTag {
-				subnetsByAZ[subnet.availabilityZone] = subnet
-			}
-			continue
-		}
+				if subnetAHasTag != subnetBHasTag {
+					return subnetAHasTag
+				}
 
-		// If we have two subnets for the same AZ we arbitrarily choose
-		// the one that is first lexicographically.
-		if strings.Compare(existing.id, subnet.id) > 0 {
-			subnetsByAZ[subnet.availabilityZone] = subnet
+				// prefer subnet id that is first lexicographically
+				return subnetA.id < subnetB.id
+			})
+
+			log.Warnf("Found multiple subnets %v for availability zone %s, using %s", subnets, az, subnets[0].id)
 		}
-	}
 
-	subnetIDs := make([]string, 0, len(subnetsByAZ))
-	for _, subnet := range subnetsByAZ {
-		subnetIDs = append(subnetIDs, subnet.id)
+		subnetIDs = append(subnetIDs, subnets[0].id)
 	}
 
 	return subnetIDs
diff --git a/aws/adapter_test.go b/aws/adapter_test.go
index 007ce1ce..c32060ca 100644
--- a/aws/adapter_test.go
+++ b/aws/adapter_test.go
@@ -704,6 +704,23 @@ func TestFindLBSubnets(tt *testing.T) {
 			scheme:          elbv2.LoadBalancerSchemeEnumInternetFacing,
 			expectedSubnets: []string{"1"},
 		},
+		{
+			name: "should select first lexicographically subnet when two match a single zone (regardless of details order)",
+			subnets: []*subnetDetails{
+				{
+					availabilityZone: "a",
+					public:           true,
+					id:               "1",
+				},
+				{
+					availabilityZone: "a",
+					public:           true,
+					id:               "2",
+				},
+			},
+			scheme:          elbv2.LoadBalancerSchemeEnumInternetFacing,
+			expectedSubnets: []string{"1"},
+		},
 		{
 			name: "should not use internal subnets for public LB",
 			subnets: []*subnetDetails{
@@ -736,6 +753,34 @@ func TestFindLBSubnets(tt *testing.T) {
 			scheme:          elbv2.LoadBalancerSchemeEnumInternetFacing,
 			expectedSubnets: []string{"2"},
 		},
+		{
+			name: "should prefer tagged subnet selected first lexicographically",
+			subnets: []*subnetDetails{
+				{
+					availabilityZone: "a",
+					public:           true,
+					id:               "1",
+				},
+				{
+					availabilityZone: "a",
+					public:           true,
+					id:               "2",
+					tags: map[string]string{
+						elbRoleTagName: "",
+					},
+				},
+				{
+					availabilityZone: "a",
+					public:           true,
+					id:               "3",
+					tags: map[string]string{
+						elbRoleTagName: "",
+					},
+				},
+			},
+			scheme:          elbv2.LoadBalancerSchemeEnumInternetFacing,
+			expectedSubnets: []string{"2"},
+		},
 		{
 			name: "should prefer tagged subnet (internal)",
 			subnets: []*subnetDetails{