Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #5114 (2024-09-04 19:40:19)

🐛 Fix subnet sorting with multiple vpcs when launching an instance
<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4541 I introduced a change to allow subnets from other subnets to be returned from the AWS describe subnets query. However, the sorting is subtly bugged. `func(i, j int) bool {` is supposed to return true when i is _less_ than j, or should be ordered first. We want any subnets with the cluster VPC to be "less" than to sort the strings.

The existing implementation (which I wrote :disappointed: ) does not do that. `strings.Compare(*filtered[i].VpcId, clusterVPC)` returns `0` (string match) or `1` if the clusterVPC > VPCID, or `-1` if VPCID < clusterVIPC. The second comparison `strings.Compare(*filtered[j].VpcId, clusterVPC)` is the same. However, it does not make sense to compare the result of these two string comparisons to sort the strings, the goal is to ensure subnets with the cluster VPC is ordered earlier.

<!-- Enter a description of the change and why this change is needed -->

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes # N/A

**Special notes for your reviewer**:

The first commit updates only the unit tests to demonstrate the bug exists in the main branch. The second commit fixes the bug and makes the tests pass.

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [ ] squashed commits
- [ ] includes documentation
- [X] includes [emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning)
- [X] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Fix subnet sorting with multiple vpcs when launching an instance
```



Patch:
diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go
index ac11fea9f..f5757e04a 100644
--- a/pkg/cloud/services/ec2/instances.go
+++ b/pkg/cloud/services/ec2/instances.go
@@ -370,10 +370,10 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) {
 
 			filtered = append(filtered, subnet)
 		}
-		// prefer a subnet in the cluster VPC if multiple match
+		// keep AWS returned orderz stable, but prefer a subnet in the cluster VPC
 		clusterVPC := s.scope.VPC().ID
 		sort.SliceStable(filtered, func(i, j int) bool {
-			return strings.Compare(*filtered[i].VpcId, clusterVPC) > strings.Compare(*filtered[j].VpcId, clusterVPC)
+			return *filtered[i].VpcId == clusterVPC
 		})
 		if len(filtered) == 0 {
 			errMessage = fmt.Sprintf("failed to run machine %q, found %d subnets matching criteria but post-filtering failed.",


Test patch:
diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go
index 654403427..e53fd3feb 100644
--- a/pkg/cloud/services/ec2/instances_test.go
+++ b/pkg/cloud/services/ec2/instances_test.go
@@ -656,17 +656,25 @@ func TestCreateInstance(t *testing.T) {
 					}})).Return(&ec2.DescribeSubnetsOutput{
 					Subnets: []*ec2.Subnet{
 						{
-							VpcId:               aws.String("vpc-bar"),
+							VpcId:               aws.String("vpc-incorrect-1"),
+							SubnetId:            aws.String("subnet-5"),
+							AvailabilityZone:    aws.String("us-east-1c"),
+							CidrBlock:           aws.String("10.0.12.0/24"),
+							MapPublicIpOnLaunch: aws.Bool(false),
+						},
+						{
+							VpcId:               aws.String("vpc-incorrect-2"),
 							SubnetId:            aws.String("subnet-4"),
 							AvailabilityZone:    aws.String("us-east-1c"),
 							CidrBlock:           aws.String("10.0.10.0/24"),
 							MapPublicIpOnLaunch: aws.Bool(false),
 						},
 						{
-							VpcId:            aws.String("vpc-foo"),
-							SubnetId:         aws.String("subnet-3"),
-							AvailabilityZone: aws.String("us-east-1c"),
-							CidrBlock:        aws.String("10.0.11.0/24"),
+							VpcId:               aws.String("vpc-foo"),
+							SubnetId:            aws.String("subnet-3"),
+							AvailabilityZone:    aws.String("us-east-1c"),
+							CidrBlock:           aws.String("10.0.11.0/24"),
+							MapPublicIpOnLaunch: aws.Bool(false),
 						},
 					},
 				}, nil)