rewrite-target with new 0.22 syntax sets x-forwarded-prefix header with unexpected capture group suffix

[Beennnn]

When using the new configuration for x-forwarded-prefix with regex filter, the capture group filter is included at the end of the x-fowarded-prefix.

Let's consider the following configuration

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /$1
    nginx.ingress.kubernetes.io/ssl-redirect: "false"
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/x-forwarded-prefix: "true"
  labels:
  name: my-service
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: my-service
          servicePort: 8080
        path: /my-service/?(.*)

The value of the x-forwared-prefix header sent to my-service is /my-service/?(.*) including the capture group, instead of the expected value: /my-service/. In fact, the prefix shall not include the capturing group.

This mechanism worked correctly for the pre-0.22 syntax version. I think there is juste one additional step to fix in the code to remove the capturing group in the x-forwarded-prefix header value.

More understanding : Why do I need this ?

_This value is traditionally used to rebuild the original url using automated url builder as the one provided by Spring Boot here : https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java#L349

This is very useful to generate REST hypermedia hyperlinks. With the extra capturing group, the original prefix cannot be automatically built._

Could you please help me to find where and how to fix this ?
Thanks.

[Beennnn]

Maybe the fix is to be made on this line : https://github.com/Shopify/ingress/blob/master/internal/ingress/controller/template/template.go#L460

[ElvinEfendi]

Yes @Beennnn , patch is welcome :) As you rightfully linked, you can normalize the path there before generating the Nginx directive to make sure path does not include capture group

[Beennnn]

Thanks @ElvinEfendi !
But I'm not "go-aware"... If somebody knows the exact syntax I would greatly appreciate the help !

[alexkursell]

What should be the behaviour of X-Forwarded-Prefix for more complicated rewrite rules like

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /$1/$2
    nginx.ingress.kubernetes.io/ssl-redirect: "false"
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/x-forwarded-prefix: "true"
  labels:
  name: my-service
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: my-service
          servicePort: 8080
        path: /my-service/(.*)/other/(.*)

or even

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /$1
    nginx.ingress.kubernetes.io/ssl-redirect: "false"
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/x-forwarded-prefix: "true"
  labels:
  name: my-service
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: my-service
          servicePort: 8080
        path: /my-service/(.*)/other

maybe we should detect the simple case and do as you suggested, and then just leave out X-Forwarded-Prefix in the more complicated ones.

[Beennnn]

The behaviour of X-Forwarded-Prefix for these use cases does not seem to be normalized.
I suggest leaving it empty.

[alexkursell]

I've been giving this some more thought, and I think that the best thing to do would just be to require the nginx.ingress.kubernetes.io/x-forwarded-prefix annotation value to just be set to the desired value of the header. Trying to be clever with detecting and stripping capture groups seems like a bunch of unnecessary complexity, and probably devolves into literally parsing the entire golang regex grammar in order to do it right. The downside to this is if you have 2 paths that you want to have different x-forwarded-prefixes for, you need to put them in separate ingress definitions.

I'd be interested if anybody else has a better idea about how to implement this without falling down a rabbit hole, but in the meantime I'll work up a PR with just the simple fix.

Oddly enough, according to the docs, that's what the annotation is already supposed to be doing. I guess it got changed and nobody ever updated it.

[Beennnn]

I agree with this approach.

Notice that lots of people already noticed the docs does not match the code. See #3670 and #3132
Other articles also refer to the value "true":

• https://www.bountysource.com/issues/58286913-spring-cloud-gateway-doesn-t-add-x-forwarded-prefix-header
• https://www.bountysource.com/issues/35595511-handle-context-path-on-x-forwarded-prefix-support

Even if the value type will now match the docs, I think it should be clearly identified a breaking change.

[Sudharma]

great to see this thing merged but do not see the changes part of 0.24.0. When can we expect this to be released? Any planned dates or versions?

[alexkursell]

@Sudharma this change was included in 0.24.0. Looks like the PR wasn't referenced in the changelog.

[aledbf]

@Sudharma @alexkursell the reason is the PR has no project

[aledbf]

Fixed