-
Notifications
You must be signed in to change notification settings - Fork 1.6k
handle cross zone disabled for alb #4496
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shuqz 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:
Approvers can indicate their approval by writing |
| return svc.Annotations, nil | ||
| } | ||
|
|
||
| func (m *defaultResourceManager) isALBIngress(svcAnnotations map[string]string) bool { |
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.
This might not always work. Customers aren't required to add ingress annotations to their Services.
It's more reliable to use the TG protocol: https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/apis/elbv2/v1beta1/targetgroupbinding_types.go#L143-L146
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.
I realized that this only works for the Ingress API. We will want a solution that works ALB Gateways too.
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.
i thought about TG protocol too, but did not realize they are not required to add annotation.
yea i forgot gateway api integrations, i will include it
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.
If the protocol isn't added by user, then we infer and inject it.
https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/webhooks/elbv2/targetgroupbinding_mutator.go#L82-L84
| return false | ||
| } | ||
|
|
||
| func (m *defaultResourceManager) isCrossZoneDisabled(svcAnnotations map[string]string, isALB bool) bool { |
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.
For Ingress objects, this could go on the Ingress or Service, meaning that this could be incorrect
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.
See my other comment, this will never work for ALB Gateways.
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 other more striaghtforward way is to use describe api get cross-zone value.
| if !ok { | ||
| az, ok = node.Labels["failure-domain.beta.kubernetes.io/zone"] | ||
| } | ||
| if !ok { |
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.
This will block reconciles on non-EKS clusters. I suspect that's not what we want.
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.
i can remove this fallback part
| return false | ||
| } | ||
|
|
||
| func (m *defaultResourceManager) getPodAvailabilityZone(ctx context.Context, pod k8s.PodInfo) (string, error) { |
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 think about using a node-az cache? That way we don't have to recalculate the node AZ each reconcile loop iteration.
| var apiErr smithy.APIError | ||
| if errors.As(err, &apiErr) { | ||
| isMatch := apiErr.ErrorCode() == "ValidationError" && | ||
| strings.Contains(apiErr.ErrorMessage(), "you must specify an Availability Zone") |
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.
i am not sure if they will change error message but checking ValidationError alone might not enough
Issue
#4477
Description
For ALB, if cross_zone=disabled and AZ is either not specified or all, we get error from api
it only succeed when az is specified. this does not apply to NLB
Fix implemented:
Checklist
README.md, or thedocsdirectory)created peered VPC, trying to register pod from VPC 1 to target group in VPC2. Before fix, no target is registered. After fix, target is registered. tested with yaml (
load_balancing.cross_zone.enabled=false)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯