Feature alb subnets #3437

jerryhe1999 · 2023-10-18T00:33:47Z

Issue

Description

Added a new feature gate named "ALBSingleSubnet" with default value false, once it set to true, the user who get whitelisted by AWS ELB team for using only one subnet for their application load balancer could be processed as expected.

Manual Test Items:

For account which doesn't get whitelisted, creating the ingress resource with only one subnet attached.
Comes up with LBC error message while ALBSIngleSubnet is set to false.
Comes up with ELB error message while ALBSIngleSubnet is set to true.

For account which get whitelisted, creating the ingress resource with only one subnet attached.
Comes up with LBC error message while ALBSIngleSubnet is set to false.
ALB is successfully provisioning while ALBSIngleSubnet is set to true.

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 · 2023-10-18T00:33:57Z

Hi @jerryhe1999. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

oliviassss · 2023-10-23T20:24:40Z

pkg/networking/subnet_resolver.go

@@ -48,6 +48,8 @@ type SubnetsResolveOptions struct {
 	AvailableIPAddressCount int64
 	// whether to check the cluster tag
 	SubnetsClusterTagCheck bool
+	// Disable subnet minimal count restriction


Maybe reword: whether to allow using only 1 subnet for provisioning ALB, default to false

oliviassss · 2023-10-23T20:24:48Z

pkg/networking/subnet_resolver.go

@@ -95,6 +97,13 @@ func WithSubnetsClusterTagCheck(SubnetsClusterTagCheck bool) SubnetsResolveOptio
 	}
 }

+// WithALBSingleSubnet generate an option that foncigure ALBSingleSubnet


nit: foncigure or configure?

pkg/networking/subnet_resolver.go

oliviassss · 2023-10-23T20:28:00Z

Thanks for the contribution. Please also add:

doc updates regarding the new feature gate flag: https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/docs/deploy/configurations.md#feature-gates
doc updates re subnet discovery: https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/docs/deploy/subnet_discovery.md#subnet-auto-discovery
Please add the manual tests in the PR descriptions.

johngmyers · 2023-10-27T20:56:25Z

pkg/networking/subnet_resolver.go

@@ -364,6 +373,9 @@ func (r *defaultSubnetsResolver) validateSubnetsMinimalCount(subnets []*ec2sdk.S
 // computeSubnetsMinimalCount returns the minimal count requirement for subnets.
 func (r *defaultSubnetsResolver) computeSubnetsMinimalCount(subnetLocale subnetLocaleType, resolveOpts SubnetsResolveOptions) int {
 	minimalCount := 1
+	if resolveOpts.ALBSingleSubnet && resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication {
+		return minimalCount
+	}
 	if resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication && subnetLocale == subnetLocaleTypeAvailabilityZone {


Would be much more straightforward as:

Suggested change

if resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication && subnetLocale == subnetLocaleTypeAvailabilityZone {

if resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication && subnetLocale == subnetLocaleTypeAvailabilityZone && !resolveOpts.ALBSingleSubnet {

Thank you for reviewing @johngmyers, the change has been made and submitted.

johngmyers · 2023-10-27T21:02:03Z

Is there any particular reason this needs to be feature-flagged?

johngmyers · 2023-10-27T21:06:22Z

To answer my own question, I think this is so that LBC can give better and earlier error messages in the common case where the ALB service requires a minimum of 2.

oliviassss · 2023-10-27T21:08:20Z

@johngmyers, it's an opt-in feature from ELB side. They are working on lifting the minimal subnet constraint for ALB. AFAIK, in some region like KIX they only require 1 subnet for ALB, but most regions still require 2 subnets as minimal unless the account is allowlisted.

oliviassss · 2023-10-28T21:37:06Z

/lgtm
/Assign @M00nF1sh

M00nF1sh

/lgtm
/approve

k8s-ci-robot · 2023-11-02T18:20:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerryhe1999, 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

codecov-commenter · 2023-11-02T18:26:17Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (631041d) 55.75% compared to head (e172a9f) 55.75%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3437      +/-   ##
==========================================
- Coverage   55.75%   55.75%   -0.01%     
==========================================
  Files         149      149              
  Lines        8838     8843       +5     
==========================================
+ Hits         4928     4930       +2     
- Misses       3576     3579       +3     
  Partials      334      334

Files	Coverage Δ
pkg/ingress/model_build_load_balancer.go	`72.46% <100.00%> (+0.22%)`	⬆️
pkg/config/feature_gates.go	`0.00% <0.00%> (ø)`
pkg/networking/subnet_resolver.go	`87.55% <33.33%> (-0.82%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jerryhe1999 added 2 commits October 18, 2023 00:26

Add new feature gate to remove subnets minimal count restirction

c78d2ca

Add new feature gate to values.yaml.

3120390

k8s-ci-robot requested review from johngmyers and M00nF1sh October 18, 2023 00:33

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 18, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 18, 2023

oliviassss reviewed Oct 23, 2023

View reviewed changes

jerryhe1999 added 2 commits October 27, 2023 20:16

Add documentation description for feature gate ALBSingleSubnet

290c878

Add load balancer type check

aa131a8

oliviassss added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 27, 2023

johngmyers reviewed Oct 27, 2023

View reviewed changes

Adjust check to make it more straightforward

e172a9f

k8s-ci-robot assigned M00nF1sh and oliviassss Oct 28, 2023

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

M00nF1sh approved these changes Nov 2, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

k8s-ci-robot merged commit 70b2799 into kubernetes-sigs:main Nov 2, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature alb subnets #3437

Feature alb subnets #3437

jerryhe1999 commented Oct 18, 2023 •

edited

Loading

k8s-ci-robot commented Oct 18, 2023

oliviassss Oct 23, 2023

oliviassss Oct 23, 2023

oliviassss commented Oct 23, 2023

johngmyers Oct 27, 2023

jerryhe1999 Oct 27, 2023

johngmyers commented Oct 27, 2023

johngmyers commented Oct 27, 2023

oliviassss commented Oct 27, 2023

oliviassss commented Oct 28, 2023

M00nF1sh left a comment

k8s-ci-robot commented Nov 2, 2023

codecov-commenter commented Nov 2, 2023

	if resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication && subnetLocale == subnetLocaleTypeAvailabilityZone {
	if resolveOpts.LBType == elbv2model.LoadBalancerTypeApplication && subnetLocale == subnetLocaleTypeAvailabilityZone && !resolveOpts.ALBSingleSubnet {

Feature alb subnets #3437

Feature alb subnets #3437

Conversation

jerryhe1999 commented Oct 18, 2023 • edited Loading

Issue

Description

Checklist

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

k8s-ci-robot commented Oct 18, 2023

oliviassss Oct 23, 2023

Choose a reason for hiding this comment

oliviassss Oct 23, 2023

Choose a reason for hiding this comment

oliviassss commented Oct 23, 2023

johngmyers Oct 27, 2023

Choose a reason for hiding this comment

jerryhe1999 Oct 27, 2023

Choose a reason for hiding this comment

johngmyers commented Oct 27, 2023

johngmyers commented Oct 27, 2023

oliviassss commented Oct 27, 2023

oliviassss commented Oct 28, 2023

M00nF1sh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 2, 2023

codecov-commenter commented Nov 2, 2023

Codecov Report

jerryhe1999 commented Oct 18, 2023 •

edited

Loading