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

Gateway API: add GRPCRoute support #5114

Merged
merged 8 commits into from
Feb 27, 2023
Merged

Gateway API: add GRPCRoute support #5114

merged 8 commits into from
Feb 27, 2023

Conversation

fangfpeng
Copy link
Contributor

@fangfpeng fangfpeng commented Feb 21, 2023

Closes #4820.

fangfpeng and others added 5 commits January 26, 2023 18:28
Add kind and cache support for GRPCRoute

Updates #4820

Signed-off-by: Fang Peng <[email protected]>
add registration controller and reconcile support for GRPCRoute;
update handler and serve to call the registration.

Updates #4820

Signed-off-by: Yu Ying <[email protected]>
Implement gatewayapi_processor.go for building GRPCRoute dag.

Updates #4820

Signed-off-by: Fang Peng <[email protected]>
* GRPCRotue: add ut test cases

for 1.TestGetListenersForRotueParentRef
    2.TestDAGInsertGatewayAPI

Updates #4820

Signed-off-by: Yu Ying <[email protected]>

* GRPCRoute: set default protocol type for mirrored service

Updates #4820

Signed-off-by: Yu Ying <[email protected]>

* GRPCRoute: remove unnecessary type conversion and comment

Updates #4820

Signed-off-by: Yu Ying <[email protected]>

---------

Signed-off-by: Yu Ying <[email protected]>
@fangfpeng fangfpeng changed the title Dev grpcroute Gateway API: add GRPCRoute support Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #5114 (ba1495d) into main (157887a) will increase coverage by 0.03%.
The diff coverage is 80.20%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5114      +/-   ##
==========================================
+ Coverage   77.97%   78.01%   +0.03%     
==========================================
  Files         137      138       +1     
  Lines       17237    17604     +367     
==========================================
+ Hits        13441    13733     +292     
- Misses       3536     3600      +64     
- Partials      260      271      +11     
Impacted Files Coverage Δ
cmd/contour/serve.go 22.20% <0.00%> (-0.10%) ⬇️
internal/status/routeconditions.go 51.04% <0.00%> (-6.61%) ⬇️
internal/controller/grpcroute.go 25.80% <25.80%> (ø)
internal/dag/policy.go 94.76% <80.00%> (+0.01%) ⬆️
internal/dag/gatewayapi_processor.go 93.98% <87.03%> (-1.84%) ⬇️
internal/contour/handler.go 90.00% <100.00%> (+0.07%) ⬆️
internal/dag/cache.go 94.36% <100.00%> (+0.10%) ⬆️
internal/gatewayapi/helpers.go 87.94% <100.00%> (+2.98%) ⬆️
internal/k8s/kind.go 60.78% <100.00%> (+1.60%) ⬆️
...ovisioner/objects/rbac/clusterrole/cluster_role.go 78.94% <100.00%> (ø)
... and 1 more

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good, had a few small comment

internal/dag/gatewayapi_processor.go Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
@skriss skriss added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Feb 21, 2023
@skriss
Copy link
Member

skriss commented Feb 22, 2023

FYI, with this path match fix in place, I was able to get gRPC traffic routing!

I used the yages example service from https://projectcontour.io/docs/v1.24.1/guides/grpc/ as the backend.

Here's the GRPCRoute I used (note it includes a match for the gRPC reflection service, which is needed by grpcurl):

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: GRPCRoute
metadata:
  name: yages
spec:
  parentRefs:
  - namespace: projectcontour
    name: contour
  hostnames:
  - my-grpc-service.foo.com 
  rules:
  - matches:
    - method:
        service: yages.Echo
        method: Ping
    - method:
        service: grpc.reflection.v1alpha.ServerReflection
        method: ServerReflectionInfo
    backendRefs:
    - name: grpc-echo
      port: 9000

My method for getting this up and running was roughly:

  • make setup-kind-cluster
  • make load-contour-image-kind
  • update examples/render/contour-gateway.yaml to use imagePullPolicy: IfNotPresent instead of imagePullPolicy: Always
  • kubectl apply -f examples/render/contour-gateway.yaml
  • Deploy the yages example server linked above
  • Create the GPRCRoute from above
  • Use grpcurl to test per https://projectcontour.io/docs/v1.24.1/guides/grpc/

@fangfpeng
Copy link
Contributor Author

FYI, with this path match fix in place, I was able to get gRPC traffic routing!

I used the yages example service from https://projectcontour.io/docs/v1.24.1/guides/grpc/ as the backend.

Here's the GRPCRoute I used (note it includes a match for the gRPC reflection service, which is needed by grpcurl):

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: GRPCRoute
metadata:
  name: yages
spec:
  parentRefs:
  - namespace: projectcontour
    name: contour
  hostnames:
  - my-grpc-service.foo.com 
  rules:
  - matches:
    - method:
        service: yages.Echo
        method: Ping
    - method:
        service: grpc.reflection.v1alpha.ServerReflection
        method: ServerReflectionInfo
    backendRefs:
    - name: grpc-echo
      port: 9000

This is great news! I will make the suggested path prefix fix.

- need to add a leading slash in the path
- need to add a prefix match if method match is not
specified.
- internal-dag: Add default prefix match with 0 matches

Updates #4820

Signed-off-by: Fang Peng <[email protected]>
@skriss
Copy link
Member

skriss commented Feb 22, 2023

@fangfpeng @vmw-yingy this is looking pretty good! There are still a few (minor) outstanding review comments above, and this PR will need a changelog file as well.

@skriss skriss marked this pull request as ready for review February 23, 2023 17:07
@skriss skriss requested a review from a team as a code owner February 23, 2023 17:07
@skriss skriss requested review from tsaarni, stevesloka and sunjayBhatia and removed request for a team February 23, 2023 17:07
Also made some changes based on review comment
on gatewayapi_processor.go.

Signed-off-by: Fang Peng <[email protected]>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Will leave for @sunjayBhatia to take another look at as well. Thanks @fangfpeng and @vmw-yingy for all your work on this.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM great work!

I'm thinking we should add some e2e tests while upstream Gateway API conformance is missing them, but that can definitely be done in a follow-up!

@skriss skriss merged commit cc1cb8c into main Feb 27, 2023
@sunjayBhatia sunjayBhatia deleted the dev-grpcroute branch February 27, 2023 23:06
vmw-yingy added a commit to vmw-yingy/contour that referenced this pull request Feb 28, 2023
Adds support for Gateway API's GRPCRoute,
an experimental v1alpha2 resource as of Gateway
API v0.6. Core conformance features are currently
implemented.

Closes projectcontour#4820.

Co-authored-by: Yu Ying <[email protected]>
Signed-off-by: Fang Peng <[email protected]>
Signed-off-by: Yu Ying <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway API: Implement GRPCRoute
4 participants