Do not allow to override kingress's ingress.class via ksvc's annotation

[nak3]

In what area(s)?

/area API /area networking

Describe the feature

Currently ingress.class in Kingress is overriden via annotation as the following steps:

1. Add ingress-class annotating to ksvc

$ oc annotate ksvc hello-example networking.knative.dev/ingress-class=foo

or

$ oc annotate ksvc hello-example networking.knative.dev/ingress.class=foo

2. the annotation in Kingress is overriden.

$ kubectl get king -o yaml |grep ingress.class
      networking.knative.dev/ingress.class: foo

This causes a problem - for example, a managed service supports Kourier only (users cannot modify config-network) but they try to customize it via annotation and cause a problem. Hence, we would like to disallow to override the annotation. i.e. do not propagate the ingress class annotation into kingress from ksvc's annotation.

Side question

Is there use-case for the annotation override? If somebody uses multiple Ingress on the same cluster, we should not change this...

[nak3]

they try to customize it via annotation and cause a problem.

For more detail about the "problem", changing annotation makes a zombie Kingress like:

1. add an annotation

$ oc annotate ksvc hello-example networking.knative.dev/ingress-class=foo

2. delete ksvc but kingress remains as a zombie.

$ kubectl delete ksvc --all
service.serving.knative.dev "hello-example" deleted

$ kubectl get king
NAME            READY   REASON
hello-example   True

$ kubectl delete king hello-example
ingress.networking.internal.knative.dev "hello-example" deleted
^C

$ kubectl get king
NAME            READY   REASON
hello-example   True

[KauzClay]

hey @nak3

Is there use-case for the annotation override? If somebody uses multiple Ingress on the same cluster, we should not change this...

I've never done this for a production setting, but just when hacking on my own, I like to have kourier and contour installed sometimes, and the annotation is nice for that.

Hence, we would like to disallow to override the annotation. i.e. do not propagate the ingress class annotation into kingress from ksvc's annotation.

So, would you like this to be an always-on type of disallow? Or are you okay with a config option to allow/disallow the override?

[nak3]

I'm fine with having a configuration option to enable or disable the override.

That being said, I want to confirm if having multiple different ingresses on the same cluster is supported. As you mentioned, it might be a development scenario or possibly a migration scenario, but there is currently no specific test case for it in the e2e testing, so we cannot guarantee that there won't be any breaking changes in the future.

[KauzClay]

I want to confirm if having multiple different ingresses on the same cluster is supported.

Hmm yeah, I don't know the answer to that. @dprotaso do you happen to know?

At the very least, I suppose we can make the default behavior of a new config option to be the same as things are currently.

Your scenario only allows for 1 ingress (kourier), right?

[KauzClay]

anyway, as a breadcrumb for someone who picks this issue up, it sounds like a possible way to start this is to add a config option to config-network. Not set in stone (default behavior might still be up for debate, for instance), but perhaps something like:

    # ingress-annotation-override specifies whether the
    # Route annotation "networking.knative.dev/ingress-class" can override the
    # value for ingress-class.
    #
    # Default is to allow the override. 
    #
    # This means that if ingress-class is set to
    # "istio.ingress.networking.knative.dev" and a Knative Service has the
    # annotation "networking.knative.dev/ingress-class=foo", then that Knative
    # Service will be handled by "foo" ingress provider.
    #
    # When ingress-annotation-override is set to "Disabled", a Knative Service
    # with the annotation "networking.knative.dev/ingress-class=foo" will be
    # handled by the value specified in ingress-class, ignoring the annotation.
    ingress-annotation-override: "Enabled"

[nak3]

If multiple ingresses are(or will be) supported, some other variation also could be consider like:

    # ingress-class-set specifies the set of ingress class supported on the
    # clster.
    #
    # Default is to allow the ingress class specified in `ingress-class` only.
    #
    # If you want to allow multiple ingress class, you can configure them like:
    # ingress-class-set: "istio.ingress.networking.knative.dev, kourier.ingress.networking.knative.dev"
    #
    ingress-class-set: ""

[dprotaso]

I want to confirm if having multiple different ingresses on the same cluster is supported.

Hmm yeah, I don't know the answer to that. @dprotaso do you happen to know?

Even though we don't advertise it I don't want to break existing users who have used this. There was also this comment where a user in production switched from kourier to contour.

It does require re-creating your services - but it lets you do gradually vs all at once.