AdditionalPrinterColumns Array Support

[Rob Scott]

Hey Everyone,

I wanted to check in on the possibility of supporting array data with JSON paths specified in AdditionalPrinterColumns. There's an old kubectl issue that was tracking this and even a PR to fix it. Unfortunately that PR went stale. Getting this fixed would be a huge help for projects relying on CRDs like Gateway API. Without this, we've been considering adding new API fields simply to provide better UX.

I'm happy to try to submit a similar fix, but wanted to make sure I'm not missing any extra context on the earlier fix attempt. I'm not as familiar with this part of the codebase, so let me know if there are reasons we should avoid supporting more complex jsonpaths like this.

Thanks!

Rob

[Clayton Coleman]

There were a few concerns over impact of new returned values to existing clients, but since any aggregated API server might follow the documented allowed value for cell at any time

// cells will be as wide as the column definitions array and may contain strings, numbers (float64 or
// int64), booleans, simple maps, lists, or null. See the type field of the column definition for a

// more detailed description.

I don't see an issue.  As far as complex jsonpath, it's really whether the implementation we have supports it and having a few relevant examples to guide good tests.  We didn't want *unbounded* JSONpath execution but frankly that's the responsibility delegated to the person installing the CRD today (if you do something crazy, it's up to you)

I'd be happy to review and handle approval.

--
You received this message because you are subscribed to the Google Groups "K8s API Machinery SIG" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-api-m...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-api-machinery/CAGY4dk%3DdDaMP68-8AhGxvAuNxB1n0OfuE4WD0BKLfzcOUZzGfw%40mail.gmail.com.

[Rob Scott]

Thanks for the help with this! I've created a PR that implements this, reviews and feedback are very appreciated.

Thanks!

Rob

[Daniel Smith]

What kind of JSON path are we talking about?

Thus far we've resisted puting JSON path extensions into the API due to the lack of a standard.

JSON pointer is drastically better defined, can that work instead?

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-api-machinery/CAGY4dknDVs3EMH_OVhB_FQpVon_SymnMAeVBsrMZ_WgyoykVTQ%40mail.gmail.com.

[Rob Scott]

In this case I'm not proposing changes to any of the existing jsonpath matching logic we're using, simply to print the results we're already getting. The existing logic prints only the first matching result which results in strange and confusing behavior. For a more concrete example, with Gateway API we want to output the Gateway(s) that are handling a Route. We have that information in status along with a variety of conditions for each Gateway, but we really just want to print out a name. Ideally something like `.status.gateways[*].name` would work for this, but unfortunately it only returns the name of the first Gateway in the list. Something that will be helpful in some cases, but very confusing when there's actually more than 1 Gateway in the list.

[Clayton Coleman]

I will note we explicitly wanted to support arrays, it just got lost at the time server side support went in for CRDs (we didn’t have any array use cases inside the server that weren’t already strings.Join() due to legacy), so it was just an oversight on my part at the time.

On Apr 16, 2021, at 8:32 PM, 'Rob Scott' via K8s API Machinery SIG <kubernetes-sig...@googlegroups.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-api-machinery/CAGY4dkkQ4wKUq_PUajuHWCEWg2tGDD47tyw3L4cbSuvM7i1F-w%40mail.gmail.com.

[Daniel Smith]

This use of JSONPath is in the CRD api object, and not directly part of the api machinery, so I guess I don't care that much.

[Jonathan Innis]

We just came up on this in Karpenter and I was still thinking that this could be a really useful feature to add. Responded to the original PR: https://github.com/kubernetes/kubernetes/pull/101205#issuecomment-1926229023. I'd assume this conversation is probably better-tracked in an issue but didn't look around too hard to find one. Let me know if we should create a new one to get conversation going again,.