From 03a05458a01c2174cc2aaad9b810bceea3258964 Mon Sep 17 00:00:00 2001
From: Andreas Kohn <andreas.kohn@gmail.com>
Date: Thu, 6 Apr 2017 10:26:50 +0200
Subject: [PATCH] Move the extraction of the 'baseuri' for add-base-url to
 buildProxyPass

nginx prioritizes regex rules in 'location' before from regular rules, and so these will
always match[1] when used in combination with regular rules. For that reason we
should avoid using regex rules in location matching.

The use of regular expressions here actually was purely done to get the side-effect
of extracting the 'baseuri' for the use of add-base-url later on in buildProxyPass. This
commit introduces a dedicated 'location' block to do that extraction instead.

See also https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#rewrite
for some documentation on the rewriting features.

[1] https://www.digitalocean.com/community/tutorials/understanding-nginx-server-and-location-block-selection-algorithms
---
 controllers/nginx/pkg/template/template.go    | 54 +++++------
 .../nginx/pkg/template/template_test.go       | 89 ++++++++++---------
 2 files changed, 67 insertions(+), 76 deletions(-)

diff --git a/controllers/nginx/pkg/template/template.go b/controllers/nginx/pkg/template/template.go
index 50fe58418..c9c9c6a0b 100644
--- a/controllers/nginx/pkg/template/template.go
+++ b/controllers/nginx/pkg/template/template.go
@@ -201,21 +201,7 @@ func buildLocation(input interface{}) string {
 		return slash
 	}
 
-	path := location.Path
-	if len(location.Redirect.Target) > 0 && location.Redirect.Target != path {
-		if path == slash {
-			return fmt.Sprintf("~* %s", path)
-		}
-		// baseuri regex will parse basename from the given location
-		baseuri := `(?<baseuri>.*)`
-		if !strings.HasSuffix(path, slash) {
-			// Not treat the slash after "location path" as a part of baseuri
-			baseuri = fmt.Sprintf(`\/?%s`, baseuri)
-		}
-		return fmt.Sprintf(`~* ^%s%s`, path, baseuri)
-	}
-
-	return path
+	return location.Path
 }
 
 func buildAuthLocation(input interface{}) string {
@@ -298,29 +284,31 @@ func buildProxyPass(b interface{}, loc interface{}) string {
 	}
 
 	if len(location.Redirect.Target) > 0 {
-		abu := ""
-		if location.Redirect.AddBaseURL {
-			// path has a slash suffix, so that it can be connected with baseuri directly
-			bPath := fmt.Sprintf("%s%s", path, "$baseuri")
-			abu = fmt.Sprintf(`subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host%v">' r;
-	subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host%v">' r;
-	`, bPath, bPath)
-		}
-
+		rewrite := ""
 		if location.Redirect.Target == slash {
 			// special case redirect to /
 			// ie /something to /
-			return fmt.Sprintf(`
-	rewrite %s(.*) /$1 break;
-	rewrite %s / break;
-	proxy_pass %s://%s;
-	%v`, path, location.Path, proto, location.Backend, abu)
+			rewrite = fmt.Sprintf(`
+				rewrite %s(.*) /$1 break;
+				rewrite %s / break;
+				proxy_pass %s://%s;`, path, location.Path, proto, location.Backend)
+		} else {
+			rewrite = fmt.Sprintf(`
+				rewrite %s(.*) %s/$1 break;
+				proxy_pass %s://%s;`, path, location.Redirect.Target, proto, location.Backend)
 		}
 
-		return fmt.Sprintf(`
-	rewrite %s(.*) %s/$1 break;
-	proxy_pass %s://%s;
-	%v`, path, location.Redirect.Target, proto, location.Backend, abu)
+		if location.Redirect.AddBaseURL {
+			// path has a slash suffix, so that it can be connected with baseuri directly
+			return fmt.Sprintf(`
+				location ~* %s(?<baseuri>.*) {
+					subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host%s$baseuri">' r;
+					subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host%s$baseuri">' r;
+					%v
+				}`, path, path, path, rewrite)
+		}
+
+		return rewrite
 	}
 
 	// default proxy_pass
diff --git a/controllers/nginx/pkg/template/template_test.go b/controllers/nginx/pkg/template/template_test.go
index 7dfe83ed4..aa39dc76d 100644
--- a/controllers/nginx/pkg/template/template_test.go
+++ b/controllers/nginx/pkg/template/template_test.go
@@ -42,49 +42,52 @@ var (
 		AddBaseURL bool
 	}{
 		"invalid redirect / to /": {"/", "/", "/", "proxy_pass http://upstream-name;", false},
-		"redirect / to /jenkins": {"/", "/jenkins", "~* /",
-			`
-	rewrite /(.*) /jenkins/$1 break;
-	proxy_pass http://upstream-name;
-	`, false},
-		"redirect /something to /": {"/something", "/", `~* ^/something\/?(?<baseuri>.*)`, `
-	rewrite /something/(.*) /$1 break;
-	rewrite /something / break;
-	proxy_pass http://upstream-name;
-	`, false},
-		"redirect /end-with-slash/ to /not-root": {"/end-with-slash/", "/not-root", "~* ^/end-with-slash/(?<baseuri>.*)", `
-	rewrite /end-with-slash/(.*) /not-root/$1 break;
-	proxy_pass http://upstream-name;
-	`, false},
-		"redirect /something-complex to /not-root": {"/something-complex", "/not-root", `~* ^/something-complex\/?(?<baseuri>.*)`, `
-	rewrite /something-complex/(.*) /not-root/$1 break;
-	proxy_pass http://upstream-name;
-	`, false},
-		"redirect / to /jenkins and rewrite": {"/", "/jenkins", "~* /", `
-	rewrite /(.*) /jenkins/$1 break;
-	proxy_pass http://upstream-name;
-	subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/$baseuri">' r;
-	subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/$baseuri">' r;
-	`, true},
-		"redirect /something to / and rewrite": {"/something", "/", `~* ^/something\/?(?<baseuri>.*)`, `
-	rewrite /something/(.*) /$1 break;
-	rewrite /something / break;
-	proxy_pass http://upstream-name;
-	subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/something/$baseuri">' r;
-	subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/something/$baseuri">' r;
-	`, true},
-		"redirect /end-with-slash/ to /not-root and rewrite": {"/end-with-slash/", "/not-root", `~* ^/end-with-slash/(?<baseuri>.*)`, `
-	rewrite /end-with-slash/(.*) /not-root/$1 break;
-	proxy_pass http://upstream-name;
-	subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/end-with-slash/$baseuri">' r;
-	subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/end-with-slash/$baseuri">' r;
-	`, true},
-		"redirect /something-complex to /not-root and rewrite": {"/something-complex", "/not-root", `~* ^/something-complex\/?(?<baseuri>.*)`, `
-	rewrite /something-complex/(.*) /not-root/$1 break;
-	proxy_pass http://upstream-name;
-	subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/something-complex/$baseuri">' r;
-	subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/something-complex/$baseuri">' r;
-	`, true},
+		"redirect / to /jenkins": {"/", "/jenkins", "/", `
+				rewrite /(.*) /jenkins/$1 break;
+				proxy_pass http://upstream-name;`, false},
+		"redirect /something to /": {"/something", "/", "/something", `
+				rewrite /something/(.*) /$1 break;
+				rewrite /something / break;
+				proxy_pass http://upstream-name;`, false},
+		"redirect /end-with-slash/ to /not-root": {"/end-with-slash/", "/not-root", "/end-with-slash/", `
+				rewrite /end-with-slash/(.*) /not-root/$1 break;
+				proxy_pass http://upstream-name;`, false},
+		"redirect /something-complex to /not-root": {"/something-complex", "/not-root", "/something-complex", `
+				rewrite /something-complex/(.*) /not-root/$1 break;
+				proxy_pass http://upstream-name;`, false},
+		"redirect / to /jenkins and rewrite": {"/", "/jenkins", "/", `
+				location ~* /(?<baseuri>.*) {
+					subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/$baseuri">' r;
+					subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/$baseuri">' r;
+					
+				rewrite /(.*) /jenkins/$1 break;
+				proxy_pass http://upstream-name;
+				}`, true},
+		"redirect /something to / and rewrite": {"/something", "/", "/something", `
+				location ~* /something/(?<baseuri>.*) {
+					subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/something/$baseuri">' r;
+					subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/something/$baseuri">' r;
+					
+				rewrite /something/(.*) /$1 break;
+				rewrite /something / break;
+				proxy_pass http://upstream-name;
+				}`, true},
+		"redirect /end-with-slash/ to /not-root and rewrite": {"/end-with-slash/", "/not-root", "/end-with-slash/", `
+				location ~* /end-with-slash/(?<baseuri>.*) {
+					subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/end-with-slash/$baseuri">' r;
+					subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/end-with-slash/$baseuri">' r;
+					
+				rewrite /end-with-slash/(.*) /not-root/$1 break;
+				proxy_pass http://upstream-name;
+				}`, true},
+		"redirect /something-complex to /not-root and rewrite": {"/something-complex", "/not-root", "/something-complex", `
+				location ~* /something-complex/(?<baseuri>.*) {
+					subs_filter '<head(.*)>' '<head$1><base href="$scheme://$http_host/something-complex/$baseuri">' r;
+					subs_filter '<HEAD(.*)>' '<HEAD$1><base href="$scheme://$http_host/something-complex/$baseuri">' r;
+					
+				rewrite /something-complex/(.*) /not-root/$1 break;
+				proxy_pass http://upstream-name;
+				}`, true},
 	}
 )