-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add request header filter support for gRPC #1909
Add request header filter support for gRPC #1909
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1909 +/- ##
==========================================
+ Coverage 86.83% 86.90% +0.06%
==========================================
Files 88 88
Lines 6025 6056 +31
Branches 50 50
==========================================
+ Hits 5232 5263 +31
Misses 741 741
Partials 52 52 ☔ View full report in Codecov by Sentry. |
3e47a3e
to
7116b06
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.
it's great that we don't need to change anything in internal/mode/static/nginx/config
package
211e59a
to
60bead2
Compare
60bead2
to
a414dcb
Compare
a414dcb
to
1f10820
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.
LGTM docs-wise but this PR is primarily code based so I will not give an approval.
50d1102
to
75f1e06
Compare
75f1e06
to
ebb3ecf
Compare
ebb3ecf
to
06416d8
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.
are we adding or updating any examples to test this?
LGTM otherwise!
a339eb5
to
44f565a
Compare
@salonichf5 No, configuring this filter is identical to how it's done for HTTPRoute, so adding a further example wouldn't add much value. Additionally, we don't have an easy way to create an echo server like we do for HTTP (in my testing I am using the echo server from the conformance test suite), so there would be additional overhead in creating and maintaining one for use in examples where we want to see the headers. |
Proposed changes
Problem: As a user of NGF with applications that require GRPC traffic
I want NGF to support a GRPCRouteRule that can handle a request HTTPHeaderFilter
So that I can append specific headers required by my applications.
Solution: Add support for RequestHeaderFilters for GRPCRoutes. This mostly reuses the logic that already exists for HTTPRoutes by converting the GRPCFilter to HTTPFilter, and adapting the existing validation. This makes most of the request header validation shared, so it has been moved to the
route_common.go
(as well as the corresponding unit tests).Also added base gRPC headers -
Host
(which maps to the:authority
pseudo-header under the hood),Authority
(which needs to be the same value asHost
) andX-Forwarded-For
which works the same as HTTP and preserves the client IP.Testing: As well as unit test coverage, local testing using the conformance test echo server:
Config:
Request and response:
Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.
Closes #1766
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.