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 derive(CELSchema) macro for generating cel validation on CRDs #1649

Merged
merged 15 commits into from
Dec 22, 2024

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Nov 24, 2024

Motivation

Related to #1367

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.

#[derive(, CELSchema)]
#[kube(
  rule = Rule::new("self.metadata.name == 'singleton'"),

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

This PR is a followup on #1621 which addresses some of the concerns.

  1. Implemented by generating a JsonSchema, which allows further additions to the schema later, struct level validation rules and is not affected by schemars version.
  2. Based on Implement CEL validation proc macro for generated CRDs #1621 (comment) - using methods from the kube::core and invoking from kube::derive.
  3. Generally simplified the code to make it easier to maintain (Implement CEL validation proc macro for generated CRDs #1621 (comment))

Other things tried (TLDR)

Visitor trait:

It is possible to generate a new Visitor implementation per each validation rule. But the problem with this approach is that the generation happens for Validator derive on the structure, while the CustomResource derive is responsible for populating additional visitors for crd(). There is no one specific method which can collect all visitors under one chain, invoked from schemars. This likely requires every individual field in each struct to implement the Validated trait, involving creation of a shemars/serde type of logic.

Then the schemars Schema has no indicators for the source structure in the schema within Visitor, so there is no way (without generating schemars(title = “FooSpec”) as a metadata) to match the added visitor on the processed object param to make modifications. It is possible to add preserve_order feature to schemars and “search” for the property of the structure, as long as the source struct name is mapped to the Schema content.

With generating JsonSchema visitor extensions for more complex scenarios are possible to explore in the future.

Generating schemars attributes

While it is a viable option, such thing is not possible with derive macro, and has to use proc_macro instead, This approach is additionally hiding the updates of derive attributes under the hood, which feels unintuitive, as it performs updates to the macro markers, meant to generate code. Explored in #1621

@Danil-Grigorev Danil-Grigorev force-pushed the cel-expression-on-crds branch 2 times, most recently from 8a8cef3 to f286169 Compare November 24, 2024 17:58
Copy link

codecov bot commented Nov 24, 2024

Codecov Report

Attention: Patch coverage is 78.64078% with 22 lines in your changes missing coverage. Please review.

Project coverage is 75.9%. Comparing base (0424cb4) to head (060ad64).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kube-core/src/cel.rs 56.8% 16 Missing ⚠️
kube-derive/src/cel_schema.rs 92.6% 4 Missing ⚠️
kube-derive/src/lib.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1649     +/-   ##
=======================================
+ Coverage   75.8%   75.9%   +0.1%     
=======================================
  Files         82      84      +2     
  Lines       7513    7612     +99     
=======================================
+ Hits        5693    5771     +78     
- Misses      1820    1841     +21     
Files with missing lines Coverage Δ
kube-derive/src/custom_resource.rs 83.5% <100.0%> (+1.0%) ⬆️
kube-derive/tests/crd_schema_test.rs 96.9% <ø> (ø)
kube/src/lib.rs 88.5% <ø> (ø)
kube-derive/src/lib.rs 0.0% <0.0%> (ø)
kube-derive/src/cel_schema.rs 92.6% <92.6%> (ø)
kube-core/src/cel.rs 56.8% <56.8%> (ø)

@Danil-Grigorev Danil-Grigorev force-pushed the cel-expression-on-crds branch 8 times, most recently from 06ea3e9 to ee96ec4 Compare November 24, 2024 20:58
@Danil-Grigorev Danil-Grigorev changed the title [WIP] Implement cel validation proc macro for generated CRDs Implement cel validation proc macro for generated CRDs Nov 24, 2024
@Danil-Grigorev Danil-Grigorev changed the title Implement cel validation proc macro for generated CRDs Implement cel validation derive(Validated) macro for generated CRDs Nov 24, 2024
@Danil-Grigorev Danil-Grigorev changed the title Implement cel validation derive(Validated) macro for generated CRDs Implement cel validation derive(Validated) macro for generated CRDs Nov 24, 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.

some comments and questions. i think this is a pretty cool approach.


/// Reason is a machine-readable value providing more detail about why a field failed the validation.
///
/// More in [docs](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-reason)
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, I see these ones in the generated docs under https://github.com/kube-rs/k8s-pb/blob/ce4261fb52266f05cd7a06dbb8f4c0fcaa41c06a/k8s-pb/src/apiextensions_apiserver/pkg/apis/apiextensions/v1/mod.rs#L736 but because of bad go enum usage it's just a doc comment :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Would not pretend I saw it being generated, but yeah, missing enum is better to have :). Maybe worth adding From/Into conversion for ensuring compatibility.

kube-core/src/validation.rs Outdated Show resolved Hide resolved
kube-core/src/validation.rs Outdated Show resolved Hide resolved
kube/src/lib.rs Outdated Show resolved Hide resolved
kube-derive/src/lib.rs Outdated Show resolved Hide resolved
kube-core/src/validation.rs Outdated Show resolved Hide resolved
@Danil-Grigorev Danil-Grigorev changed the title Implement cel validation derive(Validated) macro for generated CRDs Implement cel validation derive(ValidateSchema) macro for generated CRDs Dec 1, 2024
@Danil-Grigorev Danil-Grigorev changed the title Implement cel validation derive(ValidateSchema) macro for generated CRDs Implement derive(ValidateSchema) macro for generating cel validation on CRDs Dec 1, 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.

I think this approach makes sense and is a nice way to opt into injecting validations. Have added some comments for code organisation (file is getting big) and for naming, but ultimately am happy with this!

examples/crd_derive_schema.rs Show resolved Hide resolved
kube-derive/src/custom_resource.rs Outdated Show resolved Hide resolved
examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
kube-derive/src/custom_resource.rs Outdated Show resolved Hide resolved
- Extend with supported values from docs
- https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
- Implement as Validated derive macro
- Use the raw Rule for the validated attribute

Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
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 Implement derive(ValidateSchema) macro for generating cel validation on CRDs Implement derive(CELSchema) macro for generating cel validation on CRDs Dec 18, 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.

Thanks a lot for this! It's a lot more readable and understandable now as it's factored out. Very minor set of comments, and a few questions for my own sanity. Only want one doc field added.

kube-derive/src/lib.rs Show resolved Hide resolved
kube-derive/src/cel_schema.rs Outdated Show resolved Hide resolved
kube-derive/src/cel_schema.rs Outdated Show resolved Hide resolved
kube-derive/src/cel_schema.rs Show resolved Hide resolved
kube-core/src/cel.rs Show resolved Hide resolved
@clux clux added the changelog-add changelog added category for prs label Dec 19, 2024
@clux clux added this to the 0.98.0 milestone Dec 19, 2024
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
@clux clux linked an issue Dec 22, 2024 that may be closed by this pull request
@clux clux added the cel common expression language related label Dec 22, 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.

Thanks a lot for the long exploration here. This is a very good checkpoint for CEL support. Closing the original issue with this PR and writing one follow-up. (Obviously,fFeel free to add follow-ups as you see fit as well.)

@clux clux merged commit b104472 into kube-rs:main Dec 22, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cel common expression language related changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better support for x-kubernetes properties in schemas
2 participants