Ingress V1: support IngressClass #2146

jpeach · 2020-01-23T22:48:13Z

Ingress V1 changes the IngressClass string to a document.

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md#ingress-class

Promoting the annotation as it is currently defined as an opaque string is the most direct path but precludes any future enhancements to the concept.

An alternative is to create a new resource IngressClass to take its place. This resource will serve a couple of purposes:

Define the set of valid classes available to the user. Gives operators control over allowed classes.
Allow us to evolve the API to express concepts such a levels of service associated with a given Ingress controller.

cmluciano/kubernetes#3

stevesloka · 2020-06-16T18:03:47Z

We need to add a flag I think to buy into this appropriately before making this standard.

I'm proposing an --enable-ingress-v1 feature flag that puts Contour into a mode that enables the v1 features and changes the default behavior of how Ingress Classes are handled (e.g. annotation vs spec.class).

Today Contour matches on the following rules:

If no ingress.class is specified on Contour args
- Matches ingress.class of contour or no annotation
If ingress.class is specified on Contour args
- Matches only ingress.class equal to the one specified in args, any other object is ignored if not matching or the annotation is not supplied

The new Ingress v1 adds a ingressclassname field on the Ingress spec which references an IngressClass object.

The docs say that the annotation should take precedence over the spec.ingressclassname. If spec.ingressclassname does not match an IngressClass object then no match, however, we can have an IngressClass object that is the "default" by using the annotation: ingressclass.kubernetes.io/is-default-class: "true".

What we need is a way to know when we're going to enforce the ingressv1 behavior by requiring ingressclasses.

stevesloka · 2020-06-16T19:06:54Z

The other way to think about this is if not using annotations, we could just require a default ingressclass to be created. I think that makes sense, but I want to chat about it further first.

stevesloka · 2020-06-22T15:13:37Z

Here are the rules I've come up with which I think covers all the cases we could run into:

If --ingress-class-name is not set:

Matches any ingress object without any annotation
Matches any ingress object with annotation the default ingress.class of contour
Matches any ingress object, if a v1beta1.IngressClass exists with the name of contour and the ingress object has the spec.ingressclassname: contour
Matches any ingress object, if a v1beta1.IngressClass exists and is annotated as the "default" class (i.e. ingressclass.kubernetes.io/is-default-class, no other classes are defined as "default" (see: https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)

If --ingress-class-name is set:

Only matches an ingress object with annotation equaling the --ingress-class-name
Matches any ingress object, if a v1beta1.IngressClass exists with the name matching --ingress-class-name and the ingress object has the spec.ingressclassname matching the arg
Matches any ingress object, if a v1beta1.IngressClass exists with the name matching --ingress-class-name, is annotated as the "default" class (i.e. ingressclass.kubernetes.io/is-default-class, and no other classes are defined as "default" (see: https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)

If both annotation & spec.ingressclassname are defined, the annotation is attempted to match before the ingressclassname. If the annotation does not match then the object does not match and any logic to match the ingressclassname is not attempted.

Note: This does re-use the --ingress-class-name flag to Contour. I thought about adding a new one to the config file, but thought it might get too confusing having two with the same name but different use-cases. We could add this to the config file as a way to deprecate the command line flag, but not as important as defining the rules.

skriss · 2020-06-22T20:25:38Z

Matches any ingress object, if a v1beta1.IngressClass exists with the name of contour and the ingress object has the spec.ingressclassname: contour

From the docs it looks like the new IngressClass resource will have a spec.controller field, so I'm thinking we might want to match based on that field, rather than the name of the IngressClass -- i.e. match any Ingress that's linked to an IngressClass with a spec.controller: contour, regardless of the name of the IngressClass.

stevesloka · 2020-06-22T20:32:26Z

We need to only filter on the spec.controller (like projectcontour.io/ingress-controller), but you should be able to have multiple IngressClasses and an instance of Contour to match each one. This way you could split out which instance of Contour handled which instance of that ingress object.

skriss · 2020-06-22T20:44:25Z

you should be able to have multiple IngressClasses and an instance of Contour to match each one. This way you could split out which instance of Contour handled which instance of that ingress object.

So I guess in that scenario, the user would need to specify --ingress-class-name for each instance of Contour.

I was thinking about the scenario where they haven't specified --ingress-class-name, and that it could make more sense to match any ingress class with a spec.controller: projectcontour.io/ingress-controller. But, I guess requiring the name to be contour would work fine too.

stevesloka · 2020-06-22T20:55:57Z

Yup if you want to use multiple instances of Contour in the same cluster, you'd have to set --ingress-class-name for each.

If you do not supply --ingress-class-name, then we'll match anything regardless of the "default" ingress class. Is this is the missing gap I think in moving from annotations to ingress class. We should almost require someone to create the default ingress class, but that's a breaking change I think we should hold until ingress hits v1 officially.

jpeach · 2020-06-22T22:05:19Z

I read those rules 3 times and I'm not sure that I have got them right. That suggests to me that it's too complicated and we should simplify.

What if we add a flag to enable ingress class resource. This acts an an opt-in, and is probably required anyway since you need to specify a namespace and name for the object. The config associated with the ingress class resource can also specify the corresponding ingress class name, so we can statically know (and validate) the correspondence.

youngnick · 2020-06-23T03:27:20Z

I thought you only specified a name for an IngressClass object, implying that they're effectively global? If that's the case, then @stevesloka's proposal is the best we can get, because it's part of the implementation contract.

Adding a flag is not as simple as it sounds, because enabling the flag requires drawing a line under what version of Kubernetes you're running on. I'm not sure what version the new object was actually added in. That said, there's also no good way to programmatically enable it without doing #2219.

stevesloka · 2020-06-25T21:05:35Z

@youngnick I found another interesting case. If I insert a bunch of objects that reference an IngressClass that does not yet exist, then the objects are thrown out of our cache, however, if later the IngressClass is created, they should then all be valid, but they aren't in the cache.

I was going to move the logic to validate the ingress objects to the dag.Builder, but wanted to check in first. Thoughts?

youngnick · 2020-06-26T01:04:28Z

If we move the logic to valid the ingress objects to the Builder, that will mean that the cache will always hold all ingress objects.

That doesn't seem ideal, but the only other way I can think of to deal with this is to have a new relevant ingress class kick off a cache refresh for all the ingress objects, which also doesn't seem ideal.

This is also relevant because we will most likely have a similar problem with GatewayClass.

@jpeach, @skriss, thoughts on keeping ingress-class relevant objects in the cache, and bring them in and out of scope with filtering? The original conception of the Contour cache is that it only holds objects needed for the DAG build, so it is a bit of a change.

…gress objects Add support for IngressClass objects which match the controller name of 'projectcontour.io/ingress-controller'. Any Ingress object which defines an 'Spec.IngressClassName', Contour will validate that a corresponding IngressClass exists. Current ingress class logic for Contour is not affected and still take priority over any IngressClass logic. Signed-off-by: Steve Sloka <[email protected]>

youngnick · 2021-01-24T23:29:00Z

I think that @stevesloka's rules in that comment make sense, but implementing them means that Contour will need to keep all Ingress objects and all IngressClass objects in its cache, and filter them on DAG run. I think it's okay, it's a memory cost per Contour, but worth spending memory on, as the other options are many times more complicated.

I'm not sure if the default IngressClass behavior is implemented in a core Kube controller, or if we're supposed to do it. I suspect the latter, tbh.

The reason I was blocking this on the decision ticket (#2809 ) was that we needed to figure out if we would support multiple *Class objects in a single Contour, or not. We've decided not to do that, so the rules above are perfect. @stevesloka, sorry to have delayed this.

skriss · 2021-01-25T16:06:21Z

I'm not sure if the default IngressClass behavior is implemented in a core Kube controller, or if we're supposed to do it. I suspect the latter, tbh.

It's implemented by Kube - try creating an IngressClass annotated as default, and then an ingress with no class:

IngressClass:

apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  name: foo
  annotations:
    "ingressclass.kubernetes.io/is-default-class": "true"
spec:
  controller: projectcontour.io/contour

Ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: basic
spec:
  rules:
  - host: localhost
    http:
      paths:
      - pathType: Prefix
        path: /
        backend:
          service:
            name: s1
            port:
              number: 80

Now get the Ingress YAML (irrelevant bits removed):

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: basic
  namespace: default
spec:
  ingressClassName: foo
  rules:
  - host: localhost
    http:
      paths:
      - backend:
          service:
            name: s1
            port:
              number: 80
        path: /
        pathType: Prefix
status:
  loadBalancer:
    ingress:
    - ip: xx.xxx.xx.xxx

Note ingressClassName has been set to foo.

youngnick · 2021-01-26T22:46:48Z

Awesome, thanks @skriss !

xaleeks · 2021-02-22T21:07:41Z

is this in the v1.13 or has it been pushed out to v1.14?

sunjayBhatia · 2021-03-02T19:43:03Z

Cross referencing an Ingress class (albeit Ingress v1beta1 annotation) request: projectcontour/contour-operator#228

sunjayBhatia · 2021-03-02T22:02:34Z

@stevesloka BTW are you still actively working on this?

stevesloka · 2021-03-03T19:19:34Z

No @sunjayBhatia I'll unassign. I had an impl, then it was closed, created the design doc, then here we are. Feel free to work on it or anyone else can pick up. =)

sunjayBhatia · 2021-03-25T19:04:50Z

Another edge case related to default IngressClass:

Insert 2 IngressClasses both annotated to be default
Insert an Ingress with no annotation or Spec.IngressClassName -> Correctly see that it is rejected
Insert an Ingress with a Spec.IngressClassName set
Modify that Ingress to remove Spec.IngressClassName -> Update not rejected

sunjayBhatia · 2021-03-25T21:40:49Z

Is there any reason we would want to configure routes for an Ingress with an ingress class annotation that is not what contour is looking for but an ingress class name in the spec that is?

e.g.

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test
  annotations:
    kubernetes.io/ingress.class: notcontour
spec:
  ingressClassName: contour

skriss · 2021-03-25T21:53:00Z

The KEP had some info about priority here - from https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1453-ingress-api#interoperability-with-previous-annotation:

When both the class field and annotation are set, the annotation will take priority. The controller MAY emit a warning event if the user sets conflicting (different) values for the annotation and field.

sunjayBhatia · 2021-03-25T21:55:31Z

The KEP had some info about priority here - from https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1453-ingress-api#interoperability-with-previous-annotation:

When both the class field and annotation are set, the annotation will take priority. The controller MAY emit a warning event if the user sets conflicting (different) values for the annotation and field.

Oh perfect thanks for that!

sunjayBhatia · 2021-03-25T21:57:08Z

There is a v1beta1.IngressClass type as well as the v1.IngressClass type, is it worth also watching that one? Or should we just scope to the new version and get away from the old version (we still support v1beta1.Ingress but convert to v1.Ingress internally, we could do the same for IngressClass but would have to add an RBAC rule I believe)

sunjayBhatia · 2021-03-25T22:26:15Z

Re: Matches any ingress object, if a v1beta1.IngressClass exists and is annotated as the "default" class (i.e. ingressclass.kubernetes.io/is-default-class, no other classes are defined as "default" (see: https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)

I'm not sure I agree with this rule:

if you only have 1 IngressClass marked as default, any Ingresses that get created will be assigned that class name: Ingress V1: support IngressClass #2146 (comment)
If you have multiple marked as default, you shouldn't be able to create an Ingress without a specific class name set
The edge case mentioned here should probably be fixed in the admission controller

sunjayBhatia · 2021-03-25T22:29:46Z

Similar to the above, Re Matches any ingress object, if a v1beta1.IngressClass exists with the name matching --ingress-class-name, is annotated as the "default" class (i.e. ingressclass.kubernetes.io/is-default-class, and no other classes are defined as "default" (see: https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)

It seems to me that we can ignore this rule? Seems like we should just ensure we only configure routes for Ingresses that match the configured class name and not have to worry about what is default, the admission controller etc. should ensure new Ingress objects have a class set if they are created without one and there is a default, and should ensure a specific class is set if there are multiple defaults

skriss · 2021-03-25T22:44:59Z

Similar to the above, Re Matches any ingress object, if a v1beta1.IngressClass exists with the name matching --ingress-class-name, is annotated as the "default" class (i.e. ingressclass.kubernetes.io/is-default-class, and no other classes are defined as "default" (see: https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)

It seems to me that we can ignore this rule? Seems like we should just ensure we only configure routes for Ingresses that match the configured class name and not have to worry about what is default, the admission controller etc. should ensure new Ingress objects have a class set if they are created without one and there is a default, and should ensure a specific class is set if there are multiple defaults

Yep I agree with that, Contour shouldn't have to worry about what's the default ingress class, it just looks at what's on the Ingress vs. what's been configured for Contour to watch.

sunjayBhatia · 2021-03-25T23:35:44Z

What happens when you create an Ingress with a valid IngressClassName that Contour is watching but the actual IngressClass does not exist?

youngnick · 2021-03-26T00:17:08Z

There is a v1beta1.IngressClass type as well as the v1.IngressClass type, is it worth also watching that one? Or should we just scope to the new version and get away from the old version (we still support v1beta1.Ingress but convert to v1.Ingress internally, we could do the same for IngressClass but would have to add an RBAC rule I believe)

We should be moving to v1.Ingress everywhere as much as possible, as v1beta1.Ingress will be removed from newer versions of Kubernetes soon, and v1.Ingress has been present since 1.16.

youngnick · 2021-03-26T00:21:07Z

What happens when you create an Ingress with a valid IngressClassName that Contour is watching but the actual IngressClass does not exist?

The KEP says

The IngressClass resource is an optional way to provide additional configuration for a specific class of Ingress.
So, since we don't support extra config via the IngressClass, it seems like it's actually mostly irrelevant for Contour.

To put that another way, the IngressClass object itself is used for:

marking which thing is the default, which is done by the core admission controllers.
holding extra configuration, which we don't currently support.

So, it turns out we should have read access to them in case we add that later, and to check for default setup weirdness, but aside from that, we should just be able to key off the field in the spec.

sunjayBhatia · 2021-03-26T00:38:03Z

Somewhat a sad I have to go back to the KEP to read the spec and it's not on a site like we have with Gateway API :(

jpeach added the area/ingress Issues or PRs related to the Ingress API. label Jan 23, 2020

jpeach changed the title ~~Inverses V1: support IngressClass~~ Ingress V1: support IngressClass Jan 23, 2020

jpeach mentioned this issue Jan 23, 2020

Support Ingress V1 #2139

Closed

6 tasks

stevesloka self-assigned this May 18, 2020

stevesloka mentioned this issue May 21, 2020

add support for ingressv1.Spec.IngressClassName #2548

Closed

youngnick mentioned this issue Jun 2, 2020

HTTPProxy lacking ingressClassName field #2566

Closed

skriss mentioned this issue Jan 25, 2021

internal: Add support for watching ingress/v1 resources #3266

Merged

sunjayBhatia added this to the 1.14.0 milestone Mar 2, 2021

sunjayBhatia mentioned this issue Mar 2, 2021

Add option for Contour's --ingress-class-name projectcontour/contour-operator#228

Closed

stevesloka removed their assignment Mar 3, 2021

sunjayBhatia self-assigned this Mar 25, 2021

sunjayBhatia mentioned this issue Mar 26, 2021

Ingress: Filter Ingress resources based on Spec.IngressClassName #3520

Merged

sunjayBhatia closed this as completed in #3520 Mar 31, 2021

LukeShortCloud mentioned this issue Oct 13, 2021

[virtualization][kubernetes_administration] Set default IngressClass LukeShortCloud/rootpages#509

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingress V1: support IngressClass #2146

Ingress V1: support IngressClass #2146

jpeach commented Jan 23, 2020

stevesloka commented Jun 16, 2020

stevesloka commented Jun 16, 2020

stevesloka commented Jun 22, 2020

skriss commented Jun 22, 2020

stevesloka commented Jun 22, 2020

skriss commented Jun 22, 2020

stevesloka commented Jun 22, 2020

jpeach commented Jun 22, 2020

youngnick commented Jun 23, 2020

stevesloka commented Jun 25, 2020

youngnick commented Jun 26, 2020

youngnick commented Jan 24, 2021

skriss commented Jan 25, 2021

youngnick commented Jan 26, 2021

xaleeks commented Feb 22, 2021

sunjayBhatia commented Mar 2, 2021

sunjayBhatia commented Mar 2, 2021

stevesloka commented Mar 3, 2021

sunjayBhatia commented Mar 25, 2021 •

edited

Loading

sunjayBhatia commented Mar 25, 2021

skriss commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021 •

edited

Loading

skriss commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

youngnick commented Mar 26, 2021

youngnick commented Mar 26, 2021

sunjayBhatia commented Mar 26, 2021

Ingress V1: support IngressClass #2146

Ingress V1: support IngressClass #2146

Comments

jpeach commented Jan 23, 2020

stevesloka commented Jun 16, 2020

stevesloka commented Jun 16, 2020

stevesloka commented Jun 22, 2020

skriss commented Jun 22, 2020

stevesloka commented Jun 22, 2020

skriss commented Jun 22, 2020

stevesloka commented Jun 22, 2020

jpeach commented Jun 22, 2020

youngnick commented Jun 23, 2020

stevesloka commented Jun 25, 2020

youngnick commented Jun 26, 2020

youngnick commented Jan 24, 2021

skriss commented Jan 25, 2021

youngnick commented Jan 26, 2021

xaleeks commented Feb 22, 2021

sunjayBhatia commented Mar 2, 2021

sunjayBhatia commented Mar 2, 2021

stevesloka commented Mar 3, 2021

sunjayBhatia commented Mar 25, 2021 • edited Loading

sunjayBhatia commented Mar 25, 2021

skriss commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021 • edited Loading

skriss commented Mar 25, 2021

sunjayBhatia commented Mar 25, 2021

youngnick commented Mar 26, 2021

youngnick commented Mar 26, 2021

sunjayBhatia commented Mar 26, 2021

sunjayBhatia commented Mar 25, 2021 •

edited

Loading

sunjayBhatia commented Mar 25, 2021 •

edited

Loading