disable path overlap validation flag

[Fsero]

What this PR does / why we need it:

In https://github.com/kubernetes/ingress-nginx/issues/5651 there was a request to throw an error when there are two ingresses defining the same host and path, which was implemented as part of the validation webhook.

Despite of this there are clear rules on the ingress controller that describes what the controller would do in such situation (the oldest rule wins)

Some users are relying on this validation behaviour to prevent misconfigurations, but there are use cases where allowing it, maybe temporarily, is helpful. Those use cases includes:

• Splitting large ingresses objects in smaller ones https://github.com/kubernetes/ingress-nginx/issues/10820
• Moving ingress objects between namespaces without downtime (like when you transfer an ingress from team A that owns namespace A to team B that owns namespace B) https://github.com/kubernetes/ingress-nginx/issues/10090

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)

• [X] 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 #10820 fixes #10090

How Has This Been Tested?

building an image and testing it in a local cluster, will update later with some real life scenarios

Checklist:

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

Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9

[netlify[bot]]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	f23eb2ff85ceae5cdfc28905d19518404cf99c41
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/660bf8250ccf5100086f3b9c

[k8s-ci-robot]

Welcome @Fsero!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @Fsero. 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.

[longwuyuan]

From the K8S official resources, it will be useful to add a link here that covers the specs and KEP for any ingress-controller, to deal with duplicate routing rule factors like identical hostname and identical path.

[strongjz]

I'm not sure about adding footguns like this to the configuration.

With that said, we can add it but disable it as default, so the cluster admin has to choose to allow it consciously.

It may be in the check overlap function, but can you ensure this dumps out a warning in the admission logs?

Can you add an e2e test about this with it enabled and disabled?

[strongjz]

/kind feature

[strongjz]

/triage accepted

[Fsero]

I'm not sure about adding footguns like this to the configuration.

With that said, we can add it but disable it as default, so the cluster admin has to choose to allow it consciously.

It may be in the check overlap function, but can you ensure this dumps out a warning in the admission logs?

Can you add an e2e test about this with it enabled and disabled?

Definitely, let me take a look and I'd do the e2e test

[ailurarctos]

I don't think this fixes https://github.com/kubernetes/ingress-nginx/issues/10090 but it does complement the fix for it (https://github.com/kubernetes/ingress-nginx/pull/10091).

My understanding is that this MR will allow users who make use of overlapping paths (to perform zero-downtime migrations) to still take advantage of the validation webhook (which is great!). The issue https://github.com/kubernetes/ingress-nginx/issues/10090 is that the NGINX ingress controller will generate an invalid configuration in specific cases where overlapping paths with both prefix and exact types are used (the example in https://github.com/kubernetes/ingress-nginx/issues/10090#issue-1760116420).

[Fsero]

@strongjz I've added e2e tests, didn't add disabled ones as disabled is the default behavior and some test are already checking for path overlap (but if you think is required I can add it obviously, just wanted to keep tests readable) also added the warning when enabled the flag.

Tell me what you think

[strongjz]

/lgtm

/hold

ill let @rikatz take a look and then we can get it merged into main.

Probably going to into 1.11.0, trying not to add features in patch release.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, Fsero, strongjz

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

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [cpanato,strongjz]

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]

New changes are detected. LGTM label has been removed.

[Fsero]

@strongjz @cpanato @rikatz just rebased in top of main this PR, waiting for approvals.

Is there something else I can do to merge this one?

Thanks a lot

[KTamas]

Hi, is there an update on this? We're running into this exact issue and merging this PR would solve it.

[tao12345666333]

Moving ingress objects between namespaces without downtime

I believe that this use case has practical significance.

Please resolve the conflicts.

[Fsero]

done @tao12345666333

[Fsero]

@strongjz @rikatz @cpanato is there something else I can do to merge this one?

Thanks a lot

[cpanato]

lets wait a bit more on this, i will discuss that with @rikatz when we both have a time

[tao12345666333]

We can add it to the list for our next community meeting. @strongjz

[Fsero]

We can add it to the list for our next community meeting. @strongjz

hey! any news on this?

thanks so much

[airhorns]

What's the hesitation on this? It seems super valuable to me -- happy to provide more feedback about why if that'd be helpful!

[Fsero]

@strongjz @rikatz @cpanato sometime has passed and this one it's still open, do you need something else that I can do?

Thanks!