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

Implement CEL validation proc macro for generated CRDs #1621

Closed

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Oct 27, 2024

Motivation

CRDs allow to declare server-side validation rules using CEL. This functionality is supported via #[schemars(schema_with = "<schemagen-wrapper>")], but requires defining a method with handling logic, which may be error-prone.

Since kube owns CRD generation code, the idea is to simplify this process for added validation rules and achieve more declarative approach, similar to the kubebuilder library. This approach will be compatible with kopium generation based on existing CRD structures, already using CEL expressions.

Solution

Allow for a more native handling of CEL validation rules on the CRDs via a field macro.

Unfortunately due to limitations of derive macros, a new #[proc_macro_attribute] is required to allow adjustments of the annotations on the fields, currently named #[cel_validation] (working prototype name),

Similar approach may also be used to allow for server-side defaults and other custom modifications of the CRD in the future, if decided.

#[cel_validation]
#[derive(, JsonSchema)]
#[kube(

)]
pub struct FooSpec {
    // Field with CEL validation
    #[serde(default)]
    #[validated(rule="self != 'illegal'", message="string cannot be illegal")]
    #[validated(rule="self != 'not legal'")]
    cel_validated: Option<String>,

@Danil-Grigorev Danil-Grigorev marked this pull request as ready for review October 27, 2024 14:40
Copy link

codecov bot commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 5.55556% with 68 lines in your changes missing coverage. Please review.

Project coverage is 74.6%. Comparing base (179936a) to head (4eec7d8).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
kube-derive/src/custom_resource.rs 5.8% 66 Missing ⚠️
kube-derive/src/lib.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1621     +/-   ##
=======================================
- Coverage   75.3%   74.6%   -0.6%     
=======================================
  Files         82      82             
  Lines       7344    7416     +72     
=======================================
+ Hits        5528    5532      +4     
- Misses      1816    1884     +68     
Files with missing lines Coverage Δ
kube/src/lib.rs 88.5% <ø> (ø)
kube-derive/src/lib.rs 0.0% <0.0%> (ø)
kube-derive/src/custom_resource.rs 60.1% <5.8%> (-21.3%) ⬇️

@clux
Copy link
Member

clux commented Oct 28, 2024

Hey this is great! Thanks a lot for doing this.

I was wondering if you could elaborate a bit on a couple of things:

Unfortunately due to limitations of derive macros, a new #[proc_macro_attribute] is required to allow adjustments of the annotations on the fields, currently named #[cel_validation] (working prototype name),

is this to ensure precedence of annotations?

is there possibly something we could do to simplify these requirements by perhaps referencing a common rewrite rule in kube-core (like we do in schema.rs for structural rewriting)?

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.

quick comments / questions

message: Option<String>,
}

pub(crate) fn cel_validation(_: TokenStream, input: TokenStream) -> TokenStream {
Copy link
Member

Choose a reason for hiding this comment

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

what is the first arg here for? the new proc macro seems take args and input, but you seem to be discarding args?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably need to be checked, they are currently empty. Could be a good place for name overrides to avoid clashes

examples/crd_derive_schema.rs Show resolved Hide resolved
Comment on lines +606 to +608
let validation_method_name = field.ident.as_ref().map(|i| i.to_string()).unwrap_or_default();
let name = Ident::new(&validation_method_name, Span::call_site());
let field_type = &field.ty;
Copy link
Member

Choose a reason for hiding this comment

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

This stuff here could do with some comments. You're implementing a method fith a fixed name. Could this ave clashing issues? Can we parametrise such a function from kube-core instead?

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 long as there is no structure Validation in the scope, there should not be a clash, but makes sense to add an override for such occasion 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be added now - #[cel_validation(struct_name = "FooSpecValidation")]

Copy link
Member

Choose a reason for hiding this comment

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

i guess i am still confused about the temporary struct you are making. it seems to me that the #[cel_validation(struct_name = X struct X is there for you to implement a method, but that method feels like something we can define cleanly inside kube-core, and invoke from kube-derive.

Comment on lines 563 to 564
let struct_name = ast.ident.to_string() + "Validation";
let anchor = Ident::new(&struct_name, Span::call_site());
Copy link
Member

Choose a reason for hiding this comment

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

magic names needs a comment here probably

@Danil-Grigorev
Copy link
Member Author

Danil-Grigorev commented Oct 28, 2024

is this to ensure precedence of annotations?

yes, I was trying to replace #[validated] with schemars within the CustomResource derive, but it didn't work (even though there is an implicit derive order and CustomResource comes before JsonSchema). So the macro performs replacement in the first place.

If the keyword comes after derive, no macro will be able to process replaced #[schemars] annotations.

@Danil-Grigorev
Copy link
Member Author

I'll close it for now to find more suitable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants