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

Confusing overlap between different resource metadata types #292

Closed
nightkr opened this issue Jul 24, 2020 · 3 comments · Fixed by #385
Closed

Confusing overlap between different resource metadata types #292

nightkr opened this issue Jul 24, 2020 · 3 comments · Fixed by #385
Labels
api Api abstraction related question Direction unclear; possibly a bug, possibly could be improved.

Comments

@nightkr
Copy link
Member

nightkr commented Jul 24, 2020

We currently have the following variants:

  • k8s_openapi::Resource provides resource type metadata at compile-time
  • kube::Resource provides resource type metadata at runtime, as well as the namespace
  • kube_runtime::reflector::ErasedResourceState provides resource type metadata at runtime
  • kube_runtime::reflector::RuntimeResource can be implemented either way (on top of either k8s_openapi::Resource or ErasedResourceState)

All variants can be used to qualify a reference to an object:

  • k8s_openapi::Resource: <K: Resource>(name: &str, namespace: Option<&str>) (can't actually exist purely as a runtime value, K must be locked down at compile time)
  • kube::Resource: (resource: Resource, name: &str) (namespace is carried implicitly inside Resource)
  • kube_runtime::reflector::ErasedResourceState: (resource: ErasedResourceState, name: &str, namespace: Option<&str>)
  • kube_runtime::reflector::RuntimeResource: ObjectRef<K: RuntimeResource> (which turns into either the k8s_openapi::Resource or kube_runtime::reflector::ErasedResourceState variants depending on K)

In theory all four variants could also be used to access the K8s API, but currently we only implement the first two:

  • k8s_openapi::Resource: Can be wrapped in Api<K> which also owns the Client (kubeconfig and HTTP client), and namespace
  • kube::Resource: Bundles namespace, provides methods to prepare API calls (bring your own Client)

We could unify both of these cases by implementing Api in terms of ObjectRef and RuntimeResource instead. This would also allow you to share a single Api, rather than having to construct a new per resource type. It would also remove the kind of awkward need to clone Api (or modify its internal state) every time you want to run something in a different namespace.

I think this would be beneficial for controllers and other services that need to consider the whole cluster. That said, it does have few downsides that we'd need to consider..

  1. Worse ergonomics for vanilla "kubectl-like usage" ("get pod X" goes from pods.get("X") to k8s.get(ObjectRef::<Pod>::new("X").within("default")))
  2. ObjectRef isn't appropriate for bulk operations (list/watch). That said, maybe it would make sense to take over the search functionality of ListParams and make an ObjectSelector type for this use-case?
  3. Massive breakage of core functionality for existing users, although the existing Api<K> could be reimplemented on top of the new API as a backstop
  4. k8s_openapi doesn't provide any ObjectRef references
@nightkr nightkr added the api Api abstraction related label Jul 24, 2020
@clux
Copy link
Member

clux commented Jul 25, 2020

There are reasonable reasons for why they are split though. Some parts of the api just needs more info than others, and one is a trait with associated consts.

The ObjectRef interface feels a bit clunky, at least at the moment, to expose too heavily to users. So not super keen on any breakages in that regard. My gut feeling on this is that it's probably more worth getting the Api to get to a sans-io goal before we start consolidating too much. But happy to see PoCs.

Btw; k8s-openapi have tons of structs that are close to ObjectRef:

@nightkr
Copy link
Member Author

nightkr commented Jul 25, 2020

ObjectRef interface feels a bit clunky, at least at the moment, to expose too heavily to users.

Agreed. Mostly wanted to open the discussion.

Btw; k8s-openapi have tons of structs that are close to ObjectRef:

Yup, but none of those are typed. :/

@clux clux added the question Direction unclear; possibly a bug, possibly could be improved. label Aug 5, 2020
@clux
Copy link
Member

clux commented Aug 25, 2020

Found some similar overlap in the runtime schema package from apimachinery. Just noting it down.

@clux clux linked a pull request Feb 8, 2021 that will close this issue
@clux clux closed this as completed in #385 Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants