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

support disabling admission-webhook for creating duplicates #10090

Closed
ailurarctos opened this issue Jun 16, 2023 · 11 comments · May be fixed by #10091 or #10943
Closed

support disabling admission-webhook for creating duplicates #10090

ailurarctos opened this issue Jun 16, 2023 · 11 comments · May be fixed by #10091 or #10943
Assignees
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@ailurarctos
Copy link

What happened:

I created the following three ingress resources in the listed order with a small wait in between each apply so that they each have a different creationTimestamp:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-1
spec:
  ingressClassName: nginx
  rules:
  - host: example
    http:
      paths:
      - path: /a
        pathType: Exact
        backend:
          service:
            name: service-1
            port:
              number: 80
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-2
spec:
  ingressClassName: nginx
  rules:
  - host: example
    http:
      paths:
      - path: /a
        pathType: Prefix
        backend:
          service:
            name: service-2
            port:
              number: 80
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-3
spec:
  ingressClassName: nginx
  rules:
  - host: example
    http:
      paths:
      - path: /a
        pathType: Prefix
        backend:
          service:
            name: service-3
            port:
              number: 80

This caused the ingress controller to generate an invalid NGINX configuration:

nginx: [emerg] duplicate location "/a/" in /tmp/nginx/nginx-cfg2287376726:693
nginx: configuration file /tmp/nginx/nginx-cfg2287376726 test failed

What you expected to happen:

I expected ingress-3 to be ignored as it is older than ingress-2 and has the same path and pathType. This is documented in https://kubernetes.github.io/ingress-nginx/how-it-works/#building-the-nginx-model:

  • If the same path for the same host is defined in more than one Ingress, the oldest rule wins.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

% kubectl -ningress-nginx exec deployment/ingress-nginx-controller -ccontroller -- /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.8.0
  Build:         35f5082ee7f211555aaff431d7c4423c17f8ce9e
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.21.6

-------------------------------------------------------------------------------

Kubernetes version (use kubectl version):

WARNING: This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.3", GitCommit:"25b4e43193bcda6c7328a6d147b1fb73a33f1598", GitTreeState:"clean", BuildDate:"2023-06-14T09:53:42Z", GoVersion:"go1.20.5", Compiler:"gc", Platform:"darwin/amd64"}
Kustomize Version: v5.0.1
Server Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.3", GitCommit:"25b4e43193bcda6c7328a6d147b1fb73a33f1598", GitTreeState:"clean", BuildDate:"2023-06-15T00:36:28Z", GoVersion:"go1.20.5", Compiler:"gc", Platform:"linux/amd64"}

Environment:

I created a local environment to reproduce this issue as follows:

kind create cluster --name=ingress-nginx
kind export kubeconfig --name=ingress-nginx
kubectl config use-context kind-ingress-nginx
helm \
  upgrade --install ingress-nginx ingress-nginx \
  --repo=https://kubernetes.github.io/ingress-nginx --namespace=ingress-nginx \
  --create-namespace --set=controller.admissionWebhooks.enabled=false

Note that the admission webhook is disabled as it does not allow for identical paths. Identical paths can be useful when doing a migration from one ingress to another.

Here is the kind version:

% kind version
kind v0.20.0 go1.20.4 darwin/amd64

Here is the helm version:

% helm version
version.BuildInfo{Version:"v3.12.1", GitCommit:"f32a527a060157990e2aa86bf45010dfb3cc8b8d", GitTreeState:"clean", GoVersion:"go1.20.4"}

How to reproduce this issue:

  1. The script in Environment describes how to create the kind cluster and install ingress-nginx.
  2. Create the ingress resources listed in What happened with a minimum 1-second wait between creating each to ensure they have a different timestamp.
  3. Get the logs from nginx-ingress (kubectl -ningress-nginx logs deployment/ingress-nginx-controller).

You will see it is generating an invalid NGINX config:

nginx: [emerg] duplicate location "/a/" in /tmp/nginx/nginx-cfg1037078429:693
nginx: configuration file /tmp/nginx/nginx-cfg1037078429 test failed

Anything else we need to know:

The issue is here. The code stops looking for duplicates once it sees a different path type with the same path.

@ailurarctos ailurarctos added the kind/bug Categorizes issue or PR as related to a bug. label Jun 16, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jun 16, 2023
@longwuyuan
Copy link
Contributor

/remove-kind bug

Did you disable/delete the admission webhook and then try to create duplicates ?

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jun 16, 2023
@longwuyuan
Copy link
Contributor

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Jun 17, 2023
@ailurarctos
Copy link
Author

Did you disable/delete the admission webhook and then try to create duplicates ?

Yes.

Declaring the same path in multiple ingresses should not cause the controller to generate an invalid NGINX configuration. The documentation for this case is here:

  • If the same path for the same host is defined in more than one Ingress, the oldest rule wins.

My understanding of the above statement is that if the same path for the same host is defined in more than one Ingress then the oldest rule wins, not that the ingress controller will generate an invalid NGINX configuration.

@github-actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 18, 2023
@netzulo
Copy link

netzulo commented Nov 26, 2023

got this error message using this image https://github.com/Valian/docker-nginx-auto-ssl/

nginx: [emerg] the same path name "/tmp" used in /etc/nginx/conf.d/_.conf

I don't know what kind of logs we need it to fix this ( supposing i'm with the same "problem" ).
Some kind soul can type for us a workaround ?

Fsero added a commit to Fsero/ingress-nginx that referenced this issue Jan 30, 2024
  ## 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

fixes kubernetes#10820
fixes kubernetes#10090

<!--- 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
ailurarctos added a commit to ailurarctos/ingress-nginx that referenced this issue Feb 23, 2024
When creating ingresses that use the same path with a mix of
Exact and Prefix path types nginx-ingress-controller can
generate an NGINX config with a duplicate location:

    nginx: [emerg] duplicate location "/a/" in /tmp/nginx/nginx-cfg2287376726:693
    nginx: configuration file /tmp/nginx/nginx-cfg2287376726 test failed

The issue is that nginx-ingress-controller stops looking for
duplicates once it sees the same path with a different path type.
@tao12345666333
Copy link
Member

Hi @ailurarctos sorry for the delay.

Could you please try to use the latest version to verify what will happen?

We have added the admission rules for this situation, the request will be rejected

(⎈|moe-aks:default)➜  ~ kubectl apply -f test-10090.yaml -n demo
ingress.networking.k8s.io/ingress-1 created
(⎈|moe-aks:default)➜  ~ kubectl -n demo apply -f test-10090-2.yaml 
Error from server (BadRequest): error when creating "test-10090-2.yaml": admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: host "example" and path "/a" is already defined in ingress demo/ingress-1

@tao12345666333 tao12345666333 self-assigned this Mar 30, 2024
@tao12345666333 tao12345666333 removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 30, 2024
@ailurarctos
Copy link
Author

ailurarctos commented Apr 28, 2024

Hi @tao12345666333, thanks for taking a look at this.

We have added the admission rules for this situation, the request will be rejected

This issue only applies when the admission webhook is disabled. I know on the surface it seems questionable to disable the admission webhook and then create an issue for something that only happens when it is disabled, but we have to disable the admission webhook to have zero-downtime path migrations between ingress resources. The admission webhook prevents zero-downtime path migrations (see #10820 for more info on this issue).

If #10943 is accepted it will be possible to do zero-downtime path migrations between ingress resources with the admission webhook enabled. This will improve the situation somewhat but I think this particular issue will remain as it is a problem with how the NGINX ingress controller generates the NGINX config when there are ingress resources with overlapping paths and different path types.

Fsero added a commit to Fsero/ingress-nginx that referenced this issue Sep 5, 2024
  ## 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
@longwuyuan
Copy link
Contributor

We don't have resource to allocate to support the use-case of disabling admission-webhook and creating duplicates. We are actually deprecating features that are useful and popular but can not be maintained and supported due to lack to resources.

This issue thus does not track any item item so we can not keep issues open if they do not track action item. All resources are dedicated to security and Gateway-API. I will close this issue for now. If developers commit to supporting disabling of admission-webhook in future, this may be re-opened,

/close

@k8s-ci-robot
Copy link
Contributor

@longwuyuan: Closing this issue.

In response to this:

We don't have resource to allocate to support the use-case of disabling admission-webhook and creating duplicates. We are actually deprecating features that are useful and popular but can not be maintained and supported due to lack to resources.

This issue thus does not track any item item so we can not keep issues open if they do not track action item. All resources are dedicated to security and Gateway-API. I will close this issue for now. If developers commit to supporting disabling of admission-webhook in future, this may be re-opened,

/close

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.

@longwuyuan
Copy link
Contributor

/retitle support disabling admission-webhook for creating duplicates

@k8s-ci-robot k8s-ci-robot changed the title Duplicate location in NGINX config when using both Prefix and Exact path types support disabling admission-webhook for creating duplicates Sep 13, 2024
Fsero added a commit to Fsero/ingress-nginx that referenced this issue Oct 4, 2024
  ## 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
Fsero added a commit to Fsero/ingress-nginx that referenced this issue Oct 7, 2024
  ## 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
Fsero added a commit to Fsero/ingress-nginx that referenced this issue Nov 26, 2024
  ## 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
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
5 participants