Fix URI rewrite in VirtualServers and VirtualServerRoutes

internal/configs/version1/nginx-plus.tmpl

@@ -89,6 +89,10 @@ http {
     variables_hash_bucket_size {{.VariablesHashBucketSize}};
     variables_hash_max_size {{.VariablesHashMaxSize}};
 
+    map $request_uri $request_uri_no_args {
+        "~^(?P<path>[^?]*)(\?.*)?$" $path;
+    }
+
     map $http_upgrade $connection_upgrade {
         default upgrade;
         ''      close;

internal/configs/version1/nginx.tmpl

@@ -74,6 +74,10 @@ http {
     variables_hash_bucket_size {{.VariablesHashBucketSize}};
     variables_hash_max_size {{.VariablesHashMaxSize}};
 
+    map $request_uri $request_uri_no_args {
+        "~^(?P<path>[^?]*)(\?.*)?$" $path;
+    }
+
     map $http_upgrade $connection_upgrade {
         default upgrade;
         ''      close;

internal/configs/virtualserver.go

@@ -1395,8 +1395,11 @@ func generateRewrites(path string, proxy *conf_v1.ActionProxy, internal bool, or
 	var rewrites []string
 
 	if internal {
-		// For internal locations (splits locations) only, recover the original request_uri.
-		rewrites = append(rewrites, "^ $request_uri")
+		// For internal locations only, recover the original request_uri without (!) the arguments.
+		// This is necessary, because if we just use $request_uri (which includes the arguments),
+		// the rewrite that follows will result in an URI with duplicated arguments:
+		// for example, /test%3Fhello=world?hello=world instead of /test?hello=world
+		rewrites = append(rewrites, "^ $request_uri_no_args")
 	}
 
 	if isRegex {

internal/configs/virtualserver_test.go

@@ -5040,7 +5040,7 @@ func TestGenerateSplits(t *testing.T) {
 			Path:      "/internal_location_splits_1_split_0",
 			ProxyPass: "http://vs_default_cafe_coffee-v1",
 			Rewrites: []string{
-				"^ $request_uri",
+				"^ $request_uri_no_args",
 				fmt.Sprintf(`"^%v(.*)$" "/rewrite$1" break`, originalPath),
 			},
 			ProxyNextUpstream:        "error timeout",
@@ -5148,14 +5148,15 @@ func TestGenerateSplits(t *testing.T) {
 		"default",
 		vsc.warnings,
 	)
-	if !reflect.DeepEqual(resultSplitClient, expectedSplitClient) {
-		t.Errorf("generateSplits() returned \n%+v but expected \n%+v", resultSplitClient, expectedSplitClient)
+
+	if diff := cmp.Diff(expectedSplitClient, resultSplitClient); diff != "" {
+		t.Errorf("generateSplits() resultSplitClient mismatch (-want +got):\n%s", diff)
 	}
-	if !reflect.DeepEqual(resultLocations, expectedLocations) {
-		t.Errorf("generateSplits() returned \n%+v but expected \n%+v", resultLocations, expectedLocations)
+	if diff := cmp.Diff(expectedLocations, resultLocations); diff != "" {
+		t.Errorf("generateSplits() resultLocations mismatch (-want +got):\n%s", diff)
 	}
-	if !reflect.DeepEqual(resultReturnLocations, expectedReturnLocations) {
-		t.Errorf("generateSplits() returned \n%+v but expected \n%+v", resultReturnLocations, expectedReturnLocations)
+	if diff := cmp.Diff(expectedReturnLocations, resultReturnLocations); diff != "" {
+		t.Errorf("generateSplits() resultReturnLocations mismatch (-want +got):\n%s", diff)
 	}
 }
 
@@ -7281,23 +7282,27 @@ func TestGenerateRewrites(t *testing.T) {
 		originalPath string
 		grpcEnabled  bool
 		expected     []string
+		msg          string
 	}{
 		{
 			proxy:    nil,
 			expected: nil,
+			msg:      "action isn't proxy",
 		},
 		{
 			proxy: &conf_v1.ActionProxy{
 				RewritePath: "",
 			},
 			expected: nil,
+			msg:      "no rewrite is configured",
 		},
 		{
 			path: "/path",
 			proxy: &conf_v1.ActionProxy{
 				RewritePath: "/rewrite",
 			},
 			expected: nil,
+			msg:      "non-regex rewrite for non-internal location is not needed",
 		},
 		{
 			path:     "/_internal_path",
@@ -7306,7 +7311,8 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			originalPath: "/path",
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "non-regex rewrite for internal location",
 		},
 		{
 			path:     "~/regex",
@@ -7315,7 +7321,8 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			originalPath: "/path",
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "regex rewrite for internal location",
 		},
 		{
 			path:     "~/regex",
@@ -7324,6 +7331,7 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			expected: []string{`"^/regex" "/rewrite" break`},
+			msg:      "regex rewrite for non-internal location",
 		},
 		{
 			path:     "/_internal_path",
@@ -7333,22 +7341,23 @@ func TestGenerateRewrites(t *testing.T) {
 			},
 			originalPath: "/path",
 			grpcEnabled:  true,
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "non-regex rewrite for internal location with grpc enabled",
 		},
 		{
 			path:         "/_internal_path",
 			internal:     true,
 			originalPath: "/path",
 			grpcEnabled:  true,
 			expected:     []string{`^ $request_uri break`},
+			msg:          "empty rewrite for internal location with grpc enabled",
 		},
 	}
 
 	for _, test := range tests {
 		result := generateRewrites(test.path, test.proxy, test.internal, test.originalPath, test.grpcEnabled)
-		if !reflect.DeepEqual(result, test.expected) {
-			t.Errorf("generateRewrites(%v, %v, %v, %v) returned \n %v but expected \n %v",
-				test.path, test.proxy, test.internal, test.originalPath, result, test.expected)
+		if diff := cmp.Diff(test.expected, result); diff != "" {
+			t.Errorf("generateRewrites() '%v' mismatch (-want +got):\n%s", test.msg, diff)
 		}
 	}
 }

tests/data/rewrites/hello.yaml

@@ -0,0 +1,32 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: hello
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: hello
+  template:
+    metadata:
+      labels:
+        app: hello
+    spec:
+      containers:
+      - name: hello
+        image: nginxdemos/nginx-hello:plain-text
+        ports:
+        - containerPort: 8080
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: hello-svc
+spec:
+  ports:
+  - port: 80
+    targetPort: 8080
+    protocol: TCP
+    name: http
+  selector:
+    app: hello

tests/data/rewrites/virtual-server-parent.yaml

@@ -0,0 +1,17 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServer
+metadata:
+  name: rewrite-parent
+spec:
+  host: vsr.example.com
+  upstreams:
+  - name: hello
+    service: hello-svc
+    port: 80
+  routes:
+  - path: /
+    route: prefixes
+  - path: ~ /regex1/?(.*)
+    route: regex-1
+  - path: ~ /regex2/?(.*)
+    route: regex-2

tests/data/rewrites/virtual-server-rewrite.yaml

@@ -0,0 +1,44 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServer
+metadata:
+  name: rewrite
+spec:
+  host: vs.example.com
+  upstreams:
+  - name: hello
+    service: hello-svc
+    port: 80
+  routes:
+  - path: /backend1/
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /
+  - path: /backend2
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /backend2_1
+  - path: /match/
+    matches:
+    - conditions:
+      - cookie: user
+        value: john
+      action:
+        proxy:
+          upstream: hello
+          rewritePath: /user/john/
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /
+  - path: ~ /regex1/?(.*)
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /$1
+  - path: ~ /regex2/?(.*)
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /$1

tests/data/rewrites/virtual-server-route-prefixes.yaml

@@ -0,0 +1,34 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServerRoute
+metadata:
+  name: prefixes
+spec:
+  host: vsr.example.com
+  upstreams:
+  - name: hello
+    service: hello-svc
+    port: 80
+  subroutes:
+  - path: /backend1/
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /
+  - path: /backend2
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /backend2_1
+  - path: /match/
+    matches:
+    - conditions:
+      - cookie: user
+        value: john
+      action:
+        proxy:
+          upstream: hello
+          rewritePath: /user/john/
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /

tests/data/rewrites/virtual-server-route-regex1.yaml

@@ -0,0 +1,16 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServerRoute
+metadata:
+  name: regex-1
+spec:
+  host: vsr.example.com
+  upstreams:
+  - name: hello
+    service: hello-svc
+    port: 80
+  subroutes:
+  - path: ~ /regex1/?(.*)
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /$1

tests/data/rewrites/virtual-server-route-regex2.yaml

@@ -0,0 +1,16 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServerRoute
+metadata:
+  name: regex-2
+spec:
+  host: vsr.example.com
+  upstreams:
+  - name: hello
+    service: hello-svc
+    port: 80
+  subroutes:
+  - path: ~ /regex2/?(.*)
+    action:
+      proxy:
+        upstream: hello
+        rewritePath: /$1