add gRPC support for VirtualServer #1135

CatTail · 2020-09-10T12:36:11Z

Proposed changes

Add gRPC support for VirtualServer CRD. For example, grpc proxy traffic for specific route,

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
  routes:
  - path: /
    action:
      pass: grpc-app
      grpc: true # enables gRPC for the upstream

Fix #908

Note I'm not an expert in Go/Nginx/Kubernetes, so feel free to suggest changes so we can have a solid implementation for VirtualServer gRPC support.

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 master
I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

netlify · 2020-09-10T12:36:14Z

Deploy request for nginx-kubernetes-ingress rejected.

Rejected with commit 45cf1391d6819ae40effabf17bdb8b6645f3d8a2

https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy

CatTail · 2020-09-10T12:50:36Z

Looks like CI failed due to insufficient permission

pleshakov

Hi @CatTail

Thanks for the PR! Looks like most of the work is done.

I left a few comments and suggestions.

Additionally:

Could you add a warning(s) to VirtualServer or VirtualServerRoute if gRPC is enabled when HTTP/2 and TLS termination are both disabled? You could do that in GenerateVirtualServerConfig when you loop over VS or VSRs upstreams. To add a warning, you can do something like below:

// owner is either VS or VSR
vsc.addWarningf(owner, "gRPC will not be enabled for upstream %s. gRPC requires enabled HTTP/2 and TLS termination", u.Name)

As a result, you will see that warning in the status of VS or VSR and the events. This will provide a short feedback looks for users.

Looks like CI failed due to insufficient permission

We recently migrated to GitHub actions and haven't checked how it works for PRs from forks. We should fix it shortly.

pkg/apis/configuration/v1/types.go

docs-web/configuration/virtualserver-and-virtualserverroute-resources.md

internal/configs/version2/http.go

internal/configs/version2/nginx.virtualserver.tmpl

internal/configs/virtualserver.go

pleshakov · 2020-09-11T03:05:12Z

internal/configs/virtualserver.go

+	}
+
+	if internal {
+		return fmt.Sprintf("%v$request_uri", proxyPass)


unfortunately, this will not work for gRPC. You can check it if you deploy a VS like this:

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 - name: grpc-app-2 service: grpc-svc port: 50051 # grpc: true routes: - path: / splits: - weight: 50 action: pass: grpc-app grpc: true - weight: 50 action: pass: grpc-app-2 grpc: true

In the NGINX logs you will see something simliar, depending on your app:

2020/09/11 02:56:07 [error] 71#71: *62 invalid host in upstream "vs_default_grpc-example_grpc-app-2/helloworld.Greeter/SayHello", client: 127.0.0.1, server: grpc.example.com, request: "POST /helloworld. Greeter/SayHello HTTP/2.0", host: "grpc.example.com:8443" 127.0.0.1 - - [11/Sep/2020:02:56:07 +0000] "POST /helloworld.Greeter/SayHello HTTP/2.0" 500 1 "-" "grpc-go/1.29.0-dev" "-"

There is a solution to fix that: instead of doing grpc_pass grpc://vs_default_grpc-example_grpc-app-2$request_uri; in the Config, do rewrite ^ $request_uri break; . That will work :)

To do that, please update generateRewrites function, so that it inserts rewrite ^ $request_uri break; if the location is internal and gPRC is enabled for the upstream. Note that the function might add additional configured rewrites, so in that case rewrite ^ $request_uri break; will not have break in the end.

Thanks for the detailed explanation, it seems we already have rewrite rule for internal, correct me if I'm wrong, is it means we don't need additional rewrite rule?

https://github.com/nginxinc/kubernetes-ingress/blob/1ee2bcdfa033278a9ad2ba534072cd167b8b8a1f/internal/configs/virtualserver.go#L1186

internal/configs/version2/nginx-plus.virtualserver.tmpl

pleshakov · 2020-10-16T23:17:14Z

Hi @CatTail

Just checking if you're still planning to finish the implementation of this feature

CatTail · 2020-10-18T11:48:24Z

Hi @pleshakov, sorry for the late response, yes I still plan to implement this feature, however, I'm a little bit busy these days so I might need to come back a later to finish it.

CatTail · 2021-01-18T02:19:09Z

Hi @pleshakov I know it's been a while since I update this PR, when you have time can you help to take a look, thanks!

pleshakov · 2021-01-19T17:27:54Z

Hi @CatTail

Thanks for sending the update! Hopefully, I review it by the end of the week. We've been busy wrapping up things for the next release.

pleshakov · 2021-01-26T00:00:04Z

Hi @CatTail

Sorry for the delay. Thanks for all the work so far! However, we decided re-evaluate approach in the implementation to make sure it addressed the gRPC use case sufficiently, in particular user-facing configuration and error responses . We'll do an investigation and shall get back to you in about 2 weeks time.

CatTail · 2021-01-26T01:27:15Z

Hi @CatTail

Sorry for the delay. Thanks for all the work so far! However, we decided re-evaluate approach in the implementation to make sure it addressed the gRPC use case sufficiently, in particular user-facing configuration and error responses . We'll do an investigation and shall get back to you in about 2 weeks time.

Hi Pleshakov, thanks for your help anyway, take your time.

CatTail · 2021-03-11T14:08:38Z

Hi @pleshakov, just checking if you have come to a conclusion that how you are going to implement gRPC for VirtualServer?

pleshakov · 2021-03-13T02:04:38Z

Hi @CatTail

Apologies! we got delayed because of the other priorities. I will provide an update next week.

pleshakov

Hi @CatTail

We reevaluated approach in the implementation to make sure it addresses the gRPC use case sufficiently.

It covers the basic implementation of this feature well 👍

I left a number of comments as part of the review. Once those are resolved, we could merge this PR!

Your PR covers 2/3 of it - which is great! After the PR is merged, we can address the remaining ones - those would be supporting error pages and active health checks (for NGINX Plus template).

There is one more comment - about supporting rewrites. This is related to this discussion -- #1135 (comment)

We need to add a rewrite to support splits like below:

  - path: /
    splits:
    - weight: 50
      action:
        pass: grpc-app
        grpc: true
    - weight: 50
      action:
        pass: grpc-app-2
        grpc: true

There is a solution to fix that - adding in the Config rewrite ^ $request_uri break; when we have an internal location (such as the one in the case with splits) and gRPC is enabled.

To do that, could you add something like below to generateRewrites, at the top of the function:

	if grpcEnabled && internal {
		return []string{"rewrite ^ $request_uri break;"}
	}

That should solve the issue.

In that case, we will always ignore any rewrite, if specified by proxy.RewritePath. However, that is ok for this implementation.

pleshakov · 2021-03-18T20:42:37Z

internal/configs/version2/nginx.virtualserver.tmpl

@@ -254,15 +254,20 @@ server {
        proxy_pass {{ $l.InternalProxyPass }};
        {{ end }}

-        {{ if $l.ProxyPass }}
+        {{ if or $l.ProxyPass $l.GRPCPass}}


Could you fix the indentation here and in the other template? Please also note about the space between pass and }.

{{ if or $l.ProxyPass $l.GRPCPass }} {{ $proxyOrGRPC := "proxy" }} {{ if $l.GRPCPass }} {{ $proxyOrGRPC = "grpc" }} {{ end }}

pleshakov · 2021-03-18T20:51:39Z

internal/configs/version2/nginx.virtualserver.tmpl

+        {{ $proxyOrGRPC }}_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+        {{ $proxyOrGRPC }}_set_header X-Forwarded-Host $host;
+        {{ $proxyOrGRPC }}_set_header X-Forwarded-Port $server_port;
+        {{ $proxyOrGRPC }}_set_header X-Forwarded-Proto {{ with $s.TLSRedirect }}{{ .BasedOn }}{{ else }}$scheme{{ end }};
            {{ range $h := $l.ProxySetHeaders }}
        proxy_set_header {{ $h.Name }} "{{ $h.Value }}";


Could you update the remaining headers to use either grpc or proxy prefixes? Below is the full list:

* proxy_ssl_* -> grpc_ssl _* * proxy_set_header -> grpc_set_header (except Upgrade and Connection) * proxy_intercept_errors -> grpc_intercept_errors * proxy_hide_header -> grpc_hide_header * proxy_pass_header -> grpc_pass_header * proxy_ignore_headers -> grpc_ignore_headers * proxy_next_upstream_* -> grpc_next_upstream_*

pleshakov · 2021-03-18T21:01:31Z

pkg/apis/configuration/v1/types.go

@@ -77,6 +77,7 @@ type Upstream struct {
 	SlowStart                string            `json:"slow-start"`
 	Queue                    *UpstreamQueue    `json:"queue"`
 	SessionCookie            *SessionCookie    `json:"sessionCookie"`
+	GRPC                     bool              `json:"grpc"`


After we reconsidered gRPC feature, we decided to go with the type field, like below:

apiVersion: k8s.nginx.org/v1 kind: VirtualServer metadata: name: cafe spec: host: cafe.example.com tls: secret: cafe-secret upstreams: - name: tea service: tea-svc type: grpc # new field port: 50051 routes: - path: /tea action: pass: tea

For now, the field will support two values - http (default) and grpc. Could you add the type field and remove the grpc field?

it will be important to add validation for that field in validateUpstreams here https://github.com/nginxinc/kubernetes-ingress/blob/1ee2bcdfa033278a9ad2ba534072cd167b8b8a1f/pkg/apis/configuration/validation/virtualserver.go#L397 . A new function like validateUpstreamType could do the job.

For the documentation of the new field in the docs, I think something like below would work:

The type of the upstream. Supported values are ``http`` and ``grpc``. The default is ``http``. For gRPC, it is necessary to enable HTTP/2 in the `ConfigMap </nginx-ingress-controller/configuration/global-configuration/configmap-resource/#listeners)>`_ and configure TLS termination in the VirtualServer.

pleshakov · 2021-03-18T21:03:22Z

internal/configs/virtualserver.go

@@ -1216,6 +1224,23 @@ func generateProxyPassProtocol(enableTLS bool) string {
 	return "http"
 }

+func generateGRPCPass(grpcEnabled bool, http2Enabled bool, tlsEnabled bool, upstreamName string) string {


it is ok to generate gRPC config, even if http2 is not enabled - for the user, it will just not work. which is the same if we generate http config. Could we remove http2Enabled bool here?

pleshakov · 2021-03-18T21:07:10Z

internal/configs/virtualserver.go

@@ -284,6 +284,10 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS

 	// generate upstreams for VirtualServer
 	for _, u := range vsEx.VirtualServer.Spec.Upstreams {
+		if (vsEx.VirtualServer.Spec.TLS != nil || !vsc.cfgParams.HTTP2) && u.GRPC {


there is a bug here - it should be vsEx.VirtualServer.Spec.TLS == nil

also, there are more cases to determine TLS termination than vsEx.VirtualServer.Spec.TLS == nil. Those are covered in generateSSLConfig. Because of that, we should use sslConfig == nil here

Suggested change

if (vsEx.VirtualServer.Spec.TLS != nil || !vsc.cfgParams.HTTP2) && u.GRPC {

if (sslConfig == nil || !vsc.cfgParams.HTTP2) && u.GRPC {

same for the VSR upstreams

pleshakov · 2021-03-18T21:12:23Z

internal/configs/virtualserver_test.go

+
+	result := generateLocationForProxying(path, upstreamName, conf_v1.Upstream{GRPC: true}, &cfgParams, nil, false, 0, "", nil, "", vsLocSnippets, false, "", "")
+	if !reflect.DeepEqual(result, expected) {
+		t.Errorf("TestGenerateLocationForGrpcProxying() returned \n%v but expected \n%v", result, expected)


for comparing big structs, we recently started using this approach -- https://github.com/nginxinc/kubernetes-ingress/blob/901dc72c98e8c3ced5d9efc7f3c3e0deaeb6ca58/internal/configs/virtualserver_test.go#L645
. I suggest using that here.

github-actions · 2021-05-18T01:59:54Z

This PR 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.

CatTail · 2021-05-18T03:47:52Z

Sorry for the late response but FYI I'm still interested in merge this PR, I will get back to it as soon as I have chance to do that.

pleshakov · 2021-06-10T20:43:58Z

Hi @CatTail

Thank you for your contributions!

Expanding protocol support aligns with our current road map. We understand everyone's time is valuable and don't expect contributors to be bound by our timelines. We'd like to use your efforts as a foundation and build on them to meet broader product requirements; all contributions are attributed accordingly.

We'd like to accept this work and allocate resources to see its completion.

Binsabbar · 2021-08-04T09:17:17Z

Hi

Is there anyway to use gRPC with VirtualServer until this merge request is merged? I am currently stuck and need to use this feature.

brianehlert · 2021-08-04T15:25:47Z

@Binsabbar I see that you have a support ticket open.
We can use that to work with you to achieve a gRPC configuration while we work on making gRPC a first class notation.

lucacome · 2022-01-07T22:34:52Z

@CatTail thanks for your idea and contribution, it was very helpful and made it possible for us to add gRPC support for VirtualServer building on this PR.

The feature is released in https://github.com/nginxinc/kubernetes-ingress/releases/tag/v2.1.0 so I'm closing this PR, thanks again!

CatTail force-pushed the feat/virtualserver-grpc branch from 45cf139 to bd560aa Compare September 10, 2020 12:36

CatTail mentioned this pull request Sep 10, 2020

gRPC support for VirtualServer/VirtualServerRoute #908

Closed

pleshakov reviewed Sep 11, 2020

View reviewed changes

CatTail added 13 commits January 8, 2021 11:30

add grpc boolean field for action

ba2502d

update CRD templates

92a96bd

update web doc to explain grpc field

b3e6a39

add GRPCPass field for Location config

0e82d2f

update go template for virtualserver

202e877

add template test

2e5a4ef

move GRPC field from Action to Upstream

6b5cd5c

add isGRPCUpstream to template function

5401555

update template implementation based on CR

31dc078

calculate GRPCPass for template

e70201e

add test for template generation

efe9899

fix typo

e7206bb

fix GRPCPass not working

9621d2f

CatTail force-pushed the feat/virtualserver-grpc branch from bd560aa to 9621d2f Compare January 8, 2021 09:13

CatTail added 3 commits January 8, 2021 17:26

remove error page template

34c80db

format code

049f950

add warning message if HTTP2 and TLS not enabled

1ee2bcd

CatTail requested a review from pleshakov January 8, 2021 10:14

pleshakov reviewed Mar 18, 2021

View reviewed changes

github-actions bot added the stale Pull requests/issues with no activity label May 18, 2021

github-actions bot removed the stale Pull requests/issues with no activity label May 19, 2021

lucacome added the no-autoupdate label Aug 25, 2021

lucacome added no-autoupdate waiting for response Waiting for author's response and removed no-autoupdate labels Oct 7, 2021

lucacome closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gRPC support for VirtualServer #1135

add gRPC support for VirtualServer #1135

CatTail commented Sep 10, 2020

netlify bot commented Sep 10, 2020 •

edited

Loading

CatTail commented Sep 10, 2020

pleshakov left a comment

pleshakov Sep 11, 2020

CatTail Jan 8, 2021

pleshakov commented Oct 16, 2020

CatTail commented Oct 18, 2020

CatTail commented Jan 18, 2021

pleshakov commented Jan 19, 2021

pleshakov commented Jan 26, 2021

CatTail commented Jan 26, 2021

CatTail commented Mar 11, 2021

pleshakov commented Mar 13, 2021

pleshakov left a comment

pleshakov Mar 18, 2021

pleshakov Mar 18, 2021

pleshakov Mar 18, 2021

pleshakov Mar 18, 2021

pleshakov Mar 18, 2021

pleshakov Mar 18, 2021

github-actions bot commented May 18, 2021

CatTail commented May 18, 2021

pleshakov commented Jun 10, 2021

Binsabbar commented Aug 4, 2021

brianehlert commented Aug 4, 2021

lucacome commented Jan 7, 2022

	if (vsEx.VirtualServer.Spec.TLS != nil \|\| !vsc.cfgParams.HTTP2) && u.GRPC {
	if (sslConfig == nil \|\| !vsc.cfgParams.HTTP2) && u.GRPC {

add gRPC support for VirtualServer #1135

add gRPC support for VirtualServer #1135

Conversation

CatTail commented Sep 10, 2020

Proposed changes

Checklist

netlify bot commented Sep 10, 2020 • edited Loading

CatTail commented Sep 10, 2020

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov commented Oct 16, 2020

CatTail commented Oct 18, 2020

CatTail commented Jan 18, 2021

pleshakov commented Jan 19, 2021

pleshakov commented Jan 26, 2021

CatTail commented Jan 26, 2021

CatTail commented Mar 11, 2021

pleshakov commented Mar 13, 2021

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 18, 2021

CatTail commented May 18, 2021

pleshakov commented Jun 10, 2021

Binsabbar commented Aug 4, 2021

brianehlert commented Aug 4, 2021

lucacome commented Jan 7, 2022

netlify bot commented Sep 10, 2020 •

edited

Loading