Separate GVK from ApiResource #495

MikailBag · 2021-04-16T22:42:20Z

This pull request shrinks GVK (so that it contains only G, V and K 😄 ), additionally making GVK fields public. It introduces a new ApiResource type which is much bigger (maybe it even contains too much stuff and should be split further). Also, two ways to obtain ApiResource are provided:

from GVK (with some guessing)
from APIResource

This PR also affects #491, because Discovery would contain a method to go from GVK to ApiResource (using discovered information), and this is intended to be both a simple and correct way to get an ApiResource.

kube/src/api/dynamic.rs

MikailBag · 2021-04-16T22:49:21Z

kube/src/api/gvk.rs

+        let version = version_.to_string();
+        let group = group_.to_string();
+        let kind = kind_.to_string();
+        if version.is_empty() {


Aside: this validation is incomplete anyway (it's impossible to validate input without asking apiserver). Also, it seems to me that typo in the resource name is more likely than providing an empty string, and Rust in general discourages using empty strings as default values.

That's why I'd consider removing these checks and making this constructor infallible.

I think the checks have value, even if it's an incomplete safety. But we could move the checks to the conversion between GVK and ApiResource and have that be fallible via a TryFrom impl.

Ok, if we're not doing TryFrom, then it's probably better to drop the checks as you suggested. All the values are either generated via kube-derive or returned from the apiserver anyway.

clux

Comments

kube/src/api/dynamic.rs

clux · 2021-04-17T17:41:42Z

kube/src/api/dynamic.rs

+pub struct ApiResourceExtras {
+    /// Scope of the resource
+    pub scope: Scope,
+    /// Available subresources. Please note that returned ApiResources are not
+    /// standalone resources. Their name will be of form `subresource_name`,
+    /// not `resource_name/subresource_name`.
+    /// To work with subresources, use `Request` methods.
+    pub subresources: Vec<(ApiResource, ApiResourceExtras)>,
+    /// Supported operations on this resource
+    pub operations: Operations,
+}


Is there particular problems we can solve with this information? Is the idea to use arbitrary subresources via the new Request method you added?

Is there particular problems we can solve with this information?

Well, dynamic-api example can skip non-listable resources (instead of ignoring list errors).

As I said, this stuff is the least important part of the PR. It can be easily deleted because anyone who needs this information can get it using Client methods and looking at APIResource stuff. I don't see a way kube itself could benefit from this information.

Ah, good point. I was mostly wondering here since it wasn't used. I think it's totally okay to include in the discovery module.

Ok. Then I'll move ApiResourceExtras (with your suggestions applied) into #491 since it's more about API discovery rather than talking to the api-server.

kube/src/api/dynamic.rs

clux · 2021-04-17T17:50:12Z

kube/src/api/gvk.rs

+        let version = version_.to_string();
+        let group = group_.to_string();
+        let kind = kind_.to_string();
+        if version.is_empty() {


I think the checks have value, even if it's an incomplete safety. But we could move the checks to the conversion between GVK and ApiResource and have that be fallible via a TryFrom impl.

kube/src/api/dynamic.rs

kube-runtime/src/reflector/object_ref.rs

clux · 2021-04-17T18:01:49Z

kube/src/api/dynamic.rs

+    /// This function has to **guess** resource plural name.
+    /// While it makes it best to guess correctly, sometimes it can
+    /// be wrong, and using returned ApiResource will lead to incorrect


Hmm, not super happy about this yet. The plural builder before at least let people inject a correct value, but now it looks like it only works well with apidiscovery. We need to do something for kube-derive case which was using GVK with plural.

Maybe we should have a way to retrieve the ApiResource from a derived CustomResource? That way people wouldn't have to do the error prone setup of inserting a custom plural twice.

inject a correct value

It can be achieved by manually setting plural_name on created ApiResource. Do you think highlighting this in docs is enough? Alternatively, we can add a plural: Option<String> parameter to from_gvk.

Maybe we should have a way to retrieve the ApiResource from a derived CustomResource

Very good point. CustomResource should have something like api_resource() -> ApiResource.

I think what you've done for kube-derive makes it good enough to not worry about plural from_gvk.

I don't think it's worth keeping the old plural behaviour in the conversion if people can make a more accurate thing auto-generated.

One thing that comes up is to move the generated methods behind a trait, but I'll make a issue for that separately.

Actually, if we are in general not encouraging from_gvk and not turning it into an TryFrom or From impl, then it probably makes sense to have a way to do it explicitly.

Maybe a from_gvk_plural, and leave from_gvk to infer?

Blue-sky thinking.. another way would be to deprecate inferred from_gvk, and instead have a discovery helper to look up the details based on GVK?

discovery helper to look up the details based on GVK

I think this method implements it. I also think deprecation has some sense, but I'll better leave this decision to you and other maintainers:)

clux · 2021-04-30T09:22:34Z

kube/src/client/discovery.rs

+    /// Checks that given verb is supported on this resource.
+    pub fn supports_operation(&self, operation: &str) -> bool {
+        self.operations.iter().any(|op| op == operation)
+    }


Ah, this is actually very nice.

I thought we would have

#[derive(Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum Operations { Create, Get, ... Other(String), }

but then your method avoids users needing to grab the verbs in scope if they wish (but they can still use the typed constants).

clux · 2021-05-15T08:46:16Z

Ok, sorry for being so slow on this one.

Short story, I am happy with the functionality here, but uneasy about the name ApiResource as it introduces clashes with k8s_openapi's APIResource which we also require users deal with.

The re-use of names in the kube::Resource from k8s_openapi::Resource were fine because we are ok with only using one of them, here's it's like asking for trouble.

We could change the name, but what if we instead change our Client::list_api_group_resources and Client::list_core_api_resources so that we return something usable by the Discovery module directly? That way there's no need for us to rename, and we provide a more holistic interface.

MikailBag · 2021-05-15T12:30:26Z

uneasy about the name

I'll be happy to rename ApiResource into any other more appropriate name.

also require users deal with

Hmm, I don't completely understand this point. kube::client::Discovery is intended to completely hide APIResource and other metav1 stuff.

change our Client::list_api_group_resources and Client::list_core_api_resources so that we return something usable by the Discovery module directly

If these methods are only useful as an implementation detail of discovery (i.e. there is no need for the majority of users to look at raw APIResource), then maybe we should make them private?

clux · 2021-05-15T12:53:12Z

Hmm, I don't completely understand this point. kube::client::Discovery is intended to completely hide APIResource and other metav1 stuff.

Yeah, but the client listers ultimately expose primitives from k8s-openapi with very similar names, and it's unclear that we have easier alternatives (with almost the same name).

change our Client::list_api_group_resources and Client::list_core_api_resources so that we return something usable by the Discovery module directly

If these methods are only useful as an implementation detail of discovery (i.e. there is no need for the majority of users to look at raw APIResource), then maybe we should make them private?

Oh, yeah. That might be a better idea :D

I was thinking having the Client listers return something closer to GroupVersionData instead, but that doesn't sound that smart given how the discovery module works.

Maybe we just deprecate the client listers instead.

clux · 2021-05-15T16:40:26Z

I'll just merge this and make small changes to it in master. Have already held this up for too long.

clux · 2021-05-18T22:08:32Z

Released in 0.54.0 with some minor modifications (Group -> ApiGroup, and most of it is now actually in kube-core (but re-exported from kube). Thanks again.

MikailBag commented Apr 16, 2021

View reviewed changes

kube/src/api/dynamic.rs Outdated Show resolved Hide resolved

MikailBag commented Apr 16, 2021

View reviewed changes

clux reviewed Apr 17, 2021

View reviewed changes

clux mentioned this pull request Apr 19, 2021

define a CustomResourceExt in kube for kube-derive #497

Closed

3 tasks

MikailBag force-pushed the split-gvk-apiresource branch from f2c3642 to 2894576 Compare April 20, 2021 08:04

MikailBag added 7 commits April 24, 2021 01:26

Separate GVK from ApiResource

78d06d2

Move details to other type

7e3cc6f

Partially implement review suggestions

8106987

update

369278f

Make GVK and GVR constructors infallible

267b7b1

Update dynamic watcher

32b6083

finish rebase

62f4d0a

MikailBag force-pushed the split-gvk-apiresource branch from 2894576 to 62f4d0a Compare April 23, 2021 22:59

Remove Operations

053ae27

clux reviewed Apr 30, 2021

View reviewed changes

clux merged commit 2e3fae1 into kube-rs:master May 15, 2021

clux mentioned this pull request May 17, 2021

kube-core type movement #519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate GVK from ApiResource #495

Separate GVK from ApiResource #495

MikailBag commented Apr 16, 2021 •

edited

Loading

MikailBag Apr 16, 2021

clux Apr 17, 2021

clux Apr 19, 2021

clux left a comment

clux Apr 17, 2021

MikailBag Apr 17, 2021

clux Apr 19, 2021

MikailBag Apr 19, 2021

clux Apr 17, 2021

clux Apr 17, 2021

MikailBag Apr 17, 2021

clux Apr 19, 2021

clux Apr 19, 2021

nightkr Apr 24, 2021 •

edited

Loading

MikailBag Apr 24, 2021

clux Apr 30, 2021 •

edited

Loading

clux commented May 15, 2021

MikailBag commented May 15, 2021

clux commented May 15, 2021

clux commented May 15, 2021

clux commented May 18, 2021

Separate GVK from ApiResource #495

Separate GVK from ApiResource #495

Conversation

MikailBag commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightkr Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

clux commented May 15, 2021

MikailBag commented May 15, 2021

clux commented May 15, 2021

clux commented May 15, 2021

clux commented May 18, 2021

MikailBag commented Apr 16, 2021 •

edited

Loading

nightkr Apr 24, 2021 •

edited

Loading

clux Apr 30, 2021 •

edited

Loading