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: allow GRPCRoute without hostnames and matches #5303

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Dec 6, 2023

What this PR does / why we need it:

For GRPCRoute in GWAPI specs you can read

  • If no matches are specified, the default is to match every gRPC request.
    src

  • If no hostname is specified, traffic is routed based on GRPCRoute rules and filters (optional).
    src

The above and the fact that both fields are optional in CRD leads to the conclusion that configuration without those fields shouldn't be rejected.

In Kong documentation, you can read that for Kong Route, the required fields are

For grpc, at least one of hosts, headers or paths;
src

and to catch-all set paths to / (src).

Order of evaluation described in the docs says that routes that have a regular expression path that matches are used to route traffic first, later prefix paths (read more in the issue Kong/kong#4121).

Thus for the traditional one Kong Route's field path is set to prefix match / and protocol to grpc, grpcs (see docs). For expressions (net.protocol == "http") || (net.protocol == "https") is configured to catch every service name and method name combination (according to gRPC spec they are encoded as URL, see). AFAIK for a catch-all for standard HTTP explicit prefix path / is always configured. It shouldn't collide due to the mechanism of assigning priorities by KIC, should it?

Which issue this PR fixes:

I found related old issues/PRs that touched this code (mentioning them for reference)

This comment mentioned not supporting the behavior introduced in this PR on purpose to be consistent with HTTPRoute - #4512 (comment).
It's not true, from a user perspective. HTTPRoute without Hostnames and Rules specified in YAML works as expected (it configures Kong properly) because of defaults in GWAPI spec

	// Rules are a list of HTTP matchers, filters and actions.
	//
	// +optional
	// +kubebuilder:validation:MaxItems=16
	// +kubebuilder:default={{matches: {{path: {type: "PathPrefix", value: "/"}}}}}
	Rules []HTTPRouteRule `json:"rules,omitempty"`

src

so KIC never gets an empty rule and configures everything properly.

Special notes for your reviewer:

A separate integration test hasn't been introduced, since it would be more testing behavior of Kong Gateway and its routing priorities than KIC behavior - a unit test should be enough.

Fixed misplaced an item in CHANGELOG.md too.

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

@programmer04 programmer04 added area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API fix labels Dec 6, 2023
@programmer04 programmer04 added this to the KIC v3.1.x milestone Dec 6, 2023
@programmer04 programmer04 self-assigned this Dec 6, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b876880) 75.9% compared to head (eda2916) 75.8%.

Files Patch % Lines
...ternal/dataplane/translator/translate_grpcroute.go 50.0% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5303     +/-   ##
=======================================
- Coverage   75.9%   75.8%   -0.1%     
=======================================
  Files        170     170             
  Lines      19458   19447     -11     
=======================================
- Hits       14771   14759     -12     
- Misses      3857    3858      +1     
  Partials     830     830             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@programmer04 programmer04 marked this pull request as ready for review December 6, 2023 19:06
@programmer04 programmer04 requested a review from a team as a code owner December 6, 2023 19:06
@programmer04 programmer04 enabled auto-merge (squash) December 6, 2023 19:08
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.

Does a simple / path work? https://docs.konghq.com/gateway/latest/production/configuring-a-grpc-service/#single-grpc-service-and-route indicates it should, and that should preclude any possibility of it accidentally consuming anything else.

I don't think we want to use a regex here. I don't know exactly how this plays out in the gRPC routing engine (ask the gateway team for specifics), but in the HTTP engine at least, regexes are an exception go the specificity rule: all regex paths are treated equally and only evaluated according to their numerical priority if non-path fields match.

The existing traditional code probably has flaws as such (it uses regexes and kinda just hopes you don't choose something that overlaps). Only ATC has any priority setting, so we maybe declined to fully implement the GRPC spec for traditional because we're more limited in that engine.

Directly adding a regex path to a GRPC route also feels a bit odd due to the way we normally construct them rigidly via getGRPCMatchDefaults()andfmt.Sprintf("/%s/%s", service, method). I don't think it should matter (the second slash to separate the method can be consumed by the .*` still), but it feels like we should make the catch-all consistent with the general pattern.

@programmer04 programmer04 marked this pull request as draft December 7, 2023 10:14
auto-merge was automatically disabled December 7, 2023 10:14

Pull request was converted to draft

@programmer04
Copy link
Member Author

@rainest you're right I updated the description and used / as paths in catch-all. I think trying to reuse getGRPCMatchDefaults() for this will be less clear and harder to follow than just inlining / in this case. PTAL

@programmer04 programmer04 marked this pull request as ready for review December 7, 2023 14:01
@programmer04 programmer04 enabled auto-merge (squash) December 7, 2023 14:21
@programmer04 programmer04 merged commit a4f82f6 into main Dec 7, 2023
38 checks passed
@programmer04 programmer04 deleted the grpc-no branch December 7, 2023 17:39
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 fix size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants