-
Notifications
You must be signed in to change notification settings - Fork 266
Seperate concept of provider and class #202
Conversation
Allows use of a single supported ingress class
…ovider The "kubernetes.io/ingress.class" annotation shouldn't be used to determine which proivder to use. There's now a "kubernetes.io/ingress.provider" which can be "nginx" or "gce" (unless you implement your own).
I like this approach more than my proposal on #195. Any updates whether this may be merged? @jackhopner there is a couple of conflicts with master, would you be willing to fix it? |
@rochacon Fixed up the conflicts :) |
Thanks for this! I'm not a big fan of the |
func (i *Ingress) IngressProvider() string { | ||
val, ok := i.IngressApi.Annotations[kubelego.AnnotationIngressProvider] | ||
if !ok { | ||
return i.kubelego.LegoDefaultIngressProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some more thought, I've realised this may break current implementations of the GCE ingress controller - please do correct me if I'm wrong...
If a user currently has an ingress that specifies
kubernetes.io/ingress.class: "gce"
, and no ingress.provider
annotation (as they are upgrading), the ingress.provider
will default to nginx
, thus breaking their set up.
To work around this and perform these changes in a backwards compatible way, I think we need to set the ingress.provider
value to ingress.class
if it's not already defined on the ingress resource itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right to be backwards compatible you do need to do that
Hi @munnerz can we have a release of this please? |
Previously you're ingress provider and classes had to have the same name. These should be two different concepts and be used in combination.
For example if you have:
Ingress classes for nginx-internal and nginx-external but want them to both be provided by nginx then if you don't have the provider and class as two different concepts this isn't possible.
Below is an example ingress resource with the new annotation: