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

GRPCRoute Support #1835

Merged
merged 9 commits into from Apr 30, 2024
Merged

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Apr 17, 2024

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

--- PASS: TestConformance/GRPCExactMethodMatching (1.02s)
    --- PASS: TestConformance/GRPCExactMethodMatching/2_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/EchoThree'_should_receive_a_Unimplemented_(12) (0.00s)
    --- PASS: TestConformance/GRPCExactMethodMatching/0_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v1 (0.02s)
    --- PASS: TestConformance/GRPCExactMethodMatching/1_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/EchoTwo'_should_go_to_grpc-infra-backend-v2 (0.02s)
--- PASS: TestConformance/GRPCRouteHeaderMatching (0.01s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/6_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:blue}'_should_go_to_grpc-infra-backend-v1 (0.02s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/7_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:green}'_should_go_to_grpc-infra-backend-v1 (0.02s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/2_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:orange,Version:two}'_should_go_to_grpc-infra-backend-v1 (0.01s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/0_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Version:one}'_should_go_to_grpc-infra-backend-v1 (0.06s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/5_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Some-Other-Header:one}'_should_receive_a_Unimplemented_(12) (1.05s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/10_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:purple}'_should_receive_a_Unimplemented_(12) (1.05s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/9_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:yellow}'_should_go_to_grpc-infra-backend-v2 (1.06s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/4_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:orange}'_should_receive_a_Unimplemented_(12) (1.08s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/8_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:red}'_should_go_to_grpc-infra-backend-v2 (1.09s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/1_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Version:two}'_should_go_to_grpc-infra-backend-v2 (1.09s)
    --- PASS: TestConformance/GRPCRouteHeaderMatching/3_request_to_'/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_with_headers_'{Color:blue,Version:two}'_should_go_to_grpc-infra-backend-v2 (1.10s)
--- PASS: TestConformance/GRPCRouteListenerHostnameMatching (2.12s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/6_request_to_'foo.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_receive_a_Unimplemented_(12) (0.01s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/7_request_to_'no.matching.host/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_receive_a_Unimplemented_(12) (0.01s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/0_request_to_'bar.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v1 (0.01s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/1_request_to_'foo.bar.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v2 (0.01s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/3_request_to_'boo.bar.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v3 (0.03s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/5_request_to_'multiple.prefixes.foo.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v3 (0.03s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/2_request_to_'baz.bar.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v3 (0.04s)
    --- PASS: TestConformance/GRPCRouteListenerHostnameMatching/4_request_to_'multiple.prefixes.bar.com/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo'_should_go_to_grpc-infra-backend-v3 (0.04s)

Closes #1783 #1790 #1765 #1768 #1838 #1837

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

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.

Initial support for GRPCRoute, including Exact Method Matching, Header Matching, and Listener Hostname Matching

@ciarams87 ciarams87 requested review from a team as code owners April 17, 2024 13:22
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request dependencies Pull requests that update a dependency file helm-chart Relates to helm chart labels Apr 17, 2024
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 95.01661% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 86.71%. Comparing base (7c3da8d) to head (de21059).

Files Patch % Lines
internal/mode/static/state/graph/route_common.go 94.58% 8 Missing and 5 partials ⚠️
internal/mode/static/manager.go 46.15% 7 Missing ⚠️
internal/mode/static/state/graph/grpcroute.go 97.01% 3 Missing and 1 partial ⚠️
internal/mode/static/state/graph/httproute.go 90.90% 3 Missing and 1 partial ⚠️
internal/mode/static/status/prepare_requests.go 96.49% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pleshakov pleshakov left a 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

internal/mode/static/nginx/config/http/config.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_template.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_template.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 marked this pull request as draft April 17, 2024 16:07
@ciarams87 ciarams87 force-pushed the feat/grpc-route-support branch 2 times, most recently from aa9772c to 3f9fbf6 Compare April 24, 2024 14:52
@ciarams87 ciarams87 marked this pull request as ready for review April 24, 2024 14:53
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kate-osborn kate-osborn left a 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

conformance/Makefile Outdated Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
internal/mode/static/status/prepare_requests.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/change_processor_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/sort.go Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/grpcroute.go Show resolved Hide resolved
internal/mode/static/state/graph/route_common.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the helm-chart Relates to helm chart label Apr 26, 2024
@github-actions github-actions bot added the helm-chart Relates to helm chart label Apr 26, 2024
Copy link
Contributor

@kate-osborn kate-osborn left a 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

@sjberman
Copy link
Contributor

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.

Copy link
Contributor

@ADubhlaoich ADubhlaoich left a 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.

@ciarams87
Copy link
Member Author

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.

@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.

@sjberman
Copy link
Contributor

@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?

@ciarams87
Copy link
Member Author

@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?

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.

@ciarams87 ciarams87 merged commit 76b34af into nginxinc:main Apr 30, 2024
40 checks passed
@ciarams87 ciarams87 deleted the feat/grpc-route-support branch April 30, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

GRPCRoute Method Matching
5 participants