Expose {create,delete,...}_entity methods to manipulate objects of dynamic kind

[cben]

If you have objects whose kind you don't know ahead of time, you can't used discovery-created methods like create_service, get_services.
Computing the method name e.g. client.send("get_#{kind}", ...) is messy, especially if you need the plural name :-(

We already have generic methods: get_entity, get_entities, create_entity, update_entity, patch_entity, delete_entity, watch_entities — except they're not documented in README and not considered public (and I believe we already changed interface once based on this)

Main use case is using objects from a yaml file (or from template processing). Related #208, #187, #325, #329. (I felt before those would be good to expose, now found convincing use case)

• review the API is good
  • watch_entities does double singular/plural duty depending of if :name is given — is this elegant?
    So does watch_pods BTW — there is no watch_pod.
  • is namespace passed consistently? Harmonize positional vs keyword namespace: args #312
• document as public

[cben]

Feedback from @stiller-leser, not sure why it disappeared:

Some initial feedback from my side. I just tried create_entity and noticed a couple of things:

• Isn't it redundant that I need to specify kind and name, since they are present in entity? Maybe we could overload create_entity here with two diffrent kinds of parameters here? For example create_entity(entity_type, resource_name, entity_config) and create_entity(entity). The latter one could then be used with YAML.load_stream(file).first (basically just dumping the yaml file content in a loop into create_entity as a first step. Later an additional method like create_entities_from_yaml could be provided.
• The method should check if the keys in the hash are strings or symbols, or at least raise a warning. Currently string-keys lead to undefined method '[]' for nil:NilClass because of https://github.com/abonas/kubeclient/blob/c637dc1d44363ea3facf2c893225a844f806dbb3/lib/kubeclient/common.rb#L311
• In the same line I would additional fallback to the default K8S namespace default

Happy to provide more feedback (or even PR, if I can free up some time and get the connection to my cluster working.)

[cben]

#241 (comment) points out that among many other things #241 tried to, in offered better names and possibly better interface:

client.create(resource|json)
client.update(resource|json)
client.patch(resource|json)
client.delete(resource|json)

where the passed data was expected to contain kind and possibly apiVersion.
Ability to omit apiVersion requires client to be able from discovery on multiple api groups would depend on the rest of #241 — one client associated with discovery on multiple API groups, which I'm not yet convinced is a good approach.

However, if you must provide both kind and apiVersion, that could be independent of discovery and a nice low-level API!

EDIT: What about get, list, watch?

namespaced, single obj:
client.get(kind: ..., apiVersion: ..., metadata: {name: ..., namespace, ...})
global, single obj:
client.get(kind: ..., apiVersion: ..., metadata: {name: ..., })
namespaced, collection:
client.get(kind: ..., apiVersion: ..., metadata: {namespace, ...})
all namespaces or global collection
client.get(kind: ..., apiVersion: ...)

namespaced, single obj:
client.watch(kind: ..., apiVersion: ..., metadata: {name: ..., namespace, ...})
global, single obj:
client.watch(kind: ..., apiVersion: ..., metadata: {name: ..., })
namespaced, collection:
client.watch(kind: ..., apiVersion: ..., metadata: {namespace, ...})
all namespaces or global collection
client.watch(kind: ..., apiVersion: ...)

• not sure about overloading same method for single/plural. k8s has "list" terminology for plural get, but kubeclient used "get" for both, and no obvious name for plural watch.

• Is nested metadata best, vs top-level name: and namespace:? I'm less concerned with convenience. want this to be a simple, consistent, low-level API.

  • What about labelSelector, fieldSelector? also in metadata?
    This is not really how k8s models these, they're query params there.
    The interface I'm really trying to follow here is not k8s api but kubectl! but there too for get/watch name & selectors are not in yaml form but command-line args and flags...

• Will it work to deduce plural (for url) from singular kind, without any discovery?
  Need hardcoded irregular plurals like Endpoints -> endpoints but we have those anyway.

• How would the client know to use api/ vs apis/ vs openshift's oapi/ ?
  api/ is only v1 (and in future v2 etc.), everything else can default to apis/, this can be automatic.
  oapi/ is special but I think requiring a 2nd client for that is acceptable.
  EDIT: openshift resources also appear under apis/ e.g. oapi/v1/routes ≅ apis/route.openshift.io/v1/routes; oapi/ are gone in openshift v4!

[cben]

@abonas you've previously objected to #241 adding "more than one way of doing the same thing". Would love to hear your thoughts as I'm pushing for a variant of same :-)
^^ #332 (comment)

TLDR Kubeclient sorely lacks a way to manipulate dynamic resource types — client.send("get_#{kind}", ...) is awkward, especially if you have to pluralize — so I feel we can't avoid a 2nd way to do the same.
A major use case people are asking for is "I already have a yaml", so I think a low-level interface shaped exactly like kubectl is best.

Plus I'm hoping it can work without any discovery, from single client for any apiVersion, with rest of discovery -> define_method becoming a convenience layer on top of this.

[cben]

client.patch(resource|json)

For patch, we need a way to choose patch format (#357) — json patch / json merge patch / strategic merge patch (https://kubernetes.io/docs/tasks/run-application/update-api-object-kubectl-patch/).
This is not communicated in body but via Content-Type header, and in kubectl via separate flag --type=json|merge|strategic so probably wouldn't belong in the single "resource" argument.
Can either use separate arg or 3 separate methods, should keep consistency with #357.

[benlangfeld]

@cben It looks like you arrived at a decision on this for a method where specifying both kind and apiVersion are required. What is blocking implementing and shipping that? It seems trivial.

[cben]

Right, that decision is clear.
Nothing serious, just lack of my time... PRs very welcome 😄

See checkboxes above, there are some questions that I hoped people would have an opinion on, but I've been linking people to this issue for some time and nobody said anything so I guess the design is OK 🤷‍♂️

A major thing I hope we can achieve here is having a single client object work with any apiVersion, kind given. Can we deduce the request URL from the apiVersion and kind alone? kind is always singular, e.g. NetworkPolicy, but the URL always contains a plural e.g. .../networkpolicies/... (both for single-object and multi-object requests).
k8s devs have said they regret this, it makes life harder for clients, but we're stuck with it...

• Currently we get this plural name from discovery (name, called resource_name in rest of our code), but currently we only do discovery for a single api group.
  We could split discovery into "get and cache info by an api group" separate from "define methods". "define methods" part would still only happen for the 1 group chosen group chosen at construction time. [and in future multiple Ability to add api groups to existing client #348].

  This is definitely workable, just more work than the "trivial" refactoring you thought this needs...

• Could this low level interface work without discovery, by mechanical pluralization? Needs rules for y-> ies, s -> ses, plus a list of exceptions. It's mostly workable, though CRDs in principle allow arbitrary unrelated kind vs plular. This could stink if some resources won't work at this low level before discovery but will work after discovery...

  AFAICT, kubectl didn't attempt this, it always does discovery (cached in filesystem). Though maybe it has higher standards — also does things like client-side validation, kubectl explain etc...
  OTOH, I think generate clients like the Go one don't need discovery (?), but all supported types are hard-coded, which we don't want to do.
  Bottom line: I doubt we want to get into this...

• Can we sidestep this by requiring/allowing caller to also give the plural?
  Note we need it even for actions on a single object, which looks a bit silly, e.g. getting a single pod might look like:
  client.get({kind: 'pod', apiVersion: 'v1', metadata: {name: ..., namespace, ...}}, plural: 'pods')
  or maybe:
  client.get('pods', {kind: 'pod, apiVersion: 'v1', metadata: {name: ..., namespace, ...}})
  (If given, can we allow omitting kind? Probably, long ago it was needed for some actions (create etc), and kubeclient still sends it but doesn't really need to, [WIP] [EXPERIMENT] Don't send kind, apiVersion in create POST #333.)

  What I dislike here is that people have real use cases to take JSON/YAML as kubectl does—with kind but without plural—and have it Just Work.
  But maybe it's a useful migration path: first implement this feature requiring a 2nd plural: option, which is straightforward; later make that optional by discovery and/or pluralization?

---

Another small step that's possible is first make this work for currently discovered api group only.
Then make multi-discovery work later.