search

kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.45k stars 8.24k forks source link

Nginx should not set Connection header on requests for http/2 / grpc upstreams #8086

Closed wdullaer closed 2 years ago

wdullaer commented 2 years ago

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

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:

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 '';
longwuyuan commented 2 years ago

@theunrealgeek, any comments on this one

strongjz commented 2 years ago

/assign @strongjz

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle stale

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle rotten

wdullaer commented 2 years ago

/remove-lifecycle rotten

This issue is still relevant I believe

longwuyuan commented 2 years ago

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

wdullaer commented 2 years ago

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.

longwuyuan commented 2 years ago

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.

k8s-ci-robot commented 2 years ago

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

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](https://github.com/kubernetes/ingress-nginx/issues/8086): >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 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 , or unsubscribe . >> You are receiving this because you commented. >> > > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
longwuyuan commented 2 years ago

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

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle stale

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle rotten

k8s-triage-robot commented 2 years ago

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:

You can:

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

/close not-planned

k8s-ci-robot commented 2 years ago

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

In response to [this](https://github.com/kubernetes/ingress-nginx/issues/8086#issuecomment-1265341170): >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][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.