-
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
Conversation
@@ -381,8 +381,12 @@ func TestBuildLocation(t *testing.T) { | |||
Path: tc.Path, | |||
Rewrite: rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL}, | |||
} | |||
|
|||
newLoc := buildLocation(loc) | |||
var newLoc string |
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.
newLoc := buildLocation(loc, tc.Path != tc.Target)
?
6f4732b
to
94ec5bc
Compare
94ec5bc
to
f0c826e
Compare
|
||
for _, location := range locations { | ||
path := location.Path | ||
if len(location.Rewrite.Target) > 0 && location.Rewrite.Target != path { |
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.
Can you extract this and https://github.com/kubernetes/ingress-nginx/pull/3078/files#diff-652deabba49a2a87d4c5a79700dfa1e3R318 into a separate utility function, i.e needsRewrite(location)
? Since they are doing the same thing, and extracting it out will also increase readability of the code.
@@ -287,9 +288,26 @@ func buildResolvers(res interface{}, disableIpv6 interface{}) string { | |||
return strings.Join(r, " ") + ";" | |||
} | |||
|
|||
// ususRewriteAnnotation checks if any location uses the nginx.ingress.kubernetes.io/rewrite-target annotation | |||
func usesRewriteAnnotation(input interface{}) bool { |
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.
given we do https://github.com/kubernetes/ingress-nginx/pull/3078/files#r217158979, I think renaming this to something like atLeastOneNeedsRewrite(locations)
would make more sense.
if path == slash { | ||
return path | ||
} | ||
return fmt.Sprintf(`^~ %s`, path) |
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.
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 /
path. If rewrite annotation is used i.e on /foo
, then the generated location would like ~* ^/foo\/?(?<baseuri>.*)
, which will not match things like /.well-known/acme/challenge
.
Given this, do you think we should simplify this further and apply ^~
only when there's an annotation on /
path?
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.
technically, the conflict would occur with any leading portion of /.well-known/acme/challenge
eg. /.well-known
right?
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.
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 comment
The 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 comment
The 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:
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
annotations:
kubernetes.io/ingress.class: nginx
ingress.kubernetes.io/rewrite-target: "/new/backend"
name: example
spec:
rules:
- host: example.com
http:
paths:
- backend:
serviceName: example
servicePort: 80
path: "/apps/.*"
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
annotations:
kubernetes.io/ingress.class: nginx
name: example
spec:
rules:
- host: example.com
http:
paths:
- backend:
serviceName: example
servicePort: 80
path: "/app/myapp"
A request to /app/myapp
would match the the location ~* /app/.*
instead of the exact path due to NGINX location modifier logic.
@@ -117,4 +117,135 @@ 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 comment
The 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 /.well-known/acme/challenge
requests to /.well-known/acme/challenge
goes to the service targeted by /
)?
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.
This is the important test for that case. The other can has been be removed.
f0c826e
to
0e6f0bb
Compare
@@ -310,6 +334,12 @@ func buildLocation(input interface{}) string { | |||
return fmt.Sprintf(`~* ^%s%s`, path, baseuri) | |||
} | |||
|
|||
if rewrite == true { |
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 {
since rewrite
is boolean.
6dd1023
to
0de19c8
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElvinEfendi, zrdaley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR fixes the edge case that currently breaks kube-lego.
This issue causes requests to the path
/.well-known/acme/challenge
to be redirected to the/
path when the rewrite-target annotation is set for the index route. This annotation enforces the use of the~*
location modifier. In NGINX, regex paths take priority over regular prefixes unless the=
or^~
location modifiers are used.After this PR, when
nginx.ingress.kubernetes.io/rewrite-target
is set on ANY ingress, then the^~
location modifier will be enforced on paths that do NOT use thenginx.ingress.kubernetes.io/rewrite-target
annotation. Paths on the ingress with the annotation set will continue to use the~*
location modifier.The only exception to the enforcement of the
^~
location modifier is for the/
path. If the^~
location modifier were to be used on the index route, then it would be turn into a catch-all and regex paths would never be checked.Which issue this PR fixes :
Partially fixes https://github.com/Shopify/edgescale/issues/659
Special notes for your reviewer:
Built off
Shopify:upstream-lego-e2e
: #3076Relevant links:
@ElvinEfendi @sbfaulkner