-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Rewrite-Target Annotation Edge Case #3078
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,6 +155,7 @@ var ( | |
"buildOpentracing": buildOpentracing, | ||
"proxySetHeader": proxySetHeader, | ||
"buildInfluxDB": buildInfluxDB, | ||
"atLeastOneNeedsRewrite": atLeastOneNeedsRewrite, | ||
} | ||
) | ||
|
||
|
@@ -287,17 +288,40 @@ 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) | ||
return slash | ||
} | ||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed internally, this is not necessary every time there's rewrite annotation used, rather it is necessary only when the rewrite annotation is used on Given this, do you think we should simplify this further and apply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically, the conflict would occur with any leading portion of eg. right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and that expands further to any subpath of a path that does NOT use rewrite... so likely simpler logic this way... ie. just check that rewrite is used instead of looking for conflicting paths There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're saying this as well, the conflict you're describing can happen without rewrite target annotation which is not what we are trying to fix (I'd even argue it is fine as it is). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has the potential to break exact path matching if the ingress with the rewrite-target had a regex path that matched the exact path of another ingress (without the rewrite-target annotation). Example:
A request to |
||
} | ||
return path | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one of these tests is actually failing without your changes? Can we avoid introducing more tests? I think we concluded that there's only once case we are fixing, why not test just that (which is given ingress with path=/ and rewrite annotation, when I create another ingress for the same host with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the important test for that case. The other |
||
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)) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: you could do
if rewrite {
sincerewrite
is boolean.