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

VirtualServer with conditional routes creates wrong rewrite rules #1993

Closed
andreaspe opened this issue Sep 21, 2021 · 2 comments · Fixed by #2267
Closed

VirtualServer with conditional routes creates wrong rewrite rules #1993

andreaspe opened this issue Sep 21, 2021 · 2 comments · Fixed by #2267
Labels
bug An issue reporting a potential bug
Milestone

Comments

@andreaspe
Copy link

andreaspe commented Sep 21, 2021

Describe the bug
When creating a VirtualServer with a route that contains a match condition and a rewritePath, it seems to create wrong rewrite rules. On the proxy destination i can observere request parameters duplicated.

To Reproduce

  1. Create the following VirtualServer
apiVersion: k8s.nginx.org/v1
kind: VirtualServer
metadata:
  name: myvserver
spec:
  host: myvserver.example.org
  server-snippets: |
    rewrite_log on;
  upstreams:
    - name: app
      service: myservice
      port: 8666
  routes:
    - path: /
      action:
        pass: app
    - path: /demo
      matches:
      - conditions:
        - cookie: user
          value: john
        action:
          proxy:
            upstream: app
            rewritePath: /user/john
      action:
        proxy:
          upstream: app
          rewritePath: /
  1. Do a curl request to the created vhost. It doesn't matter it if the conditions is matched or not.
$ curl http://myvserver.example.org/demo?hello=world
...
  1. Observe in the access log of app that the request is proxied in an unexpected way
10.142.184.178 - - [21/Sep/2021:16:21:49 +0000] "GET /%3Fhello=world?hello=world HTTP/1.1" 404 153 "-" "curl/7.64.1"

The location block in the generated config seems to be like this:

...
   location /internal_location_matches_0_default {
        set $service "myservice";
        internal;
        set $default_connection_header close;
        rewrite ^ $request_uri;
        rewrite "^/demo(.*)$" "/$1" break;
...

Expected behavior
The requests are proxied to the destination as /?hello=world.

Your environment

  • 1.12.1 (installed via nginx-ingress-0.0.0-edge helm chart)
  • 1.20.7
  • Azure Kubernetes (AKS)
  • Nginx + NginxPlus (on eval license)

Additional context
Without the matches: block the request is correctly passed to the upstream.

Aha! Link: https://nginx.aha.io/features/IC-280

@github-actions
Copy link

Hi @andreaspe thanks for reporting!

Be sure to check out the docs while you wait for a human to take a look at this 🙂

Cheers!

@pleshakov
Copy link
Contributor

Hi @andreaspe

I confirm that it is a bug: If a path includes splits or matches, the rewritten URI will include duplicated request parameters.

I don't see a workaround. However, if you remove the matches, the URI rewriting will work:

  - path: /test/
    action:
      proxy:
        upstream: coffee
        rewritePath: /
$ curl "cafe.example.com/test?hello=world"
Server address: 10.244.0.8:8080
Server name: coffee-6f4b79b975-dgvlv
Date: 21/Sep/2021:21:29:39 +0000
URI: /?hello=world
Request ID: c0db4910cf21ca7c49407256e8ea2aeb

@pleshakov pleshakov added the bug An issue reporting a potential bug label Sep 21, 2021
@brianehlert brianehlert added this to the Candidates milestone Sep 21, 2021
pleshakov added a commit that referenced this issue Dec 11, 2021
Previously, if a rewrite was configured in a VS/VSR route, and the route
had a split action, match action or a regex path, NGINX would mess up
the arguments in the resulting rewrite. For example, the result would be
/test%3Fhello=world?hello=world instead of the correct /test?hello=world

Fixes #1993
nginx-bot pushed a commit that referenced this issue Dec 13, 2021
Previously, if a rewrite was configured in a VS/VSR route, and the route
had a split action, match action or a regex path, NGINX would mess up
the arguments in the resulting rewrite. For example, the result would be
/test%3Fhello=world?hello=world instead of the correct /test?hello=world

Fixes #1993
nginx-bot pushed a commit that referenced this issue Dec 15, 2021
Previously, if a rewrite was configured in a VS/VSR route, and the route
had a split action, match action or a regex path, NGINX would mess up
the arguments in the resulting rewrite. For example, the result would be
/test%3Fhello=world?hello=world instead of the correct /test?hello=world

Fixes #1993
pleshakov added a commit that referenced this issue Dec 15, 2021
Previously, if a rewrite was configured in a VS/VSR route, and the route
had a split action, match action or a regex path, NGINX would mess up
the arguments in the resulting rewrite. For example, the result would be
/test%3Fhello=world?hello=world instead of the correct /test?hello=world

Fixes #1993
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants