rewrite-target annotation validation does not allow = and ? characters

[melnikovn]

What happened:

Annotation validation rule is way too strict for rewrite-target not allowing characters like = and ? that are valid url characters:

W0221 10:03:30.532045       7 validators.go:221] validation error on ingress env-test/test: annotation rewrite-target contains invalid value /$1?=$2

The annotation works fine when not validated, so I guess it should also pass the test.

This is relevant for current master, I've checked the rules there:
https://github.com/sauterp/ingress-nginx/blob/main/internal/ingress/annotations/rewrite/main.go#L43
https://github.com/sauterp/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L74

but tested on v1.9.4.

NGINX Ingress controller version: v1.9.4

Kubernetes version: v1.26.12-eks-5e0fdde

How to reproduce this issue:

Ingress object with the following annotation fails:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /$1?x=$2
    nginx.ingress.kubernetes.io/use-regex: "true"
  name: test
spec:
  ingressClassName: nginx
  rules:
  - host: test.com
    http:
      paths:
      - backend:
          service:
            name: service-name
            port:
              number: 8080
        path: /test/(test)/(.*)$
        pathType: Prefix

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

[longwuyuan]

/remove-kind bug

I suspect you have to go simpler on the target as its coming coming from a regexpgroup. If you begin with a regexpgroup as input and then go on to use more regexp, then it may be desired but not practical for the vast majority of users or for practical for implementing regexp in the derived value of rewrite-target

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

[whale2-spread]

/remove-kind bug

I suspect you have to go simpler on the target as its coming coming from a regexpgroup. If you begin with a regexpgroup as input and then go on to use more regexp, then it may be desired but not practical for the vast majority of users or for practical for implementing regexp in the derived value of rewrite-target

I'm sorry, but it's really hard to make sense of the above. Are you suggesting there is another way to implement such a rewrite? Rewriting part of the path as a parameter seems to be an obvious thing people might want to do.

[longwuyuan]

Your expectation is fair. Not contending them.

I think I was trying to state that in my opinion, rewrite is not a K8S KEP Ingress API feature.
Rewrite directive is inherent obviously in Nginx.

Since it helps so much, rewrite has been implemented in this controller.
But if the implementation is a threat resulting from symbols/characters misuse in value, then either the validations have to be written afresh, only and only for the rewrite annotation (or other such annotations where its fair to expect these characters/symbols). Or those characters/symbols have to be disallowed from being misused into increasing risk.

[longwuyuan]

@whale2-spread we discussed this in today's community meeting. @Gacko has merged a PR to allow characters in the v1.10. and v1.11.x of the controller.

Can you please try with the recent releases and update behaviour.

[Gacko]

This change has not been released, yet, and it only added , to the allowed characters for URLs. Apart from that the mentioned characters should be allowed for URLs in more recent version.

So altogether I'd ask you to re-produce this issue on a more recent controller version, at best v1.11.x, as we are not supporting v1.9.x anymore.

[klaernie]

I can reproduce this issue in v1.12.0 - sadly I did so unknowingly by upgrading.

We use an ingress to make an internal influxdb available to our users, giving them access to the /api/v2/query endpoint, setting the required query parameter org in the process, and replacing the incoming Basic authentication with an API token for the influxdb (giving them the possibility to access multiple endpoints with the same credentials, not just the influxdb).

This is our ingress resource, stripped to only the ingress-nginx annotations needed:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: influxdb-proxy-site1
  namespace: influxdb-proxy
  annotations:
    nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
    nginx.ingress.kubernetes.io/force-ssl-redirect: "true"
    nginx.ingress.kubernetes.io/proxy-ssl-server-name: "on"
    # Replace the default SNI hostname (service hostname used in proxy_pass) with the hostname of the InfluxDB backend.
    # Otherwise this will return 502 Bad Gateway because the backend cannot provide the correct server certificate.
    nginx.ingress.kubernetes.io/proxy-ssl-name: "internal-influxdb.fqdn"
    # Replace the default host header with the hostname of the backend cluster.
    # Otherwise the backend will return 421 Misdirected Request because the negotiated hostname will not match the request hostname.
    nginx.ingress.kubernetes.io/upstream-vhost: "internal-influxdb.fqdn"
    nginx.ingress.kubernetes.io/proxy-ssl-secret: ingress-nginx/internal-ca
    nginx.ingress.kubernetes.io/proxy-ssl-verify: "on"
    nginx.ingress.kubernetes.io/proxy-ssl-verify-depth: "2"
    # Basic authentication for technical users
    nginx.ingress.kubernetes.io/auth-type: basic
    nginx.ingress.kubernetes.io/auth-secret: htpasswd
    nginx.ingress.kubernetes.io/auth-realm: "Authentication Required - Wrapped InfluxDB"
    # Rewrite the path with static elements, so that end-users don't have to care about them.
    nginx.ingress.kubernetes.io/rewrite-target: /api/v2/query?org=Our%20Org
    # Write the complete proxy_set_header directive into a secret, and use the include directive to add it from a file.
    # the contents of this file look like this:
    #     proxy_set_header Authorization "Token XXXXX";
    nginx.ingress.kubernetes.io/configuration-snippet: |
      include /etc/nginx/secrets/influxdb-proxy-token-1;
spec:
  ingressClassName: nginx
  tls:
    - hosts:
        - external-influxdb-hostname.fqdn
      secretName: external-influxdb-hostname-cert
  rules:
    - host: external-influxdb-hostname.fqdn
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: externalName-service-for-internal-influxdb
                port:
                  number: 443

The logs when starting an ingress-controller processing this ingress looks like this, when annotation validation is enabled:

W0203 15:35:05.866262 7 validators.go:237] validation error on ingress influxdb-proxy/influxdb-proxy-site1: annotation rewrite-target contains invalid value /api/v2/query?org=Our%20Org
W0203 15:35:05.866384 7 main.go:148] rewrite-targetis invalid, defaulting to empty
I0203 15:35:05.932615 7 admission.go:149] processed ingress via admission controller {testedIngressLength:8 testedIngressTime:0.066s renderingIngressLength:8 renderingIngressTime:0.001s admissionTime:0.067s testedConfigurationSize:110.7kB}
I0203 15:35:05.932655 7 main.go:107] "successfully validated configuration, accepting" ingress="influxdb-proxy/influxdb-proxy-site1"

When disabling annotation validation our ingress is accepted and configured as expected, and traffic from our customers starts to work. With enabled annotation validation the rewrite-target is cleared and hence the requests are not rewritten, breaking customer traffic.

I hope this reports helps you understand how we use ingress-nginx to wrap an internal API without running an additional manually configured nginx instance.