Add Ingress Class to kubectl describe ingress output

[mpuckett159]

/kind feature

What this PR does / why we need it:

This PR adds the Ingress Class to the output of kubectl describe ingress

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1160

Special notes for your reviewer:

Based on the Ingress.Spec.IngressClassName comments (linked below) it looks like this should just be blank if the name isn't specified, so I'm setting to an empty string then updating if the name is provided.

kubernetes/staging/src/k8s.io/api/networking/v1/types.go

Line 257 in 2b091f6

IngressClassName *string `json:"ingressClassName,omitempty" protobuf:"bytes,4,opt,name=ingressClassName"`

Does this PR introduce a user-facing change?

The output of `kubectl describe ingress` now includes an IngressClass name if available

[k8s-ci-robot]

Hi @mpuckett159. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sftim]

For the changelog, I'd write

The output of `kubectl describe ingress` now includes an IngressClass name if available

[mpuckett159]

Yup that's more accurate, thank you.

[lauchokyip]

/ok-to-test
/triage accepted
/priority backlog

[brianpursley]

Code looks good.

Can you squash your commits?
(ref: https://www.kubernetes.dev/docs/guide/github-workflow/#squash-commits)

[brianpursley]

Question: Is printing an empty string preferable to printing something like <none> or even omitting the field entirely from the describe output?

I don't think I have a preference, but wonder if there is already a precedent for this somewhere else in describe.

[mpuckett159]

From my reading of the ingress docs just before the Ingress rules header:

There are some ingress controllers, that work without the definition of a default IngressClass. For example, the Ingress-NGINX controller can be configured with a flag --watch-ingress-without-class. It is recommended though, to specify the default IngressClass as shown below.

An ingress does not technically require a class, which, imo, should result in printing a blank or <none> line rather than being ignored in the output. Having no class is important info since it's not recommended but technically possible. Writing <none> by default would be a good marker and definitely has precedent in the existing describe code.

I can add that if it sounds good.

[mpuckett159]

Nor do I unfortunately. The more I read about this the more it looks like I need to write a function to check a few things before populating a value.

The IngressSpec API docs show that there is also a deprecated annotation to take into account that takes precedence over the actual IngressClassName property value.

Seems similar to kubernetes/kubectl#1133, actually.

[mpuckett159]

I think what I've landed on after some reading and testing is the following:

1. check for setting by spec
2. check for setting by annotation and add warning if found
3. if IngressClassName is still an empty string, query the api for IngressClass objects and search for one with the default annotation and add notation it was set by cluster default

Something to note, currently when doing a kubectl get it returns <none>

❯ kc get ingress
NAME              CLASS    HOSTS              ADDRESS     PORTS   AGE
example-ingress   <none>   hello-world.info   localhost   80      27m

I'm not sure if it's desirable to show the inferred class name for get or just stick with what is explicitly set in the Ingress object itself in this situation, however. If we're only fetching what is explicitly set in the config then <none> makes sense, however it does look like it overlooks the deprecated annotation when it's used. Seems like a bit of a sharp edge, but updating describe to follow this logic at least seems to smooth that out slightly.

It also probably makes more sense, since I'm doing all these checks now, to say <none> in describe.

[brianpursley]

If kubectl get ingress just prints <none>, I’d be inclined to just do the same thing in describe.

I’m a little hesitant to try to reimplement the ingress class default/precedence rules inside kubectl. It seems like this could be tricky, especially since it sounds like some ingress providers can work without a default class name.

I’m open to it, but just want to keep things simple if possible. I understand if there is value in doing this though.

I’d like to hear from someone else who maybe has more experience than I do with ingress. @rikatz any thoughts on this? @eddiezane thoughts?

[rikatz]

tl;dr because I wrote too much: I think having ingress class reflecting the ingressClassName is fine :)

The part where I write too much:

IngressClass is a tricky implementation.

Right now, we have the "deprecated" annotation, and the new ingressClassName spec field. Users still tends to use (a lot!) the annotation and even in ingress nginx we are thinking on rolling back this "deprecation". It was a really noisy change.

From my perspective, as the field is the "official" way of saying who "owns" that ingress, it's fine to print that field content and only it.

Now going to the "how useful this is": some controllers does not implements class, while others implement on different ways. As far as I remember a class can only be used if it exists and was created by the clusteradmin, so users cannot add random things, and it's good on a description to know "who owns that ingress".

I have a feeling right now that this solves half the problem: we have a description, but we don't have the "feedback" of if a controller from that class properly implemented that ingress but this is another discussion on the lack of status and conditions on ingress objects and I'm done on my KEP quota this year :)

[rikatz]

Just complementing now that I read the whole thread: I wouldn't go with "how ingressA or B does". Stick with the API spec :)

Annotation is deprecated. An ingressClassName can be empty, so either it's "none" or it's the value on the field. This is my opinion.

Anything other than that is too specific and can lead to situations like "controller A does this way and you don't cover", etc. The API is our contract, and we should follow it.

[mpuckett159]

For easier reference re API for Ingress:

IngressClassName is the name of the IngressClass cluster resource. The associated IngressClass defines which controller will implement the resource. This replaces the deprecated kubernetes.io/ingress.class annotation. For backwards compatibility, when that annotation is set, it must be given precedence over this field. The controller may emit a warning if the field and annotation have different values. Implementations of this API should ignore Ingresses without a class specified. An IngressClass resource may be marked as default, which can be used to set a default value for this field. For more information, refer to the IngressClass documentation.

Which is how I arrived at:

1. check for setting by spec
2. check for setting by annotation and add warning if found
3. if IngressClassName is still an empty string, query the api for IngressClass objects and search for one with the default annotation and add notation it was set by cluster default

[rikatz]

Left some reviews.

[rikatz]

Hum, v1beta1 is in path to be removed from the API, so I don't think we should take care of this :)

Based on kubectl/apiserver skew contract, you should support -3 versions, which is v1.21 that already supports ingress v1 :)

[rikatz]

I wouldn't use this. Annotations are deprecated, this is stated on the API. We should stick with the API description on this

[mpuckett159]

According to the API:

For backwards compatibility, when that annotation is set, it must be given precedence over this field.

[brianpursley]

tl;dr because I wrote too much: I think having ingress class reflecting the ingressClassName is fine :)

@rikatz Are you saying it is fine to simply display the value of ingressClassName directly, without doing any additional lookup of the default if it is nil?

I think that is where I am headed: just show the value, if it is set, otherwise if nil then print <none>.

do you think this is good enough?

[rikatz]

@rikatz Are you saying it is fine to simply display the value of ingressClassName directly, without doing any additional lookup of the default if it is nil?

Yeap, I'm a +1 to keep it simple right now. It's still confusing for users this annotations vs ingressClass thing, so let's state that ingressClass is the ingressClassName field, and IMHO move on :)

[mpuckett159]

I just wanted to double check, because I feel like I'm not on the same page as everyone else, but from my understanding of the API docs, it sounds to me like the annotation takes precedence, so if we really wanted to reflect reality back to the user it seems like we should be showing that annotation back if it exists as that's what the ultimate applied class will be.

[mpuckett159]

After sleeping on it I think what everyone is saying is that k8s.io/api/networking/v1(beta1) is where we should really implement the logic that I've written and have a function made available so that anyone can figure out the actual applied IngressClassName. That sounds correct to me, and if that's the consensus, I can scale back this commit back to where it's just reading the IngressClassName property and make an issue/PR for adding a function to "calculate" the actual applied Ingress Class based on the outlined logic from the docs, and whenever that's pulled in we can update the describe code here to utilize that function properly.

[mpuckett159]

Alright all that makes sense to me now, thanks everyone for the input. I've basically reset to the original PR commits and updated the Ingress Class output to show <none> to ensure that it is explicitly stated that when ing.Spec.IngressClassName is a nil pointer we are explicitly showing the user that, and hopefully avoiding any potential confusion there. I'll work on seeing if my proposed function for calculating the actual applied Ingress Class Name for the API is acceptable or not and get something open to which ever repo I need to open it to if so.

[mpuckett159]

/retest

[brianpursley]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, mpuckett159

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/kubectl/OWNERS [brianpursley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[brianpursley]

Thanks @mpuckett159 👍