Add app-root annotation support for kubernetes ingress #2522

yue9944882 · 2017-12-04T17:33:55Z

What does this PR do?

Rediret only root path elsewhere by ingress annotation. Like Nginx Ingress Controller documented. Implementing this annotation will only perform ReplacePath on / path on all hosts present in the annotated ingress.

ingress.kubernetes.io/app-root or traefik.ingress.kubernetes.io/app-root

Motivation

We catch this use case in production deployment.

More

Added/updated tests
Added/updated documentation

Additional Notes

Seems like many new annotation support will be added to Kubernetes provider on 1.5 release, I hope this annotation can also be supported.

ldez · 2017-12-08T15:04:24Z

WDYT @containous/kubernetes ?

yue9944882 · 2017-12-08T15:46:56Z

PTAL thx

errm · 2017-12-08T15:58:20Z

I can see that this replicates the behaviour of the ngnix controller... but

It seems a bit strange that this would only work when the path is set to / without looking at the config the the nginx controller I would have assumed that it would be possible to set the path so that e.g. if path is /foo and app-root is /bar visiting /foo returns a 302 to /foo/bar/

I mean this does keep it consistent with how people expect this particular annotation to work so this is probably fine, and in any case I don't think implementing in this way precludes making this smarter in the future.

So um yeah I think this will be fine, but will require a note in the documentation that gets added to say this annotation only has any effect when the path is /

Other than that this seems reasonable in that it is only small change, follows the existing "defacto" standard and if not enabled has no way of effecting anything seriously.

So yeah with docs + tests I will be 👍

timoreimann · 2017-12-08T23:35:15Z

What keeps us from implementing the feature such that it would also work when a path other than / is set (assuming that Nginx also works like that)?

errm · 2017-12-09T00:14:15Z

Nginix only works when the path is / IDK why https://github.com/kubernetes/ingress-nginx/blob/53733483065a5619e8752c75dc2d23c628ddfc6f/rootfs/etc/nginx/template/nginx.tmpl#L636-L640

timoreimann · 2017-12-09T01:20:20Z

@aledbf could you shed some light on why the nginx.ingress.kubernetes.io/app-root annotation only works when the root path (/) is found? Any particular reason why a path /foo is not replaced like /app-root/foo?

Thanks!

yue9944882 · 2017-12-09T04:28:48Z

In my case, this annotation helps when I have a home page serving at "a.com/foo" but I want to redirect all "a.com" root requests to "a.com/foo", so that every time I want to browse this page I don't need to complete the full path manually which can be painful.. And at times, especially when the full path is rather long, typos will waste time. This will mostly happen when we put webpages on some static resouce servers. e.g. Apache？Nginx？
Although this redirection is also performed by most of backend servers. But forcing it explicitly on gateway will surely be needed in some use cases.
@timoreimann @errm

yue9944882 · 2017-12-09T04:32:34Z

BTW, I will add some tests but IDK where can I document this annotation? Should I add documentation into code/function comment?

timoreimann · 2017-12-09T20:35:09Z

@yue9944882 Documentation of the annotation should go into docs/configuration/backends/kubernetes.md. If you think that a more elaborate description or example also makes sense, you can additionally extend the Kubernetes guide at docs/user-guide/kubernetes.md.

timoreimann · 2017-12-10T07:50:58Z

docs/configuration/backends/kubernetes.md

@@ -158,6 +158,7 @@ The following security annotations can be applied to the ingress object to add s
 | `ingress.kubernetes.io/public-key:VALUE`                 | Adds pinned HTST public key header.                                                                                                                                                                 |
 | `ingress.kubernetes.io/referrer-policy:VALUE`            | Adds referrer policy  header.                                                                                                                                                                       |
 | `ingress.kubernetes.io/is-development:false`             | This will cause the `AllowedHosts`, `SSLRedirect`, and `STSSeconds`/`STSIncludeSubdomains` options to be ignored during development.<br>When deploying to production, be sure to set this to false. |
+| `ingress.kubernetes.io/app-root:VALUE`             |  Explicitly make Ingress Controller redirects `/` requests to the defined path. |


As we're documenting the Ingress controller, can we simplify this to

Redirects all requests for / to the defined path.

and also have another sentence to clearly describe what happens for non-root paths:

Non-root paths will not be affected by this annotation and handled normally.

Make perfect sense. 👍

timoreimann · 2017-12-10T08:01:28Z

provider/kubernetes/kubernetes.go

@@ -385,6 +386,10 @@ func getRuleForPath(pa v1beta1.HTTPIngressPath, i *v1beta1.Ingress) string {
 		rules = append(rules, ruleTypeReplacePath+":"+rewriteTarget)
 	}

+	// HACK: When multiple "ReplacePath" present, only the one at the last order of rule string will work


This should be handled less implicitly: we should either log a warning when we observe two ReplacePath rules being used; or even log an error and ignore the Ingress in order to force the user to make a conscious decision.

I'm not sure if it's possible technically, but doesn't Traefik support applying a series of replacement rules already? In that case, we could let them resolve naturally. The downside is that it makes things more complicated to understand.

Whatever we do, we must document the behavior beyond a code comment. And the word HACK should get dropped.

@dtomcej @errm thoughts?

https://github.com/containous/traefik/blob/328be161d6e8cd04c1066d5884e4bf35f396b40f/server/rules.go#L97
https://github.com/containous/traefik/blob/328be161d6e8cd04c1066d5884e4bf35f396b40f/server/rules.go#L184
I believe traefik doesn't support a series of replacement rules yet. And the last ReplacePath will override all the previous ReplacePath declaration because the rules is iterated as an array in the order of their occurrence in the rule-string. WDYT?

yue9944882 · 2017-12-10T09:11:21Z

@timoreimann PTAL thx

yue9944882 · 2017-12-10T10:50:00Z

@ldez @timoreimann @errm BTW, this PR should be a feature-request. Seems like I should have launched an issue or a discussion on Slack asking for further permission before this PR. Sorry for my negligence.

timoreimann

One more change request and one general note left.

We need to decide if we want to treat multiple replacement rules as warnings with last-rewrite-wins semantics, or as errors where we skip the offending Ingress. Having thought a bit more about it, I'm sort of leaning towards the latter with my rationale being that an explicit annotation indicates a specifically desirable behavior. Not being able to implement this behavior might be better exposed through a clear error.

I'm open to debate though.

timoreimann · 2017-12-10T11:55:13Z

docs/configuration/backends/kubernetes.md

@@ -158,6 +158,7 @@ The following security annotations can be applied to the ingress object to add s
 | `ingress.kubernetes.io/public-key:VALUE`                 | Adds pinned HTST public key header.                                                                                                                                                                 |
 | `ingress.kubernetes.io/referrer-policy:VALUE`            | Adds referrer policy  header.                                                                                                                                                                       |
 | `ingress.kubernetes.io/is-development:false`             | This will cause the `AllowedHosts`, `SSLRedirect`, and `STSSeconds`/`STSIncludeSubdomains` options to be ignored during development.<br>When deploying to production, be sure to set this to false. |
+| `ingress.kubernetes.io/app-root:VALUE`             |  Redirects all requests for / to the defined path. On the other hand, non-root paths will not be affected by this annotation and handled normally. |


Can we drop the on the other hand part? I think it's better to keep the prose to the necessary minimum in the formal documentation.

timoreimann · 2017-12-10T11:55:46Z

provider/kubernetes/kubernetes.go

@@ -381,10 +382,18 @@ func getRuleForPath(pa v1beta1.HTTPIngressPath, i *v1beta1.Ingress) string {

 	rules := []string{ruleType + ":" + pa.Path}

+	rewrited := false


Should be called rewritten.

timoreimann · 2017-12-10T12:08:56Z

provider/kubernetes/kubernetes.go

 	if rewriteTarget := i.Annotations[annotationKubernetesRewriteTarget]; rewriteTarget != "" {
 		rules = append(rules, ruleTypeReplacePath+":"+rewriteTarget)
+		rewrited = true


I just realized that we also have the case where the rule is explicitly set to ReplacePath. We can either handle that in this PR too or do a follow up.

yue9944882 · 2017-12-10T12:27:37Z

@timoreimann As far as I'm concerned, if we just skip the offending Ingresses, it might cause some unexpected accident or loss especially in production environment, e.g. making one or more web-site managed in the same Ingress unavailable.

How about put an error log and ignore this annotation when at least one ReplacePath rule already exists?

timoreimann · 2017-12-10T13:28:08Z

@yue9944882 my hypothesis is that failing hard and early in case the Ingress spec isn't consistent helps surfacing the problem more quickly than if we try to make the best out of it somehow. It's true that the latter could help making the Ingress work sometimes or even most of the time but may also cause it to fail in subtle, hard-to-spot corner cases. Personally I'd expect users to verify their Ingresses function properly prior to rolling them out to production systems, which seems easier to do when they don't work at all in case some error exists.

Those are my two cents. Let's hear more voices before making a decision.

yue9944882 · 2018-02-08T01:52:42Z

@timoreimann PTAL thx

timoreimann · 2018-02-13T21:04:16Z

provider/kubernetes/kubernetes.go

-					templateObjects.Frontends[baseName].Routes[pa.Path] = types.Route{
+				rule, err := getRuleForPath(pa, i)
+				if err != nil {
+					log.Errorf("fail to get rule from ingress path: %s", err)


Please update the log message as following:

log.Errorf("Failed to get rule for ingress %s/%s: %s", i.Namespace, i.Name, err)

(grammar/style of the first word + inclusion of the Ingress object's namespace and name)

timoreimann · 2018-02-13T21:06:59Z

provider/kubernetes/kubernetes.go

-					templateObjects.Frontends[baseName].Routes[pa.Path] = types.Route{
+				rule, err := getRuleForPath(pa, i)
+				if err != nil {
+					log.Errorf("fail to get rule from ingress path: %s", err)


My code coverage tool tells me that this line is not tested. Could you check please?

timoreimann · 2018-02-14T09:12:01Z

provider/kubernetes/kubernetes.go

 	if rewriteTarget := getStringValue(i.Annotations, annotationKubernetesRewriteTarget, ""); rewriteTarget != "" {
+		if pathReplaceAnnotation != "" {
+			return "", fmt.Errorf("rewrite-target must not be used together with annotation %q", pathReplaceAnnotation)


I failed to see it in my previous review round, but this part seems to be missing code coverage too.

yue9944882 · 2018-02-15T16:25:01Z

@timoreimann @ldez PTAL thx

timoreimann

LGTM. Thanks a ton! 👏

ldez · 2018-02-16T22:00:06Z

provider/kubernetes/kubernetes.go

+				rule, err := getRuleForPath(pa, i)
+				if err != nil {
+					log.Errorf("Failed to get rule for ingress %s/%s: %s", i.Namespace, i.Name, err)
+					delete(templateObjects.Frontends, r.Host+pa.Path)


could you replace r.Host+pa.Path by baseName

ldez · 2018-02-16T22:00:11Z

provider/kubernetes/kubernetes.go

+					continue
+				}
+				if rule != "" {
+					templateObjects.Frontends[r.Host+pa.Path].Routes[pa.Path] = types.Route{


could you replace r.Host+pa.Path by baseName

ldez · 2018-02-16T22:40:39Z

docs/configuration/backends/kubernetes.md

@@ -127,6 +127,7 @@ The following general annotations are applicable on the Ingress object:
 | `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/whitelist-source-range: "1.2.3.0/24, fe80::/16"` | A comma-separated list of IP ranges permitted for access. all source IPs are permitted if the list is empty or a single range is ill-formatted. |
+| `traefik.ingress.kubernetes.io/app-root: "/index.html"` | Redirects all requests for / to the defined path. Non-root paths will not be affected by this annotation and handled normally. This annotation may not be combined with the ReplacePath rule type or any other annotation leveraging that rule type. Trying to do so leads to an error and the corresponding Ingress object being ignored. |


Redirects all requests for `/` to the defined path. Non-root paths will not be affected by this annotation and handled normally. This annotation may not be combined with the `ReplacePath` rule type or any other annotation leveraging that rule type. Trying to do so leads to an error and the corresponding Ingress object being ignored.

ldez

LGTM

mmatur

LGTM

Rediret only root path elsewhere by ingress annotation Output error log when app-root and path violates Ignore app-root when ReplacePath already exists move docs to another section & rephrase error msg chore: rephrase docs & errror msg

yue9944882 requested a review from a team as a code owner December 4, 2017 17:33

traefiker added area/provider/k8s/ingress size/S status/0-needs-triage labels Dec 4, 2017

yue9944882 changed the title ~~Add app-root annotation support:~~ Add app-root annotation support for kubernetes ingress Dec 4, 2017

ldez added status/1-needs-design-review and removed status/0-needs-triage labels Dec 4, 2017

yue9944882 force-pushed the add_kubernetes_app_root branch 2 times, most recently from f47495a to ca907db Compare December 10, 2017 05:57

timoreimann suggested changes Dec 10, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Dec 10, 2017

yue9944882 force-pushed the add_kubernetes_app_root branch 2 times, most recently from a88b63f to 40bfc21 Compare December 10, 2017 08:38

ldez removed the contributor/waiting-for-corrections label Dec 10, 2017

timoreimann suggested changes Dec 10, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Dec 10, 2017

yue9944882 force-pushed the add_kubernetes_app_root branch from 128e738 to f3f0b4d Compare December 10, 2017 12:53

traefiker added the contributor/waiting-for-corrections label Feb 7, 2018

yue9944882 force-pushed the add_kubernetes_app_root branch from c736e54 to b109b4a Compare February 8, 2018 01:22

ldez removed the contributor/waiting-for-corrections label Feb 8, 2018

timoreimann suggested changes Feb 13, 2018

View reviewed changes

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

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

timoreimann reviewed Feb 14, 2018

View reviewed changes

ldez added contributor/waiting-for-corrections and removed contributor/waiting-for-corrections labels Feb 15, 2018

yue9944882 force-pushed the add_kubernetes_app_root branch 2 times, most recently from 0d5815e to d0d7728 Compare February 15, 2018 16:24

timoreimann approved these changes Feb 16, 2018

View reviewed changes

ldez suggested changes Feb 16, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Feb 16, 2018

ldez removed the contributor/waiting-for-corrections label Feb 17, 2018

ldez approved these changes Feb 17, 2018

View reviewed changes

mmatur approved these changes Feb 19, 2018

View reviewed changes

mmatur added status/3-needs-merge and removed status/2-needs-review labels Feb 19, 2018

traefiker added the status/4-merge-in-progress label Feb 19, 2018

yue9944882 added 3 commits February 19, 2018 14:10

Add app-root annotation support:

8e7433b

Rediret only root path elsewhere by ingress annotation Output error log when app-root and path violates Ignore app-root when ReplacePath already exists move docs to another section & rephrase error msg chore: rephrase docs & errror msg

update error msg & add some more testcase

3a34c73

edit docs & homogenize var name

ad9797e

traefiker force-pushed the add_kubernetes_app_root branch from a9e55a3 to ad9797e Compare February 19, 2018 14:10

traefiker removed the status/4-merge-in-progress label Feb 19, 2018

traefiker merged commit 5ef55dd into traefik:master Feb 19, 2018

traefiker removed the status/3-needs-merge label Feb 19, 2018

ldez mentioned this pull request Feb 22, 2018

Missing annotation prefix support. #2915

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add app-root annotation support for kubernetes ingress #2522

Add app-root annotation support for kubernetes ingress #2522

yue9944882 commented Dec 4, 2017 •

edited by ldez

Loading

ldez commented Dec 8, 2017

yue9944882 commented Dec 8, 2017

errm commented Dec 8, 2017

timoreimann commented Dec 8, 2017

errm commented Dec 9, 2017 •

edited

Loading

timoreimann commented Dec 9, 2017 •

edited

Loading

yue9944882 commented Dec 9, 2017 •

edited

Loading

yue9944882 commented Dec 9, 2017 •

edited

Loading

timoreimann commented Dec 9, 2017 •

edited

Loading

timoreimann Dec 10, 2017

yue9944882 Dec 10, 2017

timoreimann Dec 10, 2017

yue9944882 Dec 10, 2017

yue9944882 commented Dec 10, 2017

yue9944882 commented Dec 10, 2017 •

edited

Loading

timoreimann left a comment

timoreimann Dec 10, 2017

timoreimann Dec 10, 2017

timoreimann Dec 10, 2017

yue9944882 commented Dec 10, 2017 •

edited

Loading

timoreimann commented Dec 10, 2017 •

edited

Loading

yue9944882 commented Feb 8, 2018

timoreimann Feb 13, 2018

timoreimann Feb 13, 2018

timoreimann Feb 14, 2018 •

edited

Loading

yue9944882 commented Feb 15, 2018

timoreimann left a comment

ldez Feb 16, 2018

ldez Feb 16, 2018

ldez Feb 16, 2018

ldez left a comment

mmatur left a comment

		@@ -381,10 +382,18 @@ func getRuleForPath(pa v1beta1.HTTPIngressPath, i *v1beta1.Ingress) string {

		rules := []string{ruleType + ":" + pa.Path}

		rewrited := false

Add app-root annotation support for kubernetes ingress #2522

Add app-root annotation support for kubernetes ingress #2522

Conversation

yue9944882 commented Dec 4, 2017 • edited by ldez Loading

What does this PR do?

Motivation

More

Additional Notes

ldez commented Dec 8, 2017

yue9944882 commented Dec 8, 2017

errm commented Dec 8, 2017

timoreimann commented Dec 8, 2017

errm commented Dec 9, 2017 • edited Loading

timoreimann commented Dec 9, 2017 • edited Loading

yue9944882 commented Dec 9, 2017 • edited Loading

yue9944882 commented Dec 9, 2017 • edited Loading

timoreimann commented Dec 9, 2017 • 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

yue9944882 commented Dec 10, 2017

yue9944882 commented Dec 10, 2017 • edited Loading

timoreimann 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

yue9944882 commented Dec 10, 2017 • edited Loading

timoreimann commented Dec 10, 2017 • edited Loading

yue9944882 commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

yue9944882 commented Feb 15, 2018

timoreimann 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

ldez left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

yue9944882 commented Dec 4, 2017 •

edited by ldez

Loading

errm commented Dec 9, 2017 •

edited

Loading

timoreimann commented Dec 9, 2017 •

edited

Loading

yue9944882 commented Dec 9, 2017 •

edited

Loading

yue9944882 commented Dec 9, 2017 •

edited

Loading

timoreimann commented Dec 9, 2017 •

edited

Loading

yue9944882 commented Dec 10, 2017 •

edited

Loading

yue9944882 commented Dec 10, 2017 •

edited

Loading

timoreimann commented Dec 10, 2017 •

edited

Loading

timoreimann Feb 14, 2018 •

edited

Loading