Annotation validation should be performed by validating admission controller

[rouke-broersma]

What happened:

Annotation nginx.ingress.kubernetes.io/backend-protocol with value https is allowed by the admission controller but invalidated by the ingress controller resulting in the annotation being ignored and http being used to reach the backend.

What you expected to happen:

The admission controller should validate the annotations before allowing them through to the ingress controller.

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

NGINX Ingress controller Release: v1.9.0 Build: 4bd3d6b8a00b01b009f225a5593ce502cce5c26b Repository: https://github.com/kubernetes/ingress-nginx nginx version: nginx/1.21.6

Kubernetes version (use kubectl version):

1.27

Environment:

• Cloud provider or hardware configuration: Azure Kubernetes

• OS (e.g. from /etc/os-release): Azure Linux

• How was the ingress-nginx-controller installed:

Helm with optional validating admission webhook enabled.

How to reproduce this issue:

Deploy ingress-nginx with optional validating admission webhook. Create an ingress with invalid annotations such as nginx.ingress.kubernetes.io/backend-protocol: https. Observe that ingress is not denied by admission controller.

[k8s-ci-robot]

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[longwuyuan]

/remove-kind bug

I think you need to answer more questions, from the new issue template, that contains answers as commands and outputs asked in a new issue template because there is no detailed info here on the ingress resource and the curl request. Maybe you need to try upper-case HTTPS even if you tested with lower-case.

[rouke-broersma]

/remove-kind bug

I think you need to answer more questions, from the new issue template, that contains answers as commands and outputs asked in a new issue template because there is no detailed info here on the ingress resource and the curl request. Maybe you need to try upper-case HTTPS even if you tested with lower-case.

A new feature 'annotation validation' was added in the last version (1.9) which validates annotation values to be valid. Ingress-nginx also provides a validating admission webhook to make sure ingresses are valid before they're processed by the ingress controller. The validating admission webhook does not include these annotation validations (at least not fully) which causes invalid ingresses to reach the ingress controller at which point the invalid configuration is 'silently' discarded by the annotation validation. Exactly what the admission webhook is meant to prevent.

Imo it's a bug to allow the invalid configuration through in the validating admission webhook. You may disagree and classify this as feature request instead of a bug I guess.

I presented reproduction steps, I assumed that would be sufficient in the case.

Let me know what other info is necessary to clarify the issue.

[strongjz]

The annotations are not part of the ingress spec, which is the webhook is looking at, the annotations are more ingress-nginx implementation. We already also have issues with the webhook timing out for large ingress objects. I'm not convinced it's a bug. I'll be interested to hear @rikatz and @tao12345666333 thoughts as well.

[rikatz]

I think it makes sense to validate the annotations earlier as possible.

We do some worst validation cases on webhooks like nginx.conf creation which IMO are much heavy than regex parsing. I need some time to think how to do it. This is a feature indeed, but IMO a missing one.

Also I think we should revisit our webhooks implementation. Back on time, we discussed on having a light webhook flavor, without the whole sync validation of configs and other stuff, maybe this is a good oportunity to think on it

[rouke-broersma]

The annotations are not part of the ingress spec, which is the webhook is looking at, the annotations are more ingress-nginx implementation. We already also have issues with the webhook timing out for large ingress objects. I'm not convinced it's a bug. I'll be interested to hear @rikatz and @tao12345666333 thoughts as well.

Fair, I think that could be listed in the docs in that case. The docs currently literally mention checking the annotations: https://kubernetes.github.io/ingress-nginx/how-it-works/#avoiding-outage-from-wrong-configuration

[longwuyuan]

/kind feature

[github-actions[bot]]

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.

[NeonSludge]

I want to +1 the idea that annotation validation should be performed during Ingress admission control.

Here is a case where current behavior can lead to an unexpected outage that can sometimes be tricky to investigate:

1. There are a lot of Ingresses in a cluster, they get created/updated many times each day.
2. Someone misses a comma in the whitelist-source-range annotation value.
3. Admission validation passes without any issues, the controller logs a "successfully validated configuration, accepting" message.
4. Controller successfully reloads its configuration.
5. Several log lines later the controller rightfully starts complaining about the invalid annotation ("error reading Ingress annotation")
6. But now there is suddenly an outage because some locations in Nginx config contain a return 503 directive with a comment like "Location denied. Reason: "annotation nginx.ingress.kubernetes.io/whitelist-source-range contains invalid value".

It's understandable why it works this way right now but it can certainly be an unwelcome surprise to someone who is not familiar with what ingress-nginx admission webhook's guarantees are exactly.