Allow any kubernetes ingressClass value #3516
Conversation
|
The prefix restriction was originally introduced along #2222. Back then, we concluded that requiring the prefix would provide some basic means of protection against accidentally hooking up to another Ingress controller (by means of a poor man's "namespacing" or scoping). @rtreffer and I had a discussion on Slack about the subject preceding this PR. Given the use case he mentioned, I feel that our original assumption about protecting with the help of the prefix does not hold anymore (or never did :-) ). I feel we should drop it in order to facilitate smooth transitions from one controller to another. @dtomcej / @containous/kubernetes WDYT? |
|
In 1.7, we introduce dynamic namespacing, and ingress status updates. Being that we are now modifying the ingress objects, allowing users to set "nginx" as the ingress class could lead to problems, if there are multiple controllers trying to satisfy the ingress...however, the user is ultimately the cause. I guess the other issue was at the time, we didn't support many of the annotations, and we wanted to ensure that users knew that they would have to modify ingresses that were written for other controllers. Now we support a much larger annotation set, so 🤷♂️ I'm willing to allow it to be more flexible, and let users "figure it out"... |
timoreimann
added
status/1-needs-design-review
and removed
status/0-needs-triage
labels
timoreimann
added
status/2-needs-review
and removed
status/1-needs-design-review
labels
|
|
||
| It is also possible to set the `ingressClass` option in Træfik to a particular value. | ||
| If that's the case and the value contains a `traefik` prefix, then only those Ingress objects matching the same value will be processed. | ||
| It is also possible to set the `ingressClass` option in Træfik to a particular value. Træfik will only process matching Ingress objects. |
There was a problem hiding this comment.
Should add a note that having multiple controllers satisfying an ingress can lead to unintended behavior
There was a problem hiding this comment.
Added a !!! note underneath with a recommendation to prefix ingressClasses.
| iAnnotation(annotationKubernetesIngressClass, "custom"), | ||
| iRules( | ||
| iRule( | ||
| iHost("herp"), |
There was a problem hiding this comment.
Could you replace the herp/derp with foo/bar or foo/path to match the format of other tests?
| backends( | ||
| backend("herp/derp", | ||
| servers( | ||
| server("http://example.com", weight(1)), |
There was a problem hiding this comment.
could you update this to be an IP/Port combo instead of a URL?
Currently that is only implemented as a URL when its an externalName service IIRC
There was a problem hiding this comment.
Looks good overall (bar some extra manual testing maybe). Just a minor thing:
@rtreffer could you please also drop the restricting note from the parameter documentation in /docs/configuration/backends/kubernetes.md?
Thanks.
| @@ -54,8 +54,6 @@ See also [Kubernetes user guide](/user-guide/kubernetes). | |||
| # 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. | |||
| # | |||
There was a problem hiding this comment.
@timoreimann dropped.
docs/user-guide/kubernetes.md
Outdated
| @@ -863,6 +863,10 @@ If the annotation is missing, contains an empty value, or the value `traefik`, t | |||
| It is also possible to set the `ingressClass` option in Træfik to a particular value. Træfik will only process matching Ingress objects. | |||
| For instance, setting the option to `traefik-internal` causes Træfik to process Ingress objects with the same `kubernetes.io/ingress.class` annotation value, ignoring all other objects (including those with a `traefik` value, empty value, and missing annotation). | |||
|
|
|||
| !!! note | |||
| Letting multiple ingress controllers handle the same ingress objects can lead to uninteded behavior. | |||
There was a problem hiding this comment.
Typo: _ uninteded_ -> _ unintended_
docs/user-guide/kubernetes.md
Outdated
| @@ -863,6 +863,10 @@ If the annotation is missing, contains an empty value, or the value `traefik`, t | |||
| It is also possible to set the `ingressClass` option in Træfik to a particular value. Træfik will only process matching Ingress objects. | |||
| For instance, setting the option to `traefik-internal` causes Træfik to process Ingress objects with the same `kubernetes.io/ingress.class` annotation value, ignoring all other objects (including those with a `traefik` value, empty value, and missing annotation). | |||
|
|
|||
| !!! note | |||
| Letting multiple ingress controllers handle the same ingress objects can lead to uninteded behavior. | |||
| It is recommended to prefix all ingressClass values with traefik to avoid unintended collisions with other ingress implementations. | |||
There was a problem hiding this comment.
Could you backtick traefik so that it renders as traefik?
There was a problem hiding this comment.
Happy with the addressing of my comments; will @dtomcej cover his.
Migrating from one ingress controller to another can require an intentional collision of ingressClass values.
…tion documentation
Migrating from one ingress controller to another can require an intentional collision of ingressClass values.
What does this PR do?
The ingressClass in kubernetes can be used to run multiple ingress controllers or to otherwise split the defined ingresses into multiple sets.
The values that Traefik would allow for an ingress class must start with
traefik.This patch removes that limitation as
Motivation
I am trying to move a larger cluster over to traefik and our ingress classes were split into internal and external. Traefik can't handle these names which makes a migration harder than necessary.
More
Additional Notes
The key change is the removal of a few lines in
provider/kubernetes/kubernetes.go