Skip to content
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

Merged
merged 11 commits into from
Oct 14, 2022
Merged

Conversation

jrsmroz
Copy link
Contributor

@jrsmroz jrsmroz commented Sep 30, 2022

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:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@jrsmroz jrsmroz temporarily deployed to Configure ci September 30, 2022 22:49 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci September 30, 2022 22:51 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci September 30, 2022 23:07 Inactive
@jrsmroz jrsmroz changed the title fix(parser) add HTTPRoute combined services routes fix(parser): add HTTPRoute combined services routes Sep 30, 2022
Add support for combined service routes when parsing gw api HTTPRoutes
@jrsmroz jrsmroz marked this pull request as ready for review September 30, 2022 23:07
@jrsmroz jrsmroz requested a review from a team as a code owner September 30, 2022 23:07
@jrsmroz jrsmroz temporarily deployed to Configure ci September 30, 2022 23:07 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci September 30, 2022 23:31 Inactive
@pmalek pmalek added the area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API label Oct 4, 2022
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute_test.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_httproute_test.go Outdated Show resolved Hide resolved
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 15:12 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 15:22 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 15:45 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 15:45 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 16:07 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 16:07 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 16:35 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 12, 2022 16:35 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 13, 2022 18:15 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 13, 2022 18:39 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 13, 2022 18:39 Inactive
Copy link
Contributor

@rainest rainest left a 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:

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.

@jrsmroz
Copy link
Contributor Author

jrsmroz commented Oct 14, 2022

@rainest It's not that simple.

Multiple rules that use the same Service and Filters should result in a single route with multiple paths.

If we consolidate the rules to that degree, we need to take into consideration the method and headers hidden in the HTTPRouteMatch too. They may differ between HTTPRouteRule instances.

@jrsmroz jrsmroz temporarily deployed to Configure ci October 14, 2022 17:43 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 14, 2022 18:16 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 14, 2022 18:16 Inactive
@rainest rainest merged commit 7241cd3 into main Oct 14, 2022
@rainest rainest deleted the jrsmroz-issue-2881-v2 branch October 14, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants