PoC: Create an Api::dynamic and Api::cluster + scope constrain Api::all

[clux]

Proof of Concept

Wanted to see how this ended up looking. I am not 100% convinced about this yet.

Motivation

Currently the biggest footgun in kube right now is Api::all and its interaction with Namespaced objects.

An Api::all on a Resource = NamespacedResource is an extremely stunted implementation that only really allows Api::list, Api::watch.

All the other Api methods will fail with 404s because it by construction creates invalid request urls.

Main issues:

• get with Api::all() response can't be 404 get with Api::all() response can't be 404 #1276
• client: Api::all fails to create namespaced resources client: Api::all fails to create namespaced resources #1030

and a couple from discussions:

• Can't create 3rd party CRD Can't create 3rd party CRD #1296
• How can I get some resources with the event type? How can I get some resources with the event type? #1312 <- a more exotic one with finalizer

Proposal

1. Put a big warning on Api::all
2. Try to shift people away from Api::all unless they actually want a cross-namespace lister

This does mean one breaking change:

• Api::all now requires NamespaceResourceScope + put a big warning on it that it's only for listwatching.

and add two new main Api constructors to compensate:

• Api::cluster - limited to ClusterResourceScope
• Api::dynamic - limited to DynamicResourceScope

a potential setup for api construction from discovery to show this does not make it worse

Alternatives

• We could not add the NamespaceResourceScope scope constraint on Api::all for a non-breaking change.
• It's possible this is not worth it, and it's instead worth focusing on the Client V2. However, it's unlikely we'll remove Api no matter what, so thought we may as well make this as good as possible

Drawbacks

• More duplication of constructors (which are hard to find in the myriad of Api methods on docs.rs)
• This still does not prevent people from making the error I actually wanted to prevent, it just makes it less likely (as users will more likely flock to the methods they see in examples, or the one in docs.rs with less big warnings)

[codecov]

Codecov Report

Merging #1313 (ef77240) into main (c3fbe25) will decrease coverage by 0.0%.
The diff coverage is 89.7%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1313     +/-   ##
=======================================
- Coverage   72.4%   72.4%   -0.0%     
=======================================
  Files         75      75             
  Lines       6343    6357     +14     
=======================================
+ Hits        4587    4597     +10     
- Misses      1756    1760      +4     

Files	Coverage Δ
kube-client/src/api/core_methods.rs	73.4% <ø> (ø)
kube-client/src/api/util/mod.rs	93.6% <100.0%> (ø)
kube-client/src/discovery/apigroup.rs	90.0% <ø> (ø)
kube-client/src/discovery/oneshot.rs	80.0% <ø> (ø)
kube-client/src/lib.rs	75.2% <100.0%> (ø)
kube-derive/src/lib.rs	0.0% <ø> (ø)
kube-runtime/src/controller/mod.rs	34.1% <ø> (ø)
kube-runtime/src/events.rs	97.3% <100.0%> (ø)
kube-runtime/src/reflector/mod.rs	100.0% <ø> (ø)
kube-runtime/src/wait.rs	75.6% <ø> (-2.0%)	⬇️
... and 3 more

[Dav1dde]

I think there isn't much gain in limiting all to NamespaceResourceScope and introducing even more constructors which make it even harder to create an Api from generic code (it already is really hard), because it still doesn't address the issues, people can and will still as easily misuse it (accidentally using all instead of namespaced).

It would be different if we would also track as a second generic how the Api was created, e.g. Api<K, All> and then only implement list and watch for Api<K, All> and nothing else. But even then we wouldn't need different constructors, it could just be inferred from K: impl<K> Api<K, All> where K::Resource<Scope = NamespaceResourceScope>.

[clux]

While I am not attached to this particular design, I think it's important to keep in mind that this code setup might not help
heavy-users who already got it right, but it will help new-users who trip over themselves in the very early setup phases - because we have made a very simple-to-make illegal state representable.

Sure, the generic code case was never easy, but I don't think a setup like this would make it significantly harder - particularly if they have written all the cases you linked. It's also a minority of people that needs to write that code. It's generic code. The number of people encountering the pain needs to be considered.

As a side-note, I almost feel like those particular generic api getters are done at the wrong level. That code is setup to expose an interface for a namespace on something that doesn't have a namespace. This means you no longer know by type whether it's safe to unwrap the .namespace() result in that case in generic code. The scope associated type constraints may be big, but it does give you guarantees you can take advantage of.

To illustrate some ways you can do generics (because I ended up thinking about this a bit this week), there's a new PR at kube-rs/website#47 to show some ways to get around some of these issues in easier ways.

[clux]

As an extra point, I do think there are two orthogonal use-cases for Api that is a source of the difficulty for its use;

1. Api as a "pinned-view" for watcher (will only ever use .list and .watch - so ::all or ::namespaced is always fine)
2. Api as a mutator in reconcilers etc (generally needs to be not ::all)

It is possible we need to follow a style a bit more like client-go and have a ListWatcher style entrypoint for watcher.