-
Notifications
You must be signed in to change notification settings - Fork 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
Mergeable Ingress Types ignore root "/" paths #264
Comments
I think your suggestion makes a lot of sense -- allowing a minion Ingress resource define the "/" path. If there are conflicting paths, the handling of conflicts logic will make sure that only one Ingress resource handles the "/" path. The handling logic should be the same for "/" or any other path. By the way, there is a PR that will add the handling logic and also take care of merging annotations -- https://github.com/nginxinc/kubernetes-ingress/pull/258/files I wonder what @diazjf thinks whether minions should be able to define the "/" path. The original intention was to treat the "/" differently, as it is a special catch-all path that affects all minion Ingress resources. Also note that there is an additional case when the "/" location is created for an Ingress resource -- it is when you specify the default backend as in the example below: apiVersion: extensions/v1beta1
kind: Ingress
metadata:
name: test-ingress
spec:
backend:
serviceName: testsvc
servicePort: 80
. . . Your proposal is a good interim solution if we decide to allow a minion to define the "/" path. However, the proper handling of path conflicts must also be added, which will be done #258 |
@pleshakov @r4j4h I think it makes sense to allow for the '/' path. It can only be allowed in 1 minion. Any duplicates of the '/' path will be ignored. We will have to add some information of using the |
Adds new rules for the MergeableTypes. Minions will not be able to have conflicting locations, and can only have service level annotations. Masters will only be able to have host level annotations. Fixes nginx#264
The new Mergeable Ingress Types feature works beautifully, however while testing it out we encountered one gotchya. It seems to explicitly silently ignore root paths and we are curious if that can be safely changed.
Background
We are utilizing an Ingress with multiple paths defined with one path as only the root (
/
) and others as subpaths from there. Within a single Ingress it works well so that other defined paths go to their specific services and any other paths fall back to that service with the root path. Acting like a transparent fallback if you will.With Mergeable Ingress Types in effect, the defined paths all come through ok in the generated nginx configuration file but the root path's Location block was missing and attempts to navigate to it resulted in 404s and in the nginx Ingress controller's logs it appears to actually try to browse to the URL through its local
/etc/nginx/html
directory.Expected Behavior
We expected the root route to have a corresponding
Location
block in the generated nginx configuration file.Proposal
We propose altering the conditional in configurator.go#L112-L115:
https://github.com/nginxinc/kubernetes-ingress/blob/6a8bff81f7bf7a74ccfd1e9e044fcd537792d91c/nginx-controller/nginx/configurator.go#L112-L115
We were able to compile with it commented out and things are working as we expected.
Understandably the conditional was added for a reason though, so we are curious to learn more and figure out the best way to move forward.
We tried to think through why it might have been added and the best we could think of is that it is to prevent two minion Ingresses from both using
/
, but that same problem still exists if two minion Ingress try to both claim other paths such as/anything
. Perhaps we could allow usage of/
but add a warning log line? Or only let one in and ignore the others like we do now but with a warning? Or perhaps there is a danger in using the root path we aren't aware of?We can open an MR with those lines removed but felt opening up dialogue first would be better.
The text was updated successfully, but these errors were encountered: