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

Allow capture groups on permanent-redirect annotation #10734

Open
siegenthalerroger opened this issue Dec 7, 2023 · 7 comments
Open

Allow capture groups on permanent-redirect annotation #10734

siegenthalerroger opened this issue Dec 7, 2023 · 7 comments
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@siegenthalerroger
Copy link

What happened:

When attempting to activate annotation validation, we realised that the permanent-redirect annotation has been broken due to a lack of support for capture groups.

We are attempting to redirect to a new domain but keep the existing path. I expect this to work by having a nginx config something along these lines:

server {
    . . .
    server_name domain1.com;
    rewrite ^/(.*)$ http://domain2.com/$1 permanent;
    . . .
}

We've tried a few different variations of this, but can't find a documented way to achieve this, however the following should work based on my understanding:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test-redirect
  annotations: 
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/permanent-redirect: https://domain2\.com/$1
spec:
  rules:
  - host: domain1.com
    http:
      paths:
      - path: /(.*)
        pathType: ImplementationSpecific
        backend:
          ...

This gets denied by the admission controller as the permanent-redirect annotation contains an invalid word (or value if the . isn't escaped). Though even this leaves open the case of no path existing, which we'd also need redirected tbh.

What you expected to happen:

I'd expect capture groups and just in general regexes to work as intended for permanent-redirect.

I guess part of the issue lies in this line: Validator: parser.ValidateRegex(parser.URLIsValidRegex, false),, which isn't passing capture groups to the validator (https://github.com/kubernetes/ingress-nginx/blob/45f8262d052ee0da6133d886ec90f1fbd7a19e3b/internal/ingress/annotations/redirect/redirect.go#L64C29-L64C29 as comparted to redirect's https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/rewrite/main.go#L43C1-L43C1).

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

Kubernetes version (use kubectl version): 1.27.7

Environment: Azure AKS w/ FluxCD gitops deployment

  • Cloud provider or hardware configuration: Azure

  • How was the ingress-nginx-controller installed:

    • FluxCD Helm-Controller
    • Chroot image
    • strict-validate-path-types: true
    • enableAnnotationValidations: true

How to reproduce this issue:

  • Install the ingress controller with annotation validation active
  • Create an ingress with an example domain1 -> domain2 redirect like above
@siegenthalerroger siegenthalerroger added the kind/bug Categorizes issue or PR as related to a bug. label Dec 7, 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 Dec 7, 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.

@longwuyuan
Copy link
Contributor

/remove-kind bug

Hi Can you kindly copy/paste 2 examples that clearly illustrate the redirect behaviour expected. Am curious about the curl command you would issue and the ingress rules that would match that HTTP/HTTPS request, and the resulting rewritten URL and/or the path.

@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 Dec 9, 2023
@siegenthalerroger
Copy link
Author

Honestly all we need is a way to have a permanent redirect from one domain to another, that preserves the path.

$ curl --head https://domain1.com/
HTTP/1.1 302 Moved Permanently
Location: https://domain2.com/
...
$ curl --head https://domain2.com/bar
HTTP/1.1 302 Moved Permanently
Location: https://domain2.com/foo/bar
...
$ curl --head https://domain1.com/foo/bar
HTTP/1.1 302 Moved Permanently
Location: https://domain2.com/foo/bar
...

*note this isn't an actual output, but my understanding of how it would look if it worked.

Currently we can only achieve the first and second example (permanent-redirect respectively rewrite-target & use-regex) but the last one we can't figure out how to configure.

@longwuyuan
Copy link
Contributor

Hi,
The third example above works
image

I think what does not work is the use of regex in redirect annotation.

I am not sure if vanilla non-kubernetes nginx reverseproxy accepts regex in redirect direcives. Someone who knows that well has to comment.

@siegenthalerroger
Copy link
Author

Hi I'm sorry, I messed up the last example it should be the following, combining the redirect and rewrite.

$ curl --head https://domain1.com/bar
HTTP/1.1 302 Moved Permanently
Location: https://domain2.com/foo/bar
...

I believe to implement this you'd have to do what you mentioned, where you aren't sure if it would even be possible with a standalone nginx reverse proxy. I'll have that tested and get back to you.

@LevN0
Copy link

LevN0 commented Feb 19, 2024

Seconding the request here. Currently the rewrite-target functionality for rewriting in the backend is much more powerful than the permanent-redirect and temporal-redirect features for frontend rewrite/redirect.

@vakaobr
Copy link

vakaobr commented Apr 2, 2024

Looks somewhat similar to #9435

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 needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

No branches or pull requests

5 participants