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.

Sign up for GitHub

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

Jump to bottom

Admission Webhook fined-grained request filtering (matchConditions) #11628

Closed
maximumG opened this issue Jul 17, 2024 · 6 comments
Closed

Admission Webhook fined-grained request filtering (matchConditions) #11628

maximumG opened this issue Jul 17, 2024 · 6 comments
Assignees
@Gacko
Labels
area/helm Issues or PRs related to helm charts kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@maximumG
Copy link

What do you want to happen?

When running multiple instances of ingress-nginx with different IngressClass (e.g : external, internal) in the same cluster, the admission webhook configuration cannot currently condition calling one or the other webhook based on the IngressClass.

One workaround today is to use the webhook ObjectSelector condition (see here) and to label Ingresses accordingly (external / internal).

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: ingress-nginx-external-admission
[...]
webhooks:
  - objectSelector:
      my-custom-label: external # or internal

Since Kubernetes v1.30, admission webhook can leverage the matchConditions statement (see here). This new feature allows for fine-grained request filtering based on the IngressClassName field

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: ingress-nginx-external-admission
[...]
webhooks:
  - objectSelector: {} # not using objectSelector anymore
    matchConditions:
      - expression: object.spec.ingressClassName == "external" # or internal

ℹ️ It would be interesting to add support for matchCondition in the ingress-nginx helm chart

Is there currently another issue associated with this?

N/A

Does it require a particular kubernetes version?

Kubernetes v1.30 in stable (beta since v1.28)
@maximumG maximumG added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 17, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 17, 2024
@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-sigs/prow repository.

@longwuyuan
Copy link
Contributor

Your issue description does not provide any info that has basic relevance.

This link https://kubernetes.github.io/ingress-nginx/faq/#how-can-i-easily-install-multiple-instances-of-the-ingress-nginx-controller-in-the-same-cluster suggests how to use multiple instances of the controller so any problems on that is likely going to cause multiple reports about using more than one controller in same cluster.

You can answer all the questions asked in the template of a new bug report by editing this issue description.

If you actually copy/paste data from a kind cluster that is a proof of

annot currently condition calling one or the other webhook based on the IngressClass.

because i can install 2 instances of the controller on one single kind cluster and not reproduce the problem you state

@strongjz
Copy link
Member

/area helm
/assign @Gacko

@k8s-ci-robot k8s-ci-robot added the area/helm Issues or PRs related to helm charts label Jul 18, 2024
@github-actions 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 Aug 18, 2024
@Gacko
Copy link
Member

Gacko commented Aug 21, 2024

Hey,

one issue here might be, that your suggestion and the possibilities opposed by matchConditions do not comply to the Ingress API. Ingress Controllers must lookup IngressClass resources by their controller value. Only Ingress resources for which the controller found IngressClass resources may be reconciled.

If someone would now manually add IngressClass resources, which is totally allowed, they could be taken into account by the Ingress Controller via their controller value while Ingress resources assigned to them might be ignored by the webhook. Also there's no need to actually use the chart. One can also just use the controller image.

The current implementation of simply skipping non-matching Ingress resources in the controller might not be perfect, but it probably also isn't for other webhooks in a cluster and at least ensures all assigned IngressClass resources are always being considered.

After all this is more of an issue of the Ingress API that hasn't changed in years and will be superseded by Gateway API. I therefore ask for your understanding and will close this issue now.

Regards
Marco

@Gacko Gacko closed this as completed Aug 21, 2024
@maximumG
Copy link
Author

Thanks @Gacko for your reply. Your answer totally make sense when I read it twice 😃 . Many thanks for looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gacko Gacko

Labels
area/helm Issues or PRs related to helm charts kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
[SIG Network] Ingress NGINX
Status: Done
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Gacko @strongjz @maximumG @longwuyuan @k8s-ci-robot