-
Notifications
You must be signed in to change notification settings - Fork 689
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
Gateway API: fix for failing GatewayWithAttachedRoutes conformance test #5961
Gateway API: fix for failing GatewayWithAttachedRoutes conformance test #5961
Conversation
Signed-off-by: gang.liu <gang.liu@daocloud.io>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5961 +/- ##
==========================================
- Coverage 78.69% 78.68% -0.02%
==========================================
Files 138 138
Lines 19687 19675 -12
==========================================
- Hits 15493 15481 -12
Misses 3890 3890
Partials 304 304
|
Signed-off-by: gang.liu <gang.liu@daocloud.io>
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.
just a requested change on the release note so far but from a quick look at the test changes/code changes this looks like it achieves what looks like the right outcome for the most part, though ends up being a little repetitive
With this change, we're processing Listeners/Routes multiple times now, once looping through the Listeners for finding which ones are valid, then going through each Listener/Route another time for seeing if the Routes can attach/intersect hostnames, then doing the Listener/Route analysis again for the purposes of calculating attached route counts. It would be good to coalesce some of this logic together since it seems like extra processing we could reduce.
Maybe this looks like not returning early from computeListener
and including the error of why the listener is invalid/if it should still be allowed to have attached routes in the returned listenerInfo
struct
This isn't something the upstream conformance tests are checking yet, but if this doesn't exist would be good to ensure a test exists for a Gateway Listener that has an invalid configuration in its route selection (e.g. invalid namespace selector due to a field missing or invalid route kind) but a Route with a parent ref that would be valid should still not count
Signed-off-by: gang.liu <gang.liu@daocloud.io>
Signed-off-by: gang.liu <gang.liu@daocloud.io>
ping @sunjayBhatia |
Signed-off-by: gang.liu <gang.liu@daocloud.io>
Signed-off-by: gang.liu <gang.liu@daocloud.io>
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.
Couple nits but otherwise looks pretty good to me (I'm heavily relying on the tests here for correctness!)
examples/example-workload/gatewayapi/blue-green/blue-green.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: gang.liu <gang.liu@daocloud.io>
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.
LGTM, will leave for @sunjayBhatia
Signed-off-by: Gang Liu gang.liu@daocloud.io
fix #5920