Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fixed rewrites for paths not ending in / #2899

Merged
merged 1 commit into from
Aug 19, 2018
Merged

fixed rewrites for paths not ending in / #2899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/ingress/controller/template/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ func buildProxyPass(host string, b interface{}, loc interface{}, dynamicConfigur
var xForwardedPrefix string

if location.Rewrite.AddBaseURL {
// path has a slash suffix, so that it can be connected with baseuri directly
bPath := fmt.Sprintf("%s$escaped_base_uri", path)
regex := `(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)`
scheme := "$scheme"
Expand All @@ -494,15 +493,16 @@ subs_filter '%v' '$1<base href="%v://$http_host%v">' ro;
// ie /something to /
return fmt.Sprintf(`
rewrite (?i)%s(.*) /$1 break;
rewrite (?i)%s / break;
rewrite (?i)%s$ / break;
%v%v %s%s;
%v`, path, location.Path, xForwardedPrefix, proxyPass, proto, upstreamName, abu)
}

return fmt.Sprintf(`
rewrite (?i)%s(.*) %s/$1 break;
rewrite (?i)%s$ %s/ break;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for the target to end with slash? forgive me if it's already explained elsewhere but i can't seem to find it.

related issue: #1399

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also confused.
Any target path that ends without slash cannot be matched in this way.

%v%v %s%s;
%v`, path, location.Rewrite.Target, xForwardedPrefix, proxyPass, proto, upstreamName, abu)
%v`, path, location.Rewrite.Target, location.Path, location.Rewrite.Target, xForwardedPrefix, proxyPass, proto, upstreamName, abu)
}

// default proxy_pass
Expand Down
15 changes: 12 additions & 3 deletions internal/ingress/controller/template/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ var (
"~* /",
`
rewrite (?i)/(.*) /jenkins/$1 break;
rewrite (?i)/$ /jenkins/ break;
proxy_pass http://upstream-name;
`,
false,
Expand All @@ -137,7 +138,7 @@ proxy_pass http://upstream-name;
`~* ^/something\/?(?<baseuri>.*)`,
`
rewrite (?i)/something/(.*) /$1 break;
rewrite (?i)/something / break;
rewrite (?i)/something$ / break;
proxy_pass http://upstream-name;
`,
false,
Expand All @@ -152,6 +153,7 @@ proxy_pass http://upstream-name;
"~* ^/end-with-slash/(?<baseuri>.*)",
`
rewrite (?i)/end-with-slash/(.*) /not-root/$1 break;
rewrite (?i)/end-with-slash/$ /not-root/ break;
proxy_pass http://upstream-name;
`,
false,
Expand All @@ -166,6 +168,7 @@ proxy_pass http://upstream-name;
`~* ^/something-complex\/?(?<baseuri>.*)`,
`
rewrite (?i)/something-complex/(.*) /not-root/$1 break;
rewrite (?i)/something-complex$ /not-root/ break;
proxy_pass http://upstream-name;
`,
false,
Expand All @@ -180,6 +183,7 @@ proxy_pass http://upstream-name;
"~* /",
`
rewrite (?i)/(.*) /jenkins/$1 break;
rewrite (?i)/$ /jenkins/ break;
proxy_pass http://upstream-name;

set_escape_uri $escaped_base_uri $baseuri;
Expand All @@ -197,7 +201,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
`~* ^/something\/?(?<baseuri>.*)`,
`
rewrite (?i)/something/(.*) /$1 break;
rewrite (?i)/something / break;
rewrite (?i)/something$ / break;
proxy_pass http://upstream-name;

set_escape_uri $escaped_base_uri $baseuri;
Expand All @@ -215,6 +219,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
`~* ^/end-with-slash/(?<baseuri>.*)`,
`
rewrite (?i)/end-with-slash/(.*) /not-root/$1 break;
rewrite (?i)/end-with-slash/$ /not-root/ break;
proxy_pass http://upstream-name;

set_escape_uri $escaped_base_uri $baseuri;
Expand All @@ -232,6 +237,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
`~* ^/something-complex\/?(?<baseuri>.*)`,
`
rewrite (?i)/something-complex/(.*) /not-root/$1 break;
rewrite (?i)/something-complex$ /not-root/ break;
proxy_pass http://upstream-name;

set_escape_uri $escaped_base_uri $baseuri;
Expand All @@ -249,7 +255,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
`~* ^/something\/?(?<baseuri>.*)`,
`
rewrite (?i)/something/(.*) /$1 break;
rewrite (?i)/something / break;
rewrite (?i)/something$ / break;
proxy_pass http://upstream-name;

set_escape_uri $escaped_base_uri $baseuri;
Expand All @@ -267,6 +273,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
`~* /`,
`
rewrite (?i)/(.*) /something/$1 break;
rewrite (?i)/$ /something/ break;
proxy_pass http://sticky-upstream-name;
`,
false,
Expand All @@ -281,6 +288,7 @@ proxy_pass http://sticky-upstream-name;
`~* /`,
`
rewrite (?i)/(.*) /something/$1 break;
rewrite (?i)/$ /something/ break;
proxy_pass http://upstream_balancer;
`,
false,
Expand All @@ -295,6 +303,7 @@ proxy_pass http://upstream_balancer;
`~* ^/there\/?(?<baseuri>.*)`,
`
rewrite (?i)/there/(.*) /something/$1 break;
rewrite (?i)/there$ /something/ break;
proxy_set_header X-Forwarded-Prefix "/there/";
proxy_pass http://sticky-upstream-name;
`,
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/annotations/rewrite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
err = f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, "rewrite (?i)/something/(.*) /$1 break;") &&
strings.Contains(server, "rewrite (?i)/something / break;")
strings.Contains(server, "rewrite (?i)/something$ / break;")
})
Expect(err).NotTo(HaveOccurred())

Expand Down Expand Up @@ -114,7 +114,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {

logs, err := f.NginxLogs()
Expect(err).ToNot(HaveOccurred())
Expect(logs).To(ContainSubstring(`"(?i)/something" matches "/something", client:`))
Expect(logs).To(ContainSubstring(`"(?i)/something$" matches "/something", client:`))
Expect(logs).To(ContainSubstring(`rewritten data: "/", args: "",`))
})
})