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 Resource derive macro #1565

Merged
merged 9 commits into from
Sep 9, 2024
Merged

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Sep 1, 2024

Motivation

When working with ConfigMap data, it is often expected to have a specific data structure on the inner content. While defining a CRD is the standard way of resolving the fields validation, it is sometimes not possible or desirable at the moment.

This generally requires a manual conversion via serde, which involves creating a custom structure for the data content, which is only one field away from being a fully replicated ConfigMap or Secret definition (in the context of the problem).

Choosing this direction may require implementing Resource trait manually, which adds to the amount of glue code.

Solution

Provide a Resource derive macro, capable of inheriting Resource trait implementation of the specified upstream object.

The resulting code can look like the following:

use k8s_openapi::api::core::v1::ConfigMap;

#[derive(Resource)]
#[inherit(resource = "ConfigMap", namespaced)]
struct TypedMap {
    metadata: ObjectMeta,
    data: Option<TypedData>,
}

struct TypedData {
    field: String,
}

@clux
Copy link
Member

clux commented Sep 1, 2024

Hey, this is a cool idea.

As a quick first impression, I think it makes sense, although it does not necessarily save much in terms of user code since custom structs can re-use type information with the dynamic api:

// Here we simply steal the type info from k8s_openapi, but we could create this from scratch.
let ar = ApiResource::erase::<k8s_openapi::api::core::v1::Pod>(&());
let pods: Api<PodSimple> = Api::default_namespaced_with(client, &ar);

However, there's a reasonable argument for this in terms of complexity, by being able to just use the normal (non-dynamic api) and not have to learn about concepts of erasure.

Copy link

codecov bot commented Sep 1, 2024

Codecov Report

Attention: Patch coverage is 56.41026% with 17 lines in your changes missing coverage. Please review.

Project coverage is 75.3%. Comparing base (c975335) to head (4d59964).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kube-derive/src/resource.rs 40.0% 15 Missing ⚠️
kube-derive/src/lib.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1565     +/-   ##
=======================================
- Coverage   75.4%   75.3%   -0.1%     
=======================================
  Files         80      82      +2     
  Lines       7284    7323     +39     
=======================================
+ Hits        5489    5511     +22     
- Misses      1795    1812     +17     
Files with missing lines Coverage Δ
kube-derive/tests/resource.rs 100.0% <100.0%> (ø)
kube/src/lib.rs 88.5% <ø> (ø)
kube-derive/src/lib.rs 0.0% <0.0%> (ø)
kube-derive/src/resource.rs 40.0% <40.0%> (ø)

@Danil-Grigorev Danil-Grigorev force-pushed the resource-inherit branch 6 times, most recently from a51d872 to dae2ffa Compare September 1, 2024 20:36
@Danil-Grigorev
Copy link
Member Author

Danil-Grigorev commented Sep 1, 2024

Yes, agree, I felt that this is explored area already, but at the moment of writing this model looked natural to me. I believe unstable-client features should work with this implementation out of the box.

@Danil-Grigorev
Copy link
Member Author

Hmm, I actually tried using it with similar to dynamic_pod example:

let secret_api: Api<CertSecret> = Api::namespaced_with(
            ctx.client.clone(),
            "cattle-fleet-system",
            ApiResource::erase::<Secret>(&()),
 );

and got:

60 |         let secret_api: Api<CertSecret> = Api::namespaced_with(
   |                                           ^^^^^^^^^^^^^^^^^^^^ the trait `k8s_openapi::Metadata` is not implemented for `CertSecret`

Since the secret follows different model - data vs spec, does it make sense to use generated resource definition?

@Danil-Grigorev Danil-Grigorev force-pushed the resource-inherit branch 2 times, most recently from 8a3c468 to 85ec5af Compare September 2, 2024 20:19
@clux
Copy link
Member

clux commented Sep 4, 2024

hm, yeah. you'd actually have to use the dynamic api fully for the non-compliant core objects with erase (no Object or derive CustomResource bypass):

let secret_api: Api<DynamicObject> = Api::namespaced_with(
            ctx.client.clone(),
            "cattle-fleet-system",
            ApiResource::erase::<Secret>(&()),
 );

which means secondary Value unpacking, and it does make the dynamic api less attractive for known types. let me review this a bit more properly.

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of minor comments. i like this, particularly after realising the dynamic api is very limited for known types. i do think we can improve the ergonomics a little bit.

(also i am not 100% sure about the name, but we can bikeshed that later)

kube-derive/src/resource_inherit.rs Outdated Show resolved Hide resolved
kube-derive/src/resource_inherit.rs Outdated Show resolved Hide resolved
kube-derive/tests/resource_inherit.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-add changelog added category for prs label Sep 4, 2024
Allows to generate Resource trait implementation for types which proxy
another type internally.

ConfigMaps and Secrets can be strictly typed on the client side. While
using DeserializeGuard, resources can be listed and watched, skipping
invalid resources.

Signed-off-by: Danil-Grigorev <[email protected]>
@clux clux added this to the 0.94.0 milestone Sep 6, 2024
- Display usage in errorbounded CM watcher

Signed-off-by: Danil-Grigorev <[email protected]>
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two naming two minor naming things

kube-derive/src/lib.rs Outdated Show resolved Hide resolved
kube-derive/tests/resource.rs Outdated Show resolved Hide resolved
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
@Danil-Grigorev Danil-Grigorev changed the title Add ResourceInherit derive macro Add Resource derive macro Sep 6, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments on new example

examples/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +94 to +102
let _ca: ConfigMap = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
let _ca: CaConfigMapManual = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
let ca: CaConfigMap = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
Copy link
Member

@clux clux Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a helper comment here:

Suggested change
let _ca: ConfigMap = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
let _ca: CaConfigMapManual = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
let ca: CaConfigMap = client
.get("kube-root-ca.crt", &Namespace::from(ns.name_any()))
.await?;
for namespace in namespaces {
let ns = Namespace::from(namespace);
// Equivalent ways to GET using different structs and different Resource impls
let _ca: ConfigMap = client.get("kube-root-ca.crt", &ns).await?;
let _ca: CaConfigMapManual = client.get("kube-root-ca.crt", &ns).await?;
let ca: CaConfigMap = client.get("kube-root-ca.crt", &ns).await?;

Signed-off-by: Danil-Grigorev <[email protected]>
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last minor nit

examples/README.md Outdated Show resolved Hide resolved
@clux clux mentioned this pull request Sep 7, 2024
Signed-off-by: Danil-Grigorev <[email protected]>
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thank you so much!

@clux clux merged commit f45ac81 into kube-rs:main Sep 9, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants