-
-
Notifications
You must be signed in to change notification settings - Fork 374
Implement derive(CELSchema) macro for generating cel validation on CRDs
#1649
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
Conversation
8a8cef3 to
f286169
Compare
Codecov ReportAttention: Patch coverage is
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
|
06ea3e9 to
ee96ec4
Compare
derive(Validated) macro for generated CRDs
clux
left a comment
There was a problem hiding this 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.
kube-core/src/validation.rs
Outdated
|
|
||
| /// 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) |
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
derive(Validated) macro for generated CRDsderive(ValidateSchema) macro for generated CRDs
derive(ValidateSchema) macro for generated CRDsderive(ValidateSchema) macro for generating cel validation on CRDs
clux
left a comment
There was a problem hiding this 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!
- 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]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
Signed-off-by: Danil-Grigorev <[email protected]>
5026bdc to
97a9131
Compare
Signed-off-by: Danil-Grigorev <[email protected]>
derive(ValidateSchema) macro for generating cel validation on CRDsderive(CELSchema) macro for generating cel validation on CRDs
clux
left a comment
There was a problem hiding this 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.
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]>
0f9082d to
060ad64
Compare
clux
left a comment
There was a problem hiding this 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.)
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
kubeowns CRD generation code, the idea is to simplify this process for added validation rules and achieve more declarative approach, similar to thekubebuilderlibrary. This approach will be compatible withkopiumgeneration 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.
This PR is a followup on #1621 which addresses some of the concerns.
JsonSchema, which allows further additions to the schema later,structlevel validation rules and is not affected byschemarsversion.kube::coreand invoking fromkube::derive.Other things tried (TLDR)
Visitor trait:
It is possible to generate a newVisitorimplementation per each validation rule. But the problem with this approach is that the generation happens forValidatorderive on the structure, while theCustomResourcederive is responsible for populating additional visitors forcrd(). There is no one specific method which can collect all visitors under one chain, invoked fromschemars. This likely requires every individual field in each struct to implement theValidatedtrait, involving creation of ashemars/serdetype of logic.Then theschemars Schemahas no indicators for the source structure in the schema within Visitor, so there is no way (without generatingschemars(title = “FooSpec”)as a metadata) to match the added visitor on the processed object param to make modifications. It is possible to addpreserve_orderfeature to schemars and “search” for the property of the structure, as long as the source struct name is mapped to theSchemacontent.With generating
JsonSchemavisitor 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
derivemacro, and has to useproc_macroinstead, 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