Move `ApiCapabilities` into `ApiResource` and add `Scope` trait #1038

clux · 2022-09-30T22:53:23Z

Generally simplified code everywhere. The main edge cases worth documenting here is that ApiResource can be constructed in multiple ways, and some of these will not yield all the fields on it.

~~Merged~~Moved ApiCapabilities data into ApiResource
Add shortnames to ApiResource for Add shortnames field in kube::discovery::ApiResource #1002
Removed separate ApiCapabilities returns (always constructed together with ApiResource, so now returned as one)
Added a Scope trait implemented for all k8s-openapi::ResourceScope + our DynamicResourceScope
Refined the Resource trait to require the associated Scope type to implement the new Scope trait
Added Resource::is_namespaced using Scope trait or dyn type inspection
Extended ApiResource::erase to be able to determine namespaced bool from the Scope trait
Reclassified ApiResource ctors (removed ApiResource::from_gvk_with_plural in favour of ApiResource::new)
Added setters of properties previously in ApiCapabilities to ApiResource
Extended kube-derive to default verbs on its ApiResource
LATER: Extend kube-derive to provide subresources on its ApiResource (todo in WIP: Core methods on Client #1037)
Re-documented everything related / fix tests

codecov-commenter · 2022-10-01T08:41:25Z

Codecov Report

Merging #1038 (39f8116) into main (a26e7f3) will decrease coverage by 0.46%.
The diff coverage is 66.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
- Coverage   72.29%   71.82%   -0.47%     
==========================================
  Files          65       66       +1     
  Lines        4681     4699      +18     
==========================================
- Hits         3384     3375       -9     
- Misses       1297     1324      +27

Impacted Files	Coverage Δ
kube-client/src/lib.rs	`93.75% <ø> (ø)`
kube-core/src/crd.rs	`82.92% <ø> (ø)`
kube-core/src/dynamic.rs	`63.63% <0.00%> (-3.04%)`	⬇️
kube-core/src/object.rs	`77.41% <0.00%> (-2.59%)`	⬇️
kube-derive/src/custom_resource.rs	`13.83% <ø> (ø)`
kube-runtime/src/reflector/store.rs	`95.16% <ø> (ø)`
kube-runtime/src/utils/mod.rs	`62.96% <ø> (ø)`
kube-core/src/scope.rs	`16.66% <16.66%> (ø)`
kube-core/src/discovery.rs	`86.66% <36.84%> (-11.09%)`	⬇️
kube-client/src/discovery/parse.rs	`93.75% <91.66%> (-0.85%)`	⬇️
... and 6 more

clux · 2022-10-03T18:23:02Z

Eyes on this would be appreciated @kube-rs/maintainers .

Clarity on how to convert between scope requirement on dynamic and static types is needed for a good way forward on the new client api in #1037, but this should stand well on its own without that (even though it doesn't go all the way with subresources).

nightkr

+1 on the overall concept, having to distinguish resource and caps was pretty weird and awkward.

examples/dynamic_watcher.rs

nightkr · 2022-10-03T18:48:48Z

kube-client/src/discovery/apigroup.rs

+                let mut ar = parse::parse_apiresource(res, &list.group_version).map_err(
                    |ParseGroupVersionError(s)| Error::Discovery(DiscoveryError::InvalidGroupVersion(s)),
                )?;
-                let caps = parse::parse_apicapabilities(&list, &res.name)?;
-                return Ok((ar, caps));
+                ar.subresources = parse::find_subresources(&list, &res.name)?;
+                return Ok(ar);


Would it make sense to reorganize this to:

struct ApiResourceMeta; // current ApiResource struct ApiCapabilities; // current ApiCapabilities struct ApiResource { // current (ApiResource, ApiCapabilities) meta: ApiResourceMeta, caps: ApiCapabilities, }

That way we could provide wrapper functions as needed on ApiResource to avoid having the user care about the distinction, but also statically guarantee that caps are actually available when required. We might even want to impl Deref<Target = ApiResourceMeta> for ApiResource to approximate subtyping...

Sorry, s/caps/subresources/g.

I think that sounds like the nicest possible way to split the structs. It is still a bit awkward though:

the upstream struct does not have a split like that

we are not really able to split APIResource into Caps/Info now (where does short_names go? is scope a capability?), and it might be even harder in the future if upstream adds more info

we probably need to push the distinction stuff into ApiResourceMeta instead as a user could want to query the api with just the GVK + plural, so now they need a partial ApiResourceMeta constructed somehow

The distinction here might not be that big of a deal because if we provide clean paths to get fully speced out variants of ApiResource via kube-derive or discovery then that's the happy path for users. If people want to construct them manually, then they probably should care about the distinction.

(As for the subresource parts of ApiResource, I'm not entirely sure what is the best way to represent that inside ApiResource. Nesting ApiResource inside ApiResource to represent subresources is unnecessary (and leaves many fields unused). Hoping to experiment and clean up that part in the client api pr - for now have left it as it was)

What do you think?

After looking into it a bit more it feels like there was a useful distinction where ApiResource was "the information you need to start querying the API" and ApiCapabilities was "auxilary data that might be useful for a UI (like kubectl)". I think it makes sense to keep those types distinct to document whether ApiCapabilities is also available at some given point.

Where the old API (IMO) fell down was in that having to deal with ApiCapabilities ended up being a constant annoyance for people who only care about ApiResource.

APIResource is a slightly different beast, since it's always generated by the K8s API server, which always knows the canonical information for all of the fields. from_gvk isn't really an API that makes sense for the K8s type.

we are not really able to split APIResource into Caps/Info now (where does short_names go? is scope a capability?), and it might be even harder in the future if upstream adds more info

short_names is definitely an info/cap/whatever-we-end-up-calling-it. scope I could see going both ways, but I'm leaning towards info as well since if you already have the object (or know how to construct it) then you can infer the scope based on that. If you want to guide the user towards how to construct the object then you'll want an ApiResourceInfo.

Yeah, I don't think Option-wrapping at runtime is great.

Have put a capabilities member as Option<ApiCapabilities> directly on ApiResource now.

Have not included namespaced in there because namespaced is currently the one thing we can find through all forms of construction:

kube-derive/discovery (exists by construction)

ApiResource::erase (using the new Scope trait through Resource)

ApiResource manual construction (if setting namespaced manually - although that's up to the user to get right in that case)

In fact, namespaced is now universally accessible so have made a Resource::is_namespaced method to help determine the namespace (which even works in the dynamic case).

Having namespaced within ApiCapabilities then we would need to partially construct it in ApiResource::erase and that means all fields within ApiCapabilities need to be option wrapped, which is not great for users of the dynamic_api and similar (multiple guaranteed unwraps necessary in their code). Currently it's now limited to a single unwrap in dynamic_api to access the caps:

// use discovery to get an AR let caps = ar.capabilities.as_ref().expect("guaranteed because we used discovery"); caps.supports_operation(...); // can now do a check without option unwrapping from caps

Have lifted the namespaced into capabilities struct by removing the option, and relying on default instead.

I think this is the least worst option, that still preserves a reasonable split (although the setters have been moved into ApiResource because we need to set namespaced from the root (it's the resource that can extract it) - but that's fine, these setters are only set by either kube-derive or hardcore manual users anyway).

So now ApiCapabilities has namespace/verbs/subresources/shortnames, and all of these will be set in the happy path (kube-derive/discovery). It's the happy path we want to be as frictionless as possible, so having options for just the manual path ends up mandating unwraps on the happy path which is not great.

I think we just keep talking past each other.

Okay, I did a bit more digging.

I tried implementing the ApiResource/ApiResourceCore split to see how it'd go, and Deref/DerefMut helped hide it from most usage sites (aside from constructors): https://github.com/teozkr/kube-rs/tree/rm-caps-/typed.

However, this also highlit another problem with merging caps into ApiResource: ApiResource is currently embedded into ObjectRef<DynamicObject>, so any fields on it are also considered for ObjectRef's Eq/Hash implementations. This means that an ObjectRef<DynamicObject> constructed using a discovered type and an ObjectRef constructed by erasing a k8s_openapi type would not be considered equal, even if they refer to same version of the exact same Kubernetes object!.

Signed-off-by: clux <[email protected]>

Co-authored-by: Teo Klestrup Röijezon <[email protected]> Signed-off-by: Eirik A <[email protected]>

Signed-off-by: clux <[email protected]>

clux · 2022-10-27T18:46:00Z

Going to put this on hold until 0.77. I have some ideas for it, but it's been a while since a release and will get that out the door first.

clux added the changelog-change changelog change category for prs label Sep 30, 2022

clux linked an issue Sep 30, 2022 that may be closed by this pull request

Merge ApiCapabilities into ApiResource #1036

Open

clux changed the title ~~Merge ApiCapabilities into ApiResource~~ Merge ApiCapabilities into ApiResource and add Scope trait Oct 1, 2022

clux added this to the 0.76.0 milestone Oct 1, 2022

clux mentioned this pull request Oct 1, 2022

Add shortnames field in kube::discovery::ApiResource #1002

Open

clux linked an issue Oct 1, 2022 that may be closed by this pull request

Add shortnames field in kube::discovery::ApiResource #1002

Open

clux marked this pull request as ready for review October 1, 2022 11:24

clux requested review from kazk and nightkr and removed request for kazk October 1, 2022 11:25

nightkr reviewed Oct 3, 2022

View reviewed changes

clux and others added 7 commits October 5, 2022 15:04

Merge ApiCapabilities into ApiResource

a342634

Signed-off-by: clux <[email protected]>

Add Scope to distinguish between scopes

dd5fdf4

Signed-off-by: clux <[email protected]>

re-classify ApiResource constructors

9d8c221

Signed-off-by: clux <[email protected]>

last reference to ApiCapabilities

9d26b2a

Signed-off-by: clux <[email protected]>

avoid breaking both ApiResource ctors

66db2e3

Signed-off-by: clux <[email protected]>

Update examples/dynamic_watcher.rs

6046a11

Co-authored-by: Teo Klestrup Röijezon <[email protected]> Signed-off-by: Eirik A <[email protected]>

minor clippy + typo + capitalization

ac5b93e

Signed-off-by: clux <[email protected]>

clux force-pushed the rm-caps branch from 9be6563 to ac5b93e Compare October 5, 2022 14:04

clux added 4 commits October 5, 2022 15:17

fmt newline

f767bb1

Signed-off-by: clux <[email protected]>

Re-add ApiCapabilities in limited form

24840ac

Signed-off-by: clux <[email protected]>

leftin todo

7219659

Signed-off-by: clux <[email protected]>

simplify dynamic interface and put namespaced inside capabilities

39f8116

Signed-off-by: clux <[email protected]>

clux changed the title ~~Merge ApiCapabilities into ApiResource and add Scope trait~~ Move ApiCapabilities into ApiResource and add Scope trait Oct 7, 2022

clux mentioned this pull request Oct 17, 2022

Remove dyntype from ObjectRef #1053

Open

clux removed this from the 0.76.0 milestone Oct 27, 2022

clux added this to the 0.77.0 milestone Oct 27, 2022

clux modified the milestones: 0.77.0, 0.78.0 Dec 12, 2022

clux modified the milestones: 0.78.0, 0.79.0 Jan 5, 2023

clux modified the milestones: 0.79.0, 0.80.0 Feb 23, 2023

clux removed this from the 0.80.0 milestone Mar 2, 2023

clux mentioned this pull request Apr 9, 2023

Create APIResource from ApiResource for extension an APIService #1195

Closed

clux modified the milestones: 0.83.0, 0.82.1 Apr 9, 2023

clux modified the milestones: 0.83.0, 0.84.0 Jun 4, 2023

clux removed this from the 0.84.0 milestone Jul 13, 2023

Uh oh!

Move ApiCapabilities into ApiResource and add Scope trait #1038

Are you sure you want to change the base?

Move ApiCapabilities into ApiResource and add Scope trait #1038

Uh oh!

Conversation

clux commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

clux commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nightkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clux commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Move `ApiCapabilities` into `ApiResource` and add `Scope` trait #1038

Move `ApiCapabilities` into `ApiResource` and add `Scope` trait #1038

clux commented Sep 30, 2022 •

edited

Loading

codecov-commenter commented Oct 1, 2022 •

edited

Loading

clux commented Oct 3, 2022 •

edited

Loading