ingress-nginx Nginx should not set Connection header on requests for http/2 / grpc upstreams

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

bash-5.1$ /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.1.0
  Build:         cacbee86b6ccc45bde8ffc184521bed3022e7dee
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.19.9

-------------------------------------------------------------------------------

Issue is applicable to master based on the code I checked

Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:48:33Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:42:41Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"linux/amd64"}

Environment:

$ cat /etc/os-release
NAME="Ubuntu"
VERSION="20.04.3 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.3 LTS"
VERSION_ID="20.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=focal
UBUNTU_CODENAME=focal
$ uname -a
Linux <redacted> 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

How was the ingress-nginx-controller installed: Using kustomize. The only relevant parameter from the configmap is upstream-keepalive-connections

apiVersion: v1
kind: ConfigMap
data:
  upstream-keepalive-connections: "0"

Current State of the controller: Not Applicable
Others:

What happened: As of 3 months ago, the go-grpc http/2 server performs stricter enforcement of HTTP/2 requests. Specifically, any request that includes a Connection header is to be considered malformed. See here for relevant code: https://github.com/grpc/grpc-go/blame/master/internal/transport/http2_server.go#L410-L416 This MDN page also highlights the risk of using Connection headers in HTTP/2 requests and responses: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

Unfortunately the nginx.conf from the ingress controller will always insert a Connection to 'enable websockets' when the config variable UpstreamKeepaliveConnections is set to 0. See https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl#L384-L392 and https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl#L1261-L1267

If the upstream is configured as HTTP/2 or GRPC, those stanza's should not be included in the config at all. (Enabling websockets makes little sense for gRPC services anyway).

Nginx will log errors of the form

*12839444 upstream rejected request with error 1 while reading response header from upstream, client: <redacted>, server: <grpc-server>, request: "POST /opentelemetry.proto.collector.trace.v1.TraceService/Export HTTP/2.0", upstream: "grpc://<upstream-ip-port>", host: "<host>"

when trying to proxy to a go-grpc based service.

When setting the GRPC_GO_LOG_SEVERITY_LEVEL=info GRPC_GO_LOG_VERBOSITY_LEVEL=2 headers, the following logs will be printed in the gRPC service:

transport: http2Server.operateHeaders parsed a :connection header which makes a request malformed as per the HTTP/2 spec

What you expected to happen: Request should be proxied

How to reproduce it:

Install the ingress controller

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/static/provider/baremetal/deploy.yaml

Install a gRPC application built with the latest go-grpc (you can follow the instructions here for the k8s part: https://github.com/kubernetes/ingress-nginx/tree/main/docs/examples/grpc)

You may want to set the aforementioned headers to get some actionable debug output from the webserver.

kubectl apply -f your-svc.yaml

Create an ingress

echo " apiVersion: networking.k8s.io/v1 kind: Ingress metadata: name: foo-bar annotations: kubernetes.io/ingress.class: nginx nginx.ingress.kubernetes.io/backend-protocol: "GRPC" spec: ingressClassName: nginx # omit this if you're on controller version below 1.0.0 rules: - host: foo.bar http: paths: - path: / pathType: Prefix backend: service: name: go-grpc-greeter-server port: number: 80 " | kubectl apply -f -

make a request

POD_NAME=$(k get pods -n ingress-nginx -l app.kubernetes.io/name=ingress-nginx -o NAME) kubectl exec -it -n ingress-nginx $POD_NAME -- grpcurl -H 'Host: foo.bar' localhost list

Anything else we need to know: You can workaround the issue by using a server snippet annotation that 'blanks' out the internal nginx.conf variables for this server:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/server-snippet: |
      set $http_upgrade '';
      set $connection_upgrade '';

Dec 29 '21 10:12 wdullaer

@theunrealgeek, any comments on this one

Dec 30 '21 03:12 longwuyuan

/assign @strongjz

Jan 04 '22 17:01 strongjz

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Apr 04 '22 17:04 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

May 04 '22 18:05 k8s-triage-robot

/remove-lifecycle rotten

This issue is still relevant I believe

May 04 '22 19:05 wdullaer

@wdullaer, do you have a PR in mind.

Thanks, ; Long

On Thu, 5 May, 2022, 12:30 AM wdullaer, @.***> wrote:

/remove-lifecycle rotten

This issue is still relevant I believe

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8086#issuecomment-1117697256, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWWZZO6TI34AEVSHYZTVILCOVANCNFSM5K5VMXJQ . You are receiving this because you commented.Message ID: @.***>

May 05 '22 04:05 longwuyuan

Wrapping the code here with

{{ if not (eq $proxySetHeader "grpc_set_header") }}

            # Allow websocket connections
            {{ $proxySetHeader }}                        Upgrade           $http_upgrade;
            {{ if $location.Connection.Enabled}}
            {{ $proxySetHeader }}                        Connection        {{ $location.Connection.Header }};
            {{ else }}
            {{ $proxySetHeader }}                        Connection        $connection_upgrade;
            {{ end }}

{{ end }}

will fix the issue for grpc backends.

If you really want to be spec compliant this is probably not sufficient: the HTTP/2 spec prohibits the use of the Connection header, and this will only prevent its use for gRPC based backends. In the future other web servers may start rejecting requests.

May 06 '22 08:05 wdullaer

The project has to be spec compliant.

/triage accepted /help

Thanks,

; Long Wu Yuan

On 06-May-2022, at 1:49 PM, wdullaer @.***> wrote:

Wrapping the code here https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl#L1357-L1362 with

{{ if not (eq $proxySetHeader "grpc_set_header") }}
        # Allow websocket connections
        {{ $proxySetHeader }}                        Upgrade           $http_upgrade;
        {{ if $location.Connection.Enabled}}
        {{ $proxySetHeader }}                        Connection        {{ $location.Connection.Header }};
        {{ else }}
        {{ $proxySetHeader }}                        Connection        $connection_upgrade;
        {{ end }}
{{ end }} will fix the issue for grpc backends.

If you really want to be spec compliant this is probably not sufficient: the HTTP/2 spec prohibits the use of the Connection header, and this will only prevent its use for gRPC based backends. In the future other web servers may start rejecting requests.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8086#issuecomment-1119370987, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWXJZU4EVKJDYWHPSCTVITIXVANCNFSM5K5VMXJQ. You are receiving this because you commented.

May 06 '22 10:05 longwuyuan

@longwuyuan: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

Why are we solving this issue?
To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
Does this issue have zero to low barrier of entry?
How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to this:

The project has to be spec compliant.

/triage accepted /help

Thanks,

; Long Wu Yuan
On 06-May-2022, at 1:49 PM, wdullaer @.***> wrote:

Wrapping the code here https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl#L1357-L1362 with

{{ if not (eq $proxySetHeader "grpc_set_header") }}
        # Allow websocket connections
        {{ $proxySetHeader }}                        Upgrade           $http_upgrade;
        {{ if $location.Connection.Enabled}}
        {{ $proxySetHeader }}                        Connection        {{ $location.Connection.Header }};
        {{ else }}
        {{ $proxySetHeader }}                        Connection        $connection_upgrade;
        {{ end }}
{{ end }} will fix the issue for grpc backends.

If you really want to be spec compliant this is probably not sufficient: the HTTP/2 spec prohibits the use of the Connection header, and this will only prevent its use for gRPC based backends. In the future other web servers may start rejecting requests.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/issues/8086#issuecomment-1119370987, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWXJZU4EVKJDYWHPSCTVITIXVANCNFSM5K5VMXJQ. You are receiving this because you commented.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

May 06 '22 10:05 k8s-ci-robot

@wdullaer nit help. Please fix the md formatting in the original post. Only some parts after "what happened" are malformed.

May 06 '22 10:05 longwuyuan

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Aug 04 '22 11:08 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

Sep 03 '22 11:09 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue with /reopen
Mark this issue as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Oct 03 '22 12:10 k8s-triage-robot

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue with /reopen

Mark this issue as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Oct 03 '22 12:10 k8s-ci-robot