Enhance subnet discovery #4109

M00nF1sh · 2025-03-26T00:34:19Z

This PR refactor and enhances the subnet discovery module as follows

Added a fallback mechanism to find subnets in VPC based on reachability(public/subnet) that matches ELB's requirement.
- this provides a way to remove the requirement of "kubernetes.io/role" tag on subnets by default.
- this fallback mechanism requires new IAM permission (ec2:DescribeRouteTables) to work.
- this fallback mechanism can be turned off by SubnetDiscoveryByReachability feature flag. We have enabled this featureFlag by default. It shall have no impact to customers since this fallback only invokes when no subnets with role tag found. (even cx forgets to add the new IAM permission during upgrade, subnet discovery will work just as before)
Refactor the subnetResolver to be more clear and have consistent behavior in multiple scenarios.
- moved the "minimalIPAddressCount"/"albSingleSubnet"/"SubnetClusterTagCheck" from resolveOptions into fields of subnetResolver.
  - Those are per cluster/controller settings, instead of need to be set per ALB/NLB
  - Currently, some restrictions are not enforced in certain scenarios and caused inconsistency. e.g. when DiscoveryViaSelector, the minimalIPAddressCount is not validated against subnets.
- refactor the logic within subnetResolver to be clear and split logic into smaller reusable chunks.
- made the validations consistent for resolveViaSelector(ID) to resolveViaNameOrIDSlice. (basically this TODO)
Rendered doc:

Issue

Description

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

k8s-ci-robot · 2025-03-26T00:34:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [M00nF1sh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ublic/private)

zac-nixon · 2025-03-28T16:19:49Z

pkg/networking/subnet_resolver.go

+	// - The subnet has a Kubernetes cluster tag matching the current cluster
+	clusterTagCheckEnabled bool
+	// whether to enable a single subnet as ALB subnet
+	// by default ALB requires two subent, only whitelisted users can use a single subnet


nit whitelisted -> allowlisted

zac-nixon · 2025-03-28T16:36:53Z

pkg/networking/subnet_resolver.go

+	req := &ec2sdk.DescribeSubnetsInput{
+		Filters: []ec2types.Filter{
+			{
+				Name:   awssdk.String("vpc-id"),


nit: you re-define this query names multiple times. maybe use a constant?

zac-nixon · 2025-03-28T16:49:51Z

pkg/networking/subnet_resolver.go

 		if err != nil {
 			return nil, err
 		}
 		resolvedSubnets = append(resolvedSubnets, subnets...)
 	}
-	if len(resolvedSubnets) != len(subnetNameOrIDs) {


Why did you remove this check? in the other function that queries for subnets by ID, you actually added this exact check.

oh, it's due to i moved this check into the listSubnetsByIDs and listSubnetsByNames
The length must equal when listSubnetsByIDs and listSubnetsByNames didn't return error

zac-nixon · 2025-03-28T16:58:52Z

pkg/networking/subnet_resolver.go

+			chosenSubnets = append(chosenSubnets, subnets[0])
+		}
+	}
+	sortSubnetsByID(chosenSubnets)


Doesn't this sort mess up the EIP allocation feature? #3925

it doesn't
This sort only triggers for "Discovery" via tag selector or role tag or reachability.
When subnet explicitly specified, no sort will occur

zac-nixon · 2025-03-28T20:31:41Z

/lgtm

wweiwei-li · 2025-03-28T21:40:34Z

/lgtm

fix bug(introduced by #4109) in subnet resolver

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2025

k8s-ci-robot requested review from oliviassss and wweiwei-li March 26, 2025 00:34

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2025

M00nF1sh force-pushed the subnet-reachability branch from 0bb20f7 to 598e0e6 Compare March 27, 2025 22:10

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2025

M00nF1sh force-pushed the subnet-reachability branch 3 times, most recently from 3bff4f6 to 1140a0d Compare March 28, 2025 00:31

M00nF1sh changed the title ~~Enhance subnet discovery by fallback to find subnets in VPC whose reachability(public/subnet).~~ Enhance subnet discovery Mar 28, 2025

M00nF1sh force-pushed the subnet-reachability branch from 1140a0d to 707ad82 Compare March 28, 2025 00:41

refactor subnet resolver and add discovery by subnet's reachability(p…

2f6ce5c

…ublic/private)

M00nF1sh force-pushed the subnet-reachability branch from 707ad82 to 2f6ce5c Compare March 28, 2025 00:45

M00nF1sh requested a review from zac-nixon March 28, 2025 00:45

zac-nixon reviewed Mar 28, 2025

View reviewed changes

address comments

c183219

k8s-ci-robot assigned zac-nixon Mar 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2025

k8s-ci-robot assigned wweiwei-li Mar 28, 2025

k8s-ci-robot merged commit 539245d into kubernetes-sigs:main Mar 28, 2025
9 checks passed

M00nF1sh mentioned this pull request Mar 31, 2025

fix bug(introduced by #4109) in subnet resolver #4114

Merged

6 tasks

k8s-ci-robot added a commit that referenced this pull request Mar 31, 2025

Merge pull request #4114 from M00nF1sh/fix_subr

45ba355

fix bug(introduced by #4109) in subnet resolver

zac-nixon mentioned this pull request May 6, 2025

🥳 aws-load-balancer-controller v2.13.0 Automated Release! 🥑 aws/eks-charts#1214

Merged

RafiGreenberg mentioned this pull request Jun 4, 2025

feat: Update AWS Load Balancer controller policy to match v2.13 of the upstream project terraform-aws-modules/terraform-aws-iam#573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enhance subnet discovery #4109

Enhance subnet discovery #4109

Uh oh!

M00nF1sh commented Mar 26, 2025 •

edited

Loading

Uh oh!

k8s-ci-robot commented Mar 26, 2025

Uh oh!

zac-nixon Mar 28, 2025

Uh oh!

M00nF1sh Mar 28, 2025

Uh oh!

zac-nixon Mar 28, 2025

Uh oh!

M00nF1sh Mar 28, 2025

Uh oh!

zac-nixon Mar 28, 2025

Uh oh!

M00nF1sh Mar 28, 2025

Uh oh!

zac-nixon Mar 28, 2025

Uh oh!

M00nF1sh Mar 28, 2025

Uh oh!

zac-nixon commented Mar 28, 2025

Uh oh!

wweiwei-li commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

Enhance subnet discovery #4109

Enhance subnet discovery #4109

Uh oh!

Conversation

M00nF1sh commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

k8s-ci-robot commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zac-nixon commented Mar 28, 2025

Uh oh!

wweiwei-li commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

M00nF1sh commented Mar 26, 2025 •

edited

Loading