From aff61dc2dcf48ef3969a307c2e1e494487913b87 Mon Sep 17 00:00:00 2001 From: Zenara Daley Date: Wed, 12 Sep 2018 10:26:28 -0400 Subject: [PATCH 1/3] Add e2e test for rewrite-target annotation kube-lego failure --- test/e2e/annotations/rewrite.go | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/e2e/annotations/rewrite.go b/test/e2e/annotations/rewrite.go index 07cf8f845..452665595 100644 --- a/test/e2e/annotations/rewrite.go +++ b/test/e2e/annotations/rewrite.go @@ -117,4 +117,57 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() { Expect(logs).To(ContainSubstring(`"(?i)/something$" matches "/something", client:`)) Expect(logs).To(ContainSubstring(`rewritten data: "/", args: "",`)) }) + + It("should use correct longest path match", func() { + host := "rewrite.bar.com" + expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/.well-known/acme/challenge", host) + annotations := map[string]string{} + rewriteAnnotations := map[string]string{ + "nginx.ingress.kubernetes.io/rewrite-target": "/new/backend", + } + + By("creating a regular ingress definition") + ing := framework.NewSingleIngress("kube-lego", "/.well-known/acme/challenge", host, f.IngressController.Namespace, "http-svc", 80, &annotations) + _, err := f.EnsureIngress(ing) + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "/.well-known/acme/challenge") + }) + Expect(err).NotTo(HaveOccurred()) + + By("making a request to the non-rewritten location") + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL+"/.well-known/acme/challenge"). + Set("Host", host). + End() + + Expect(len(errs)).Should(Equal(0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + Expect(body).Should(ContainSubstring(expectBodyRequestURI)) + + By(`creating an ingress definition with the rewrite-target annotation set on the "/" location`) + rewriteIng := framework.NewSingleIngress("rewrite-index", "/", host, f.IngressController.Namespace, "http-svc", 80, &rewriteAnnotations) + _, err = f.EnsureIngress(rewriteIng) + Expect(err).NotTo(HaveOccurred()) + Expect(rewriteIng).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "location ~* / {") + }) + Expect(err).NotTo(HaveOccurred()) + + By("making a second request to the non-rewritten location") + resp, body, errs = gorequest.New(). + Get(f.IngressController.HTTPURL+"/.well-known/acme/challenge"). + Set("Host", host). + End() + + Expect(len(errs)).Should(Equal(0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + Expect(body).Should(ContainSubstring(expectBodyRequestURI)) + }) }) From 0e6f0bb88d33dfcf3cb2cfdc01d4b62a265a2fde Mon Sep 17 00:00:00 2001 From: Zenara Daley Date: Thu, 13 Sep 2018 10:39:52 -0400 Subject: [PATCH 2/3] enforce ^~ location modifier when rewrite-target annotation is set --- .../ingress/controller/template/template.go | 34 +++++++++++++++++-- .../controller/template/template_test.go | 2 +- rootfs/etc/nginx/template/nginx.tmpl | 6 ++-- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 39cfa5ddb..0b9044110 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -155,6 +155,7 @@ var ( "buildOpentracing": buildOpentracing, "proxySetHeader": proxySetHeader, "buildInfluxDB": buildInfluxDB, + "atLeastOneNeedsRewrite": atLeastOneNeedsRewrite, } ) @@ -287,9 +288,32 @@ func buildResolvers(res interface{}, disableIpv6 interface{}) string { return strings.Join(r, " ") + ";" } +func needsRewrite(location *ingress.Location) bool { + if len(location.Rewrite.Target) > 0 && location.Rewrite.Target != location.Path { + return true + } + return false +} + +// atLeastOneNeedsRewrite checks if the nginx.ingress.kubernetes.io/rewrite-target annotation is used on the '/' path +func atLeastOneNeedsRewrite(input interface{}) bool { + locations, ok := input.([]*ingress.Location) + if !ok { + glog.Errorf("expected an '[]*ingress.Location' type but %T was returned", input) + return false + } + + for _, location := range locations { + if needsRewrite(location) { + return true + } + } + return false +} + // buildLocation produces the location string, if the ingress has redirects // (specified through the nginx.ingress.kubernetes.io/rewrite-target annotation) -func buildLocation(input interface{}) string { +func buildLocation(input interface{}, rewrite bool) string { location, ok := input.(*ingress.Location) if !ok { glog.Errorf("expected an '*ingress.Location' type but %T was returned", input) @@ -297,7 +321,7 @@ func buildLocation(input interface{}) string { } path := location.Path - if len(location.Rewrite.Target) > 0 && location.Rewrite.Target != path { + if needsRewrite(location) { if path == slash { return fmt.Sprintf("~* %s", path) } @@ -310,6 +334,12 @@ func buildLocation(input interface{}) string { return fmt.Sprintf(`~* ^%s%s`, path, baseuri) } + if rewrite == true { + if path == slash { + return path + } + return fmt.Sprintf(`^~ %s`, path) + } return path } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 7068d295e..7f75689f5 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -382,7 +382,7 @@ func TestBuildLocation(t *testing.T) { Rewrite: rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL}, } - newLoc := buildLocation(loc) + newLoc := buildLocation(loc, tc.Path != tc.Target) if tc.Location != newLoc { t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc) } diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 027010882..915ce66a7 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -452,8 +452,9 @@ http { {{/* build the maps that will be use to validate the Whitelist */}} {{ range $server := $servers }} + {{ $usesRewrite := atLeastOneNeedsRewrite $server.Locations }} {{ range $location := $server.Locations }} - {{ $path := buildLocation $location }} + {{ $path := buildLocation $location $usesRewrite }} {{ if isLocationAllowed $location }} {{ if gt (len $location.Whitelist.CIDR) 0 }} @@ -818,8 +819,9 @@ stream { {{ $server.ServerSnippet }} {{ end }} + {{ $usesRewrite := atLeastOneNeedsRewrite $server.Locations }} {{ range $location := $server.Locations }} - {{ $path := buildLocation $location }} + {{ $path := buildLocation $location $usesRewrite }} {{ $proxySetHeader := proxySetHeader $location }} {{ $authPath := buildAuthLocation $location }} From 0de19c806214e663f63f5cf8d5a767cb9aa78079 Mon Sep 17 00:00:00 2001 From: Zenara Daley Date: Fri, 14 Sep 2018 15:07:57 -0400 Subject: [PATCH 3/3] Fix/add unit tests; Styling changes --- .../ingress/controller/template/template.go | 2 +- .../controller/template/template_test.go | 77 +++++++++++++++---- test/e2e/annotations/rewrite.go | 11 ++- 3 files changed, 66 insertions(+), 24 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 0b9044110..ff4ca1e5b 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -334,7 +334,7 @@ func buildLocation(input interface{}, rewrite bool) string { return fmt.Sprintf(`~* ^%s%s`, path, baseuri) } - if rewrite == true { + if rewrite { if path == slash { return path } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 7f75689f5..5e489d867 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -50,6 +50,7 @@ var ( XForwardedPrefix bool DynamicConfigurationEnabled bool SecureBackend bool + atLeastOneNeedsRewrite bool }{ "when secure backend enabled": { "/", @@ -61,7 +62,8 @@ var ( false, false, false, - true}, + true, + false}, "when secure backend and stickeness enabled": { "/", "/", @@ -72,7 +74,8 @@ var ( true, false, false, - true}, + true, + false}, "when secure backend and dynamic config enabled": { "/", "/", @@ -83,7 +86,8 @@ var ( false, false, true, - true}, + true, + false}, "when secure backend, stickeness and dynamic config enabled": { "/", "/", @@ -94,7 +98,8 @@ var ( true, false, true, - true}, + true, + false}, "invalid redirect / to / with dynamic config enabled": { "/", "/", @@ -105,6 +110,7 @@ var ( false, false, true, + false, false}, "invalid redirect / to /": { "/", @@ -116,6 +122,7 @@ var ( false, false, false, + false, false}, "redirect / to /jenkins": { "/", @@ -131,7 +138,8 @@ proxy_pass http://upstream-name; false, false, false, - false}, + false, + true}, "redirect /something to /": { "/something", "/", @@ -146,7 +154,8 @@ proxy_pass http://upstream-name; false, false, false, - false}, + false, + true}, "redirect /end-with-slash/ to /not-root": { "/end-with-slash/", "/not-root", @@ -161,7 +170,8 @@ proxy_pass http://upstream-name; false, false, false, - false}, + false, + true}, "redirect /something-complex to /not-root": { "/something-complex", "/not-root", @@ -176,7 +186,8 @@ proxy_pass http://upstream-name; false, false, false, - false}, + false, + true}, "redirect / to /jenkins and rewrite": { "/", "/jenkins", @@ -194,7 +205,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1]|"[^"]*")*>)' '$1]|"[^"]*")*>)' '$1]|"[^"]*")*>)' '$1]|"[^"]*")*>)' '$1