don't block TGB reconciliation loop on failed SG ingress reconciliation

[michaelsaah]

Issue

#3285

Description

the controller performs an SG reconciliation step for all (cluster-wide) SGs gathered from all TGBs during the TGB reconciliation loop, before TG endpoints are reconciled:

aws-load-balancer-controller/pkg/targetgroupbinding/resource_manager.go

Lines 139 to 141 in d177c89

	if err := m.networkingManager.ReconcileForPodEndpoints(ctx, tgb, endpoints); err != nil {
	return err
	}

the way the code is currently written, this means that any failure during SG reconciliation blocks reconciliation of all targets across the whole cluster. such a failure can be caused by something as innocuous as a SG being deleted before the associated TGB is deleted, or a SG being entered on a TGB erroneously. this can easily lead to severe outages if not remediated quickly.

this commit changes the method reconcileWithIngressPermissionsPerSG to not exit on a single failed SG ingress reconciliation - instead it will log the offending error and continue through the loop.

I tested this by setting up two TGBs, modifying one to have a non-existent ingress SG, deleting a target pod, then ensuring that the new target is reconciled. the new error message was also logged:

{"level":"error","ts":"2023-07-26T19:39:03Z","msg":"Security group reconciliation failed for SG","SecurityGroupID":"sg-0d904125da9f258a7","error":"InvalidGroup.NotFound: The security group 'sg-0b4935e438dcca2ee' does not exist\n\tstatus code: 400, request id: 34e0001c-c85e-4f6e-b7e1-c32093afe402"}

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 🌟

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: michaelsaah (5e8c6df, d6e8667, 6b78ca3, 4607bb4)

[k8s-ci-robot]

Welcome @michaelsaah!

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]

Hi @michaelsaah. 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.

[johngmyers]

I cannot support what-me-worry ignoring of errors. At the very least, the error needs to be surfaced as a Kubernetes event and the handler needs to mark the work item for retry.

[michaelsaah]

thanks @johngmyers. surfacing the error via an Event sounds fine to me.

I'm not sure what you mean by "mark the work item for retry." can you point me towards any examples in the code where this is performed?

[michaelsaah]

@johngmyers @M00nF1sh just bumping this, it's an ongoing operational issue for us (Twilio) on EKS. any help appreciated.

[michaelsaah]

just another bump, any eyes appreciated

[M00nF1sh]

@michaelsaah

Thanks for this contribution. I agree with @johngmyers that the error cannot be ignored without retry. Since such SG modification errors can be transient errors such as EC2 api throttle and shall be retried to avoid issues in security group settings.

How about below change:

1. inside reconcileWithIngressPermissionsPerSG, when reconcile for each SG(aggregatedIngressPermissionsPerSG), we don't fail upon error but aggregate all errors into a single error and return it. (e.g. use errors.Join)
2. in

   aws-load-balancer-controller/pkg/targetgroupbinding/resource_manager.go

   Line 139 in d177c89

   if err := m.networkingManager.ReconcileForPodEndpoints(ctx, tgb, endpoints); err != nil {

   , if ReconcileForPodEndpoints failed with error, we don't return the error immediately, instead we continue target registration/deregistration and return runtime.NewRequeueNeeded("networking reconciliation") if the ReconcileForPodEndpoints returned a error before.
3. don't emit the event inside reconcileWithIngressPermissionsPerSG, emit the event during inside targetgroupbinding/resource_manager.go instead, so there is only one event if multiple SG reconcillation failed.

[michaelsaah]

thanks for the review @M00nF1sh, addressed your feedback. have a good weekend!

[michaelsaah]

just bumping this for a re-review, thanks

[michaelsaah]

morning, just a monday review bump, thanks

[johngmyers]

Just a minor style nit

/lgtm

[johngmyers]

/ok-to-test

[michaelsaah]

thanks for the re-review @johngmyers, committed your style suggestion

[johngmyers]

/lgtm
/assign @M00nF1sh

[michaelsaah]

looks like that test might be flaky: #3289 (comment)

[johngmyers]

/retest

[M00nF1sh]

Sorry for late review. This change looks good to me now, thanks a lot for your contribution :D

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, M00nF1sh, michaelsaah

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]

Codecov Report

Patch coverage has no change and project coverage change: +0.85% 🎉

Comparison is base (d177c89) 54.70% compared to head (4607bb4) 55.56%.
Report is 40 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3296      +/-   ##
==========================================
+ Coverage   54.70%   55.56%   +0.85%     
==========================================
  Files         148      149       +1     
  Lines        8590     8828     +238     
==========================================
+ Hits         4699     4905     +206     
- Misses       3559     3586      +27     
- Partials      332      337       +5     

Files Changed	Coverage Δ
pkg/targetgroupbinding/networking_manager.go	33.92% <0.00%> (-0.62%)	⬇️
pkg/targetgroupbinding/resource_manager.go	14.96% <0.00%> (-0.21%)	⬇️

... and 13 files with indirect coverage changes

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

[michaelsaah]

/retest

[johngmyers]

/retest