From 7130ed85bc69a79a20bc9b3054efbb0aeb5c7629 Mon Sep 17 00:00:00 2001 From: Andreas Kohn Date: Thu, 6 Apr 2017 10:26:10 +0200 Subject: [PATCH 1/2] Fix the name of the annotation in comments --- controllers/nginx/pkg/template/template.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/nginx/pkg/template/template.go b/controllers/nginx/pkg/template/template.go index 14ae2025b..50fe58418 100644 --- a/controllers/nginx/pkg/template/template.go +++ b/controllers/nginx/pkg/template/template.go @@ -194,7 +194,7 @@ func buildSSLPassthroughUpstreams(b interface{}, sslb interface{}) string { } // buildLocation produces the location string, if the ingress has redirects -// (specified through the ingress.kubernetes.io/rewrite-to annotation) +// (specified through the ingress.kubernetes.io/rewrite-target annotation) func buildLocation(input interface{}) string { location, ok := input.(*ingress.Location) if !ok { @@ -264,7 +264,7 @@ func buildLogFormatUpstream(input interface{}) string { } // buildProxyPass produces the proxy pass string, if the ingress has redirects -// (specified through the ingress.kubernetes.io/rewrite-to annotation) +// (specified through the ingress.kubernetes.io/rewrite-target annotation) // If the annotation ingress.kubernetes.io/add-base-url:"true" is specified it will // add a base tag in the head of the response from the service func buildProxyPass(b interface{}, loc interface{}) string { From 03a05458a01c2174cc2aaad9b810bceea3258964 Mon Sep 17 00:00:00 2001 From: Andreas Kohn Date: Thu, 6 Apr 2017 10:26:50 +0200 Subject: [PATCH 2/2] 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 := `(?.*)` - 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 '' '' r; - subs_filter '' '' 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(?.*) { + subs_filter '' '' r; + subs_filter '' '' 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\/?(?.*)`, ` - 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", "~* /", ` - rewrite /(.*) /jenkins/$1 break; - proxy_pass http://upstream-name; - subs_filter '' '' r; - subs_filter '' '' r; - `, true}, - "redirect /something to / and rewrite": {"/something", "/", `~* ^/something\/?(?.*)`, ` - rewrite /something/(.*) /$1 break; - rewrite /something / break; - proxy_pass http://upstream-name; - subs_filter '' '' r; - subs_filter '' '' r; - `, true}, - "redirect /end-with-slash/ to /not-root and rewrite": {"/end-with-slash/", "/not-root", `~* ^/end-with-slash/(?.*)`, ` - rewrite /end-with-slash/(.*) /not-root/$1 break; - proxy_pass http://upstream-name; - subs_filter '' '' r; - subs_filter '' '' r; - `, true}, - "redirect /something-complex to /not-root and rewrite": {"/something-complex", "/not-root", `~* ^/something-complex\/?(?.*)`, ` - rewrite /something-complex/(.*) /not-root/$1 break; - proxy_pass http://upstream-name; - subs_filter '' '' r; - subs_filter '' '' 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 ~* /(?.*) { + subs_filter '' '' r; + subs_filter '' '' r; + + rewrite /(.*) /jenkins/$1 break; + proxy_pass http://upstream-name; + }`, true}, + "redirect /something to / and rewrite": {"/something", "/", "/something", ` + location ~* /something/(?.*) { + subs_filter '' '' r; + subs_filter '' '' 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/(?.*) { + subs_filter '' '' r; + subs_filter '' '' 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/(?.*) { + subs_filter '' '' r; + subs_filter '' '' r; + + rewrite /something-complex/(.*) /not-root/$1 break; + proxy_pass http://upstream-name; + }`, true}, } )