Skip to content

Commit ee724da

Browse files
sbernauersbernauer
committed
fix: ConversionReview not working correctly.
Also added some tests
1 parent c0c928f commit ee724da

20 files changed

+665
-12
lines changed

‎Cargo.lock‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/stackable-versioned-macros/src/codegen/container/struct/k8s.rs‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -186,8 +186,8 @@ impl Struct {
186186
.ok_or_else(|| #parse_object_error::FieldNotStr)?;
187187

188188
let object = match api_version {
189-
#(#api_versions | #variant_strings => {
190-
let object = #serde_json_path::from_value(value)
189+
#(#api_versions => {
190+
let object = #serde_json_path::from_value(value)
191191
.map_err(|source| #parse_object_error::Deserialize { source })?;
192192

193193
Self::#variant_idents(object)
@@ -324,8 +324,10 @@ impl Struct {
324324
let convert_object_error = quote! { #versioned_path::ConvertObjectError };
325325

326326
// Generate conversion paths and the match arms for these paths
327-
let match_arms =
327+
let conversion_match_arms =
328328
self.generate_kubernetes_conversion_match_arms(versions, kubernetes_arguments);
329+
let noop_match_arms =
330+
self.generate_kubernetes_noop_match_arms(versions, kubernetes_arguments);
329331

330332
// TODO (@Techassi): Make this a feature, drop the option from the macro arguments
331333
// Generate tracing attributes and events if tracing is enabled
@@ -435,12 +437,13 @@ impl Struct {
435437
.map_err(|source| #convert_object_error::Parse { source })?;
436438

437439
match (current_object, desired_api_version) {
438-
#(#match_arms,)*
439-
// If no match arm matches, this is a noop. This is the case if the desired
440-
// version matches the current object api version.
441-
// NOTE (@Techassi): I'm curious if this will ever happen? In theory the K8s
442-
// apiserver should never send such a conversion review.
443-
_ => converted_objects.push(object),
440+
#(#conversion_match_arms,)*
441+
#(#noop_match_arms,)*
442+
(_, unknown_desired_api_version) => return ::std::result::Result::Err(
443+
#convert_object_error::DesiredApiVersionUnknown{
444+
unknown_desired_api_version: unknown_desired_api_version.to_string()
445+
}
446+
)
444447
}
445448
}
446449

@@ -454,6 +457,7 @@ impl Struct {
454457
versions: &[VersionDefinition],
455458
kubernetes_arguments: &KubernetesArguments,
456459
) -> Vec<TokenStream> {
460+
let group = &kubernetes_arguments.group;
457461
let variant_data_ident = &self.common.idents.kubernetes_parameter;
458462
let struct_ident = &self.common.idents.kubernetes;
459463
let spec_ident = &self.common.idents.original;
@@ -470,7 +474,10 @@ impl Struct {
470474
let current_object_version_string = &start.inner.to_string();
471475

472476
let desired_object_version = path.last().expect("the path always contains at least one element");
473-
let desired_object_version_string = desired_object_version.inner.to_string();
477+
let desired_object_api_version_string = format!(
478+
"{group}/{desired_object_version}",
479+
desired_object_version= desired_object_version.inner
480+
);
474481
let desired_object_variant_ident = &desired_object_version.idents.variant;
475482
let desired_object_module_ident = &desired_object_version.idents.module;
476483

@@ -493,7 +500,7 @@ impl Struct {
493500

494501
let convert_object_trace = kubernetes_arguments.options.enable_tracing.is_present().then(|| quote! {
495502
::tracing::trace!(
496-
#DESIRED_API_VERSION_ATTRIBUTE = #desired_object_version_string,
503+
#DESIRED_API_VERSION_ATTRIBUTE = #desired_object_api_version_string,
497504
#API_VERSION_ATTRIBUTE = #current_object_version_string,
498505
#STEPS_ATTRIBUTE = #steps,
499506
#KIND_ATTRIBUTE = #kind,
@@ -507,7 +514,7 @@ impl Struct {
507514
.then(|| quote! { status: #variant_data_ident.status, });
508515

509516
quote! {
510-
(Self::#current_object_version_ident(#variant_data_ident), #desired_object_version_string) => {
517+
(Self::#current_object_version_ident(#variant_data_ident), #desired_object_api_version_string) => {
511518
#(#conversions)*
512519

513520
let desired_object = Self::#desired_object_variant_ident(#desired_object_module_ident::#struct_ident {
@@ -528,6 +535,30 @@ impl Struct {
528535
.collect()
529536
}
530537

538+
fn generate_kubernetes_noop_match_arms(
539+
&self,
540+
versions: &[VersionDefinition],
541+
kubernetes_arguments: &KubernetesArguments,
542+
) -> Vec<TokenStream> {
543+
let group = &kubernetes_arguments.group;
544+
545+
versions
546+
.iter()
547+
.map(|version| {
548+
let version_ident = &version.idents.variant;
549+
let version_string = version.inner.to_string();
550+
let api_version_string = format!("{group}/{version_string}");
551+
552+
quote! {
553+
// This is the case if the desired version matches the current object api version.
554+
// NOTE (@Techassi): I'm curious if this will ever happen? In theory the K8s
555+
// apiserver should never send such a conversion review.
556+
(Self::#version_ident(_), #api_version_string) => converted_objects.push(object)
557+
}
558+
})
559+
.collect()
560+
}
561+
531562
fn generate_kubernetes_conversion_tracing(
532563
&self,
533564
kubernetes_arguments: &KubernetesArguments,

‎crates/stackable-versioned/Cargo.toml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,8 @@ serde = { workspace = true, optional = true }
3131
serde_json = { workspace = true, optional = true }
3232
serde_yaml = { workspace = true, optional = true }
3333
snafu = { workspace = true, optional = true }
34+
35+
[dev-dependencies]
36+
insta.workspace = true
37+
k8s-openapi.workspace = true
38+
kube.workspace = true

‎crates/stackable-versioned/fixtures/inputs/fail/request_missing.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1"
4+
}

‎crates/stackable-versioned/fixtures/inputs/fail/unkown_current_version.json‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v3",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v99",
10+
"kind": "Person",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {
17+
"username": "sbernauer"
18+
}
19+
}
20+
]
21+
}
22+
}

‎crates/stackable-versioned/fixtures/inputs/fail/unkown_desired_version.json‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v99",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v1alpha1",
10+
"kind": "Person",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {
17+
"username": "sbernauer"
18+
}
19+
}
20+
]
21+
}
22+
}

‎crates/stackable-versioned/fixtures/inputs/fail/unparseable_missing_field.json‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v3",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v1alpha1",
10+
"kind": "Person",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {}
17+
}
18+
]
19+
}
20+
}

‎crates/stackable-versioned/fixtures/inputs/fail/wrong_kind.json‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v3",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v1alpha1",
10+
"kind": "SomeOtherResource",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {
17+
"username": "sbernauer"
18+
}
19+
}
20+
]
21+
}
22+
}

‎crates/stackable-versioned/fixtures/inputs/pass/persons_to_v1alpha1.json‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v1alpha1",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v1alpha1",
10+
"kind": "Person",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {
17+
"username": "sbernauer"
18+
}
19+
},
20+
{
21+
"apiVersion": "test.stackable.tech/v1alpha2",
22+
"kind": "Person",
23+
"metadata": {},
24+
"spec": {
25+
"username": "sbernauer",
26+
"firstName": "Sebastian",
27+
"lastName": "Bernauer"
28+
}
29+
},
30+
{
31+
"apiVersion": "test.stackable.tech/v1beta1",
32+
"kind": "Person",
33+
"metadata": {},
34+
"spec": {
35+
"username": "sbernauer",
36+
"firstName": "Sebastian",
37+
"lastName": "Bernauer"
38+
}
39+
},
40+
{
41+
"apiVersion": "test.stackable.tech/v2",
42+
"kind": "Person",
43+
"metadata": {},
44+
"spec": {
45+
"username": "sbernauer",
46+
"firstName": "Sebastian",
47+
"lastName": "Bernauer",
48+
"gender": "Male"
49+
}
50+
},
51+
{
52+
"apiVersion": "test.stackable.tech/v3",
53+
"kind": "Person",
54+
"metadata": {},
55+
"spec": {
56+
"username": "sbernauer",
57+
"firstName": "Sebastian",
58+
"lastName": "Bernauer",
59+
"gender": "Male"
60+
}
61+
}
62+
]
63+
}
64+
}

‎crates/stackable-versioned/fixtures/inputs/pass/persons_to_v3.json‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
{
2+
"kind": "ConversionReview",
3+
"apiVersion": "apiextensions.k8s.io/v1",
4+
"request": {
5+
"uid": "c4e55572-ee1f-4e94-9097-28936985d45f",
6+
"desiredAPIVersion": "test.stackable.tech/v3",
7+
"objects": [
8+
{
9+
"apiVersion": "test.stackable.tech/v1alpha1",
10+
"kind": "Person",
11+
"metadata": {
12+
"name": "sbernauer",
13+
"namespace": "default",
14+
"uid": "d41e2019-5de3-409c-a7b2-0799ecb95e4b"
15+
},
16+
"spec": {
17+
"username": "sbernauer"
18+
}
19+
},
20+
{
21+
"apiVersion": "test.stackable.tech/v1alpha2",
22+
"kind": "Person",
23+
"metadata": {},
24+
"spec": {
25+
"username": "sbernauer",
26+
"firstName": "Sebastian",
27+
"lastName": "Bernauer"
28+
}
29+
},
30+
{
31+
"apiVersion": "test.stackable.tech/v1beta1",
32+
"kind": "Person",
33+
"metadata": {},
34+
"spec": {
35+
"username": "sbernauer",
36+
"firstName": "Sebastian",
37+
"lastName": "Bernauer"
38+
}
39+
},
40+
{
41+
"apiVersion": "test.stackable.tech/v2",
42+
"kind": "Person",
43+
"metadata": {},
44+
"spec": {
45+
"username": "sbernauer",
46+
"firstName": "Sebastian",
47+
"lastName": "Bernauer",
48+
"gender": "Male"
49+
}
50+
},
51+
{
52+
"apiVersion": "test.stackable.tech/v3",
53+
"kind": "Person",
54+
"metadata": {},
55+
"spec": {
56+
"username": "sbernauer",
57+
"firstName": "Sebastian",
58+
"lastName": "Bernauer",
59+
"gender": "Male"
60+
}
61+
}
62+
]
63+
}
64+
}

0 commit comments

Comments
 (0)