feat(kuma-cp): add support for inspect api for new kind Dataplane and section name for selecting single inbound

[Automaat]

Fix: #12361

Motivation

We have introduced new kind and we need inspect api support for this, as it turns out it works out of the box so I've just added test for it.

This needs #12573 to be merged first

[github-actions]

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

• Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
• PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
• This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
• IPv6 is taken into account (.e.g: no string concatenation of host port)
• Tests (Unit test, E2E tests, manual test on universal and k8s)
  • Don't forget ci/ labels to run additional/fewer tests
• Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
• Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

[lobkovilya]

why is the name required?

[Automaat]

I've made it required, just so it is not pointer to. string but I can change this if you think it should not be required

[lobkovilya]

Technically it doesn't matter as an empty value name: "" means name is not set. Could you please check with @johncowen or @schogges to understand what's the preferred way for such kind of situation?

[schogges]

I think it's better to omit name if it's not set. Theoretically it doesn't really matter because we'd catch that in our data-layer, but if name is an optional property, I'd treat it as such 🤔 from a TypeScript perspective I think it's better because we are forced to catch that case. An empty string is falsy, but we are not forced to handle that case. If we leave it as an empty string, we'd have to know that it could be an empty string and handle that.

For consistency reasons, are there similar cases and how do we handle that in these cases?

[johncowen]

TLDR; I'd go with name: "" if not set.

---

I'd say a non-union type is always best. So one single type, i.e. empty string "" in the case of strings, 0 (or sometimes even -1) in the case of numbers, arrays/slices should be empty [], objects are slightly trickier but generally {}. No three way booleans, always true or false, never omitted/undefined

No omitted's, undefineds or nulls

This makes the properties of an API response stable and clients are then less prone to error when they expect a property to be there and it suddenly isn't.

Theoretically it doesn't really matter because we'd catch that in our data-layer

We can fix these up in our data layer if necessary (and this is what we've been doing with older APIs), but moving forwards it would be great if the HTTP APIs follow this thinking also.

Theoretically the GUI is not the only HTTP API client, and not everyone will have a thin layer in between to smooth out any rough edges.

[johncowen]

Yeah so it looks like kubernetes follows the typical go-lang use omitempty everywhere, which isn't that surprising I suppose seeing as its go-lang:

In most cases, optional fields should also have the omitempty struct tag (the omitempty option specifies that the field should be omitted from the json encoding if the field has an empty value). However, If you want to have different logic for an optional field which is not provided vs. provided with empty values, do not use omitempty (e.g. kubernetes/kubernetes#34641).

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

This also seems to be pretty common with go-lang software generally. So if we are following go-lang/kubernetes conventions for HTTP APIs, then we should stick to that I guess, i.e. name shouldn't be marked as required and it should be omitted entirely when empty (so string | undefined). The GUI will continue to maintain a separate layer between the HTTP API and our application to smooth over these sharp edges so we only deal with a single type.

I'm leaning towards John opinion, to avoid nullable values in api. I think they should be unique, on Kubernetes they will be unique, on universal I think we should add validation for this. What do you think about it @lobkovilya?

As a frontend eng who is always consuming these sorts of APIs, my opinion on HTTP APIs stays the same, avoiding nullable values is generally the best thing to do, especially in the case of three way booleans and even more especially when there is a three way boolean and the absence of which actually means true.

The difference here is this is a go-lang HTTP API which in the limited experience I've had are always difficult to use, and if we want to stick to this convention (which I think we should, considering everything), then we just go with that.

[johncowen]

Another follow up question here, do we have some sort of doc somewhere that explains the style/feel of our HTTP APIs. So if I'm a beginner contributing to the software I have something to follow to help me make all the APIs feel the same? (Maybe this is just the same link to those kubernetes API conventions)

[johncowen]

Just to note on this one, we had a discussion around supporting a terraform provider via our HTTP API, and also around what our most common HTTP API client, Envoy, expects. Similar questions and sharp edges arose.

Would be really good to get these sharp edges outlined and a common consistent approach to those documented somewhere so we know (both from a backend and frontend perspective) what to expect (even if its just "in this case, do what kube does"). Something I think we are going to follow up on.

[johncowen]

So I had a good read over the Kubernetes sig conventions and came to the conclusion that it is very much inline with my initial thinking in #12644 (comment).

TLDR; explicitness, no ambiguity, good defaults

Also wanted to say to @Automaat and @lobkovilya sorry for taking this thread completely our into space, around the planets and back again 😬 😆 🚀 . Honestly, I'm pretty convinced of my opinion at this point, but that doesn't mean people have to agree with mine, I'll leave it up to you you folks. I'll probably dip out of this conversation after this, unless people have want clarifications or have questions specifically for me.

Lastly, thanks for asking for my opinion, I found out a lot, so thanks 🙏

Anyway here goes...:

---

The section on defaulting states:

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#defaulting

In general we want default values to be explicitly represented in our APIs, rather than asserting that "unspecified fields get the default behavior". This is important so that:

• default values can evolve and change in newer API versions
• the stored configuration depicts the full desired state, making it easier for the system to determine how to achieve the state, and for the user to know what to anticipate

Apart from that my feeling was that the entire document frequently talks about explicitness, i.e. avoid ambiguity don't let API consumers guess.

The Optional vs Required section (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required) was mentioned. 'Required' fields are obviously filled out, but I'd go as far as to say optional fields should have a default in order to be explicit (see previous Defaults link in the same document).

Going back to my original comment. Booleans can only be one of two states, if not its not a boolean by its very definition, a boolean's default should be either true or false, not non-existent. This explicitness ensures that the end user of an API knows whether a default state is true or false, we had (I think its been changed now) a problematic lack of explicitness with this via a health type property a while back.

Moving on from booleans a little more, if we are using an enum for states or similar, the Kubernetes API conventions mention an explicit Unknown state, i.e. the state is Unknown not undefined/null.

For numbers, there is almost always a good default, usually 0, but in some cases as I found out, in our software we have defaults of 2 in some places. For me these are important to expose in the API so we are explicit about what their values are so the consumer doesn't have to guess.

We have a maxInterval property somewhere, which multiplies the base interval by its value from what I understand. In my eyes, this should have a default of 1 not undefined/omitempty.

If an array is "unset" then its an empty array, if an object is unset then its an empty object. Its being explicit about what this property is, leaving no room for ambiguity and errors resulting from that.

The only thing we haven't really mentioned are strings.

I found a couple of mentions relevant to strings and defaults specifically in the kubernete's convention doc:

// reason contains a programmatic identifier indicating the reason for the condition's last transition.
// Producers of specific condition types may define expected values and meanings for this field,
// and whether the values are considered a guaranteed API.
// The value should be a CamelCase string.
// This field may not be empty.
// +required
Reason string `json:"reason" protobuf:"bytes,5,opt,name=reason"`
// message is a human readable message indicating details about the transition.
// This may be an empty string.
// +required
Message string `json:"message" protobuf:"bytes,6,opt,name=message"`

Both Reason and Message are strings, both are required but both can be an empty string i.e. Message: "". As they are required I am guessing those don't use omitempty.

If we wanted to look specifically at string property that are distinctly Name: (quite an important field in kubernetes) I found that Name is mentioned in the docs as an example:

ports:
  - name: www
    containerPort: 80

But the docs doesn't give an guidance or advice on whether to use omitempty or not on a string field specifically called Name:

I have comparatively little knowledge of kubernetes, but I did find https://kubernetes.io/docs/concepts/services-networking/service/#multi-port-services

For some Services, you need to expose more than one port. Kubernetes lets you configure multiple port definitions on a Service object. When using multiple ports for a Service, you must give all of your ports names so that these are unambiguous.

I don't know how relevant or important that is to what we are doing here.

At the very least if we are trying to provide consistent APIs to our consumers, wouldn't it be easier at least in this InspectAPI case to just say "we do not allow omitempty"?

---

I also found the following related and interesting:

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

The specification is a complete description of the desired state, including configuration settings provided by the user, default values expanded by the system, and properties initialized or otherwise changed after creation by other ecosystem components (e.g., schedulers, auto-scalers), and is persisted in stable storage with the API object

The status summarizes the current state of the object in the system, and is usually persisted with the object by automated processes but may be generated on the fly. As a general guideline, fields in status should be the most recent observations of actual state, but they may contain information such as the results of allocations or similar operations which are executed in response to the object's spec

---

Just to note there is an ongoing conversation on certain API endpoints that due to internal usage require fields to be omitempty/undefined. I understand why these need to be as they are, and I agree that these are a separate case/group of API endpoints and not necessary covered by what we are discussing here. Also note this PR seems to be seems to be part of the "Inspect API" i.e. whose purpose I believe is to show the expanded system result of the user configuration.

[johncowen]

Sorry I know I said I would dip out now, but! I noticed this:

kumahq/kuma-website#2163

to:
  - targetRef:
      kind: MeshService
      name: backend
      sectionName: port-name # or stringify port's value like "8080"

I don't think its such a bad suggestion that a port name could default to the stringified port's value like "8080". One benefit is, as this comes out of the user facing HTTP API, there is no need to document what to do if the port doesn't have a name, because it always has one. If you have explicitly set one yourself, its that, if you haven't its the port number expressed as a string.

As usual I don't understand the details of any under-the-hood technical implications this might have, but from a user-perspective this sounds like another option for a reasonable default to me.