Add support for CRD conversion

[MikailBag]

Motivation

Should close #865

Solution

This PR contains multiple API additions (I can split them into separate PRs if needed):

• kube::core::conversion::low_level - typings for the conversion-related k8s apis.
• kube::core::conversion::Converter - simple utility which abstracts away annoying details (like the fact that kubernetes batches conversion requests)
• kube::core::conversion::StarConverter - high-level utility which helps implement conversion in k8s-way (i.e. you define unversioned representation, converters to/from each version, and StarConverter implements the rest)
• kube::client::api::migrate_resources - function which strips .status.storedVersions of the CRD, migrating all objects to storage version.

Additionally, crd_derive_multi is substantially modified: it now includes star-based conversion webhook and shows how this webhook achieves lossless conversion between versions.

TODO:

• tests?
• fix formatting
• Maybe move Converter and StarConverter to kube_runtime?

[codecov-commenter]

Codecov Report

Merging #992 (b37e10e) into master (251728e) will decrease coverage by 0.28%.
The diff coverage is 64.96%.

❗ Current head b37e10e differs from pull request most recent head 3d41e45. Consider uploading reports for the commit 3d41e45 to get more accurate results

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
- Coverage   72.60%   72.32%   -0.29%     
==========================================
  Files          64       67       +3     
  Lines        4505     4661     +156     
==========================================
+ Hits         3271     3371     +100     
- Misses       1234     1290      +56     

Impacted Files	Coverage Δ
kube-client/src/api/util/mod.rs	92.10% <0.00%> (-1.23%)	⬇️
kube-core/src/discovery.rs	87.87% <0.00%> (-9.88%)	⬇️
kube-core/src/response.rs	44.00% <22.22%> (-56.00%)	⬇️
kube-core/src/conversion/low_level.rs	51.85% <51.85%> (ø)
kube-core/src/conversion/star.rs	81.42% <81.42%> (ø)
kube-core/src/conversion.rs	86.66% <86.66%> (ø)
kube-client/src/lib.rs	93.75% <100.00%> (ø)
kube-runtime/src/wait.rs	69.81% <0.00%> (-1.89%)	⬇️

[clux]

Thanks a lot for looking into this! Have left initial comments on the core parts because I think those parts are more important than the more opinionated converter, and it's important to get those right first.

[clux]

the types in low_level are the core types it's expected for us to export in kube_core::conversion imo, so everything in low_level should be re-exported at the top level (and the low_level module name should not be public)

[clux]

If we need the user to keep track of uid, then it probably makes more sense to create the response from the request like we do for AdmissionResponse, add mutate builders success/error (where success takes the converted_objects)

[MikailBag]

This design worries me a bit - when user at first creates logically invalid (neither allow nor deny) response, and then makes a decision, user can forget to do second step in some branches. Do you think this risk is negligible enough?

[clux]

Well, it is real a point of concern, but the consequences are pretty benign as all of these will be rejected without an explicit success, so at worst you have a bug that will likely be caught early (also easy to check for in unit tests), and in all other cases you are probably just missing the rejection reason.

I think there may be some follow-up point to perhaps later tweak the design of this to go through a type-safe builder that requires the the key choices to be made at compile-time (like what rustls does for its ClientConfig which we use in tls.rs), but I don't think this is critical enough to warrant that level of engineering. It's only one choice.

[clux]

This is the meat of the handler. If we factor out the other bits (above and below), and make a helpers for conversion, then writing this fn is no longer that daunting.

Given the precedent in admission, I think we should try to mimic that as much as possible, particularly since it maximises the amount of things the user can do with the structs (without having to reimplement everything).

[MikailBag]

Given the precedent in admission

To be honest, I think that kube::core::admission is too low-level and also needs some beautiful abstractions on top of it (like in kubebuilder, where you just need to implement on_create, on_update and on_delete) :)

then writing this fn is no longer that daunting.

I agree that there is nothing complex in this function (especially after applying your suggestion about .into_review). I also agree that sometimes user will write their own handler (e.g. when conversion function uses IO, so that async-ness is required). However it seems to me that in most cases this function will suit user, so it makes some sense to have it in kube.

[clux]

admission too low-level

We do actually want to stay as unopinionated as possible in the core part of kube here and represent the api, rather than make judgements on how it should be used. The runtime is really the only part that gets to make big judgements to more fit rust, otherwise deviations should be fairly small.

For admission controllers, which is so hairy to deal with in terms of cert setup anyway, I think it's probably a bigger win for the ecosystem if most users just picked or piggy-backed on something larger like kubewarden rather than us making two ways to do things and still forcing the full low-level cert/app pipeline on everyone.

[clux]

That said, if rust-centric, non-opinionated helper methods on those types can be made (as you said to perhaps match on various event types), in ways that does not detract from the original design, then I'm all for minor impls that help this. Admission should probably be a separate PR though. This is already large.

[MikailBag]

The runtime is really the only part that gets to make big judgements to more fit rust

I'm all for moving some parts of this PR (e.g. StarConversion and/or ConversionHandler) to kube_runtime. I however propose that we do these movements right before merging, after review is done.

This is already large.

Yeah :). If this PR became in any way too big, I'm ready to split it into smaller ones (e.g. one for new structs, one for Conversion, one for StarConversion).

[clux]

If the user just has a Value here, how do we know what to do with that? We need to presumably cast that into our old version first, but then we need to know what version it is. An example on the trait would be nice (without invoking the handler).

[clux]

Can we have this trait be generic and return some dyn Resource + DeserializeOwned rather than forcing everyone to go through Value? I figure most users that are dilligent enough to do conversions will probably have proper structs for both versions anyway.

[MikailBag]

1. User will likely want to work with concrete type, so either way downcast is required.
2. There is no way to go from serde_json::Value to dyn ... without requiring additional information from user (i.e. we can't guess which Deserialize::deserialize function to call based on just api version and kind). And I see no "nice" way to ask this information.

I think that we may add a ResourceRegistry with the following API:

type ResourceRegistry;
/// Creates empty registry
fn new();
/// Adds resource to the registry.
fn register<K: Resource>(&mut self, dt: &K::DynamicType);
/// if apiVersion and kind are known, parses this obj and upcasts to Box<Any>, otherwise returns None
fn parse(&self, obj: serde_json::Value) -> Option<serde_json::Result<Box<dyn Any>>>

Or, alternatively, more complex and more type-safe API

type ResourceRegistry<U>; // when U = Box<dyn Any>, equivalent to the previous API

/// Creates empty registry
fn new();
/// Adds resource to the registry.
fn register<K: Resource + Into<U>>(&mut self, dt: &K::DynamicType);
/// if apiVersion and kind are known, parses this obj and upcasts to U, otherwise returns None
fn parse(&self, obj: serde_json::Value) -> Option<serde_json::Result<U>>

Now user can create such a registry and than use it inside impl Conversion, but it is also useful in other contexts (e.g. writing admission webhook for multiple resources, or re-implementing kubectl describe in rust).

If you think that such an abstraction suits kube-rs, I can send a PR for it.

[clux]

1. If we make the trait generic for the serialization type, won't this work?

pub trait Conversion<Source: DeserializeOwned, Destination: Serialize> {
    fn convert(&self, obj: Source, desired_version: &str) -> Result<Destination>;
}

if Source turns out to be DynamicObject then that's fine (easy to convert from a Value), and if users just convert them into their own K then we do deserialization early and have this fn focus on the business logic - which it feels like it should be doing.

..probably would be hard to turn it into a generic TryFrom but that would be ideal.

2. I think that is probably overkill for kube for what it is; Deserialisation into a specific type. If there are many types the conversion hook deals with (which i think is a rare case), then it should be up to the app to multiplex.

[MikailBag]

In current design, Conversion represents not one possible conversion, but all of them (i.e. it does one of the N*(N-1) things depending on input and desired versions, where N is total version count). Consequently Source = Destination (because each version may appear as both input and output). And this is either DynamicObject (or other highly dynamic value), or serde(untagged)-enum of all supported versions.
So some downstream converter may look like

#[serde(untagged)]
enum FooVersions {
    V1(FooV1),
    V2(FooV2),
}

struct FooConversion;

impl Conversion<FooVersions> for FooConversion {
    fn convert(&self, input: FooVersions, desired_version: &str) {
         match (input, desired_version) {
             (V1(v1), "v2") => /*convert v1 -> v2*/,
             (V2(v2), "v1") => /*convert v2 -> v1*/,
             _ => /*noop conversions v1 -> v1, or v2 -> v2*/
         }
    }
}

Better than parsing serde_json::Value-es, but still seems like a boilerplate.

Alternatively, we may change Conversion responsibility - from "thing which converts any version into any other" to "thing which converts from one particular version into another particular version". This will require putting more type-erasure magic into ConversionHandler, but that's probably worth it.
This api usage might look something like:

impl Conversion<FooV1, Foov2> for FooConversion {
   // strongly-typed!
   fn convert(&self, foo: FooV1) -> Result<FooV2, String>;
}

impl Conversion<FooV2, FooV1> foo FooConversion {
  // strongly-typed!
  fn convert(&self, foo: FooV2) -> Result<FooV1>, String;
}

let handler = ConversionHandlerBuilder::new(FooConversion {})
    .register::<FooV1, FooV2>()
    .register::<FooV2, FooV1>()
    .build();

What do you think?

probably would be hard to turn it into a generic TryFrom

Yep, I don't see a way to go from impl TryFrom<FooV1> for FooV2, impl TryFrom<FooV2> for V1 to FooVersions + string -> FooVersions.

[clux]

Ah, I see. Yeah I think it would be better if the user just wrote plain conversions from type1 to type2 rather than a match (particularly if the normal case is a single in-flight version migration) and lift the version->type matching into the handler somehow.

In your expression:

impl TryFrom for FooV2, impl TryFrom for V1 to FooVersions + string -> FooVersions

What is the string there? The desired version?

I imagine if all the types we register impl Resource we wouldn't even need to tell the handler what version it is, it can figure that out via api_version() and then deserialize into the correct one at runtime.

[MikailBag]

What is the string there? The desired version?

Yes, from the Kubernetes PoV conversion webhook is fn(obj: json, desired: string) -> converted: json.

lift the version->type matching into the handler somehow.

OK, I'm sure we can achieve this.

[MikailBag]

clippy job fails on code, not introduced in this PR, so it needs to be rebased on fresh master.

If you don't object, I will use that opportunity to split PR into several smaller ones.

[clux]

Ok, yeah, that is sensible. 👍