Fix rewrite path for ReplacePrefixMatch to parse request arguments correctly #2951

salonichf5 · 2024-12-28T18:56:39Z

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: Users want query params to be parsed correctly when using ReplacePrefixMatch.

Solution: Updates the regex to parse query params to be parsed separately using the $args variable.

I updated the regex to capture everything from the matching variable to a ? found (if present) and then use nginx variable $args to append the request arguments. This avoids processing the entire variable after the prefix via nginx, leading to encoded characters in the resulting path.

Regex update explanation

^%s(.*)$ --> ^%s([^?]*)? - Captures the path till a ? is found. Using ? at the end makes the capture group optional
%s$1 --> %s$1?$args? - $1 indicates the path captured above till a ? is found. Concatenating the path captured + ? + args associated with the request. The ? at the end, tells Nginx to not double append args.

Updated documentation in Redirects and Rewrite example to showcase expected behavior when using query parameters when using ReplaceFullPath and ReplacePrefixMatch for users.

Testing:

Updated existing unit tests
Manual testing with Redirects and Redirects example with HTTPRoute slightly modified by adding method GET

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: coffee
spec:
  parentRefs:
  - name: cafe
    sectionName: http
  hostnames:
  - "cafe.example.com"
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /latte
      method: GET
    filters:
    - type: URLRewrite
      urlRewrite:
        path:
          type: ReplacePrefixMatch
          replacePrefixMatch: /
    backendRefs:
    - name: coffee
      port: 80
EOF

Results

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte/prices
Handling connection for 8080
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:20:46:15 +0000
URI: /prices
Request ID: 235d5fb72bf8c4be343926d27da0c996

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte
Handling connection for 8080
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:20:46:48 +0000
URI: /
Request ID: a7f8c33333984f922090f9ae672db40f

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte/prices\?test\=v1\&test\=v2
Handling connection for 8080
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:20:47:11 +0000
URI: /prices?test=v1&test=v2
Request ID: b21f0fd7005760a338eb99dfd6595af1

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/
Handling connection for 8080
<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>nginx</center>
</body>
</html>

Testing with the POST method

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: coffee
spec:
  parentRefs:
  - name: cafe
    sectionName: http
  hostnames:
  - "cafe.example.com"
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /latte
      method: POST
    filters:
    - type: URLRewrite
      urlRewrite:
        path:
          type: ReplacePrefixMatch
          replacePrefixMatch: /
    backendRefs:
    - name: coffee
      port: 80
EOF

Results

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte\?test\=v1 -X POST
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:18:39:38 +0000
URI: /?test=v1
Request ID: e2231bc3899fa29e1ce1e345c40cfa9b

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte/prices\?test\=v1 -X POST
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:18:40:27 +0000
URI: /prices?test=v1
Request ID: 72068bd237709ec7d4a7a02558cb461a

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/latte/ -X POST                
Server address: 10.244.0.235:8080
Server name: coffee-6db967495b-twn6x
Date: 23/Dec/2024:18:40:43 +0000
URI: /
Request ID: bcfb4194707d4c38f35e4e68987745fe

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #2862

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Fixes rewrite path for ReplacePrefixMatch to parse request arguments correctly.

github-actions · 2024-12-28T18:58:00Z

✅ Deploy Preview will be available once build job completes!

Name	Link
😎 Deploy Preview	https://frontdoor-test-docs.nginx.com/previews/nginx-gateway-fabric/2951/

kate-osborn

LGTM

codecov · 2025-01-02T18:28:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.93%. Comparing base (3fd9912) to head (22d7eef).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   89.93%   89.93%           
=======================================
  Files         111      111           
  Lines       11421    11423    +2     
  Branches       50       50           
=======================================
+ Hits        10271    10273    +2     
  Misses       1089     1089           
  Partials       61       61

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

salonichf5 requested review from a team as code owners December 28, 2024 18:56

github-actions bot added bug Something isn't working documentation Improvements or additions to documentation labels Dec 28, 2024

sjberman approved these changes Dec 30, 2024

View reviewed changes

kate-osborn approved these changes Jan 2, 2025

View reviewed changes

salonichf5 enabled auto-merge (squash) January 2, 2025 17:30

fix rewrite path for prefix match

22d7eef

salonichf5 force-pushed the bug/url-rewrite branch from 0befa29 to 22d7eef Compare January 2, 2025 18:26

salonichf5 merged commit 3683b58 into main Jan 2, 2025
43 checks passed

salonichf5 deleted the bug/url-rewrite branch January 2, 2025 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rewrite path for ReplacePrefixMatch to parse request arguments correctly #2951

Fix rewrite path for ReplacePrefixMatch to parse request arguments correctly #2951

salonichf5 commented Dec 28, 2024 •

edited

Loading

github-actions bot commented Dec 28, 2024

kate-osborn left a comment

codecov bot commented Jan 2, 2025 •

edited

Loading

Fix rewrite path for ReplacePrefixMatch to parse request arguments correctly #2951

Fix rewrite path for ReplacePrefixMatch to parse request arguments correctly #2951

Conversation

salonichf5 commented Dec 28, 2024 • edited Loading

Proposed changes

Checklist

Release notes

github-actions bot commented Dec 28, 2024

✅ Deploy Preview will be available once build job completes!

kate-osborn left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

salonichf5 commented Dec 28, 2024 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading