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

ingress-nginx adds locations for "^/" for no reason #9485

Open
rittneje opened this issue Jan 6, 2023 · 12 comments
Open

ingress-nginx adds locations for "^/" for no reason #9485

rittneje opened this issue Jan 6, 2023 · 12 comments
Assignees
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@rittneje
Copy link

rittneje commented Jan 6, 2023

What happened:

I looked at the /etc/nginx/nginx.conf file that got generated inside the pod. It contains several occurrences of location ~* "^/" blocks.

This may well be the cause of #8283, as well as some other bizarre behavior we've observed.

What you expected to happen:

None of those blocks should exist, because we don't have any Ingresses that specify such a route.

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

NGINX Ingress controller
Release: v1.0.5
Build: 7ce96cb
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.19.9


Kubernetes version (use kubectl version):
1.22

How to reproduce this issue:

Look at the nginx.conf file. Easiest way is to use kubectl cp to copy it from the pod to your host, and then look at it in a text editor.

@rittneje rittneje added the kind/bug Categorizes issue or PR as related to a bug. label Jan 6, 2023
@k8s-ci-robot
Copy link
Contributor

@rittneje: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jan 6, 2023
@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 6, 2023

Can you please try latest release and update if same behavior exists.

@longwuyuan
Copy link
Contributor

Also it seems odd that you expect a controller not to configure / location. I thought all ingress-controllers did that. If someone decided not to spec the path field, then at least the / path exists for a "normal" request

@rittneje
Copy link
Author

rittneje commented Jan 8, 2023

I don't expect that path to map to anything, because I didn't define any Ingress for it. It should instead yield a trivial 404 for any incorrect paths. But that is very much not what ingress-nginx does. Instead, it picks a seemingly random Ingress, and then decides to do auth and CORS for no reason.

@longwuyuan
Copy link
Contributor

/assign

I would check if vanilla nginx, without kubernetes, would add that location inside all the server blocks, if I create 2 vhosts. I will do that and update.

Hope you can do the same and update.

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 9, 2023

I tried to look at this further ;

  • I have 3 ingress resources but I don't have a single location ~* "^/" block

image

  • What I have in my ingress-controller's nginx.conf is the same as what vanilla non-kubernetes nginx has

image

So default config of the controller is not creating those blocks. Very likely that some annotations or configmaps you use are creating those blocks. And if that is true, then it looks a logical requirement for making your config work

@longwuyuan
Copy link
Contributor

/remove-kind bug

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jan 9, 2023
@crinjes
Copy link

crinjes commented Feb 1, 2023

I do think there is some kind of bug. I have a location ~* "^/" that references an ingress in $namespace and $ingress_name. that's not configured that way.

Here's the ingress that's duplicated in the ^/ location:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/auth-realm: Authentication Required
    nginx.ingress.kubernetes.io/auth-secret: basic-auth
    nginx.ingress.kubernetes.io/auth-type: basic
    nginx.ingress.kubernetes.io/proxy-body-size: 2m
    nginx.ingress.kubernetes.io/rewrite-target: /api/$2
  name: backend
  namespace: foo
spec:
  rules:
  - host: foo.example.com
    http:
      paths:
      - backend:
          service:
            name: backend
            port:
              number: 80
        path: /bar/api(/|$)(.*)
        pathType: ImplementationSpecific
  tls:
  - hosts:
    - foo.example.com
    secretName: tls-foo-example

Here's the difference in the two location blocks:

--- good        2023-02-01 15:54:47.104269377 +0100
+++ bad 2023-02-01 15:54:55.734180711 +0100
@@ -1,12 +1,12 @@
-                location ~* "^/bar/api(/|$)(.*)" {
+                location ~* "^/" {

                         set $namespace      "foo";
                         set $ingress_name   "backend";
-                        set $service_name   "backend";
-                        set $service_port   "80";
-                        set $location_path  "/bar/api(/|${literal_dollar})(.*)";
+                        set $service_name   "";
+                        set $service_port   "";
+                        set $location_path  "/";
                         set $global_rate_limit_exceeding n;

                         rewrite_by_lua_block {
                                 lua_ingress.rewrite({
                                         force_ssl_redirect = false,
@@ -44,11 +44,11 @@
                         }

                         port_in_redirect off;

                         set $balancer_ewma_score -1;
-                        set $proxy_upstream_name "foo-backend-80";
+                        set $proxy_upstream_name "upstream-default-backend";
                         set $proxy_host          $proxy_upstream_name;
                         set $pass_access_scheme  $scheme;

                         set $pass_server_port    $server_port;

@@ -117,11 +117,11 @@
                         # In case of errors try the next upstream server before returning an error
                         proxy_next_upstream                     error timeout;
                         proxy_next_upstream_timeout             0;
                         proxy_next_upstream_tries               3;

-                        rewrite "(?i)/foo/api(/|$)(.*)" /api/$2 break;
+                        rewrite "(?i)/" /api/$2 break;
                         proxy_pass http://upstream_balancer;

                         proxy_redirect                          off;

                 }

There are more ingress resources defined for that host in different namespaces, but all of them have a path other than / defined. I'll see if I can find a way to reproduce the behavior.

@longwuyuan
Copy link
Contributor

Ok, please help with info that helps change label on this to a bug

@crinjes
Copy link

crinjes commented Feb 2, 2023

It does that even with a single ingress it seems. Note that you need a nginx.ingress.kubernetes.io/use-regex: "true" annotation for a location ~* "^/" block to appear, otherwise it's a location / block that duplicates configuration from another ingress.

kind create cluster
kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/static/provider/baremetal/deploy.yaml
kubectl wait deployment -n ingress-nginx ingress-nginx-controller --for=condition=Available=True --timeout=600s
POD_NAME=$(kubectl get pods -n ingress-nginx -l app.kubernetes.io/name=ingress-nginx,app.kubernetes.io/component=controller -o NAME)

# should return 404
kubectl -n ingress-nginx exec $POD_NAME -- curl -sIH 'Host: foo.example.com' http://localhost

# add and wait for ingress
echo "apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: |
      # this should only be in /foo
      return 418;
  name: foo
spec:
  ingressClassName: nginx
  rules:
  - host: foo.example.com
    http:
      paths:
      - backend:
          service:
            name: backend
            port:
              number: 80
        path: /foo
        pathType: ImplementationSpecific
" | kubectl apply -f-
until kubectl get ingress foo -o=jsonpath='{.status.loadBalancer}' |grep 'ingress'; do : sleep 1; done

# this should still return 404, but now returns 418
kubectl -n ingress-nginx exec $POD_NAME -- curl -sIH 'Host: foo.example.com' http://localhost

# location / contains configuration for location /foo
kubectl -n ingress-nginx exec $POD_NAME -- cat /etc/nginx/nginx.conf

@crinjes
Copy link

crinjes commented Feb 2, 2023

I think this is related to/duplicate of #9054

@github-actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

No branches or pull requests

4 participants