-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Feature: Multi Cluster TargetGroupBindings #3853
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zac-nixon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @zac-nixon. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
| [alb.ingress.kubernetes.io/conditions.${conditions-name}](#conditions) | json |N/A| Ingress | N/A | | ||
| [alb.ingress.kubernetes.io/target-node-labels](#target-node-labels) | stringMap |N/A| Ingress,Service | N/A | | ||
| [alb.ingress.kubernetes.io/mutual-authentication](#mutual-authentication) | json |N/A| Ingress | Exclusive | | ||
| [alb.ingress.kubernetes.io/multi-cluster-target-group](#multi-cluster-target-group) | boolean |N/A| Ingress, Service | Merge | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge behavior does not apply here as each ingress in a group creates a its own target group and not necessarily wold want to merge this setting across the ingresses sharing the group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you recommend for this setting?
9ea5c6b
to
7f042cc
Compare
7f042cc
to
7ebb0a6
Compare
Issue
#2173
Description
Initial approach done here, but didn't handle all use cases such as instance target groups or clusters that share subnets. This new approach will work with IP and Instance targets and makes no assumptions on the clusters' composition, subnets, etc.
This PR adds a new flag to the TGB CR that allows users to specify that a TGB TG ARN might be associated to more than one cluster. This flag can be set from ingress / svc annotations as well for managed TGBs.
The basis of the solution is to maintain a per TGB config map that tracks what targets have been registered in the ELB API. When performing deregistrations, we first check the TGB config map and filter out any targets that haven't been registered in the TGB config map. We maintain an in-memory cache of this config map in order to reduce reads to the k8s API. Every reconcile cycle that changes the endpoints will also incur a write of this config map.
I've placed some warnings in the documentation about potential pitfalls of this solution, such as moving an existing TGB to multicluster support could potentially leak target deregistrations while the config map state is generated. The other warning is around flipping between multicluster / non-multicluster for the same reason.
I've load tested this feature and on a relatively big target group of 150 targets there is roughly 25 additional ms of latency during reconciles due to the write of the CM. This 25ms latency represented about a 5% latency increase in my cluster.
Tests done
Checklist
README.md
, or thedocs
directory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯