DevFW-CICD/ingress-nginx-helm
14
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #3078 from Shopify/rewrite-fix

Browse source 
Fix Rewrite-Target Annotation Edge Case
This commit is contained in:
k8s-ci-robot 2018-09-14 15:29:33 -07:00 committed by GitHub
commit 80933bc8d4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 148 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

34
internal/ingress/controller/template/template.go
View file

@ -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 {
if path == slash {
return path
}
return fmt.Sprintf(`^~ %s`, path)
}
return path
}

77
internal/ingress/controller/template/template_test.go
View file

@ -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<base href="
false,
false,
false,
false},
false,
true},
"redirect /something to / and rewrite": {
"/something",
"/",
@ -212,7 +224,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
false,
false,
false,
false},
false,
true},
"redirect /end-with-slash/ to /not-root and rewrite": {
"/end-with-slash/",
"/not-root",
@ -230,7 +243,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
false,
false,
false,
false},
false,
true},
"redirect /something-complex to /not-root and rewrite": {
"/something-complex",
"/not-root",
@ -248,7 +262,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
false,
false,
false,
false},
false,
true},
"redirect /something to / and rewrite with specific scheme": {
"/something",
"/",
@ -266,7 +281,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
false,
false,
false,
false},
false,
true},
"redirect / to /something with sticky enabled": {
"/",
"/something",
@ -281,7 +297,8 @@ proxy_pass http://sticky-upstream-name;
true,
false,
false,
false},
false,
true},
"redirect / to /something with sticky and dynamic config enabled": {
"/",
"/something",
@ -296,7 +313,8 @@ proxy_pass http://upstream_balancer;
true,
false,
true,
false},
false,
true},
"add the X-Forwarded-Prefix header": {
"/there",
"/something",
@ -312,7 +330,32 @@ proxy_pass http://sticky-upstream-name;
true,
true,
false,
false},
false,
true},
"do not use ^~ location modifier on index when ingress does not use rewrite target but at least one other ingress does": {
"/",
"/",
"/",
"proxy_pass http://upstream-name;",
false,
"",
false,
false,
false,
false,
true},
"use ^~ location modifier when ingress does not use rewrite target but at least one other ingress does": {
"/something",
"/something",
"^~ /something",
"proxy_pass http://upstream-name;",
false,
"",
false,
false,
false,
false,
true},
}
)
@ -382,7 +425,7 @@ func TestBuildLocation(t *testing.T) {
Rewrite: rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL},
}
newLoc := buildLocation(loc)
newLoc := buildLocation(loc, tc.atLeastOneNeedsRewrite)
if tc.Location != newLoc {
t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
}

6
rootfs/etc/nginx/template/nginx.tmpl
View file

@ -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 }}

52
test/e2e/annotations/rewrite.go
View file

@ -117,4 +117,56 @@ 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)
By("creating a regular ingress definition")
ing := framework.NewSingleIngress("kube-lego", "/.well-known/acme/challenge", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{})
_, 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`)
annotations := map[string]string{
"nginx.ingress.kubernetes.io/rewrite-target": "/new/backend",
}
rewriteIng := framework.NewSingleIngress("rewrite-index", "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
_, 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))
})
})