-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[feat aga] Implement AGA endpoint resource references loading and monitoring #4458
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
[feat aga] Implement AGA endpoint resource references loading and monitoring #4458
Conversation
54f87b6 to
9e99f31
Compare
89591fa to
f2d0ac0
Compare
f2d0ac0 to
1287b0e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shraddhabang 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 &DNSToLoadBalancerResolver{ | ||
| elbv2Client: elbv2Client, | ||
| cache: cache, | ||
| ttl: 5 * time.Minute, // Default TTL of 5 minutes |
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.
IMO, this is too short. DNS names / ARNs do not change that frequently. What do you think about increasing the cache time?
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 kept it small because I wanted to remove the older dns from the cache and keep the cache upto date. Having large TTL will keep the older dns and invalid (Deleted) ones for longer time. If we encounter any older dns because the our k8s were not updated for some reason, we will resolve that to invalid arn. Hence I kept it for 5 minutes so that we get faster failure feedback. What do you think?
| return "", fmt.Errorf("object is not a Service") | ||
| } | ||
|
|
||
| if svc.Spec.Type != corev1.ServiceTypeLoadBalancer { |
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 believe NodePort types are supported too.
https://kubernetes-sigs.github.io/aws-load-balancer-controller/latest/guide/service/nlb/
| "fatal", fatal) | ||
|
|
||
| // Log individual endpoints | ||
| for i, endpoint := range endpoints { |
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.
nit: this will potentially construct a lot of strings. i would be mindful of leaving this in here.
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.
Yes. I am aware of that. But I think this is crucial to log this info for troubleshooting since the endpoints work dynamically. And we need to know which endpoints we filtered out so that we can debug this better. We are not expecting many endpoints per accelerator so I think verbose level 1 logs are okay for this.
| } | ||
| t.resourceMap[resourceKey].Insert(gaKey.String()) | ||
|
|
||
| t.logger.V(1).Info("Resource referenced by GA", |
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.
same comment about a potential lot of strings of constructed
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.
Same reasoning as above.
1287b0e to
4b91e08
Compare
|
/lgtm |
This PR implements two key components for the AWS Global Accelerator controller:
Commit 1: [feat aga] Implement endpoint loader with DNS resolution
This commit implements the endpoint loading system for the AGA controller. It provides:
The endpoint loader enables GlobalAccelerator resources to reference Kubernetes objects and automatically resolve them to the appropriate AWS resources.
Commit 2: [feat aga] Implement resource monitoring for referenced resources
This commit implements the resource monitoring system for the AGA controller. It provides:
This monitoring system ensures that when a referenced resource changes (e.g., a Service gets a new load balancer), the GlobalAccelerator is automatically reconciled to use the updated endpoint.
Note for temporary limitations for cross namespace reference
We want to allow references to Kubernetes resources (Services, Ingresses, Gateways) that exist in different namespaces from the GlobalAccelerator CR itself. This enables more flexible architectural patterns but requires careful security considerations. We will implement this later as we will need to come up with a proper cross-namespace reference system keeping security concerns in mind. For now in the current implementation cross-namespace references are detected but only result in a warning - this means:
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯