Allow custom value for kubernetes.io/ingress.class annotation #2222

yuvipanda · 2017-10-07T03:07:12Z

Description

Allow configuring the values of the kubernetes.io/ingress.class annotation
on ingress objects that traefik will process. This is useful when you have
multiple traefik servers in one kubernetes cluster, and you want tight control
over which Ingresses are processed by which servers.

Fixes #2221

ldez · 2017-10-07T08:47:23Z

@containous/kubernetes WDYT? (Design review)

timoreimann · 2017-10-07T09:10:48Z

Technically, I'd tentatively approve the design.

Whether we should accept the PR though is still subject to an agreement that the existing ways to distinguish separate Traefik deployments is really insufficient. I have commented the related feature request issue for that purpose. I suggest we wait for a resolution prior to moving forward with an actual review.

timoreimann · 2017-10-08T23:36:25Z

As explained on the related issue, the desirable feature is to have a subset of Traefik controllers ignore certain Ingresses by default unless intent to process is signaled explicitly (i.e., opt-in instead of opt-out). Traefik label selectors don't fulfill this requirement since the default behavior of a missing selector is to grab Ingress objects.

Acknowledging the feature request, I'm uncertain if we should reuse kubernetes.io/ingress.class for the desired purpose. As far as I know, other Ingress implementations use it to rule out a particular class of controllers only (as the name implies), not for fine-grained discriminations within a single controller. My worry is that allowing arbitrary values may collide with potential other Ingress controllers in the future, possibly becoming a source of confusion.

Maybe a better approach could be to introduce a new annotation -- I'm thinking of something including the term sub-class. One thing I'm not sure about though is if it would live under the ingress.kubernetes.io/ or a custom traefik.*/ prefix.

@aledbf any thoughts on how the feature should be best implemented in terms of annotations? Could you maybe share some information on how new ingress.kubernetes.io/ annotations get added? As far as I know, there's no standardization process available yet. Is it as simple as one Ingress controller implementation taking a lead and others following (with the lead usually being Nginx)?

Pulling in @dtomcej and @errm for additional feedback.

yuvipanda · 2017-10-08T23:53:27Z

I'll note that the nginx controller does allow you to change this (in the --ingress-class commandline parameter), which is where I got this idea from.

timoreimann · 2017-10-08T23:58:06Z

@yuvipanda thanks, I missed that! (Was only looking at the annotation documentation.)

Let's see what the folks I pinged for feedback think.

aledbf · 2017-10-08T23:58:29Z

@timoreimann the annotation kubernetes.io/ingress.class is the "proper" way to filter which ingress rules should be processed or not.

any thoughts on how the feature should be best implemented in terms of annotations?

The use of annotations is being reviewed and possible Custom Resources is going to be the way to handle the ingress configuration. I am not sure when this will discussed in #sig-network

Edit: the ingress.class annotation is present since the first Ingress iteration https://github.com/aledbf/ingress/blob/master/examples/PREREQUISITES.md#ingress-class

errm · 2017-10-09T08:25:23Z

I think that I am fine with this... I think it can cause some confusion, but since the default will behave no differently from the current situation I think this is ok.

I would want a clear paragraph explaining the options in the documentation before we merge this this though...

timoreimann · 2017-10-09T17:15:20Z

@aledbf @errm thanks for your feedback, appreciated.

I think we're good to move forward with the actual review then.

ldez

Thanks 👍

ldez · 2017-10-09T00:11:02Z

provider/kubernetes/kubernetes.go

+		ingressClassRequired = p.IngressClass
+	}
+
+	switch ingressClass {


Could you simplify like that (I know this code already exists before your changes):

return ingressClass == "" || ingressClass == ingressClassRequired

Appreciated! I think it was me who originally made this more complicated than necessary. :/

yuvipanda · 2017-10-09T18:20:53Z

w00t, thanks! Should I figure out where to add the docs and just add it now or wait for a code review ok first?

ldez · 2017-10-09T18:25:06Z

You can write the documentation right now. This can facilitate the code review.

timoreimann

Two general remarks:

We should extend TestIngressAnnotations by a new test case that sets p.IngressClass to a custom value.
We recently merged Document ways to partition Ingresses in the k8s guide. #2223 into the v1.4 which added a section on the class annotation here. I believe it would be the ideal place to extend our documentation. @ldez, any chance we can merge v1.4 into master soonish to make it available for this PR? If not, I'm okay with omitting more detailed docs out now and amending them once we have completed the merge.

timoreimann · 2017-10-09T17:18:11Z

docs/configuration/backends/kubernetes.md

@@ -51,6 +51,15 @@ See also [Kubernetes user guide](/user-guide/kubernetes).
 #
 # labelselector = "A and not B"

+# Value of `kubernetes.io/ingress.class` annotation that identify Ingress objects to be processed.


Grammar nit-pick: identify -> identifies.

timoreimann · 2017-10-09T17:23:44Z

provider/kubernetes/kubernetes.go

+	ingressClassRequired := "traefik"
+	if p.IngressClass != "" {
+		ingressClassRequired = p.IngressClass
+	}


I think we can skip the if block if we choose a default value of "" (empty string) for IngressClass.

timoreimann · 2017-10-09T19:21:55Z

docs/configuration/backends/kubernetes.md

+# will also be processed.
+#
+# Optional
+# Default: traefik (process all Ingresses)


I think we can make things easier by choosing the empty string as default. Specifically:

We don't specifically need to set IngressClass to traefik in tests.

We can simplify shouldProcessIngress (see below).

timoreimann · 2017-10-09T19:24:48Z

provider/kubernetes/kubernetes.go

@@ -135,6 +136,20 @@ func (p *Provider) Provide(configurationChan chan<- types.ConfigMessage, pool *s
 	return nil
 }

+func (p *Provider) shouldProcessIngress(ingressClass string) bool {
+	ingressClassRequired := "traefik"


We could make a constant out of this like

const ingressClassRequired = "traefik"

to indicate that this is not a string to be mutated.

That said, I don't think we need the variable/constant anyway if we decide to use the empty string as default value for p.IngressClass.

@timoreimann you should not this but use a flag with a default ("traefik"). This allows the user to deploy multiple controllers with different class annotations to expose different ingress

Ah sorry, late night logic flaw on my end. @aledbf is right, we need the default of "traefik".

@yuvipanda please ignore my comments in this regard.

So in summary, keeping the default of p.IngressClass to traefik should still allow us to simplify the method implementation like this:

func (p *Provider) shouldProcessIngress(ingressClass string) bool { return ingressClass == "" || ingressClass == p.IngressClass }

I think this should work, though only tests will bring certainty...

yuvipanda · 2017-10-11T16:38:12Z

Thanks for the review everyone! I'm caught up with other work this week, but will try to find some time within the next few days to address them!

dtomcej

Ok Sorry for being late to the party on this one.

I have a few concerns:

It appears that we are allowing custom ingress classes, but we do not do any sanity checking on them. What happens if we set the ingress class to "nginx"? Is that something we want to allow/support?

I totally get the nested controller/namespace issue that you are encountering. However, I would propose the following:

Require the traefik ingress class to be traefik or prefixed with traefik-. This would be a sanity check, and would avoid crossover with other ingress controllers etc.

Your jupyter app could create ingresses with the traefik-<namespace> or traefik-<UUID> format, and would be fine, and would function the way you expect them to, while still being visibly traefik related :) .

I am hesitant to approve fully custom classes.

timoreimann · 2017-10-13T05:34:37Z

I like the prefix idea too, even though it means diverging from the Nginx equivalent option somewhat.

yuvipanda · 2017-10-17T07:17:30Z

That looks good to me too! I'm making the required changes now.

yuvipanda · 2017-10-17T07:26:05Z

So looks like the things I need to do still are:

Figure out where defaults are set, and set default for IngressClass to 'traefik'. The only place I could find is in cmd/traefik/configuration.go. Is that the right place? I've already modified it...
Figure out where validation logic for this config goes, and make sure that this value is either 'traefik' or starts with 'traefik-'
Write an additional test.

Does that sound right?

I'm still sortof new to go, so any pointers for where to find (1) and (2) (or links to similar PRs / functionality in other providers) will be highly appreciated!

yuvipanda · 2017-10-17T07:27:05Z

(I also did a quick rebase to prevent my branch from getting too stale)

timoreimann · 2017-10-17T11:52:24Z

@yuvipanda

I think you need to add a line here.
We need to validate the annotation for every Ingress object we're inspecting. Presumably, the check should happen inside shouldProcessIngress and log in case we use an illegal prefix.
Yes. :-)

Side note: I think we should allow the prefix traefik, not only traefik-, supporting cases where users want to have something like traefik1, traefik2, traefik42, etc. The dash gives extra structure, which in my opinion should be up to the user to decide for or against.

yuvipanda · 2017-10-17T19:26:20Z

@timoreimann thank you for your quick response!

I've already done (1) early on, so I've just gotten rid of the custom check in shouldProcessIngress. I wasn't sure if (1) sets the defaults everywhere (including tests) or just when running the binary.

For (2), can you help me understand why we validating that there instead of wherever it is the config.toml file is being parsed? Wouldn't that help us error out earlier? Or do we need to do this for each ingress because traefik's configuration is so dynamic?

I'll work on tests next!

…ation

Since we set the default already in cmd/traefik/configuration.go

dtomcej

LGTM

timoreimann

LGTM -- thanks a ton to both @yuvipanda for the original effort and @nmengin for finishing off! 👏

I can take care of extending the guide in a follow up PR.

beyond-code-github · 2018-04-26T09:57:46Z

I think we should also extend the Kubernetes guide at https://github.com/containous/traefik/blob/d426126a9294af8df041e7e55cbd61b8b899a4e1/docs/user-guide/kubernetes.md#between-tr%C3%A6fik-and-other-ingress-controller-implementations.

Did this ever get completed? I just read this thread with interest but it's not clear how to make use of the new feature that this PR added.

Can I just add IngressClass = "traefik-xxx" to my .toml file?

timoreimann · 2018-04-26T10:36:55Z

@beyond-code-github I could have sworn that I pushed a follow-up PR to update the docs. Looks like I never did though!

Thanks for the hint, will fix that soon. In the meantime, the comment to the ingressClass option should answer your question:

# Value of `kubernetes.io/ingress.class` annotation that identifies Ingress objects to be processed.
# If the parameter is non-empty, only Ingresses containing an annotation with the same value are processed.
# Otherwise, Ingresses missing the annotation, having an empty value, or the value `traefik` are processed.
#
# Note : `ingressClass` option must begin with the "traefik" prefix.
#
# Optional
# Default: empty
#
# ingressClass = "traefik-internal"

So the answer to your question is: yes, traefik-xxx will work because it comes with the necessary traefik prefix.

beyond-code-github · 2018-04-26T10:54:37Z

Thanks for the swift response @timoreimann !

yuvipanda requested a review from a team as a code owner October 7, 2017 03:07

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

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

timoreimann added status/2-needs-review and removed status/1-needs-design-review labels Oct 9, 2017

ldez suggested changes Oct 9, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Oct 9, 2017

ldez added this to the 1.5 milestone Oct 9, 2017

timoreimann suggested changes Oct 9, 2017

View reviewed changes

dtomcej suggested changes Oct 13, 2017

View reviewed changes

nmengin removed the contributor/waiting-for-corrections label Feb 1, 2018

yuvipanda and others added 17 commits February 1, 2018 13:32

[kubernetes] Allow custom value for kubernetes.io/ingress.class annot…

46a2c9a

…ation

Specify annotation default properly

56d1e11

Fix typo

94d03b9

Rewrite superfluous switch

01c89c0

Simplify shouldProcessIngress

06f1360

Since we set the default already in cmd/traefik/configuration.go

Make sure that kubernetes.io/ingress.class values start with 'traefik'

e08c02e

Replace traefik by empty value as default

07919d8

Fix Unit tests

3741004

Move Kubernetes.shouldProcessIngress to the same place as before.

0bd9353

Fix anonymize test with empty value for Ingress annotation

a57ff33

Fix for review

4d7d273

Fix for review and add test

661f9d7

Improve test

5c76735

Fix fmt

f94c0f8

Improve terst

7beee75

Fix for review

660917c

Refacto constants

c2eb82d

nmengin removed the contributor/needs-resolve-conflicts label Feb 1, 2018

dtomcej approved these changes Feb 1, 2018

View reviewed changes

timoreimann approved these changes Feb 1, 2018

View reviewed changes

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Feb 1, 2018

traefiker merged commit 04ebd9d into traefik:master Feb 1, 2018

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

timoreimann mentioned this pull request Apr 27, 2018

Document custom k8s ingress class usage in guide. #3242

Merged

timoreimann mentioned this pull request Jun 19, 2018

Allow any kubernetes ingressClass value #3516

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom value for kubernetes.io/ingress.class annotation #2222

Allow custom value for kubernetes.io/ingress.class annotation #2222

yuvipanda commented Oct 7, 2017 •

edited by ldez

Loading

ldez commented Oct 7, 2017

timoreimann commented Oct 7, 2017

timoreimann commented Oct 8, 2017 •

edited

Loading

yuvipanda commented Oct 8, 2017

timoreimann commented Oct 8, 2017

aledbf commented Oct 8, 2017 •

edited

Loading

errm commented Oct 9, 2017

timoreimann commented Oct 9, 2017

ldez left a comment

ldez Oct 9, 2017

timoreimann Oct 9, 2017

yuvipanda commented Oct 9, 2017

ldez commented Oct 9, 2017

timoreimann left a comment

timoreimann Oct 9, 2017

timoreimann Oct 9, 2017

timoreimann Oct 9, 2017

timoreimann Oct 9, 2017 •

edited

Loading

aledbf Oct 9, 2017 •

edited

Loading

timoreimann Oct 9, 2017

timoreimann Oct 9, 2017 •

edited

Loading

yuvipanda commented Oct 11, 2017

dtomcej left a comment

timoreimann commented Oct 13, 2017

yuvipanda commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

timoreimann commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

dtomcej left a comment

timoreimann left a comment

beyond-code-github commented Apr 26, 2018

timoreimann commented Apr 26, 2018

beyond-code-github commented Apr 26, 2018

Allow custom value for kubernetes.io/ingress.class annotation #2222

Allow custom value for kubernetes.io/ingress.class annotation #2222

Conversation

yuvipanda commented Oct 7, 2017 • edited by ldez Loading

Description

ldez commented Oct 7, 2017

timoreimann commented Oct 7, 2017

timoreimann commented Oct 8, 2017 • edited Loading

yuvipanda commented Oct 8, 2017

timoreimann commented Oct 8, 2017

aledbf commented Oct 8, 2017 • edited Loading

errm commented Oct 9, 2017

timoreimann commented Oct 9, 2017

ldez left a comment

Choose a reason for hiding this comment

ldez Oct 9, 2017

Choose a reason for hiding this comment

timoreimann Oct 9, 2017

Choose a reason for hiding this comment

yuvipanda commented Oct 9, 2017

ldez commented Oct 9, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann Oct 9, 2017

Choose a reason for hiding this comment

timoreimann Oct 9, 2017

Choose a reason for hiding this comment

timoreimann Oct 9, 2017

Choose a reason for hiding this comment

timoreimann Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

aledbf Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

timoreimann Oct 9, 2017

Choose a reason for hiding this comment

timoreimann Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

yuvipanda commented Oct 11, 2017

dtomcej left a comment

Choose a reason for hiding this comment

timoreimann commented Oct 13, 2017

yuvipanda commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

timoreimann commented Oct 17, 2017

yuvipanda commented Oct 17, 2017

dtomcej left a comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

beyond-code-github commented Apr 26, 2018

timoreimann commented Apr 26, 2018

beyond-code-github commented Apr 26, 2018

yuvipanda commented Oct 7, 2017 •

edited by ldez

Loading

timoreimann commented Oct 8, 2017 •

edited

Loading

aledbf commented Oct 8, 2017 •

edited

Loading

timoreimann Oct 9, 2017 •

edited

Loading

aledbf Oct 9, 2017 •

edited

Loading

timoreimann Oct 9, 2017 •

edited

Loading