-
Notifications
You must be signed in to change notification settings - Fork 113
GRPCRoute Support #1835
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
GRPCRoute Support #1835
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1835 +/- ##
==========================================
+ Coverage 86.41% 86.71% +0.29%
==========================================
Files 86 88 +2
Lines 5712 5967 +255
Branches 50 50
==========================================
+ Hits 4936 5174 +238
- Misses 727 741 +14
- Partials 49 52 +3 ☔ View full report in Codecov by Sentry. |
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.
overall, the approach looks good to me
please note: this is high level review that was originally done for the DRAFT PR
aa9772c
to
3f9fbf6
Compare
3f9fbf6
to
bca2e0e
Compare
bca2e0e
to
d7bcd4b
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.
Great work @ciarams87!
Just a few small comments
fbf535b
to
073c7f5
Compare
073c7f5
to
e989183
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.
one small nit I missed in the first review, otherwise LGTM
Sort of going back on my original comment, it's probably worth having an example grpcroute and app in the examples folder, just no need for the README guide because we can write a doc instead. |
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 though docs changes are minimal, so I will not approve.
@sjberman The issue is the app though - for the other examples, we're using a sample NGINX so it's pretty straightforward, but for a gRPC app, we'll need to provide an image. There is a basic hello world test image here that we could use, but I'm not sure we should. The image I've been using for testing isn't ours, it's the test image from the conformance suite, and we should not use that in user facing examples. Without a working sample app, we're not offering anything over what's already available. |
1941f61
to
ce85035
Compare
@ciarams87 I just want devs to be able to easily test things out. Maybe we include the grpcroute example configs, and then the README in there can link to the test app, without us directly providing it? |
ce85035
to
de21059
Compare
That makes sense. I've created a new PR for the example though, just to allow feedback there with requiring a full re-review of this one from Kate and Michael. |
* GRPCRoute Support
Proposed changes
Problem: As a user of NGF with applications that require GRPC traffic
I want NGF to support GRPCRoute Exact Method Matching, Header Matching, and Listener Hostname Matching
So that I have a method to ingress GRPC traffic to my applications.
Solution: Add GRPCRoute support, reusing logic from HTTPRoute where possible and where it makes sense. The initial ticket was for only Hostname and Backend Ref matching, but it made sense to add the other support as so much of the validation and logic can be reused from HTTPRoute, instead of adding logic to reject those routes now.
Note: The only solution I have for turning on http2 is to hardcode it into the base NGINX conf files. I don't think there is a way to make this configurable. Do we foresee any potential issues with this? UPDATE: This will be addressed in a future PR by offering an option to disable http2
Testing: Ran conformance tests with GRPCRoutes locally (GRPCRoute conformance tests are in main branch only) , unit testing, manual testing
Closes #1783 #1790 #1765 #1768 #1838 #1837
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.