ingress-nginx icon indicating copy to clipboard operation
ingress-nginx copied to clipboard

Allow capture groups on permanent-redirect annotation

Open siegenthalerroger opened this issue 1 year ago • 7 comments

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 avatar Dec 07 '23 09:12 siegenthalerroger

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.

k8s-ci-robot avatar Dec 07 '23 09:12 k8s-ci-robot

/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.

longwuyuan avatar Dec 09 '23 00:12 longwuyuan

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.

siegenthalerroger avatar Dec 14 '23 15:12 siegenthalerroger

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.

longwuyuan avatar Dec 15 '23 03:12 longwuyuan

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.

siegenthalerroger avatar Jan 16 '24 15:01 siegenthalerroger

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.

LevN0 avatar Feb 19 '24 17:02 LevN0

Looks somewhat similar to https://github.com/kubernetes/ingress-nginx/issues/9435

vakaobr avatar Apr 02 '24 08:04 vakaobr