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 e9664c2
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/6745e21e433cde0008b63a70

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

@sarahkadar
Copy link

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 Fsero force-pushed the allow_duplicates branch from 352bce5 to 1852275 Compare April 2, 2024 11:18
@Fsero
Copy link
Author

Fsero commented Apr 2, 2024

done @tao12345666333

@Fsero Fsero force-pushed the allow_duplicates branch 2 times, most recently from d90d036 to f23eb2f Compare April 2, 2024 12:20
@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!

@Fsero
Copy link
Author

Fsero commented Jun 7, 2024

resurfacing this again @strongjz @rikatz @cpanato do you need me to do something else? do you want me to join the community meeting to explain I think this is useful (we are using this internally in our fork)

Thanks a lot

@tao12345666333
Copy link
Member

Sorry for the long delay.

As I replied before, I think this case has practical significance.
#10943 (comment)

@Fsero @airhorns You can join the community meeting next time or post details here.
We want to know more about the background and the value of this use case.

@airhorns
Copy link

airhorns commented Jun 10, 2024

My use case is described here: #10820 -- in order to make that change, we needed to manually remove the validating webhook in k8s which felt quite risky, make the change, then add it back after removing the duplicates. I can confirm the actual controller was fine with the duplicate resources.

I had an ingress resource that had multiple host names, which locked me in to using the same set of ingress-nginx annotations for each. I wanted to start customizing some annotations for various hostnames, like turning on consistent hashing for a subset of customer domains, or adjusting the rate limit for big tenants up, but I couldn't split the ingress into one ingress resource per hostname without this flag. Can I provide any more info?

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

This would be so incredibly useful.

@Fsero Fsero force-pushed the allow_duplicates branch 3 times, most recently from 6d4077a to b8d00ff Compare September 5, 2024 14:39
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024
@Fsero
Copy link
Author

Fsero commented Sep 6, 2024

let's see if i can explain an use case, imagine you have a company with several teams deploying microservices, there is a good amount of traffic in the service and in the ingress, and due to an internal change you want to transfer the service from team foo to team bar.

Original microservice offered two endpoints /apple and /banana and team bar will get ownership of /banana while /apple will remain managed by foo.

you can setup this scenario in a local kind (I actually created one with make dev-env) and try to apply the following manifest

apiVersion: v1
kind: Namespace
metadata:
  labels:
    kubernetes.io/metadata.name: foo
  name: foo
spec:
  finalizers:
  - kubernetes
---
apiVersion: v1
kind: Namespace
metadata:
  labels:
    kubernetes.io/metadata.name: bar
  name: bar
spec:
  finalizers:
  - kubernetes
---
kind: Pod
apiVersion: v1
metadata:
  name: banana-app
  namespace: foo
  labels:
    app: banana
spec:
  containers:
    - name: banana-app
      image: hashicorp/http-echo
      args:
        - "-text=banana"
---
kind: Service
apiVersion: v1
metadata:
  name: banana-service
  namespace: foo
spec:
  selector:
    app: banana
  ports:
    - port: 5678 # Default port for image
---
kind: Pod
apiVersion: v1
metadata:
  name: apple-app
  namespace: bar
  labels:
    app: apple
spec:
  containers:
    - name: apple-app
      image: hashicorp/http-echo
      args:
        - "-text=apple"
---

kind: Service
apiVersion: v1
metadata:
  name: apple-service
  namespace: bar
spec:
  selector:
    app: apple
  ports:
    - port: 5678 # Default port for image
    
---


apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: one-ingress
  namespace: foo
  annotations:
    ingress.kubernetes.io/rewrite-target: /
spec:
  ingressClassName: nginx
  rules:
  - host: foo.bar.io
    http:
      paths:
        - path: /apple
          pathType: Prefix
          backend:
            service:
               name: apple-service
               port: 
                 number: 5678
        - path: /banana
          pathType: Prefix
          backend:
            service:
                name: banana-service
                port:
                  number: 5678
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: another-ingress
  namespace: bar
  annotations:
    ingress.kubernetes.io/rewrite-target: /
spec:
  ingressClassName: nginx
  rules:
  - host: foo.bar.io
    http:
      paths:
        - path: /banana
          pathType: Prefix
          backend:
            service:
                name: banana-service
                port:
                  number: 5678

bar team members have read this part of the documentation of ingress nginx and thought that would be very neat to create an ingress and deploy the service for some local test and allow the original service to handle the traffic so when everything is ready foo team just need to edit one-ingress object and remove the /banana path and team bar will start receiving traffic with 0 downtime.

but when someone in bar tries to apply the manifest it finds the following error

ingress.networking.k8s.io/one-ingress unchanged
Error from server (BadRequest): error when creating "manifest.yaml": admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: host "foo.bar.io" and path "/banana" is already defined in ingress foo/one-ingress

for most cases that might be a valid protection but when you want to achieve zero downtime traffic migrations between ingress objects is counterproductive.

This is the purpose of this PR give an opt-in flag that allows maintainers to bypass this and only this validation without disabling the rest of admission validations that are still useful.

cc @tao12345666333 @strongjz @rikatz

@Fsero Fsero force-pushed the allow_duplicates branch 2 times, most recently from 3685df7 to 08b08ec Compare October 7, 2024 14:11
  ## 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
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