Mergeable Ingress Types ignore root "/" paths

[r4j4h]

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.

2018/03/30 00:41:57 [error] 327#327: *594 open() "/etc/nginx/html/testing-api/v1/schema" failed (2: No such file or directory), client: 10.22.122.237, server: xxx.branch-testing.lab.k8s.xxx.com, request: "GET /testing-api/v1 HTTP/1.1", host: "xxx.branch-testing.lab.k8s.xxx.com"
10.22.122.237 - - [30/Mar/2018:00:41:57 +0000] "GET /testing-api/v1/schema HTTP/1.1" 404 564 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36" "-"

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:

kubernetes-ingress/nginx-controller/nginx/configurator.go

Lines 112 to 115 in 6a8bff8

	if loc.Path != "/" {
	loc.IngressResource = objectMetaToFileName(&minion.Ingress.ObjectMeta)
	locations = append(locations, loc)
	}

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.

[pleshakov]

@r4j4h

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

[diazjf]

@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 / path in the documentation. Just a Note that it will affect all minions.