Admission webhook: properly check that there is no overlapping ingress

[dud225]

What this PR does / why we need it:

The admission webhook fails to detect a conflicting host+path when an ingress with more than one rule is involved.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

Fixes #8972

How Has This Been Tested?

Applying the following ingresses:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ing-a
spec:
  ingressClassName: nginx
  rules:
    - host: domain2
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: svc-a
                port:
                  number: 443
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ing-b
spec:
  ingressClassName: nginx
  rules:
    - host: domain1
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: svc-b
                port:
                  number: 443
    - host: domain2
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: svc-b
                port:
                  number: 443

results in the following behaviour with the current codebase:

$ kubectl apply -f 8972.yaml
ingress.networking.k8s.io/ing-a created
ingress.networking.k8s.io/ing-b created
$ kubectl ingress_nginx -n ingress-nginx conf --host domain2 | grep service_name
                        set $service_name   "svc-b";

The ingress ing-a gets overwritten.

With this change, the admission controller properly detects the conflicting rule:

$ kubectl apply -f 8972.yaml 
ingress.networking.k8s.io/ing-a created
Error from server (BadRequest): error when creating "8972.yaml": admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: host "domain2" and path "/" is already defined in ingress default/ing-a

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Does my pull request need a release note?

The admission webhook now properly detects a conflicting host+path when an ingress with more than one rule is involved.

Additional comment

The code section that checks among the existing ingresses whether any of them pertains to a canary test seems flaky.
According to my tests there is no canary ingresses present in the server configuration, which means that the test always evaluates to false.
When looking closer I've observed that the canary mechanism is implemented from the Lua area and that there is no mention of it within the plain Nginx configuration, that would explain why this data isn't present from the Go area.
As this is my first contribution on the project I'm not super confident about this, so for now I've left it as is.

[k8s-ci-robot]

Hi @dud225. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[strongjz]

Can you add an e2e test for this case as well. one with overlapping ingress, to prove it catches it.

It looks like there a single ingress with multiple rules here
https://github.com/kubernetes/ingress-nginx/blob/0a702723dd1c0c3c0d351a9c0d26016c539806ae/test/e2e/ingress/multiple_rules.go

But not multiple ingress and same rules, which your change would catch.

thank you,
James

/triage accepted
/kind bug
/priority backlog
/ok-to-test

[dud225]

Can you add an e2e test for this case as well. one with overlapping ingress, to prove it catches it.

Done.

I found out that the go get command used to install ginkgo was messing up the files go.mod and go.sum which then prevented me from building the test suite:

Reached DIND check ELSE block, inside run-in-docker.sh
+ docker run --tty --rm -e DEBUG=true -e GOCACHE=/go/src/k8s.io/ingress-nginx/.cache -e GOMODCACHE=/go/src/k8s.io/ingress-nginx/.modcache -e DOCKER_IN_DOCKER_ENABLED=true -v /home/myuser/.kube:/home/myuser/.kube -v /home/myuser/Git/ingress-nginx:/go/src/k8s.io/ingress-nginx -v /home/myuser/Git/ingress-nginx/bin/amd64:/go/bin/linux_amd64 -v /var/run/docker.sock:/var/run/docker.sock -v /tmp/tmp.YNDf4QhEuy:/etc/ingress-controller/ -w /go/src/k8s.io/ingress-nginx registry.k8s.io/ingress-nginx/e2e-test-runner:v20221221-controller-v1.5.1-62-g6ffaef32a@sha256:8f025472964cd15ae2d379503aba150565a8d78eb36b41ddfc5f1e3b1ca81a8e /bin/bash -c 'MAC_OS= ginkgo build ./test/e2e'
Failed to compile e2e:

../../.modcache/golang.org/x/[email protected]/google/default.go:17:2: missing go.sum entry for module providing package cloud.google.com/go/compute/metadata (imported by golang.org/x/oauth2/google); to add:
        go get golang.org/x/oauth2/[email protected]

ginkgo build failed
  Failed to compile all tests

Replacing the go get invocation by a go install flavour let me overcome this issue.

However I'm not quite sure ginkgo is needed locally as the tool is actually pulled from a container image.

[longwuyuan]

• Correct. ginkgo is not needed locally, true
• Some dependent (direct/indirect) packages are getting either installed new or changing versions (upgrade/downgrade). Its a complicated scene and not fully elucidated. Glad you can get by but please do add as much info as possible if tests fail

[dud225]

Glad you can get by but please do add as much info as possible if tests fail

Not sure I got it, could you please elaborate?

[longwuyuan]

others have observed go.mod and go.sum changing midway through tests. So if you have details of the change like diff or reasons for changes visible in diff, it will help.

[dud225]

Here is the output when installing ginkgo from go get:

$ go get github.com/onsi/ginkgo/v2/[email protected]
go: downgraded cloud.google.com/go/compute v1.12.1 => v1.9.0
go: downgraded cloud.google.com/go/compute/metadata v0.2.1 => v0.2.0
go: removed google.golang.org/grpc/examples v0.0.0-20221220003428-4f16fbe410f7

git_diff.txt

However the official instruction to set up Ginkgo mentions the command go install. When running that command it doesn't tweak the go.sum & go.mod files.

I'm not an experienced Go developer so I can't explain the reason nor comment the subtleties between go get and go install.

I assume that this problem is mostly hidden by the fact that probably the majority of the developers on this project already have ginkgo installed.

[longwuyuan]

@dud225 thanks for the update. It helps.

Basically ginkgo install is the visible event. But we have PRs merged that bump ginkgo in multiple places. The not-so-visible event is "other" indirect dependencies either getting newly installed or getting bumped. One of these days it will get in critical path and then new go.{mod,sum} will get committed/merged. Thanks

[dud225]

I've revisited this PR to properly handle the case of a overlapping ingress having a canary annotation. Currently the condition that checks if there is any canary annotation among all the existing ingresses always fails because no canary ingress is ever present in the server slice, it only contains non-canary ingresses:

ingress-nginx/internal/ingress/controller/controller.go

Lines 1254 to 1257 in 5628f76

	if anns.Canary.Enabled {
	klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
	continue
	}

and

ingress-nginx/internal/ingress/controller/controller.go

Lines 1329 to 1332 in 5628f76

	if anns.Canary.Enabled {
	klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
	continue
	}

I've added the relevant E2E test.

@strongjz This PR is now ready to be merged.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dud225
Once this PR has been reviewed and has the lgtm label, please assign elvinefendi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

PR needs rebase.

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]

@dud225: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-ingress-nginx-gofmt	0a702723dd1c0c3c0d351a9c0d26016c539806ae	link	true	/test pull-ingress-nginx-gofmt
pre-ingress-nginx-lua-test	7d97cce	link	true	/test pre-ingress-nginx-lua-test
pre-ingress-nginx-codegen	7d97cce	link	true	/test pre-ingress-nginx-codegen
pre-ingress-nginx-boilerplate	7d97cce	link	true	/test pre-ingress-nginx-boilerplate

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.