Add annotation to allow modifiers to be used properly in kubernetes #3481

dtomcej · 2018-06-12T15:31:07Z

What does this PR do?

Adds a request-modifier annotation to the kubernetes provider, that will allow the modifiers to be used (AddPrefix, ReplacePath, ReplacePathRegex) properly, and without inferfering with the matcher rules.

Motivation

Fixes #2985, #3230

More

Added/updated tests
Added/updated documentation

Additional Notes

This PR also introduces a warning when users use ReplacePath as a rule-type, as it is a modifier, not a matcher rule.

timoreimann · 2018-06-12T17:59:35Z

docs/configuration/backends/kubernetes.md

@@ -156,7 +156,8 @@ The following general annotations are applicable on the Ingress object:
 | `traefik.ingress.kubernetes.io/redirect-regex: ^http://localhost/(.*)`          | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-replacement`.                               |
 | `traefik.ingress.kubernetes.io/redirect-replacement: http://mydomain/$1`        | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-regex`.                                     |
 | `traefik.ingress.kubernetes.io/rewrite-target: /users`                          | Replaces each matched Ingress path with the specified one, and adds the old path to the `X-Replaced-Path` header.                               |
-| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Default: `PathPrefix`.                                                                                 |
+| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Only path related matchers should be used (`Path`, `PathPrefix`, `PathStrip`, `PathPrefixStrip`). Default: `PathPrefix`.                                                                                 |


should or rather can?

There is crazy legacy code in there that allows for "ReplacePath" to be used as the rule-type. But we want to discourage it (hence the log.warn, and it not being listed in the "should" list), but it can technically be used to maintain backwards compatibility to people already using it.

But I will update the wording to "can" and anyone that goes "but can you do X" can bite me

timoreimann · 2018-06-12T18:00:59Z

provider/kubernetes/kubernetes.go

+		rules = append(rules, ruleType+":"+pa.Path)
+	case ruleTypeReplacePath:
+		rules = append(rules, ruleType+":"+pa.Path)
+		log.Warnf("Using %s as rule-type will be deprecated in the future. Please use request-modifier annotation instead", ruleType)


Can we name the full new annotation name?

timoreimann · 2018-06-12T18:03:00Z

provider/kubernetes/kubernetes.go

@@ -421,6 +443,27 @@ func getRuleForPath(pa extensionsv1beta1.HTTPIngressPath, i *extensionsv1beta1.I
 		}
 		rules = append(rules, ruleTypeReplacePath+":"+rootPath)
 	}
+
+	if requestModifier := getStringValue(i.Annotations, annotationKubernetesRequestModifier, ""); requestModifier != "" {


What happens if the annotation consists of all whitespaces only?

Also, added tests to include a bunch of other oddities

timoreimann · 2018-06-13T23:06:01Z

provider/kubernetes/kubernetes.go

+	// Split annotation to determine modifier type
+	modifierParts := strings.Split(strings.Trim(requestModifier, " :"), ":")
+	if len(modifierParts) < 2 {
+		return "", fmt.Errorf("could not get rule due to incomplete rule: %q", requestModifier)


Can we make this error message different from the one in line 452 to allow distinguishing?

timoreimann · 2018-06-13T23:06:46Z

provider/kubernetes/kubernetes.go

+	switch modifier {
+	case ruleTypeAddPrefix, ruleTypeReplacePath, ruleTypeReplacePathRegex:
+		if ruleType == ruleTypeReplacePath {
+			return "", fmt.Errorf("cannot use '%s: %s' and '%s: %s'",


Why not? Can we include a reason?

timoreimann · 2018-06-19T19:31:08Z

provider/kubernetes/annotations.go

@@ -31,6 +31,7 @@ const (
 	annotationKubernetesErrorPages                = "ingress.kubernetes.io/error-pages"
 	annotationKubernetesBuffering                 = "ingress.kubernetes.io/buffering"
 	annotationKubernetesAppRoot                   = "ingress.kubernetes.io/app-root"
+	annotationKubernetesRequestModifier           = "ingress.kubernetes.io/request-modifier"


Did you possibly check if a similar annotation already exists with Nginx, and if so, whether it makes sense to copy the name?

Not really.

^^ Not really as in: I checked, and nginx does not provide that kind of functionality through an annotation. We are providing "new" functionality with this one.

timoreimann · 2018-06-19T22:21:44Z

docs/configuration/backends/kubernetes.md

@@ -156,7 +156,8 @@ The following general annotations are applicable on the Ingress object:
 | `traefik.ingress.kubernetes.io/redirect-regex: ^http://localhost/(.*)`          | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-replacement`.                               |
 | `traefik.ingress.kubernetes.io/redirect-replacement: http://mydomain/$1`        | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-regex`.                                     |
 | `traefik.ingress.kubernetes.io/rewrite-target: /users`                          | Replaces each matched Ingress path with the specified one, and adds the old path to the `X-Replaced-Path` header.                               |
-| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Default: `PathPrefix`.                                                                                 |
+| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Only path related matchers can be used (`Path`, `PathPrefix`, `PathStrip`, `PathPrefixStrip`). Default: `PathPrefix`.                                                                                 |


How about linking to the path matchers documentation here?

timoreimann · 2018-06-19T22:23:22Z

docs/configuration/backends/kubernetes.md

@@ -156,7 +156,8 @@ The following general annotations are applicable on the Ingress object:
 | `traefik.ingress.kubernetes.io/redirect-regex: ^http://localhost/(.*)`          | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-replacement`.                               |
 | `traefik.ingress.kubernetes.io/redirect-replacement: http://mydomain/$1`        | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-regex`.                                     |
 | `traefik.ingress.kubernetes.io/rewrite-target: /users`                          | Replaces each matched Ingress path with the specified one, and adds the old path to the `X-Replaced-Path` header.                               |
-| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Default: `PathPrefix`.                                                                                 |
+| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Only path related matchers can be used (`Path`, `PathPrefix`, `PathStrip`, `PathPrefixStrip`). Default: `PathPrefix`.                                                                                 |
+| `traefik.ingress.kubernetes.io/request-modifier: AddPath: /users`               | Add a request modifier to the backend request.                                                                                                  |


Link to request modifiers docs from here?

timoreimann · 2018-06-19T22:24:31Z

docs/configuration/backends/kubernetes.md

@@ -156,7 +156,8 @@ The following general annotations are applicable on the Ingress object:
 | `traefik.ingress.kubernetes.io/redirect-regex: ^http://localhost/(.*)`          | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-replacement`.                               |
 | `traefik.ingress.kubernetes.io/redirect-replacement: http://mydomain/$1`        | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-regex`.                                     |
 | `traefik.ingress.kubernetes.io/rewrite-target: /users`                          | Replaces each matched Ingress path with the specified one, and adds the old path to the `X-Replaced-Path` header.                               |
-| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Default: `PathPrefix`.                                                                                 |


Should we leave a note that ReplacePath is deprecated?

I guess if we are going to leave the message in the log, we might as well note in the docs.

You could argue that we should phrase the log message even stronger: that the old way will not be deprecated but is from now on effectively. This means that we will still support it but plan to remove support in a future release. At least, that's what I remember we (in particular @ldez) did in the past to migrate options.

I have completely remove annotations with the previous "label" style from the documentation.
I think can do the same here.

timoreimann · 2018-06-19T22:29:00Z

provider/kubernetes/kubernetes.go

+	case ruleTypeReplacePath:
+		log.Warnf("Using %s as %s will be deprecated in the future. Please use the %s annotation instead", ruleType, annotationKubernetesRuleType, annotationKubernetesRequestModifier)
+	default:
+		return "", fmt.Errorf("cannot use nonexistent, or non-matcher rule: %q", ruleType)


How about adding a case "": and log the non-existent case separately? This would be slightly clearer as we don't have to log an empty string.

timoreimann · 2018-06-19T22:34:25Z

provider/kubernetes/kubernetes.go

 	return strings.Join(rules, ";"), nil
 }

+func parseRequestModifier(requestModifier, ruleType string) (string, error) {
+	if strings.Trim(requestModifier, " :") == "" {
+		return "", fmt.Errorf("could not get rule due to empty rule: %q", requestModifier)


I'd simplify to "rule %q is empty" as the calling site already provides context that the error is about rule retrieval.

timoreimann · 2018-06-19T22:35:13Z

provider/kubernetes/kubernetes.go

+	// Split annotation to determine modifier type
+	modifierParts := strings.Split(strings.Trim(requestModifier, " :"), ":")
+	if len(modifierParts) < 2 {
+		return "", fmt.Errorf("could not get rule due to incomplete rule (missing rule type or value): %q", requestModifier)


Suggestion to simplify to "rule %q is missing type or value".

timoreimann · 2018-06-19T22:37:41Z

provider/kubernetes/kubernetes.go

 	return strings.Join(rules, ";"), nil
 }

+func parseRequestModifier(requestModifier, ruleType string) (string, error) {
+	if strings.Trim(requestModifier, " :") == "" {


TrimRight should suffice, shouldn't it?

timoreimann · 2018-06-19T22:39:36Z

provider/kubernetes/kubernetes.go

+	switch modifier {
+	case ruleTypeAddPrefix, ruleTypeReplacePath, ruleTypeReplacePathRegex:
+		if ruleType == ruleTypeReplacePath {
+			return "", fmt.Errorf("cannot use '%s: %s' and '%s: %s, as this leads to rule duplication, and unintended behavior'",


Missing closing '.

timoreimann · 2018-06-19T22:42:34Z

provider/kubernetes/kubernetes.go

+				annotationKubernetesRuleType, ruleTypeReplacePath, annotationKubernetesRequestModifier, modifier)
+		}
+	default:
+		return "", fmt.Errorf("cannot use nonexistent, or non-modifier rule: %q", modifier)


Suggestion for separate empty string case here too.

nmengin

LGTM

timoreimann

LGTM.

ldez

LGTM

wording fix review add tests update regex to be more friendly add docs allll the tests add space review: simplify. review: parseRequestModifier. review: doc. fix error wording fix review

tompson · 2018-07-12T12:31:41Z

docs/configuration/backends/kubernetes.md

@@ -154,7 +154,8 @@ The following general annotations are applicable on the Ingress object:
 | `traefik.ingress.kubernetes.io/redirect-regex: ^http://localhost/(.*)`          | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-replacement`. |
 | `traefik.ingress.kubernetes.io/redirect-replacement: http://mydomain/$1`        | Redirect to another URL for that frontend. Must be set with `traefik.ingress.kubernetes.io/redirect-regex`.       |
 | `traefik.ingress.kubernetes.io/rewrite-target: /users`                          | Replaces each matched Ingress path with the specified one, and adds the old path to the `X-Replaced-Path` header. |
-| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Default: `PathPrefix`.                                                   |
+| `traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip`                      | Override the default frontend rule type. Only path related matchers can be used [(`Path`, `PathPrefix`, `PathStrip`, `PathPrefixStrip`)](/basics/#path-matcher-usage-guidelines). Note: ReplacePath is deprecated in this annotation, use the `traefik.ingress.kubernetes.io/request-modifier` annotation instead. Default: `PathPrefix`.                                                                                 |
+| `traefik.ingress.kubernetes.io/request-modifier: AddPath: /users`               | Add a [request modifier](/basics/#modifiers) to the backend request.                                                                                                  |


It looks like there is a type - should'n it be AddPrefix instead of AddPath?

@tompson thanks. After changing to AddPrefix my ingress started working :)

@ferrarimarco can you maybe share your ingress configuration? I still cannot get mine working

@tompson

apiVersion: extensions/v1beta1 kind: Ingress metadata: name: a12 labels: app: a1 chart: a1 release: a1 heritage: Tiller annotations: kubernetes.io/ingress.class: traefik traefik.ingress.kubernetes.io/preserve-host: "true" traefik.ingress.kubernetes.io/app-root: "/index.html" traefik.ingress.kubernetes.io/request-modifier: "AddPrefix: /a1" spec: rules: - host: host1.tld http: paths: - path: /test/aaaa backend: serviceName: service-name servicePort: http

Thank you very much!

Unfortunately I am still not able to get my desired configuration to work.

I got a backend that is available under /app and I want a traefik configuration that allows user to call it with /app or with /oldapp - both ways should work exactly the same.

I thought my config should look something like

apiVersion: extensions/v1beta1 kind: Ingress metadata: name: default annotations: kubernetes.io/ingress.class: traefik spec: rules: - host: httpecho.kube.troii.net http: paths: - path: /app backend: serviceName: echo-server servicePort: 80 --- apiVersion: extensions/v1beta1 kind: Ingress metadata: name: default annotations: kubernetes.io/ingress.class: traefik traefik.ingress.kubernetes.io/request-modifier: "AddPrefix: /app" spec: rules: - host: httpecho.kube.troii.net http: paths: - path: /oldapp backend: serviceName: echo-server servicePort: 80 ---

apiVersion: extensions/v1beta1 kind: Ingress metadata: name: default annotations: kubernetes.io/ingress.class: traefik traefik.ingress.kubernetes.io/rule-type: PathPrefixStrip traefik.ingress.kubernetes.io/request-modifier: "AddPrefix: /app" spec: rules: - host: httpecho.kube.troii.net http: paths: - path: /oldapp backend: serviceName: echo-server servicePort: 80 - path: /app backend: serviceName: echo-server servicePort: 80

thanks @dtomcej ! should this already work with v1.6.5 ?

I looked into the commit history and it seems like this will be in included in 1.7.0 but is not part of 1.6.5 - so I will have to wait to try it

dtomcej requested a review from a team as a code owner June 12, 2018 15:31

traefiker added area/provider/k8s/ingress size/S status/0-needs-triage labels Jun 12, 2018

dtomcej changed the title ~~Issue 2985~~ Add annotation to allow modifiers to be used properly in kubernetes Jun 12, 2018

ldez added status/2-needs-review contributor/waiting-for-corrections contributor/waiting-for-documentation and removed status/0-needs-triage labels Jun 12, 2018

dtomcej mentioned this pull request Jun 12, 2018

Allow Modifier type to be changed via annotation in a Kubernetes Ingress #3473

Closed

2 tasks

ldez removed contributor/waiting-for-corrections contributor/waiting-for-documentation labels Jun 12, 2018

timoreimann reviewed Jun 12, 2018

View reviewed changes

dtomcej force-pushed the issue-2985 branch from 23fa2f6 to 34200df Compare June 12, 2018 20:52

ldez force-pushed the issue-2985 branch from 0bb4843 to 7d27cc0 Compare June 12, 2018 22:51

ldez added the kind/enhancement a new or improved feature. label Jun 12, 2018

timoreimann suggested changes Jun 13, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 13, 2018

ldez removed the contributor/waiting-for-corrections label Jun 14, 2018

timoreimann suggested changes Jun 19, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 19, 2018

dtomcej added this to the 1.7 milestone Jun 21, 2018

dtomcej mentioned this pull request Jun 22, 2018

Traefik with Kubernetes Ingress is unable to prepend uri path #3230

Closed

ldez removed the contributor/waiting-for-corrections label Jun 22, 2018

dtomcej force-pushed the issue-2985 branch 2 times, most recently from 76e5f66 to f6c5d5b Compare July 3, 2018 17:53

nmengin approved these changes Jul 4, 2018

View reviewed changes

timoreimann approved these changes Jul 5, 2018

View reviewed changes

ldez approved these changes Jul 5, 2018

View reviewed changes

ldez added the status/3-needs-merge label Jul 6, 2018

ldez removed the status/2-needs-review label Jul 6, 2018

traefiker added the status/4-merge-in-progress label Jul 6, 2018

dtomcej and others added 2 commits July 6, 2018 07:48

base commit

d1de74a

wording fix review add tests update regex to be more friendly add docs allll the tests add space review: simplify. review: parseRequestModifier. review: doc. fix error wording fix review

review cleanup

54a9013

traefiker force-pushed the issue-2985 branch from 18f5b2d to 54a9013 Compare July 6, 2018 07:48

traefiker removed the status/4-merge-in-progress label Jul 6, 2018

traefiker merged commit 2303301 into traefik:master Jul 6, 2018

traefiker removed the status/3-needs-merge label Jul 6, 2018

tompson reviewed Jul 12, 2018

View reviewed changes

dtomcej mentioned this pull request Jul 12, 2018

Correct Modifier in Kubernetes Documentation #3610

Merged

1 task

Add annotation to allow modifiers to be used properly in kubernetes #3481

Add annotation to allow modifiers to be used properly in kubernetes #3481

Conversation

dtomcej commented Jun 12, 2018 • edited Loading

What does this PR do?

Motivation

More

Additional Notes

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

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

dtomcej Jun 22, 2018 • edited Loading

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

nmengin left a comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

ldez left a 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

ferrarimarco Jul 12, 2018 • edited by ldez Loading

Choose a reason for hiding this comment

tompson Jul 12, 2018 • edited by ldez Loading

Choose a reason for hiding this comment

dtomcej Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej commented Jun 12, 2018 •

edited

Loading

dtomcej Jun 22, 2018 •

edited

Loading

ferrarimarco Jul 12, 2018 •

edited by ldez

Loading

tompson Jul 12, 2018 •

edited by ldez

Loading

dtomcej Jul 12, 2018 •

edited

Loading