-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[feat aga] Add AGA listener support without auto-discovery #4436
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] Add AGA listener support without auto-discovery #4436
Conversation
ec5f770 to
b8e3336
Compare
b8e3336 to
1b71350
Compare
1b71350 to
315a334
Compare
| // isSDKListenerSettingsDrifted checks if the listener configuration has drifted from the desired state | ||
| func (m *defaultListenerManager) isSDKListenerSettingsDrifted(resListener *agamodel.Listener, sdkListener *ListenerResource) bool { | ||
| // Check if protocol differs | ||
| if string(resListener.Spec.Protocol) != string(sdkListener.Listener.Protocol) { |
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.
Is there any implicit defaults that we need to worry about here? E.g. if the protocol is missing then it's assumed to be TCP. If that's the case, we will always say that the listener setting has drifted. We run into this problem with quite a few ELB features.
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.
Client Affinity defaults to NONE: https://docs.aws.amazon.com/global-accelerator/latest/api/API_CreateListener.html
The ports and protocols must be specified.
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.
Thanks. I was just confirming this. We will always set Protocols and Ports from our model build. And Client Affinity is None by default in our case too. So we wont see the false drift problems.
| } | ||
|
|
||
| // findSimilarityMatches matches remaining listeners based on similarity score | ||
| func (s *listenerSynthesizer) findSimilarityMatches(resListeners []*agamodel.Listener, sdkListeners []*ListenerResource) ( |
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.
interesting approach, this would be useful for updating ELB listener rules as well. Is this similarity score based off of a well-known algorithm?
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 idea came from hamming distance algo and Jaccard Index which are standard similarity algorithms. I had this algorithm idea when I was working with rules management for ELB. But it seemed complex for it since our rules are much more complex. I was worried about performance at that time. So never actually implemented it.
Since the AGA listeners were much more easier structures, it was easier to implement the algo on them and I did not want to repeat our rules problem again. But I think we can definitely give it a try now for our rules management as well.
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.
Thanks for the thorough explanation. I find it helpful to detail the algorithm in the code itself. That way developers don't need to come in find these PR comments when working on this piece of code.
8d5d2b5 to
e5bc6be
Compare
e5bc6be to
f5e1cda
Compare
f5e1cda to
012790e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shraddhabang, zac-nixon 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 |
b65911e
into
kubernetes-sigs:AGAController
Description
This PR introduces the AGA listener lifecycle management in AGA controller. It has two commits as follows.
1. [feat aga] Add AGA listener builder without auto-discovery
This commit introduces the model builder changes for AGA Listener Resources & Port Range Validation
Model Builder for Listener Resources: Updated the listener model builder to directly use values specified in the CRD spec. We plan to set these values based on endpoints with auto-discovery if not specified. We will implement that later. This is simple implementation which directly builds object from CRD specs.
Port Range Validation Webhook: Implemented validation logic that prevents overlapping port ranges between listeners with the same protocol if specified in CRD. We will extend the same validation during auto-discovery.
2. [feat aga] Add AGA listener deployer with clean up
This PR implements the complete AGA Listener deployer, which reconciles listener resources defined in Kubernetes with the AGA service with Intelligent Matching Algorithm.
Key Features
Two-phase Listener Matching Algorithm: Implements a sophisticated matching algorithm that:
Optimized Operation Order: The deployer performs operations in an order that minimizes service disruption:
Comprehensive Test Suite: Includes extensive test cases that verify:
Cleanup Logic
The current implementation includes a simple cleanup mechanism that safely removes listeners while cleaning up the accelerators. A more sophisticated cleanup strategy that considers endpoint groups and other dependent resources will be implemented separately once all Global Accelerator resource types are supported. Future PRs will enhance the cleanup logic with more sophisticated resource dependency tracking once all GA resource types are supported.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯