regex annotation validator does not accept some regex characters

[sauterp]

Hi,

What happened:

I'm running v1.9.3 and enabled annotation validation which results in some errors of the form W1026 08:49:04.900855 7 validators.go:221] validation error on ingress argocd/argocd: annotation auth-tls-match-cn contains invalid value CN=(my\.common\.name) E1026 08:49:04.900969 7 annotations.go:189] "ingress contains invalid annotation value" err="annotation nginx.ingress.kubernetes.io/auth-tls-match-cn contains invalid value"

What you expected to happen:

Since I provided a correct regex, I expected no error.

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

-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.9.3
  Build:         be93503b57a0ba2ea2e0631031541ca07515913a
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.21.6

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

Kubernetes version (use kubectl version):

v1.28.2

Anything else we need to know:

Why this happens is quite clear, when you validate the supplied regex for the common name you are not accepting all the valid regex characters:
https://github.com/kubernetes/ingress-nginx/pull/9673/files#diff-c698c1176d5e4ebe3b9702c2d0487ae52c28bd499fde57cf6cbbf414021f3e25R45-R48

Is there a reason for this?
How about we just validate whether the regex can be parsed with the Go standard library regexp package, instead of checking a list of characters?

[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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[strongjz]

It would be very helpful if you provided the regex that you think should be working.

[sauterp]

Sorry if I was unclear.

The regex that should be working is (my\.common\.name), this is a valid regex and it is also matches "my.common.name" which is a valid SSL common name(https://support.dnsimple.com/articles/what-is-common-name/).

The current validator implementation doesn't really validate whether the annotation contains a regex, it only accepts a subset of all possible regexes. In my example, it doesn't accept the backslash.
I wrote an example that illustrates this here: https://go.dev/play/p/0vaLLLdXoiz

Here is how I would implement the validator: https://go.dev/play/p/27FODZudJhh

Instead of trying to create a regex that validates whether the string is a regex(I actually don't know if this is possible to do correctly), we should IMO just compile the string as a regex and pass it as valid if it compiles.

[strongjz]

thanks for more info, i was on mobile looking at issues, I see that it was buried in the logs.

That does seem like it should; we can look into the annotations there. 1.9.0, we turned on validation by default...i think there are so many switches. Let me look it up.

[strongjz]

The Examples are super helpful too.

[sauterp]

Thanks, happy to help :)

[baileydoestech]

We are also seeing some failures since upgrading to 1.9.3 and enabling validation on annotations, in our example this is the culprit (running on EKS 1.27.6).

nginx.ingress.kubernetes.io/x-forwarded-prefix: /$1

Full details of ours in a separate issue: #10597

[longwuyuan]

/remove-kind bug

@sauterp thanks.

Even though you posted a link to the code bits, i was wondering if you can you explicitly list what are the valid characters that should be accepted and the actual list of characters the the ingress-nginx controller accepts.

[sauterp]

/remove-kind bug

@sauterp thanks.

Even though you posted a link to the code bits, i was wondering if you can you explicitly list what are the valid characters that should be accepted and the actual list of characters the the ingress-nginx controller accepts.

Hi,
thanks for considering this issue. I just realized that the title I gave the issue can be misunderstood, because it talks about regex characters. The problem is actually not individual characters. If I gave you a list of characters and you add them to the list that the current validator uses, that would actually not solve the problem correctly. Even if I gave you the entire list of all characters that can appear in a valid regex, the current validator still wouldn't correctly validate "regex", because regex is a kind of language with a syntax if you will.

The code snippets I made show how to validate a regex by actually compiling the given string as a regex and checking if there is an error. This functionality is provided by the Go standard library and the computational complexity of the operation is linear with respect to the input AFAIK. So IMHO there is no reason to do it any other way.

[longwuyuan]

cc @rikatz @tao12345666333

[github-actions]

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.