add support for targetgroupbinding cross-account AZ aware registration - #4877
add support for targetgroupbinding cross-account AZ aware registration#4877cqi1217 wants to merge 2 commits into
Conversation
|
|
|
Welcome @cqi1217! |
|
Hi @cqi1217. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cqi1217 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
hi @wweiwei-li @shraddhabang can you review the code? thank you |
Issue
#4831
Description
Cross-account TargetGroupBindings always register IP targets with
AvailabilityZone: "all".Because every zonal load balancer node can then forward to every target, disabling NLB
cross-zone load balancing has no effect and inter-AZ data transfer is unavoidable for
anyone using the hub/spoke pattern (NLB + target groups in a central account, workloads
in spoke accounts).
The
"all"value is hardcoded for cross-account inprepareRegistrationCall, which gatedthe pod-AZ path on
usePodAZ && !usingCrossAccount.This PR adds an opt-in
spec.registerTargetsWithPodAvailabilityZonefield that registerseach target with the AZ of the node its pod runs on.
Why the AZ name can't be used directly
AZ names are randomized per account —
us-west-2ain the cluster's account is usually adifferent physical zone than
us-west-2ain the target group owner's account. AZ IDs(e.g.
usw2-az1) are stable across accounts, so the newAZIDTranslatorresolves:pod's zone name --(CO credentials)--> zone ID --(assumed role)--> TGO zone name
Key changes
pkg/networking/az_id_translator.go(new) —AZIDTranslator.TranslateAZName()performs the two-hop lookup above. Results are cached for 60 minutes to avoid throttling
DescribeAvailabilityZoneson every reconcile. The cache key is{scope, lookup}wherescopeis the assumed role ARN (empty for the cluster's ownaccount), so zone names from different accounts can never collide — an important detail
given that the same name means different things per account.
Assumed-role EC2 client — the translator's second lookup has to run with TGO
credentials, so
EC2gainedAssumeRole()anddefaultCloudgainedGetAssumedRoleEC2()plus an
assumeRoleEc2Cache, mirroring the existingGetAssumedRoleELBV2()path. The STSassume-role + config construction shared by both is factored out into
generateAssumedRoleConfig(). Allec2Clientmethods now route through agetClient()helper so a statically-provided assumed-role client is honored.
pkg/targetgroupbinding/resource_manager.go— the gate becomes(usePodAZ && !usingCrossAccount) || usePodAZForCrossAccount. The newresolveTargetAvailabilityZone()looks up the pod AZ and translates it when cross-account.Webhook validation — rejects the field without
iamRoleArnToAssume.API/CRD — field added to both
v1beta1andv1alpha1. Since the CRD has noconversion webhook (
strategy: None), a field present in only one served version would bepruned when the object is read or written through the other, so both need it. This matches
how the existing
iamRoleArnToAssume/assumeRoleExternalIdfields are defined.Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯