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

Annotation validation breaks x-forwarded-prefix annotation #10597

Closed
bodgit opened this issue Nov 1, 2023 · 3 comments · Fixed by #10598
Closed

Annotation validation breaks x-forwarded-prefix annotation #10597

bodgit opened this issue Nov 1, 2023 · 3 comments · Fixed by #10598
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@bodgit
Copy link
Contributor

bodgit commented Nov 1, 2023

What happened:

When we enable annotation validation our Ingress resources fail due to this annotation:

nginx.ingress.kubernetes.io/x-forwarded-prefix: /$1
error: ingresses.networking.k8s.io "test-ingress" could not be patched: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation nginx.ingress.kubernetes.io/x-forwarded-prefix contains invalid value

What you expected to happen:

No validation errors.

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


NGINX Ingress controller
Release: v1.9.4
Build: 846d251
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.21.6


Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.7", GitCommit:"07a61d861519c45ef5c89bc22dda289328f29343", GitTreeState:"clean", BuildDate:"2023-10-18T11:42:32Z", GoVersion:"go1.20.10", Compiler:"gc", Platform:"linux/amd64"}
Kustomize Version: v5.0.1
Server Version: version.Info{Major:"1", Minor:"27+", GitVersion:"v1.27.6-eks-f8587cb", GitCommit:"b6911bf9eade7d8ca7dd82af5e80626965829947", GitTreeState:"clean", BuildDate:"2023-09-18T22:12:19Z", GoVersion:"go1.20.8", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Cloud provider or hardware configuration: EKS

  • How was the ingress-nginx-controller installed: ArgoCD/Helm

How to reproduce this issue:

With annotation validation enabled, try adding the following annotation to an Ingress:

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

Anything else we need to know:

Our Ingress resources often tend to have these annotations in tandem:

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

The rewrite-target annotation is accepted, but not the x-forwarded-prefix. Comparing:

rewriteTargetAnnotation: {
Validator: parser.ValidateRegex(parser.RegexPathWithCapture, false),
Scope: parser.AnnotationScopeIngress,
Risk: parser.AnnotationRiskMedium,
Documentation: `This annotation allows to specify the target URI where the traffic must be redirected. It can contain regular characters and captured
groups specified as '$1', '$2', etc.`,
},

xForwardedForPrefixAnnotation: {
Validator: parser.ValidateRegex(parser.BasicCharsRegex, true),
Scope: parser.AnnotationScopeLocation,
Risk: parser.AnnotationRiskLow, // Low, as it allows regexes but on a very limited set
Documentation: `This annotation can be used to add the non-standard X-Forwarded-Prefix header to the upstream request with a string value`,
},

Using parser.RegexPathWithCapture instead of parser.BasicCharsRegex for x-forwarded-prefix would probably fix it.

@bodgit bodgit added the kind/bug Categorizes issue or PR as related to a bug. label Nov 1, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 1, 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.

@rikatz
Copy link
Contributor

rikatz commented Nov 1, 2023

Hum, can you send a PR with the change and some unit tests so we can quickly check and accept it?

Otherwise I will be able to check this just after Kubecon

Thanks!

@bodgit
Copy link
Contributor Author

bodgit commented Nov 1, 2023

@rikatz PR #10598 raised

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants