-
Notifications
You must be signed in to change notification settings - Fork 594
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
fix(parser): add HTTPRoute combined services routes #3008
Conversation
1bf6813
to
277538c
Compare
277538c
to
5e861c4
Compare
Add support for combined service routes when parsing gw api HTTPRoutes
5e861c4
to
80cfa49
Compare
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com> Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
894f5d3
to
4a955dd
Compare
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.
Well, this response ended up being much longer than I wanted. The short version:
- Multiple rules that use the same Service and Filters should result in a single route with multiple paths.
- We should have combined test cases for multiple multi-Service backends, some which are equal and some which are not (rule 1:serviceA,serviceB, rule 2: serviceA,serviceB, rule 3: serviceA, serviceC).
- We should have tests for backends with Namespace set (to something other than the route namespace) and multi-Service backends where one or more entries has Namespace set. This is less because I'd expect it to break and more to just have a clear before/after example--working forwards through the code to determine the output is complicated for HTTPRoute.
Having the separate combined test cases makes it clearer why I was confused earlier--the comment about having separate test cases was less to simply make them separate tests, but rather because the two variants should not produce compatible outputs. If we were getting the same outputs for both, that should point to an issue with the combined translation not doing what it should, but I wasn't sure what.
These two rules which share a service, should not produce these separate routes, but rather a single route with both /httpbin-1
and /httpbin-2
paths.
Unlike Ingress (where modification annotations apply to all rules), HTTPRoute Filters can apply modifications per rule. These should all require plugins, and thus require different routes, so if the rules have different Filter sets, we won't be able to combine them.
For comparison, a similar Ingress test rule set results in a single route with many paths, whose name indicates the Service those paths target. Ingress consolidation also has to check for equal hostnames, but that's part of the spec, not rules, in HTTPRoute and we don't need to worry about it for HTTPRoute consolidation.
Combining those should inherently de-duplicate Services since there's not more than one route using a given Service.
I forget why we were even generating those separately per HTTPRoute rule to begin with--I guess we had to name them per rule for multi-Service backends and then used that logic for all HTTPRoute Services, rather than the traditional namespace.name.port
convention.
I thought we would be able to use the traditional for single-service backends when consolidated, since multi-service backends would be de-duplicated to a single instance. However:
- this is inconsistent with the Ingress equivalent, which uses Ingress info in the service name. I forget why we needed to do this.
- my
namespace.name.multi-0.xx
attempt at stuffing multi-Service into that format doesn't work well if the Services are in different namespaces.
Given that it's the status quo and the ambiguity around Services in other namespaces, continuing to use the first rule number the Service set appears seems fine for that--there's unfortunately just not really a good way to put the Service name in consistently.
@rainest It's not that simple.
If we consolidate the rules to that degree, we need to take into consideration the |
e5efb4a
to
2ae2e1d
Compare
What this PR does / why we need it:
Add support for combined service routes when parsing gw api HTTPRoutes. Rules from HTTPRoute are grouped under matching set of backend references.
Which issue this PR fixes:
Part of #2881
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR