Fix Rewrite-Target Annotation Edge Case #3078

zrdaley · 2018-09-11T20:10:42Z

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 the nginx.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: #3076

Relevant links:

@ElvinEfendi @sbfaulkner

aledbf · 2018-09-11T20:56:07Z

internal/ingress/controller/template/template_test.go

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


newLoc := buildLocation(loc, tc.Path != tc.Target) ?

ElvinEfendi · 2018-09-12T19:26:26Z

internal/ingress/controller/template/template.go

+
+	for _, location := range locations {
+		path := location.Path
+		if len(location.Rewrite.Target) > 0 && location.Rewrite.Target != path {


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.

ElvinEfendi · 2018-09-12T19:27:40Z

internal/ingress/controller/template/template.go

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


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.

ElvinEfendi · 2018-09-12T19:48:30Z

internal/ingress/controller/template/template.go

+		if path == slash {
+			return path
+		}
+		return fmt.Sprintf(`^~ %s`, path)


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?

technically, the conflict would occur with any leading portion of /.well-known/acme/challenge

eg. /.well-known

right?

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

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).

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.

ElvinEfendi · 2018-09-12T19:51:40Z

test/e2e/annotations/rewrite.go

@@ -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() {


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 /)?

This is the important test for that case. The other ~~can~~ has been be removed.

ElvinEfendi · 2018-09-14T14:11:53Z

internal/ingress/controller/template/template.go

@@ -310,6 +334,12 @@ func buildLocation(input interface{}) string {
 		return fmt.Sprintf(`~* ^%s%s`, path, baseuri)
 	}

+	if rewrite == true {


nitpick: you could do if rewrite { since rewrite is boolean.

ElvinEfendi · 2018-09-14T22:28:09Z

/lgtm
/approve

k8s-ci-robot · 2018-09-14T22:28:15Z

[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:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2018

aledbf reviewed Sep 11, 2018

View reviewed changes

zrdaley changed the title ~~Rewrite fix~~ Fix Rewrite-Target Edge Case Sep 12, 2018

zrdaley changed the title ~~Fix Rewrite-Target Edge Case~~ Fix Rewrite-Target Annotation Edge Case Sep 12, 2018

zrdaley force-pushed the rewrite-fix branch from 6f4732b to 94ec5bc Compare September 12, 2018 13:46

Add e2e test for rewrite-target annotation kube-lego failure

aff61dc

zrdaley force-pushed the rewrite-fix branch from 94ec5bc to f0c826e Compare September 12, 2018 14:45

zrdaley mentioned this pull request Sep 12, 2018

Add e2e test for rewrite-target annotation kube-lego failure #3076

Closed

ElvinEfendi reviewed Sep 12, 2018

View reviewed changes

enforce ^~ location modifier when rewrite-target annotation is set

0e6f0bb

zrdaley force-pushed the rewrite-fix branch from f0c826e to 0e6f0bb Compare September 13, 2018 14:40

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2018

ElvinEfendi reviewed Sep 14, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 14, 2018

Fix/add unit tests; Styling changes

0de19c8

zrdaley force-pushed the rewrite-fix branch from 6dd1023 to 0de19c8 Compare September 14, 2018 19:08

k8s-ci-robot assigned ElvinEfendi Sep 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2018

k8s-ci-robot merged commit 80933bc into kubernetes:master Sep 14, 2018

zrdaley deleted the rewrite-fix branch September 17, 2018 16:46

zrdaley mentioned this pull request Sep 24, 2018

Revert Kube-Lego Edge Case Fix and Enforce Regex on All Locations #3123

Closed

zrdaley mentioned this pull request Sep 26, 2018

Add "use-regex" Annotation to Toggle Regular Expression Location Modifier #3145

Merged

Congelli501 mentioned this pull request Dec 26, 2023

Optimization: don't use regex location when avoidable #10808

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Rewrite-Target Annotation Edge Case #3078

Fix Rewrite-Target Annotation Edge Case #3078

zrdaley commented Sep 11, 2018

aledbf Sep 11, 2018

ElvinEfendi Sep 12, 2018

ElvinEfendi Sep 12, 2018

ElvinEfendi Sep 12, 2018

sbfaulkner Sep 12, 2018

sbfaulkner Sep 12, 2018

ElvinEfendi Sep 12, 2018

zrdaley Sep 12, 2018

ElvinEfendi Sep 12, 2018

zrdaley Sep 12, 2018 •

edited

Loading

ElvinEfendi Sep 14, 2018

ElvinEfendi commented Sep 14, 2018

k8s-ci-robot commented Sep 14, 2018

Fix Rewrite-Target Annotation Edge Case #3078

Fix Rewrite-Target Annotation Edge Case #3078

Conversation

zrdaley commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrdaley Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Sep 14, 2018

k8s-ci-robot commented Sep 14, 2018

zrdaley Sep 12, 2018 •

edited

Loading