Skip to content

fix(stackable-versioned): Fix bugs in ConversionReviews integration #1061

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.

Sign up for GitHub

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

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

fix(stackable-versioned): Fix bugs in ConversionReviews integration #1061

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
ee724da
fix: ConversionReview not working correctly.
sbernauer Jun 17, 2025
35f1a8f
formatting
sbernauer Jun 17, 2025
41776eb
Move tests into dedicated folder
sbernauer Jun 23, 2025
ca11c44
move fixtures
sbernauer Jun 23, 2025
db2f65c
Check Kind of send object
sbernauer Jun 23, 2025
a80857d
Add comment
sbernauer Jun 23, 2025
a42d579
changelog
sbernauer Jun 23, 2025
def0da2
Merge branch 'main' into test/conversion
sbernauer Jun 23, 2025
f88e0ad
clippy
sbernauer Jun 23, 2025
1ef6561
Merge branch 'main' into test/conversion
sbernauer Jun 23, 2025
073c481
Move kind check to from_json_value
sbernauer Jun 23, 2025
548b300
Parse desired api version early
sbernauer Jun 23, 2025
95a10ae
Remove leftover comment
sbernauer Jun 24, 2025
9acd912
Update crates/stackable-versioned-macros/src/codegen/container/struct…
sbernauer Jun 24, 2025
129d6b4
Update crates/stackable-versioned-macros/src/codegen/container/struct…
sbernauer Jun 24, 2025
58b330b
Avoid stringify!
sbernauer Jun 24, 2025
d5a3109
adopt unexpected kind error message
sbernauer Jun 24, 2025
4364c53
Add ParseApiVersionError variant
sbernauer Jun 24, 2025
30b36dc
Update crates/stackable-versioned-macros/src/codegen/container/struct…
sbernauer Jun 24, 2025
9fb395c
Apply suggestions from code review
sbernauer Jun 24, 2025
4a79d52
Update crates/stackable-versioned/src/k8s.rs
sbernauer Jun 24, 2025
8f77619
Move into tests folder
sbernauer Jun 24, 2025
cb3dbfe
to_string -> to_owned
sbernauer Jun 24, 2025
119a845
Rename fn to try_from_json_object
sbernauer Jun 24, 2025
83e2438
Remove try_prefix
sbernauer Jun 24, 2025
7400d39
Move test inputs folder
sbernauer Jun 24, 2025
b7080ac
Rename run_for_file -> convert_via_file
sbernauer Jun 24, 2025
3f9e21f
Derive copy, clone and debug
sbernauer Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

118 changes: 93 additions & 25 deletions crates/stackable-versioned-macros/src/codegen/container/struct/k8s.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ impl Struct {
// Generate additional Kubernetes code, this is split out to reduce the complexity in this
// function.
let status_struct = self.generate_kubernetes_status_struct(kubernetes_arguments, is_nested);
let version_enum = self.generate_kubernetes_version_enum(tokens, vis, is_nested);
let version_enum =
self.generate_kubernetes_version_enum(kubernetes_arguments, tokens, vis, is_nested);
let convert_method = self.generate_kubernetes_conversion(versions);

let parse_object_error = quote! { #versioned_path::ParseObjectError };
Expand All @@ -173,21 +174,39 @@ impl Struct {
#k8s_openapi_path::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceDefinition,
#kube_core_path::crd::MergeError>
{
#kube_core_path::crd::merge_crds(vec![#(#crd_fns),*], stored_apiversion.as_str())
#kube_core_path::crd::merge_crds(vec![#(#crd_fns),*], stored_apiversion.as_version_str())
}

#convert_method

fn from_json_value(value: #serde_json_path::Value) -> ::std::result::Result<Self, #parse_object_error> {
let api_version = value
fn from_json_value(object: #serde_json_path::Value) -> ::std::result::Result<Self, #parse_object_error> {
Copy link
Member

Choose a reason for hiding this comment

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

note: I think we should stick with the parameter name value here. In my mind, this can only be called an object after it has been parsed as such. Also, the function name suggests that this parameter should be called value.

Copy link
Member Author

@sbernauer sbernauer Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I already had all of this thoughts and though about renaming the function to smh like `try_from_json_object) but didn't want to get into bikeshedding. But here we are :)

This function expects you to pass a k8s object to it, not an arbitrary JSON value such as f32 or String. I think we should make this clear in the name, I would therefore propose to name it fn try_from_json_object(object: #serde_json_path::Value) or fn try_from_json_k8s_object(k8s_object: #serde_json_path::Value) (the try_ prefix is already under discussion in #1061 (comment))

Copy link
Member

@Techassi Techassi Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would be fine with fn try_from_json_object(object_value: #serde_json_path::Value) and internally clearly separate between the "raw" object and the successfully deserialized "object" we return in the end.

In my opinion the try_ prefix doesn't provide a whole lot here. The function clearly indicates that it is fallible by returning a Result.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like a good compromise! 119a845

let object_kind = object
.get("kind")
.ok_or_else(|| #parse_object_error::FieldMissing{ field: "kind".to_string() })?
.as_str()
.ok_or_else(|| #parse_object_error::FieldNotStr{ field: "kind".to_string() })?;

// Note(@sbernauer): The kind must be checked here, because it is
// possible for the wrong object to be deserialized.
// Checking here stops us assuming the kind is correct and
// accidentally updating upgrade/downgrade information in the
// status in a later step.
if object_kind != stringify!(#enum_ident) {
return Err(#parse_object_error::UnexpectedObjectKind{
kind: object_kind.to_string(),
supported_kind: stringify!(#enum_ident).to_string(),
});
}

let api_version = object
.get("apiVersion")
.ok_or_else(|| #parse_object_error::FieldNotPresent)?
.ok_or_else(|| #parse_object_error::FieldMissing{ field: "apiVersion".to_string() })?
.as_str()
.ok_or_else(|| #parse_object_error::FieldNotStr)?;
.ok_or_else(|| #parse_object_error::FieldNotStr{ field: "apiVersion".to_string() })?;

let object = match api_version {
#(#api_versions | #variant_strings => {
let object = #serde_json_path::from_value(value)
#(#api_versions => {
let object = #serde_json_path::from_value(object)
.map_err(|source| #parse_object_error::Deserialize { source })?;

Self::#variant_idents(object)
Expand Down Expand Up @@ -218,6 +237,7 @@ impl Struct {

fn generate_kubernetes_version_enum(
&self,
kubernetes_arguments: &KubernetesArguments,
tokens: &KubernetesTokens,
vis: &Visibility,
is_nested: bool,
Expand All @@ -228,9 +248,16 @@ impl Struct {
// module (in standalone mode).
let automatically_derived = is_nested.not().then(|| quote! {#[automatically_derived]});

let versioned_path = &*kubernetes_arguments.crates.versioned;
let convert_object_error = quote! { #versioned_path::ConvertObjectError };

// Get the per-version items to be able to iterate over them via quote
let variant_strings = &tokens.variant_strings;
let variant_idents = &tokens.variant_idents;
let api_versions = variant_strings
.iter()
.map(|version| format!("{group}/{version}", group = &kubernetes_arguments.group))
.collect::<Vec<_>>();

quote! {
#automatically_derived
Expand All @@ -241,17 +268,33 @@ impl Struct {
#automatically_derived
impl ::std::fmt::Display for #enum_ident {
fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::result::Result<(), ::std::fmt::Error> {
f.write_str(self.as_str())
// The version (without the Kubernetes group) is probably more human-readable
f.write_str(self.as_version_str())
}
}

#automatically_derived
impl #enum_ident {
pub fn as_str(&self) -> &str {
pub fn as_version_str(&self) -> &str {
match self {
#(#variant_idents => #variant_strings),*
}
}

pub fn as_api_version_str(&self) -> &str {
match self {
#(#variant_idents => #api_versions),*
}
}

pub fn try_from_api_version(api_version: &str) -> Result<Self, #convert_object_error> {
match api_version {
#(#api_versions => Ok(Self::#variant_idents)),*,
_ => Err(#convert_object_error::DesiredApiVersionUnknown {
unknown_desired_api_version: api_version.to_string(),
}),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Remove the try_ prefix and use separate error type.

The function name doesn't need to indicate this function is fallible, because it already indicates that via the function signature (it returns a Result). As such, the try_ prefix can be removed.

Also, the returned error seems to indicate that this function is only used in the context of a conversion, which doesn't have to be the case. I instead suggest designing the errors like this:

#[derive(Debug, Snafu)]
pub enum ConvertObjectError {
    // ...

    #[snafu(display("unknown desired API version"))]
    UnknownDesiredApiVersion { source: ParseVersionError },
}

#[derive(Debug, Snafu)]
#[snafu(display("failed to parse {input:?} as version))]
pub struct ParseVersionError {
    input: String,
}

The suggested changes result in:

Suggested change
pub fn try_from_api_version(api_version: &str) -> Result<Self, #convert_object_error> {
match api_version {
#(#api_versions => Ok(Self::#variant_idents)),*,
_ => Err(#convert_object_error::DesiredApiVersionUnknown {
unknown_desired_api_version: api_version.to_string(),
}),
}
}
pub fn from_api_version(api_version: &str) -> Result<Self, ParseVersionError> {
match api_version {
#(#api_versions => Ok(Self::#variant_idents)),*,
_ => Err(ParseVersionError {
input: api_version.to_owned(),
}),
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

error added in 4364c53

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the name I mimicked https://doc.rust-lang.org/std/convert/trait.TryFrom.html. I didn't want to commit to the trait, as there might be cases we need to parse the versions (without the group).
try_from_api_version sounds like a logical evolution of try_from to me

Copy link
Member

Choose a reason for hiding this comment

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

error added in 4364c53

Could you please also change to_string to to_owned?

As for the name I mimicked doc.rust-lang.org/std/convert/trait.TryFrom.html. I didn't want to commit to the trait, as there might be cases we need to parse the versions (without the group).
try_from_api_version sounds like a logical evolution of try_from to me

The TryFrom trait is meant to be used for fallible conversion. The try_ prefix is used here to differentiate between this trait and the non-fallible From trait.

We are however not dealing with a conversion here. We are instead dealing with parsing (a string to an enum variant). In this case, the FromStr trait would be the go-to. This trait however can only be used if there is a single way to parse the string.

Therefore we have the following options:

  • Support parsing from both version and apiVersion - the FromStr trait cannot be used, stick with from_version and from_api_version.
  • Support parsing only from apiVersion - the FromStr trait can be used

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you are right => 83e2438
The to_owned was already done in bulk with a different commit

}
}
}
Expand Down Expand Up @@ -315,6 +358,7 @@ impl Struct {
let kubernetes_arguments = self.common.options.kubernetes_arguments.as_ref()?;

let struct_ident = &self.common.idents.kubernetes;
let version_enum_ident = &self.common.idents.kubernetes_version;

let kube_client_path = &*kubernetes_arguments.crates.kube_client;
let serde_json_path = &*kubernetes_arguments.crates.serde_json;
Expand All @@ -324,8 +368,9 @@ impl Struct {
let convert_object_error = quote! { #versioned_path::ConvertObjectError };

// Generate conversion paths and the match arms for these paths
let match_arms =
let conversion_match_arms =
self.generate_kubernetes_conversion_match_arms(versions, kubernetes_arguments);
let noop_match_arms = self.generate_kubernetes_noop_match_arms(versions);

// TODO (@Techassi): Make this a feature, drop the option from the macro arguments
// Generate tracing attributes and events if tracing is enabled
Expand Down Expand Up @@ -375,11 +420,8 @@ impl Struct {
}
};

// Extract the desired api version
let desired_api_version = request.desired_api_version.as_str();

// Convert all objects into the desired version
let response = match Self::convert_objects(request.objects, desired_api_version) {
let response = match Self::convert_objects(request.objects, &request.desired_api_version) {
::std::result::Result::Ok(converted_objects) => {
#successful_conversion_response_event

Expand Down Expand Up @@ -426,6 +468,8 @@ impl Struct {
)
-> ::std::result::Result<::std::vec::Vec<#serde_json_path::Value>, #convert_object_error>
{
let desired_api_version = #version_enum_ident::try_from_api_version(desired_api_version)?;

let mut converted_objects = ::std::vec::Vec::with_capacity(objects.len());

for object in objects {
Expand All @@ -434,13 +478,11 @@ impl Struct {
let current_object = Self::from_json_value(object.clone())
.map_err(|source| #convert_object_error::Parse { source })?;

match (current_object, desired_api_version) {
#(#match_arms,)*
// If no match arm matches, this is a noop. This is the case if the desired
// version matches the current object api version.
// NOTE (@Techassi): I'm curious if this will ever happen? In theory the K8s
// apiserver should never send such a conversion review.
_ => converted_objects.push(object),
match (current_object, &desired_api_version) {
Copy link
Member

Choose a reason for hiding this comment

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

note: Using a reference here seems a little weird. We could instead add #[derive(Copy, Clone)] to the version enum, to be able to cheaply copy the enum, which is also suggested by the docs.

Would be interesting to see if there are any performance differences. This StackOverflow post seems to suggest that copying is smaller in memory but there will be likely no performance difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

3f9e21f

#(#conversion_match_arms,)*
// We explicitly list the remaining no-op cases, so the compiler ensures we
// did not miss a conversion.
#(#noop_match_arms,)*
Comment on lines +487 to +489
Copy link
Member

Choose a reason for hiding this comment

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

note: I'd like to avoid generating a bunch of match arms which should basically never be hit. The only benefit of this is compiler checking, which is basically already covered by our snapshot tests.

If we don't generate all non-noop combinations, the compiler error doesn't help. The snapshot tests are there to ensure this never happens.

}
}

Expand All @@ -454,8 +496,10 @@ impl Struct {
versions: &[VersionDefinition],
kubernetes_arguments: &KubernetesArguments,
) -> Vec<TokenStream> {
let group = &kubernetes_arguments.group;
let variant_data_ident = &self.common.idents.kubernetes_parameter;
let struct_ident = &self.common.idents.kubernetes;
let version_enum_ident = &self.common.idents.kubernetes_version;
let spec_ident = &self.common.idents.original;

let versioned_path = &*kubernetes_arguments.crates.versioned;
Expand All @@ -470,7 +514,10 @@ impl Struct {
let current_object_version_string = &start.inner.to_string();

let desired_object_version = path.last().expect("the path always contains at least one element");
let desired_object_version_string = desired_object_version.inner.to_string();
let desired_object_api_version_string = format!(
"{group}/{desired_object_version}",
desired_object_version= desired_object_version.inner
);
let desired_object_variant_ident = &desired_object_version.idents.variant;
let desired_object_module_ident = &desired_object_version.idents.module;

Expand All @@ -493,7 +540,7 @@ impl Struct {

let convert_object_trace = kubernetes_arguments.options.enable_tracing.is_present().then(|| quote! {
::tracing::trace!(
#DESIRED_API_VERSION_ATTRIBUTE = #desired_object_version_string,
#DESIRED_API_VERSION_ATTRIBUTE = #desired_object_api_version_string,
#API_VERSION_ATTRIBUTE = #current_object_version_string,
#STEPS_ATTRIBUTE = #steps,
#KIND_ATTRIBUTE = #kind,
Expand All @@ -507,7 +554,7 @@ impl Struct {
.then(|| quote! { status: #variant_data_ident.status, });

quote! {
(Self::#current_object_version_ident(#variant_data_ident), #desired_object_version_string) => {
(Self::#current_object_version_ident(#variant_data_ident), #version_enum_ident::#desired_object_variant_ident) => {
#(#conversions)*

let desired_object = Self::#desired_object_variant_ident(#desired_object_module_ident::#struct_ident {
Expand All @@ -528,6 +575,27 @@ impl Struct {
.collect()
}

fn generate_kubernetes_noop_match_arms(
&self,
versions: &[VersionDefinition],
) -> Vec<TokenStream> {
let version_enum_ident = &self.common.idents.kubernetes_version;

versions
.iter()
.map(|version| {
let version_ident = &version.idents.variant;

quote! {
// This is the case if the desired version matches the current object api version.
// NOTE (@Techassi): I'm curious if this will ever happen? In theory the K8s
// apiserver should never send such a conversion review.
(Self::#version_ident(_), #version_enum_ident::#version_ident) => converted_objects.push(object)
}
})
.collect()
}

fn generate_kubernetes_conversion_tracing(
&self,
kubernetes_arguments: &KubernetesArguments,
Expand Down
Loading