feat: add support for global accelerator endpoint groups

[kaessert]

Issue

https://github.com/kubernetes-sigs/aws-load-balancer-controller/issues/1572

Description

It's a first try to get some support to register and deregister endpoints to an existing endpointgroup. Because users are limited to one endpointgroup per region, i also think it makes the most sense to approach it that way. Later down the line one could work with weights even which would give users a mechanism to switch traffic between clusters.

The approach i chose for now has an ugly limitation of being: You can't delete an endpoint by deleting the listener. It looks to me like this is a design-limitation because as far as i see it:

• logic to destroy stuff is omitting it's output in the generated stack per ingress group
• ususally the external state is pictured with tags
• endpoints on ga don't support tags
• this means to me that the current logic of deleting resources is not capable of capturing the deletion case :(

What i tried next is to implement a custom cleanup logic, inspired by the way we cleanup security groups but that has the pitfall that we do the cleanup AFTER the stack is deployed (deleted in that case) which leaves us with no way of identifying the endpoint we created earlier on because we can't access the arn of the load-balancer anymore.

Maybe someone who knows the code better knows how to work around that. I considered adding labels as a workaround but wanted to get feedback first :)

Checklist

• [x] Added tests that cover your change (if possible)
• [x ] Added/modified documentation as required (such as the README.md, or the docs directory)
• [x] Manually tested
• [x] 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?! :exploding_head:

• [ ] Backfilled missing tests for code in same general area :tada:
• [ ] Refactored something and made the world a better place :star2:

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: atarax (b604e54dec7509b81f2edea1a745213fb3494b05)

[k8s-ci-robot]

Welcome @atarax!

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. :smiley:

[k8s-ci-robot]

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: atarax Once this PR has been reviewed and has the lgtm label, please assign m00nf1sh for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[johngmyers]

I don't think we should use an annotation for the ARN; that's in the domain of the cluster operator, not the service developer. This should instead be a field in IngressClassParams. It should also support searching by tag selector.

[johngmyers]

Documentation of the new annotations are missing from docs/guide/ingress/annotations.md (not that there should be any annotations).

[johngmyers]

If we can't find anywhere in the AWS API to stash ownership tags, we could at least store them somewhere in the Kubernetes API.

[johngmyers]

You could also put a tag on the ALBs listing any endpoint group each is registered with.

[kaessert]

I don't think we should use an annotation for the ARN; that's in the domain of the cluster operator, not the service developer. This should instead be a field in IngressClassParams. It should also support searching by tag selector.

That sound great. Shouldn't we not support both ways though like we do it for the ingress group name? I don't really get that last sentence about searching by tag selector though, sorry :/

[kaessert]

If we can't find anywhere in the AWS API to stash ownership tags, we could at least store them somewhere in the Kubernetes API.

What do you suggest? Would the status of the ingress object be an option?

[kaessert]

For cleanup, can't you delete the endpoints to the ARNs of all ALBs in the stack which aren't requesting to be attached to GA?

Sounds like a nice idea. Do you mean doing this by the same logic as in the load balancer synthesizer? I'm struggling to find a good way to abstract this without making methods of it public.

[johngmyers]

Shouldn't we not support both ways though like we do it for the ingress group name?

Other fields are done both ways because they started out when IngressClassParams didn't exist. My opinion is that new things that affect the ALB as a whole should only go into IngressClassParams, but I don't know what the approvers or other reviewers think.

I don't really get that last sentence about searching by tag selector though, sorry :/

I was referring to the tags fields in the selectors added in #2945 and #3277. One should follow that pattern for being able to discover AWS resources by either ID or tags.

What do you suggest? Would the status of the ingress object be an option?

I don't think the ingress status has a place to put that. You could perhaps put it in an annotation and rely on a finalizer, but I think the tag-on-ALB idea is better.

[kaessert]

@johngmyers maybe we can start here because there is many open point. The main thing i'm stuck with is the fact that i don't know how to identify the load balancers which are about to get deleted in the Synthesize method. In the current setup, when in arriving in Synthesize for the endpoint the load balancer is already deleted.

[kaessert]

Other fields are done both ways because they started out when IngressClassParams didn't exist. My opinion is that new things that affect the ALB as a whole should only go into IngressClassParams, but I don't know what the approvers or other reviewers think.

Oh, thanks a lot for that explanation. I'll work it that way then and i also see what you mean with searchable now :)

I don't think the ingress status has a place to put that. You could perhaps put it in an annotation and rely on a finalizer, but I think the tag-on-ALB idea is better.

I don't really like the idea of putting an annotation either, that's why i decided to get some feedback first before working on that. But the whole idea with keeping the state on the lb-tags kind of crumbles for me on the fact that in the endpoint Synthesize method, the lb is already deleted when a delete operation is taking place.

[johngmyers]

I think you might need to add to the ALB tear-down code. But I haven't delved into the back-end code yet; I've been working on the front-end so far. @M00nF1sh has more expertise there.

[kaessert]

I think you might need to add to the ALB tear-down code.

would be doable but would highly violate the modularity of the current design though

[kaessert]

@johngmyers @M00nF1sh , any news here?

[hhamalai]

Great that you're working on this issue allowing proper lifecycle management of GA+AWS LB integration.

Have you thought should e.g. status.loadBalancer.ingress contain the GA IPs instead of the LB hostname? This could make this change interoperable with other projects like external-dns.

Plus, when GA is used with the LB the traffic probably should be routed via GA-only, and LB type should be internal so that GA cannot be skipped.

[kaessert]

@hhamalai regarding the load-balancer ip, yes i thought about implementing that later down the line. But there is still an architecutal problem because current logic completely relies on the fact that all objects are taggable which GA endpoints are not. We don't delete lb-dependencies first either, so i wanted feedback from architects how they would approach this. This line for reference: https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/f38fe4911cc17e96e8179eaf2b63ddec8fc36aff/pkg/deploy/elbv2/load_balancer_synthesizer.go#L62

Regarding second point, i like the idea but not sure, need to think about it :)

[k8s-ci-robot]

PR needs rebase.

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.