support cli flag to enable manage backend SG rules for ALB #4145

shuqz · 2025-04-15T22:38:35Z

Issue

Description

This change introduces a new CLI flag named --enable-manage-backend-security-group-rules which takes true|false, default is false. This change applies to ALB for now, NLB will be fast follow-up.

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 with multiple test cases

Flag: Enable-backend-security-group	Flag: Enable-manage-backend-security-group-rules	Annotation: manage-backend-security-group-rules	Expected Behavior
false	true	N/A	error with backend security group must be enabled when manage backend security group rule is enabled
true	true	not specified	manage backend security group rules by controller enabled
true	true	false	manage backend security group rules by controller NOT enabled
true	false	true	manage backend security group rules by controller enabled

[✅] E2E test for ingress & service
[✅] 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 🌟

linux-foundation-easycla · 2025-04-15T22:38:39Z

The committers listed above are authorized under a signed CLA.

✅ login: shuqz (dbe9af8)

k8s-ci-robot · 2025-04-15T22:38:44Z

Welcome @shuqz!

It looks like this is your first PR to kubernetes-sigs/aws-load-balancer-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-load-balancer-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-04-15T22:38:45Z

Hi @shuqz. 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-sigs/prow repository.

zac-nixon · 2025-04-15T23:32:25Z

docs/deploy/security_groups.md

+- To enable managing backend security group rules for all resources, using cli flag `--enable-manage-backend-security-group-rules`
+ - when set to `true`, the controller will automatically manage backend security group rules for all resources
+ - individual ingress annotation takes precedence over cli flag, meaning it can be overridden with annotation `alb.ingress.kubernetes.io/manage-backend-security-group-rules: "false"`
+ - for this to take effect, `--enable-backend-security-group` needs to be true and user explicitly specify security group using annotation: alb.ingress.kubernetes.io/security-groups


Make sure to include the nlb annotation :) when you add support.

zac-nixon · 2025-04-15T23:34:50Z

pkg/ingress/model_builder.go

+defaultTargetType: elbv2model.TargetType(defaultTargetType),
+defaultLoadBalancerScheme: elbv2model.LoadBalancerScheme(defaultLoadBalancerScheme),
+enableBackendSG: enableBackendSG,
+enableManageBackendSGRules: enableManageBackendSecurityGroupRules,


nit: can you call this defaultEnableManageBackendSGRules to denote that this is the default value.

it is defined in a different package(ingress VS config), i can export it if want to. but i do not know why we want to do this. based on the current pattern we have in this function, we do not denote any default value? the default value is assigned in func (cfg *ControllerConfig) BindFlags(fs *pflag.FlagSet) let me know if i misunderstand it.

synced offline, will update the name

zac-nixon · 2025-04-15T23:35:37Z

pkg/ingress/model_builder.go

+// first check cli flag
+if t.enableManageBackendSGRules {
+manageSGRules = t.enableManageBackendSGRules
+t.logger.Info("Enable manage backend security group rules flag is configured to be true via cli flag.")


this log message might get noisy. Recommend removing it or adding .V(1) to the the log.

i will update it with .V(1)

zac-nixon · 2025-04-15T23:36:21Z

pkg/ingress/model_builder.go

 func (t *defaultModelBuildTask) buildManageSecurityGroupRulesFlag(_ context.Context) (bool, error) {
 explicitManageSGRulesFlag := make(map[bool]struct{})
-manageSGRules := false
+manageSGRules := false // default value


you could save some LoC by just doing

manageSGRules := t.enableManageBackendSGRules

also found a little bug for case when annotation is not specified, going to fix it

zac-nixon · 2025-04-15T23:36:38Z

pkg/ingress/model_builder.go

-manageSGRules = rawManageSGRule
+if rawManageSGRule != manageSGRules {
+manageSGRules = rawManageSGRule
+t.logger.Info("Override enable manage backend security group rules flag with annotation", "value: ", rawManageSGRule, "for ingress", k8s.NamespacedName(member.Ing).String(), "in ingress yaml file")


same comment, might get noisy.

shraddhabang

/lgtm

zac-nixon · 2025-04-17T04:40:49Z

/ok-to-test

remove auto-generated file address comments support enable manage backend SG rules for NLB

zac-nixon

/lgtm

k8s-ci-robot · 2025-04-17T17:41:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shuqz, zac-nixon

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~~ [zac-nixon]

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

wweiwei-li · 2025-04-17T17:47:31Z

/lgtm

k8s-ci-robot requested review from M00nF1sh and zac-nixon April 15, 2025 22:38

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 15, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 15, 2025

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 15, 2025

zac-nixon reviewed Apr 15, 2025

View reviewed changes

shraddhabang reviewed Apr 16, 2025

View reviewed changes

k8s-ci-robot assigned shraddhabang Apr 16, 2025

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

shuqz force-pushed the main branch from 3ab878e to d77b8bb Compare April 16, 2025 22:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2025

k8s-ci-robot 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 Apr 17, 2025

support cli flag to enable manage backend SG rules for ALB

dbe9af8

remove auto-generated file address comments support enable manage backend SG rules for NLB

shuqz force-pushed the main branch from d77b8bb to dbe9af8 Compare April 17, 2025 17:29

zac-nixon approved these changes Apr 17, 2025

View reviewed changes

k8s-ci-robot assigned zac-nixon Apr 17, 2025

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2025

k8s-ci-robot assigned wweiwei-li Apr 17, 2025

k8s-ci-robot merged commit 409a680 into kubernetes-sigs:main Apr 17, 2025
5 checks passed

wweiwei-li mentioned this pull request Apr 18, 2025

Ability to make manage-backend-security-group-rules: true by default #4086

Closed

2 tasks

This was referenced Apr 30, 2025

[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

support cli flag to enable manage backend SG rules for ALB #4145

support cli flag to enable manage backend SG rules for ALB #4145

Uh oh!

shuqz commented Apr 15, 2025 •

edited

Loading

linux-foundation-easycla bot commented Apr 15, 2025 •

edited

Loading

k8s-ci-robot commented Apr 15, 2025

k8s-ci-robot commented Apr 15, 2025

zac-nixon Apr 15, 2025

zac-nixon Apr 15, 2025

shuqz Apr 16, 2025

shuqz Apr 16, 2025

zac-nixon Apr 15, 2025

shuqz Apr 16, 2025

zac-nixon Apr 15, 2025

shuqz Apr 16, 2025

zac-nixon Apr 15, 2025

shraddhabang left a comment

zac-nixon commented Apr 17, 2025

zac-nixon left a comment

k8s-ci-robot commented Apr 17, 2025

wweiwei-li commented Apr 17, 2025

Uh oh!

Labels

5 participants

support cli flag to enable manage backend SG rules for ALB #4145

support cli flag to enable manage backend SG rules for ALB #4145

Uh oh!

Conversation

shuqz commented Apr 15, 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?! 🤯

linux-foundation-easycla bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

k8s-ci-robot commented Apr 15, 2025

k8s-ci-robot commented Apr 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shraddhabang left a comment

Choose a reason for hiding this comment

zac-nixon commented Apr 17, 2025

zac-nixon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 17, 2025

wweiwei-li commented Apr 17, 2025

Uh oh!

Labels

5 participants

shuqz commented Apr 15, 2025 •

edited

Loading

linux-foundation-easycla bot commented Apr 15, 2025 •

edited

Loading