kubernetes ingress rewrite-target implementation #1723

mlaccetti · 2017-06-08T19:59:33Z

Monitor the kubernetes ingress for the ingress.kubernetes.io/rewrite-target annotation, and if seen, configure the frontend to strip off the provided path, and replace with the one provided in the annotation.

We create a rule using the `PathPrefixStrip` to trim out the bit in the rewrite rule.

timoreimann · 2017-06-08T20:15:20Z

@mlaccetti do you possibly know if the ingress.kubernetes.io/rewrite-target annotation is somewhat of a standard, or rather something that comes from a specific controller implementation (presumably Nginx)?

/cc @dtomcej @errm; also @emilevauge who I remember was involved in a Slack discussion on this topic fairly recently.

ldez · 2017-06-08T20:28:54Z

Related to #1722

mlaccetti · 2017-06-08T20:29:09Z

@timoreimann The spec doesn't have any annotations listed, but I'm finding them cropping up in places, and it seemed like low-hanging fruit.

It seems that folks have started using the nginx ingress as a defacto standard, rather than actually chasing down the spec. I'm going to chase that one down to figure out what the long term plan is.

Edit: I take it back, they are in the docs! https://github.com/kubernetes/ingress/blob/master/docs/annotations.md - Seems they are actually standard, now.

ldez · 2017-06-08T20:30:32Z

See https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md

dtomcej · 2017-06-09T17:51:09Z

Should we write this as a project-wide rule type? and just have ingresses implement it on annotation?

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

The annotations look more like de-facto standards to me. Regardless, I'm okay with supporting it.

I'd probably scope it to Kubernetes for now. If there's demand for spreading, we can have further PRs.

timoreimann · 2017-06-09T20:18:23Z

provider/kubernetes/kubernetes.go

+					default:
+						templateObjects.Frontends[r.Host+pa.Path].Routes[pa.Path] = types.Route{
+							Rule: ruleType + ":" + pa.Path,
+						}


To me it looks like we could simplify this block. How about:

rule := ruleType + ":" + pa.path if rewriteTarget := i.Annotations[annotationKubernetesRewriteTarget]; rewriteTarget != "" { rule = ruleTypeReplacePath + ":" + rewriteTarget } templateObjects.Frontends[r.Host+pa.Path].Routes[pa.Path] = types.Route{ Rule: rule, }

timoreimann · 2017-06-09T20:18:45Z

provider/kubernetes/kubernetes_test.go

+			ObjectMeta: v1.ObjectMeta{
+				Namespace: "testing",
+				Annotations: map[string]string{
+					"kubernetes.io/ingress.class":          "traefik",


We don't need the Ingress class for the test. Please remove it.

timoreimann · 2017-06-09T20:23:58Z

provider/kubernetes/kubernetes.go

+					default:
+						templateObjects.Frontends[r.Host+pa.Path].Routes[pa.Path] = types.Route{
+							Rule: ruleType + ":" + pa.Path,
+						}


Could we move the logic pertaining to setting the route type to a dedicated function to ease readability?

Moved it into a function.

timoreimann · 2017-06-09T20:32:54Z

provider/kubernetes/kubernetes_test.go

 		},
 	}

 	actualJSON, _ := json.Marshal(actual)
 	expectedJSON, _ := json.Marshal(expected)

 	if !reflect.DeepEqual(actual, expected) {
-		t.Fatalf("expected %+v, got %+v", string(expectedJSON), string(actualJSON))
+		t.Fatalf("expected %+v \n got %+v", string(expectedJSON), string(actualJSON))


We should stop comparing JSON representations as they can hide differences in very subtle ways. For instance, an empty slice and a nil one would not be DeepEqual but look the same in the JSON output.

Either compare on the actual and expected structs, or use assert.Equal().

Cut over to asserting on the structs.

timoreimann · 2017-06-09T21:14:22Z

Related discussion on the Kubernetes mailing thread I just ran into: https://groups.google.com/d/topic/kubernetes-users/jkaCCykAwXA/discussion

(tl;dr: The current naming isn't too well-defined.)

ldez · 2017-06-09T21:59:38Z

@timoreimann design review before review please. 😉

timoreimann · 2017-06-09T22:08:57Z

@ldez true. however, the only open point left seems to be the one from @dtomcej's comment, which should be unrelated to my comments.

dtomcej · 2017-06-09T22:53:31Z

My concern is that every other rule we have is project wide. It would seem weird IMO to have rule types lumped in that ONLY work on kubernetes ingresses.

timoreimann · 2017-06-11T06:25:34Z

I agree that it'd look weird on first sight. IMHO though, we might lose more than we can win by spreading the rule for a few reasons:

We have already established a common, consistent naming scheme across all providers (mostly surrounding the notion of paths) which the new name doesn't fit in well (which obviously is because it comes from an external source). I can imagine non-Kubernetes users being irritated by this, and our only answer would be "it's because of Kubernetes".
Even if we made it a global rule, we likely can't even make it consistent within Traefik: The ingress.kubernetes.io/ prefix would only exist with Kubernetes while all the other providers would come without it. I'm concerned this may lead to more confusion.
The Kubernetes provider already diverges from the other ones: you only specify the rule type but not the entire rule, and AddPath doesn't exist yet. The reason from my view is that Ingress already provides a specification that we have to cope with. I'm not entirely sure if we can solve the problem in a consistent manner. I'm inclined to say that Kubernetes is "special" and that we shouldn't necessarily try to squeeze it into our existing scheme.
The way I interpreted the mailing thread I posted above is that the Kubernetes maintainers themselves aren't overly happy with the naming. To me there's still room for the naming to change -- at least, it's nothing like an official standard for now. So I'd rather not have it spread to other providers until there's reasonable certainty.
If we were to do this for one Kubernetes rule, we'd have to do it for future ones coming as well. (The list is there already.) This would multiply my consistency concerns.

Regardless of how we decide on this point, do we agree that this PR is okay to proceed? AFAICS, there's nothing in it that would affect any future decision to the extent we're concerned about. Maybe we can then postpone the bigger decision to a dedicated issue?

mlaccetti · 2017-06-13T13:49:29Z

@timoreimann I've made the changes, though I have no idea if I made it any better. :D

timoreimann · 2017-06-13T13:51:57Z

@mlaccetti seems better on first sight to me. :-)

@dtomcej WDYT, should we treat the should-or-should-not-be-a-project-wide-rule-type question separate from the functionality this PR provides?

dtomcej · 2017-06-13T15:13:45Z

@timoreimann Yes, that discussion should be a separate Proposal for handling these sort of annotation updates. More will come.

timoreimann

Moar comments. :-)

timoreimann · 2017-06-14T06:48:22Z

provider/kubernetes/kubernetes.go

@@ -294,6 +291,25 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
 	return &templateObjects, nil
 }

+func setPath(pa v1beta1.HTTPIngressPath, i *v1beta1.Ingress, templateObjects types.Configuration, r v1beta1.IngressRule) {
+	if len(pa.Path) > 0 {


Can we reduce the set of input parameters to the bare minimum needed for the implementation? For instance, it looks like we don't need the entire Ingress struct, just the annotations.

timoreimann · 2017-06-14T06:51:03Z

provider/kubernetes/kubernetes.go

@@ -294,6 +291,25 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
 	return &templateObjects, nil
 }

+func setPath(pa v1beta1.HTTPIngressPath, i *v1beta1.Ingress, templateObjects types.Configuration, r v1beta1.IngressRule) {


Another approach to implement this function might be to just have it return the computed rule and let the caller assign it to the configuration. That way, we don't need to pass in the configurations.

I twiddled the implementation from setPath to getRuleForPath, and, assuming a rule is found, apply it. That should address the two issues you highlighted.

timoreimann · 2017-06-14T06:52:52Z

provider/kubernetes/kubernetes_test.go

-	if !reflect.DeepEqual(actual, expected) {
-		t.Fatalf("expected %+v, got %+v", string(expectedJSON), string(actualJSON))
-	}
+	assert.Equal(t, expected, actual, "The rewritten data did not match the expected.")


I think we can drop the extra message. testify will tell us by default that the expected and actual values mismatch.

Removed the extra message - legacy of JUnit where you don't always get a "nice" error message, and spitting out some words helped make more sense of things.

timoreimann

Thanks a lot! LGTM! 👏

mlaccetti · 2017-06-28T02:09:06Z

@timoreimann The build is failing, but looks like a glitch with Semaphore CI - any chance you can kick it? Otherwise, I'll try to find some code to change, then revert, just to force it.

timoreimann · 2017-06-28T10:56:27Z

@mlaccetti updating the branch should fix things. could you do that please?

timoreimann · 2017-06-28T10:56:44Z

@mlaccetti please rebase instead of merging.

mlaccetti · 2017-06-28T16:10:09Z

@timoreimann Sorry, hitting the update branch button in GitHub, seems to do a merge instead of a rebase. (And, to be honest, I haven't learned how to do a rebase, either.)

Hopefully somebody can merge the PR, now!

timoreimann · 2017-06-28T17:03:28Z

@mlaccetti no worries. PR looks good now!

By the way: rebasing is as simple as doing the following two git operations:

git fetch origin master (or whatever your local repository name for containous/traefik is).
(in your local PR branch:) git rebase origin/master

That'll unwrap all local commits from your PR branch temporarily; fast-forward the branch to the latest of upstream/master; and reapply your local commits again. Naturally, you may have to resolve conflicts just like with a regular merge (though in a rebase, the roles of "mine" and "theirs" are swapped).

timoreimann · 2017-06-28T17:04:33Z

@dtomcej @errm and others: PTAL. :-)

errm

LGTM

The change seems clean to me, and the tests look good.

I am honestly not that bothered by divergence from the other providers, if we are doing something that provides compatibility with a "defacto" standard from the kubernetes ecosystem...

I was thinking about some other options WRT this that we could explore in the future... e.g. providing an alternate kubernetes provider that used a CustomResourceDefinition closely aligned to traefik...

dtomcej

Clean implementation.

LGTM

kachkaev · 2017-08-30T12:10:04Z

@mlaccetti could you please consider adding an example for rewrite-target to https://github.com/containous/traefik/blob/master/docs/configuration/backends/kubernetes.md? Do you think it could solve the trailing slash issue? #563 (comment), #563 (comment)?

mlaccetti · 2017-09-02T16:57:26Z

@kachkaev I'll see if I can find some time this week to add the example in - could arguably solve the problem - just have two ingresses! :)

kachkaev · 2017-09-14T09:49:39Z

ping @mlaccetti :–)

ldez · 2018-01-08T22:16:13Z

@kachkaev look at #2676

mlaccetti added 2 commits June 8, 2017 15:20

Adding support for ingress.kubernetes.io/rewrite-target

b0c6c61

We create a rule using the `PathPrefixStrip` to trim out the bit in the rewrite rule.

Looks like we want to simply replace the path

2024c91

ldez added area/provider/k8s/ingress area/rules kind/enhancement a new or improved feature. status/1-needs-design-review labels Jun 8, 2017

Turns out I broke the formatting

07e1f7d

ldez changed the title ~~kubernetes ingress rewrite-target implementation - for #1722~~ kubernetes ingress rewrite-target implementation Jun 8, 2017

timoreimann suggested changes Jun 9, 2017

View reviewed changes

Updated as per the code review

ec3e104

ldez added status/2-needs-review and removed status/1-needs-design-review labels Jun 12, 2017

mlaccetti added 2 commits June 12, 2017 11:28

Format, yarr

e24be6e

Pulled the setPath code out into a function

406b752

Merge branch 'master' into master

429a0ce

timoreimann suggested changes Jun 14, 2017

View reviewed changes

timoreimann approved these changes Jun 28, 2017

View reviewed changes

ldez removed the contributor/waiting-for-corrections label Jun 28, 2017

Merge branch 'master' into master

08c7f91

timoreimann requested review from juliens, ldez, emilevauge and nmengin June 28, 2017 17:04

mlaccetti added 2 commits July 2, 2017 11:07

Merge branch 'master' into master

784e6ff

Merge branch 'master' into master

8e596a5

errm approved these changes Jul 7, 2017

View reviewed changes

Merge branch 'master' into master

80783bb

dtomcej approved these changes Jul 7, 2017

View reviewed changes

Merge branch 'master' into master

c65c167

ldez added status/3-needs-merge and removed status/2-needs-review labels Jul 7, 2017

ldez merged commit 41dd124 into traefik:master Jul 7, 2017

ldez removed the status/3-needs-merge label Jul 7, 2017

ldez added this to the 1.4 milestone Jul 7, 2017

krallistic mentioned this pull request Oct 1, 2017

nginx-specific annotations should say nginx kubernetes/ingress-nginx#1436

Closed

onitake mentioned this pull request Sep 19, 2018

Documentation should not mention ingress controller specific annotations [File: /content/en/docs/concepts/services-networking/ingress.md] kubernetes/website#10272

Closed

2 tasks

kubernetes ingress rewrite-target implementation #1723

kubernetes ingress rewrite-target implementation #1723

Conversation

mlaccetti commented Jun 8, 2017

timoreimann commented Jun 8, 2017

ldez commented Jun 8, 2017

mlaccetti commented Jun 8, 2017 • edited Loading

ldez commented Jun 8, 2017

dtomcej commented Jun 9, 2017 • edited Loading

timoreimann commented Jun 9, 2017

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

timoreimann commented Jun 9, 2017

ldez commented Jun 9, 2017

timoreimann commented Jun 9, 2017

dtomcej commented Jun 9, 2017

timoreimann commented Jun 11, 2017 • edited Loading

mlaccetti commented Jun 13, 2017

timoreimann commented Jun 13, 2017 • edited Loading

dtomcej commented Jun 13, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

mlaccetti commented Jun 28, 2017

timoreimann commented Jun 28, 2017

timoreimann commented Jun 28, 2017

mlaccetti commented Jun 28, 2017

timoreimann commented Jun 28, 2017

timoreimann commented Jun 28, 2017

errm left a comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

kachkaev commented Aug 30, 2017

mlaccetti commented Sep 2, 2017

kachkaev commented Sep 14, 2017

ldez commented Jan 8, 2018

mlaccetti commented Jun 8, 2017 •

edited

Loading

dtomcej commented Jun 9, 2017 •

edited

Loading

timoreimann commented Jun 11, 2017 •

edited

Loading

timoreimann commented Jun 13, 2017 •

edited

Loading