Skip to content
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

disable path overlap validation flag #10943

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fsero
Copy link

@Fsero Fsero commented Jan 30, 2024

What this PR does / why we need it:

In #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:

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 #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:

  • 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.

Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 30, 2024
Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for kubernetes-ingress-nginx canceled.

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

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 30, 2024
@k8s-ci-robot
Copy link
Contributor

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. 😃

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jan 30, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2024
@Fsero Fsero force-pushed the allow_duplicates branch 7 times, most recently from 441b7bf to ce87884 Compare January 30, 2024 09:37
@Fsero Fsero changed the title feat: disable path overlap validation flag disable path overlap validation flag Jan 30, 2024
@Fsero Fsero force-pushed the allow_duplicates branch 2 times, most recently from 385038c to 3cdcd47 Compare January 30, 2024 09:40
@longwuyuan
Copy link
Contributor

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.

@Fsero Fsero force-pushed the allow_duplicates branch 2 times, most recently from 7cd5ea9 to a671012 Compare February 1, 2024 10:09
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 1, 2024
@strongjz
Copy link
Member

strongjz commented Feb 6, 2024

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
Copy link
Member

strongjz commented Feb 6, 2024

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 6, 2024
@strongjz strongjz added this to the release-1.11 milestone Feb 29, 2024
@strongjz
Copy link
Member

/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 k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 29, 2024
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot
Copy link
Contributor

[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:

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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024
@Fsero
Copy link
Author

Fsero commented Mar 8, 2024

@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
Copy link

KTamas commented Mar 14, 2024

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2024
@tao12345666333
Copy link
Member

Moving ingress objects between namespaces without downtime

I believe that this use case has practical significance.

Please resolve the conflicts.

@Fsero
Copy link
Author

Fsero commented Apr 2, 2024

done @tao12345666333

  ## What this PR does / why we need it:

In kubernetes#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)](https://github.com/kubernetes/ingress-nginx/blob/main/docs/how-it-works.md?plain=1#L27)

  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 kubernetes#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) kubernetes#10090

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

  ## 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

It might help with kubernetes#10820

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

  ## How Has This Been Tested?

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

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
  ## Checklist:

- [X] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [X] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/main/CONTRIBUTING.md) guide
- [X] I have added unit and/or e2e tests to cover my changes.
- [X] All new and existing tests passed.

Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2024
@Fsero
Copy link
Author

Fsero commented Apr 4, 2024

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

Thanks a lot

@cpanato
Copy link
Member

cpanato commented Apr 4, 2024

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

@tao12345666333
Copy link
Member

tao12345666333 commented Apr 8, 2024

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

@Fsero
Copy link
Author

Fsero commented Apr 18, 2024

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

hey! any news on this?

thanks so much

@airhorns
Copy link

airhorns commented May 1, 2024

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
Copy link
Author

Fsero commented May 6, 2024

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
9 participants