-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
catch-all location answer as an other namespace/ingress_name #9054
Comments
@Guidurand: This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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. |
/remove-kind bug Hi @Guidurand, To help identify the problem in the controller, Please edit your post and explain the justification/reasoning/basis for this statement ;
Your update could help others get more insight into the problem |
Hi @Guidurand |
Thanks for the suggestion, I just tried and got the same result :-( |
Hi @longwuyuan
In my mind, it should always be the backend that answer, in this case, this is the nginx-ingress-controller or the default-backend when activated
Indeed I understand better the behaviour of the choice of the "backend", in our case we have several services and several ingress, so it is the first "ingress" created which will inherit the statistics of the requests in error (/deadendpoint), nevertheless would it not be more logical to attribute them to the controller or default-backend rather than to the first ingress created whereas it does not process these requests? Thanks |
I am not sure what problem has to be solved yet. I think you should read this https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#default-backend . Then you should configure a service as the default-backend as described in that annotation documentation. Then you should look at the nginx.conf. |
I've disable the catch-all
I don't clearly understand why requests made on an unknown endpoints should be associated to the oldest location created when he will never answer them, anyway maybe nobody is monitoring 4xx 🤷♂️ |
I don't understand. Usually when such an issue is reported, the discussion becomes better after the following info is posted to the issue, from the namespace in question and the clusterwide objects in question;
If I make a guess on your last post, it will be based on some guess and assumptions so trying to avoid that Also, its clear that you are seeking support and github issue is watched by few very very few people. You can get more people watching your message if you discuss support issues on the kubernetes slack |
We're seeing the same thing, in v1.4.0. We use regex paths in ingresses, and the controller is adding this catch-all location at the end of every server. However, the location block is copying configuration from another ingress for that same server, which results in incorrect behavior. We want the nginx issue 404s for requests unmatched by paths defined in ingresses, but this catch-all is copying a config from another ingress that requires auth, so all requests that fall through to that are instead returning 401. For example, I see the following location:
So it certainly seems to be taking values from another ingress (up-ingress in local-devcenter-apigw namespace), even though that ingress (up-ingress) does not define a rule with / prefix. |
ingress-nginx/internal/ingress/controller/controller.go Lines 1243 to 1261 in 6931835
|
I agree that In other words, all the oldest annotations for a There are some annotations that apply to the entire server block, such as This reproduces the behavior: #9485 (comment) |
I think I'm running into this right now. I have 3 ingress resources, which define the following rules respectively: apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: ingress-foo-api
annotations:
nginx.ingress.kubernetes.io/rewrite-target: /$2
spec:
ingressClassName: nginx
rules:
- host: foo.com
http:
paths:
- path: /api(/|$)(.*)
pathType: Prefix
backend:
service:
name: service-api
port:
number: 80 apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: ingress-foo-web
annotations:
nginx.ingress.kubernetes.io/rewrite-target: /$2
spec:
ingressClassName: nginx
rules:
- host: foo.com
http:
paths:
- path: /web(/|$)(.*)
pathType: Prefix
backend:
service:
name: service-web
port:
number: 80 apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: ingress-foo-web-root
annotations:
nginx.ingress.kubernetes.io/permanent-redirect: https://foobar.com/blablabla
spec:
ingressClassName: nginx
rules:
- host: foo.com
http:
paths:
- path: /
pathType: Exact
backend:
service:
name: http-svc
port:
number: 80 The last redirect isn't working - it returns the default Nginx 404 page. When I go look at nginx.conf I see something like this:
The 3rd location is the redundant one, and from what I understand, it's the bug described in this issue. When I comment it out, the redirect from above ( Is there any workaround for this? Even if it's not for the bug in general, I'd appreciate any advice which will solve my case specifically. |
After thinking about this some more, I think this actually shows another issue: the catch-all route is not placed in the correct place. In my case, the "redundant" catch-all route wouldn't be a problem if it was last, but it gets inserted between the specific routes and my custom-defined "catch-all" (which is actually an exact path, but that doesn't seem to matter much). I'll open a different issue for that if needed, but I'm leaving this here for anyone who stumbles upon a similar problem. |
Think I did bump into this problem as well. I did use The whole behavior seems to me as if there is an implicit configuration similar to the following snipped made: paths:
path: /
pathType: Prefix I guessed this because an exact match would generate something like As a workaround I tried to add another Ingress object which has this prefix match and a configuration snippet to return a 404 response: apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
nginx.ingress.kubernetes.io/configuration-snippet: |
return 404;
name: frontend-workaround
namespace: default
spec:
ingressClassName: nginx
rules:
- host: localhost
http:
paths:
- backend:
service:
name: frontend
port:
name: http
path: /
pathType: Prefix Posting this here since there has been a question for a workaround, maybe this can help some people. |
We also bumped into the same issue. I was setting up external authentication and was confused about why it tries to authenticate seemingly arbitrary requests that don't match any ingresses. Then I found the |
After cycling a bunch of old ingresses I'm struggling with this a lot. I think There's also a Edit: Had some incorrect information. This change likely introduced this issue in the first place: #3696 I think the current default backend location implementation might be broken. Applying the following configuration: ---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
nginx.ingress.kubernetes.io/proxy-buffer-size: "4k"
name: prefix-foo
spec:
ingressClassName: nginx
rules:
- host: example.com
http:
paths:
- backend:
service:
name: foo-backend-service
port:
number: 80
path: /foo
pathType: ImplementationSpecific
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
nginx.ingress.kubernetes.io/proxy-buffer-size: "8k"
name: prefix-and-default
spec:
defaultBackend:
service:
name: default-backend-service
port:
number: 80
ingressClassName: nginx
rules:
- host: example.com
http:
paths:
- backend:
service:
name: bar-backend-service
port:
number: 80
path: /bar
pathType: ImplementationSpecific creates the following:
applying them in reverse order (or delete
My expectation would be that
Currently, it appears to be:
Which, IMHO, is a bug. |
To add a little more info to this, this root Location object that a Server object is automatically initialized with can have its properties overwritten by defining a rule with the root path and pathType of Prefix. You can see that happening here: Note that at that point in the code, server.Locations will contain the initial and problematic root Location object. Your additional rule will match that root Location object because the Path will match, the PathType will match, and the root Location object has IsDefBackend set to true. If your root rule is working correctly, you should see a log message like this (don't forget to use the --v=5 flag on your controller)
|
IMO, the core issue is that the ingress being used to associate/configure the initial root Location object is selected based on creationTimestamp (which is the sort order of the list of ingresses). This means the ingress being used will change, for example if the oldest ingress is deleted and then recreated, it will no longer be used to configure the initial root Location object. Instead of using the ingress with the oldest creationTimestamp, the ingress that is used should be the one that has a host rule defined with no path, like the example provided in #3696. For the case where there is no such ingress, don't initialize the Server.Locations list with an initial Location object. |
No, the core issue is that the root location is initialized by configuration for anything other than the root location. Order does not matter. Only system default configuration and root location configuration has business configuring the root location. @longwuyuan you remove the bug label two years ago. This is clearly a bug and not a support request. |
@crinjes I hope you will see that the controller needs to be maintained to be compliant to K8S KEP and secure by default. Practically that has translated into the project treating snippets and regexs with highest level of attention. In the above context, using the regex annotation and selectively not having a catchall location is not precisely inline with current release and the described status of this problem. @Guidurand please edit the issue description after testing with the latest release of the controller and the more applicable pathType spec. Also know that the next release of the controller is going to make several security related changed that may or may not be impacting this. Once you have edited the issue description with the information like the state of K8S resources, the configMaps, the logs, the requests sent and response (curl -v) etc., maybe a developer will find it easier to arrive at a comment. You can then re-open the issue. I will close the issue for now as there is info here that is insightful, but not actionable in the current situation. There is acute shortage of deveoper time and the very difficult to give priority to topics that are further away from the specs of the K8S KEP Ingress API (This topic seems like closer to what the Nginx component of the Openresty base being used to build the controller, should behave & do, out of the box) /close |
@longwuyuan: Closing this issue. In response to this:
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-sigs/prow repository. |
What happened:
Hello 👋
The catch-all location, (
location ~* "^/"
) automatically added by the controller when we use annotationuse-regex:true
is associated to a backend that have not answer to the request itselfWhen a request arrives at the catch-all location it should not be associated with a backend service that has not responded itself
This is problematic in the sense that the catch-all statistics are attributed to a backend that does not process these requests
If we make a request to an endpoint that doesn't exist, (e.g. /deadendpoint) we get a 404, but that same 404 will be counted for a backend that has nothing to do with
We have not identified the backend selection criterion by ingress-nginx, and we have not found a way to define it ourselves
What you expected to happen:
when the
location ~* "^/"
is added the values of $namespace and $ingress_name should be the default-backend or emptyNGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.): v1.2.1
Kubernetes version (use
kubectl version
): v1.22.12Environment:
Cloud provider or hardware configuration: GCP
How was the ingress-nginx-controller installed:
helm chart
values
kubectl describe ingressclasses
How to reproduce this issue:
Install minikube
Install the ingress controller
kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/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/main/docs/examples/http-svc.yaml
create multiple namespace
Create an ingress (please add any additional annotation required)
make a request
look at the config nginx.conf in the nginx-controller pod, you should see in server foo.bar a
location ~* "^/"
where the namespace and ingress_name value is foo or test1 or test2 instead of the default backend/emptyThanks 🙏
The text was updated successfully, but these errors were encountered: