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

gRPC support for VirtualServer/VirtualServerRoute #908

Closed
SchambergSe opened this issue Apr 1, 2020 · 10 comments
Closed

gRPC support for VirtualServer/VirtualServerRoute #908

SchambergSe opened this issue Apr 1, 2020 · 10 comments
Labels
enhancement Pull requests for new features/feature enhancements proposal An issue that proposes a feature request

Comments

@SchambergSe
Copy link

SchambergSe commented Apr 1, 2020

gRPC with VirtualServer I want to configure a VirtualServer with a gRPC backend. I can't find anything about gRPC in the docs of VirtualServer and my test also failed, because the ingress controller tried to contact the backend service with http1. Therefore I assume, gRPC support is not yet implemented.

Possible solution In best case, gRPC should work without any extra configuration necessary. If necessary I would prefer an option on the Upstream configuration. Something like "grpc-service".

My test configuration ```apiVersion: apps/v1 kind: Deployment metadata: name: hello-grpc namespace: nginx-test labels: app: hello-grpc spec: replicas: 1 selector: matchLabels: app: hello-grpc template: metadata: labels: app: hello-grpc spec: containers: - name: nginx image: grpc/java-example-hostname ports: - containerPort: 50051


apiVersion: v1 kind: Service metadata: name: hello-grpc namespace: nginx-test spec: selector: app: hello-grpc ports: - protocol: TCP port: 80 name: grpc targetPort: 50051


apiVersion: k8s.nginx.org/v1 kind: VirtualServer metadata: name: asa-cloud namespace: nginx spec: host: build-asa-cloud.**.de tls: secret: **-cert redirect: enabled: true routes: - path: "=/" action: return: code: 200 type: text/plain body: "Hello from cluster" - path: /helloworld route: nginx-test/hello-grpc


apiVersion: k8s.nginx.org/v1 kind: VirtualServerRoute metadata: name: hello-grpc namespace: nginx-test spec: host: build-asa-cloud.lowellgroup.de upstreams: - name: hello-grpc service: hello-grpc port: 80 subroutes: - path: /helloworld action: pass: hello-grpc ``` I get the following log entries for a call to this gRPC service: 2020/04/01 09:28:30 [warn] 221#221: *252 a client request body is buffered to a temporary file /var/cache/nginx/client_temp/0000000007, client: 51.144.5.90, server: build-asa-cloud.lowellgroup.de, request: "POST /helloworld.Greeter/SayHello HTTP/2.0", host: "build-asa-cloud.lowellgroup.de" 2020/04/01 09:28:30 [error] 221#221: *252 upstream sent no valid HTTP/1.0 header while reading response header from upstream, client: 51.144.5.90, server: build-asa-cloud.lowellgroup.de, request: "POST /helloworld.Greeter/SayHello HTTP/2.0", upstream: " http://172.25.65.136:50051/helloworld.Greeter/SayHello", host: "build-asa-cloud.lowellgroup.de"

Aha! Link: https://nginx.aha.io/features/IC-105

@pleshakov pleshakov added the proposal An issue that proposes a feature request label Apr 1, 2020
@pleshakov
Copy link
Contributor

Hi @SchambergSe

We haven't implemented support for gRPC yet in VirtualServer/VirtualServerRoute resources. It is an important use case, and it is necessary to support it.

Thanks for the proposal. We'll keep this issue open for tracking the feature status. I will post an update once we have an ETA on this issue or more details.

As an alternative, for the time being, I can recommend using the Ingress resource with gRPC annotations and also leverage Mergeable Ingresses for cross-namespace configuration.

@CatTail
Copy link

CatTail commented Aug 28, 2020

Hi @pleshakov, do we have a plan when that can be supported. I'm working on a canary rollout for gRPC service, however, this Ingress doesn't support gRPC annotation on VirtualServer and VirtualServer is the only way to split traffic 😢

@pleshakov
Copy link
Contributor

Hi @CatTail

Unfortunately, we still don't support gRPC for VirtualServer yet and I don't have a ETA to share.

However, in 1.8.0 we added support for snippets in VirtualServer, so I can suggest the following workaround:

apiVersion: k8s.nginx.org/v1
kind: VirtualServer
metadata:
  name: grpc
  namespace: default
spec:
  http-snippets: |
    split_clients $request_id $sc_vs_default_grpc {
      50% vs_default_grpc_grpc-v1;
      50% vs_default_grpc_grpc-v2;
    }
  host: my.grpc.example.com
  tls:
    secret: grpc-secret
  upstreams:
  - name: grpc-v1 
    service: grpc-svc
    port: 50051 
  - name: grpc-v2 
    service: grpc-v2-svc
    port: 50051
  server-snippets: |
    location / {
      grpc_connect_timeout 60s;
      grpc_read_timeout 60s;
      grpc_send_timeout 60s;
      grpc_set_header Host $host;
      grpc_set_header X-Real-IP $remote_addr;
      grpc_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
      grpc_set_header X-Forwarded-Host $host;
      grpc_set_header X-Forwarded-Port $server_port;
      grpc_set_header X-Forwarded-Proto $scheme;

      grpc_pass grpc://$sc_vs_default_grpc;
    }

    error_page 400 @grpcerror400;
    error_page 401 @grpcerror401;
    error_page 403 @grpcerror403;
    error_page 404 @grpcerror404;
    error_page 405 @grpcerror405;
    error_page 408 @grpcerror408;
    error_page 414 @grpcerror414;
    error_page 426 @grpcerror426;
    error_page 500 @grpcerror500;
    error_page 501 @grpcerror501;
    error_page 502 @grpcerror502;
    error_page 503 @grpcerror503;
    error_page 504 @grpcerror504;


    location @grpcerror400 { default_type application/grpc; return 400 "\n"; }
    location @grpcerror401 { default_type application/grpc; return 401 "\n"; }
    location @grpcerror403 { default_type application/grpc; return 403 "\n"; }
    location @grpcerror404 { default_type application/grpc; return 404 "\n"; }
    location @grpcerror405 { default_type application/grpc; return 405 "\n"; }
    location @grpcerror408 { default_type application/grpc; return 408 "\n"; }
    location @grpcerror414 { default_type application/grpc; return 414 "\n"; }
    location @grpcerror426 { default_type application/grpc; return 426 "\n"; }
    location @grpcerror500 { default_type application/grpc; return 500 "\n"; }
    location @grpcerror501 { default_type application/grpc; return 501 "\n"; }
    location @grpcerror502 { default_type application/grpc; return 502 "\n"; }
    location @grpcerror503 { default_type application/grpc; return 503 "\n"; }
    location @grpcerror504 { default_type application/grpc; return 504 "\n"; }

Notes:

  • make sure that the variable you define in the http-snippets (here $sc_vs_default_grpc) is unique among all VirtualServers. Here I used the name of the VS resource grpc and its namespace default as a convention to guarantee uniqueness.
  • vs_default_grpc_grpc-v1 and vs_default_grpc_grpc-v2 are the names of the upstreams that the IC generates for this VirtualServer. Here default is the namespace of VS, grpc - name of the VS and grpc-v* - name of the upstream.
  • Make sure the snippets feature is enabled, as it is disabled by default. See https://docs.nginx.com/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/#using-snippets

More about split clients -- http://nginx.org/en/docs/http/ngx_http_split_clients_module.html
More about gRPC load balancing with NGINX -- https://www.nginx.com/blog/nginx-1-13-10-grpc/

Hope this helps

@CatTail
Copy link

CatTail commented Aug 29, 2020

@pleshakov thank you for your detailed explanation, I have tried your code snippet and it works like a charm, it helps a lot.

@CatTail
Copy link

CatTail commented Sep 9, 2020

Hi @pleshakov, I'd like to contribute to this open source project by adding gRPC support for VirsualServer CRD, do you know if there is anything I need to be aware of, like is there any pending work already there, is it ok to add an annotation for VirtualServer (like what we do for Ingress), or should I add new fields into VirtualServer spec section.

@pleshakov
Copy link
Contributor

Hi @CatTail

I'd like to contribute to this open source project by adding gRPC support for VirsualServer CRD,

That's great! Thank you

I think it makes sense to configure the gRPC protocol as part of the upstream:

apiVersion: k8s.nginx.org/v1
kind: VirtualServer
metadata:
  name: grpc-example
spec:
  host: grpc.example.com
  tls:
    secret: grpc-secret
    redirect:
      enable: true
  upstreams:
  - name: grpc-app 
    service: grpc-svc
    port: 50051 
    grpc: true # enables gRPC for the upstream
  routes:
  - path: /
    action:
      pass: grpc-app

if there is anything I need to be aware of, like is there any pending work already there, is it ok to add an annotation for VirtualServer (like what we do for Ingress), or should I add new fields into VirtualServer spec section.

Let me spend a bit more time thinking about requirements and any potential problems with this feature. We haven't done any thorough research yet.

I wonder what requirements make sense to you?

Btw, here is an example of a recent feature where we added a few fields in VirtualServer and VirtualServerRoute resouces -- 9bc383c Although, adding a boolean field to an upstream should be simpler. Hope the examples gives some idea on what it takes to add a field.

@CatTail
Copy link

CatTail commented Sep 10, 2020

Hi @pleshakov thanks for the suggestions.

I wonder what requirements make sense to you?

Basically I'm working on a project that allows canary rollout gRPC traffic, which means I need to split traffic for gRPC request.
I'm not an expert in nginx configuration, but from what I see maybe we should add grpc boolean field in route instead of upstream because the original nginx config configures grpc with grpc_pass inside location instead of upstream.

Anyway, I create a PR in #1135 to add gRPC support for VirtualServer, we can move our discussion to there. BTW I'm not an expert in nginx/kubernetes/go, so feel free to point out my silly mistakes in that PR 😄.

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Mar 9, 2021
@ogarrett ogarrett removed the stale Pull requests/issues with no activity label Mar 11, 2021
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label May 11, 2021
@pleshakov pleshakov removed the stale Pull requests/issues with no activity label May 11, 2021
@brianehlert brianehlert added the enhancement Pull requests for new features/feature enhancements label Jul 2, 2021
@lucacome
Copy link
Member

Added support in #2110

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements proposal An issue that proposes a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants