Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add core_methods directly on Client #1032

Open
2 of 8 tasks
clux opened this issue Sep 26, 2022 · 2 comments
Open
2 of 8 tasks

Add core_methods directly on Client #1032

clux opened this issue Sep 26, 2022 · 2 comments
Assignees
Labels
api Api abstraction related client http issues with the client umbrella major roadmap tracking issue

Comments

@clux
Copy link
Member

clux commented Sep 26, 2022

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: #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:

@clux clux added api Api abstraction related client http issues with the client labels Sep 26, 2022
@olix0r
Copy link
Contributor

olix0r commented Sep 27, 2022

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

@clux
Copy link
Member Author

clux commented Sep 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related client http issues with the client umbrella major roadmap tracking issue
Projects
Status: In Progress
Development

No branches or pull requests

2 participants