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

Ingresses with auth-url annotation add authentication configuration to default backend. #5995

Closed
ghost opened this issue Aug 10, 2020 · 9 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@ghost
Copy link

ghost commented Aug 10, 2020

NGINX Ingress controller version: v0.34.1

We saw this when we tried to upgrade from v0.22.0 It appears in v0.34.1, but I was able to reproduce the issue starting in v0.23.0.

Kubernetes version (use kubectl version): 1.18.3 (also seen in 1.16.8)

Environment:

  • Cloud provider or hardware configuration: minikube (also saw the issue in EKS)
  • OS (e.g. from /etc/os-release): Buildroot (minikube)
  • Kernel (e.g. uname -a): Linux minikube 4.19.114 Basic structure  #1 SMP Mon Jul 6 11:11:02 PDT 2020 x86_64 GNU/Linux
  • Install tools: kubectl (also seen with Helm)
  • Others: consul for a super-simple backend

What happened:

When upgrading ingress-nginx and using an ingress with nginx.ingress.kubernetes.io/auth-url, we would see the default backend get an auth configuration that we did not expect. It added a location = /_external-auth-Lw { block and the following lines to location / {:

			# this location requires authentication
			auth_request        /_external-auth-Lw;
			auth_request_set    $auth_cookie $upstream_http_set_cookie;
			add_header          Set-Cookie $auth_cookie;

it seemed nondeterministic (sometimes it was there, sometime it wasn't), but I was able to reproduce it by adding the ingresses in a certain order.

What you expected to happen:

I expected the default backend to remain the same when I added an ingress.

How to reproduce it:

Install minikube/kind

Install the ingress controller

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

Install an application that will act as default backend (is just an echo app)

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/docs/examples/http-svc.yaml

Create two ingresses

echo "
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: example-with-auth
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-url: http://127.0.0.1:3000/auth
spec:
  rules:
  - host: http-svc.default.svc.cluster.local
    http:
      paths:
      - path: /example-with-auth
        backend:
          serviceName: http-svc
          servicePort: 80
" | kubectl apply -f -


echo "
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: example-without-auth
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  rules:
    - host: http-svc.default.svc.cluster.local
      http:
        paths:
          - path: /example-without-auth
            backend:
              serviceName: http-svc
              servicePort: 80
" | kubectl apply -f -

get rendered nginx template

kubectl -n ingress-nginx exec $(kubectl -n ingress-nginx get pod -l app.kubernetes.io/component=controller -o name) -- cat /etc/nginx/nginx.conf > rendered-nginx-first-ingress-has-auth

delete ingresses and recreate in opposite order

kubectl delete ing example-with-auth example-without-auth
echo "
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: example-without-auth
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  rules:
    - host: http-svc.default.svc.cluster.local
      http:
        paths:
          - path: /example-without-auth
            backend:
              serviceName: http-svc
              servicePort: 80
" | kubectl apply -f -

echo "
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: example-with-auth
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-url: http://127.0.0.1:3000/auth
spec:
  rules:
  - host: http-svc.default.svc.cluster.local
    http:
      paths:
      - path: /example-with-auth
        backend:
          serviceName: http-svc
          servicePort: 80
" | kubectl apply -f -

get rendered nginx configs

kubectl -n ingress-nginx exec $(kubectl -n ingress-nginx get pod -l app.kubernetes.io/component=controller -o name) -- cat /etc/nginx/nginx.conf > rendered-nginx-first-ingress-does-not-have-auth

diff nginx configs:

$ diff rendered-nginx-first-ingress-has-auth rendered-nginx-first-ingress-does-not-have-auth
2c2
< # Configuration checksum: 6040406927179167520
---
> # Configuration checksum: 4045628235080502490
714,754d713
< 		location = /_external-auth-Lw {
< 			internal;
<
< 			# ngx_auth_request module overrides variables in the parent request,
< 			# therefore we have to explicitly set this variable again so that when the parent request
< 			# resumes it has the correct value set for this variable so that Lua can pick backend correctly
< 			set $proxy_upstream_name "upstream-default-backend";
<
< 			proxy_pass_request_body     off;
< 			proxy_set_header            Content-Length          "";
< 			proxy_set_header            X-Forwarded-Proto       "";
< 			proxy_set_header            X-Request-ID            $req_id;
<
< 			proxy_set_header            Host                    127.0.0.1;
< 			proxy_set_header            X-Original-URL          $scheme://$http_host$request_uri;
< 			proxy_set_header            X-Original-Method       $request_method;
< 			proxy_set_header            X-Sent-From             "nginx-ingress-controller";
< 			proxy_set_header            X-Real-IP               $remote_addr;
<
< 			proxy_set_header            X-Forwarded-For        $remote_addr;
<
< 			proxy_set_header            X-Auth-Request-Redirect $request_uri;
<
< 			proxy_buffering                         off;
<
< 			proxy_buffer_size                       4k;
< 			proxy_buffers                           4 4k;
< 			proxy_request_buffering                 on;
< 			proxy_http_version                      1.1;
<
< 			proxy_ssl_server_name       on;
< 			proxy_pass_request_headers  on;
<
< 			client_max_body_size        1m;
<
< 			# Pass the extracted client certificate to the auth provider
<
< 			set $target http://127.0.0.1:3000/auth;
< 			proxy_pass $target;
< 		}
<
810,814d768
< 			# this location requires authentication
< 			auth_request        /_external-auth-Lw;
< 			auth_request_set    $auth_cookie $upstream_http_set_cookie;
< 			add_header          Set-Cookie $auth_cookie;
<

Anything else we need to know:

Happy to help reproduce. I have the above steps saved in a gist: https://gist.github.com/davidnortonjr-sps/d9f5c67dfff3d83a3b78bca6d3f8da2f

/kind bug

@ghost ghost added the kind/bug Categorizes issue or PR as related to a bug. label Aug 10, 2020
@kd7lxl
Copy link
Contributor

kd7lxl commented Aug 21, 2020

Do you see this behavior consistently? We are seeing slightly different behavior, but might be the same bug. If we have for example 5 different Ingress resources using a single hostname and none of them binds the path /, it seems to randomly choose one of these ingresses and applies all the annotations from that ingress to the default backend each time it does its config generation loop. If it happens to be one with auth-url, we see external auth added to the default backend. Then a few minutes later when it generates the config again, it might choose annotations from an Ingress without auth-url, and the external auth on the default backend is removed. The same flip-flop behavior in the default backend config happens with other annotations like nginx.ingress.kubernetes.io/backend-protocol: "GRPCS" and nginx.ingress.kubernetes.io/proxy-body-size when those happen to vary between Ingress. 6 ingress-controller pods might have 2-3 different nginx.conf at any given time.

@ghost
Copy link
Author

ghost commented Aug 25, 2020

@kd7lxl I was able to see it consistently with 1 pod, depending on the order I created the ingresses. I didn't try with multiple pods. But I think we are seeing the same issue... perhaps we could get some input from the ingress-nginx team.

@ghost
Copy link
Author

ghost commented Sep 25, 2020

Hi @aledbf, I was wondering if you to take a look at this. The problem seems to be that if you have multiple ingresses with the same host and different annotations, the annotations are copied from whichever ingress is processed first. This change came in 0.23.0 with #3696

The indeterminate nature of this seems undesirable. I am happy to take a stab at a solution, but I'm not sure what the desired behavior is. Here is the current location of the problematic code

Thankfully, it seems I can get around the issue and bring back determinate behavior, if I add another ingress for the host without the annotations, with a path of /. As an example, in the case of the gist above, I added the following ingress:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: example-default-backend
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  rules:
    - host: http-svc.default.svc.cluster.local
      http:
        paths:
          - path: /
            backend:
              serviceName: mydefault-svc
              servicePort: 80

cc @kd7lxl , hope this helps with your issue!

@kd7lxl
Copy link
Contributor

kd7lxl commented Sep 25, 2020

cc @kd7lxl , hope this helps with your issue!

Yes, thanks. We've been using this exact workaround. I alluded to it in my comment above but didn't share an example as good as yours.

I also added some alerting so we can catch new instances of this bug quickly and contact the associated team. Here's the query. Values greater than one are bad. count(count_values("config_hash", nginx_ingress_controller_config_hash) by (controller_class)) by (controller_class)

@ghost
Copy link
Author

ghost commented Sep 25, 2020

@kd7lxl I reread your comment, and you were perfectly clear -- if I'd taken the time to read closely. Alas, time seems to run shorter every day.

Also, that is a great Prom query, thanks for sharing!

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 24, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 23, 2021
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

3 participants