Add core_methods directly on `Client`

clux commented 2 years ago

What problem are you trying to solve?

To use any part of the Kubernetes api on a Resource you currently have to instantiate an Api object. In many cases, this abstraction saves some time by giving the user a type bound to a resource that you can do multiple queries on (within the same scope), but in many other instances it leads to lots of client.clone() calls to create throwaway Api instances that are used for a single query. We see this all the time in reconcilers.

We see users working around it by implementing these things more or less directly on the Client themselves

It also leads to problems where people create an Api::all to perhaps query across all namespaces, and legitimately ending up with an "illegal" way to create a namespaced resource. See #1030

It's also not the first time this idea of avoiding Api has been floated around: https://github.com/kube-rs/kube/pull/594. But I think we can do this in a non-breaking way, without having to rewrite so much as the original PR attempted to do (though there are some great simplifications in there).

Describe the solution you'd like

We should implement these methods directly on client:

impl Client {
  // All of these with constraint:
  // where K: Resource<Scope = NamespaceResourceScope> {}
  fn create_namespaced<K>(name: &str, ns: &Namespace, pp: &PostParams, data: &K) -> Result<K>
  fn get_namespaced<K>(name: &str, ns: &Namespace) -> Result<K>
  fn list_namespaced<K>(ns: &Namespace, lp: &ListParams) -> Result<ObjectList<K>>
  fn list_all<K>(lp: &ListParams) -> Result<ObjectList<K>>
  fn delete_namespaced<K>(name: &str, ns: &Namespace, dp: &DeleteParams) -> Result<Either<K, Status>>
  fn delete_namespaced_collection<K>(name: &str, ns: &Namespace, dp: &DeleteParams, lp: &ListParams) -> Result<Either<ObjectList<K>, Status>>
  fn patch_namespaced<K, P>(name: &str, ns: &Namespace, pp: &PatchParams, patch: &Patch<P>) -> Result<K>
  fn replace_namespaced<K>(name: &str, ns: &Namespace, pp; &PostParams, data: &K) -> Result<K>
  fn watch_namespaced<K>(ns: &Namespace, lp: &ListParams, version: &str) -> Result<impl Stream<Item = Result<WatchEvent<K>>>>
  fn watch_all<K>(lp: &ListParams, version: &str) -> Result<impl Stream<Item = Result<WatchEvent<K>>>>
}

and near identical impls for ClusterResourceScope but without the ability to do anything namespaced (basically the same fn interfaces as on Api since they don't take a namespace.

The function bodies should leverage kube-core's Request objects and be functionally equivalent to the Api method's parallels, but without having to read namespace from self and instead get it from some newtype (to avoid two string args in a row).

Describe alternatives you've considered

We could hide some of this behind an async trait to make this stuff mockable, but then we would need to pull in async_trait (which we currently do not). Not sure what is the best solution for this. Is it a problem to have a bunch new pub methods on Client?

Documentation, Adoption, Migration Strategy

We don't need to deprecate Api I think. The abstraction is useful, just not nearly as universal as originally envisioned. However, this does mean that more of runtime can create some of its impls directly on Client in a less or more constrained way. This might be nicer in some places, but it might be more problematic since we now force dealing with scope down to the resource.

To minimize the amount of code-duplication within kube-client, we could make the methods on Client canonical, and make Api a more empty shell that defers to these.

Less sure what to do about the subresource scope though. That requires some thought.

Target crate for feature

kube-client

Future Ideas

Future things that should be considered later on (open follow-ups):

feature gated automock derives to allow mocking the client without tower_test

Implementation Plan

WIP:

[x] Decide on general module structure and how to deal with a namespace param - https://github.com/kube-rs/kube/pull/1375
[x] Implement an initial setup for Client::list+ Client:get - https://github.com/kube-rs/kube/pull/1375
[ ] Implement Client::delete + Client::delete_collection
[ ] Implement Client::patch
[ ] Implement subresource standard methods Client::patch_status + Client::patch_status_namepaced and decide whether to make a typed variant (so you can take the Status object - e.g. https://github.com/kube-rs/kube/discussions/1465)
[ ] ...more to come
[ ] generic subresources
[ ] dynamic resources

olix0r commented 2 years ago

We might also consider the metadata-only API to be included on the Client type.

clux commented 2 years ago

Another extension discussed in the discord that's possible with query methods on Client: dynamic get lookups from ObjectRef:

fn get_by_ref<K>(oref: &ObjectRef) -> Result<K>
# or if following get_opt and catching the 404
fn get_by_ref<K>(oref: &ObjectRef) -> Option<Result<K>>

then users can pick K as DynamicObject or whatever Resource type they might know they are looking up for.

We could add nice converters to more easily construct ObjectRef to simplify working with these.

kube-rs / kube