🐛 Fix creation of target groups and listeners in the reconcile loop #5017

r4f4 · 2024-06-12T20:24:57Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Fixes an error where capa will create duplicate target groups and listeners.

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 #5015

Special notes for your reviewer:

Checklist:

Release note:

Fixes target group and listeners creation for v2 Load Balancers.

We were comparing pointers that were never going to be equal. Let's check their pointed-to values instead. Also added a break when the listener is found. Changed the `createdListeners` list to only include a listener if it was created. This list is currently not used for anything, so this change should have no impact.

We cannot compare target groups by name because every time we get the desired spec, it contains newly-generated random names. Instead, let's check that the prefixes match and that the port and protocol properties are the same.

k8s-ci-robot · 2024-06-12T20:25:07Z

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

nrb · 2024-06-12T20:36:53Z

/ok-to-test

r4f4 · 2024-06-12T20:40:02Z

I'm currently running the Openshift e2e on this fix. Will report back when I have results.

nrb · 2024-06-12T20:41:43Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass

nrb · 2024-06-12T21:32:50Z

/retest

patrickdillon · 2024-06-12T21:33:41Z

LGTM

mtulio

/lgtm

k8s-ci-robot · 2024-06-12T22:13:42Z

@mtulio: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

r4f4 · 2024-06-12T22:24:47Z

All the openshift tests in this PR, except for one, reached a running cluster stage.

In the job that failed, no duplicated target groups/listeners are created so the issue seems to be fixed.

nrb · 2024-06-12T22:50:43Z

/retest

nrb · 2024-06-12T22:57:26Z

/approve
/hold

Holding until it passes e2e. Failures appear to be unrelated to this change, though.

/assign @damdo
for second review.

k8s-ci-robot · 2024-06-12T22:57:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nrb

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~~ [nrb]

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

damdo · 2024-06-13T07:10:25Z

/test pull-cluster-api-provider-aws-e2e

JoelSpeed

Matching the groups based on naming sounds like it could become a little fragile over time, for example if we ever decided to change the names.

Is there any reason not to go straight to checking the port and the type rather than relying on names?

What would happen if a manually created target group matched the spec but wasn't named correctly, would the cluster still function?

r4f4 · 2024-06-13T07:59:39Z

Matching the groups based on naming sounds like it could become a little fragile over time, for example if we ever decided to change the names.

Yes. I tried to move the used prefixes to consts to help with that.

Is there any reason not to go straight to checking the port and the type rather than relying on names?

I was discussing this with @mtulio. Ideally we would check the owned tag but I'm reluctant about adding even more API calls (one of the failures reasons we observed was API rate limitting). If it's acceptable that we might match target groups that were not created by CAPA, then we can check just port/type.

What would happen if a manually created target group matched the spec but wasn't named correctly, would the cluster still function?

Then CAPA would create a new target group. As long as an associated listener doesn't exist for that port, it should work. If there is no listener, then the target group was not associated with the LB and CAPA wouldn't have discovered it in the first place. I haven't tested this though.

damdo · 2024-06-13T08:50:58Z

/test pull-cluster-api-provider-aws-e2e

JoelSpeed · 2024-06-13T09:23:35Z

Having discussed out of band, the current approach is better than what we have in main today, which is broken and has potential for leaking resources.

I would like to see some more testing of the scenarios where users bring their own target groups or start modifying the target groups after the LB is created, but that needn't block this PR

r4f4 · 2024-06-13T09:28:39Z

pkg/cloud/services/elb/loadbalancer.go

@@ -1604,8 +1615,9 @@ func (s *Service) reconcileTargetGroupsAndListeners(lbARN string, spec *infrav1.

 		var listener *elbv2.Listener
 		for _, l := range existingListeners.Listeners {
-			if l.DefaultActions != nil && len(l.DefaultActions) > 0 && l.DefaultActions[0].TargetGroupArn == group.TargetGroupArn {
+			if l.DefaultActions != nil && len(l.DefaultActions) > 0 && *l.DefaultActions[0].TargetGroupArn == *group.TargetGroupArn {


According to https://docs.aws.amazon.com/sdk-for-go/api/service/elbv2/#ELBV2.CreateListener:

This operation is idempotent, which means that it completes at most one time. If you attempt to create multiple listeners with the same settings, each call succeeds.

So we could probably drop this check for an existing listener and always try createListener.

damdo

E2Es passed. The fix LGTM
Thanks @r4f4
/lgtm

r4f4 · 2024-06-13T11:04:06Z

/hold cancel
e2e passed.

r4f4 added 2 commits June 12, 2024 22:11

🐛: elbv2: fix target group creation

be21aac

We cannot compare target groups by name because every time we get the desired spec, it contains newly-generated random names. Instead, let's check that the prefixes match and that the port and protocol properties are the same.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2024

k8s-ci-robot requested review from fiunchinho and nrb June 12, 2024 20:25

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 12, 2024

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 Jun 12, 2024

mtulio reviewed Jun 12, 2024

View reviewed changes

k8s-ci-robot assigned damdo Jun 12, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2024

mtulio mentioned this pull request Jun 13, 2024

Duplicate target groups and listeners created in the ELBv2 reconcile loop #5015

Closed

JoelSpeed reviewed Jun 13, 2024

View reviewed changes

r4f4 commented Jun 13, 2024

View reviewed changes

damdo reviewed Jun 13, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2024

k8s-ci-robot merged commit 888c659 into kubernetes-sigs:main Jun 13, 2024
29 checks passed

r4f4 mentioned this pull request Jun 14, 2024

OCPBUGS-34819: capi/aws: bump provider for fixes openshift/installer#8545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

r4f4 commented Jun 12, 2024

k8s-ci-robot commented Jun 12, 2024

nrb commented Jun 12, 2024

r4f4 commented Jun 12, 2024

nrb commented Jun 12, 2024

nrb commented Jun 12, 2024

patrickdillon commented Jun 12, 2024

mtulio left a comment

k8s-ci-robot commented Jun 12, 2024

r4f4 commented Jun 12, 2024

nrb commented Jun 12, 2024

nrb commented Jun 12, 2024

k8s-ci-robot commented Jun 12, 2024

damdo commented Jun 13, 2024

JoelSpeed left a comment

r4f4 commented Jun 13, 2024

damdo commented Jun 13, 2024

JoelSpeed commented Jun 13, 2024

r4f4 Jun 13, 2024

damdo left a comment

r4f4 commented Jun 13, 2024

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

Conversation

r4f4 commented Jun 12, 2024

k8s-ci-robot commented Jun 12, 2024

nrb commented Jun 12, 2024

r4f4 commented Jun 12, 2024

nrb commented Jun 12, 2024

nrb commented Jun 12, 2024

patrickdillon commented Jun 12, 2024

mtulio left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 12, 2024

r4f4 commented Jun 12, 2024

nrb commented Jun 12, 2024

nrb commented Jun 12, 2024

k8s-ci-robot commented Jun 12, 2024

damdo commented Jun 13, 2024

JoelSpeed left a comment

Choose a reason for hiding this comment

r4f4 commented Jun 13, 2024

damdo commented Jun 13, 2024

JoelSpeed commented Jun 13, 2024

r4f4 Jun 13, 2024

Choose a reason for hiding this comment

damdo left a comment

Choose a reason for hiding this comment

r4f4 commented Jun 13, 2024