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

Better support for x-kubernetes properties in schemas #1367

Open
clux opened this issue Dec 7, 2023 · 6 comments
Open

Better support for x-kubernetes properties in schemas #1367

clux opened this issue Dec 7, 2023 · 6 comments
Labels
derive kube-derive proc_macro related discussions possibly more of a discussion piece than an issue question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Dec 7, 2023

There is a slew of x-kubernetes extension properties that is usable inside CRD schemas to improve the validation and merge behaviour of structures of members.

From Kubernetes 1.25 this is being leaned into even more heavily with x-kubernetes-validations rules on CRD schemas:

  openAPIV3Schema:
    type: object
    properties:
      spec:
        type: object
        x-kubernetes-validations:
          - rule: "self.minReplicas <= self.replicas"
            message: "replicas should be greater than or equal to minReplicas."
          - rule: "self.replicas <= self.maxReplicas"
            message: "replicas should be smaller than or equal to maxReplicas."

This type of validation makes it possible to write controllers with much richer type of native validation without having to lean on the clunkier admission controllers / admission controller frameworks to manage extraneous policy rules - and has a lot of potential use. There's a KubeConNA'23 talk called Declarative Everything that explores this and the direction of the feature from sig apimachinery.

Current Usage

Such properties can be specified via schemars attributes to override the schema generation for a type as per

// Listable field with specified 'set' merge strategy
#[serde(default)]
#[schemars(schema_with = "set_listable_schema")]
set_listable: Vec<u32>,
}
// https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy
fn set_listable_schema(_: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema {
serde_json::from_value(serde_json::json!({
"type": "array",
"items": {
"format": "u32",
"minium": 0,
"type": "integer"
},
"x-kubernetes-list-type": "set"
}))
.unwrap()
}

This is not ideal because you then have to manage all the other schema properties for that type yourself.

Ideas

Schemars Extensions

To properly support adding properties like this, it would be interesting to see if schemas which has support for a large amount of attributes already, could be enhanced with a kubernetes attribute feature set to allow stuff like e.g.:

struct Spec {
    #[schemars(x-kubernetes-validations(
        rule = "self.minReplicas <= self.replicas",
        message = "replicas should be greater than or equal to minReplicas."
    ))]
    spec: usize,
}

Have asked upstream about what they think about this in GREsau/schemars#258

CEL Crate?

Maybe it's better to start some of this from a CEL POV because maybe we want to be able to run validations we put into CEL rules locally. In this case a crate that allows us to do something like

struct Spec {
    #[cel_validation(
        rule = "self.minReplicas <= self.replicas",
        message = "replicas should be greater than or equal to minReplicas."
    )]
    spec: usize,
}

in either case we probably still need something in schemars so that the rules are forwarded into the schema, but it means we will have our own properties (like serde/validator) that schemars can pickup on later.

And we likely would need a new crate or majorly extend the cel-parser / cel-interpreter crate (both of which live in cel-rust).

@clux clux added discussions possibly more of a discussion piece than an issue derive kube-derive proc_macro related question Direction unclear; possibly a bug, possibly could be improved. labels Dec 7, 2023
clux added a commit that referenced this issue Dec 14, 2023
for #1367 and kube-rs/website#53

Signed-off-by: clux <sszynrae@gmail.com>
clux added a commit that referenced this issue Dec 15, 2023
* Naive CEL validation example

for #1367 and kube-rs/website#53

Signed-off-by: clux <sszynrae@gmail.com>

* fmt

Signed-off-by: clux <sszynrae@gmail.com>

---------

Signed-off-by: clux <sszynrae@gmail.com>
@benkeil
Copy link

benkeil commented Feb 14, 2024

This would be a great feature. Looking for how to enforce immutability

@azat
Copy link

azat commented Jul 30, 2024

FWIW here is a better example, that allows to preserve existing schema - https://github.com/s3rius/kuo/blob/4c83b4033093e84035cefb0f5a22c46c7b9d471c/src/crds/managed_user.rs#L70-L84

@clux
Copy link
Member Author

clux commented Jul 30, 2024

That example is really useful @azat, thank you.

We could maybe create a higher level version of that that allows generating schema modifying functions like that, if we can curry the "rule" and "message" as initial arguments, and return the mutating fn from this.

@Danil-Grigorev
Copy link
Member

Danil-Grigorev commented Nov 24, 2024

I was looking into this recently, but I think right now I need help to decide if any of these approaches are correct

  1. Simplest validation which generates a method defined by method, using raw Rule structures exposed via kube::core.
#[derive(Validated, JsonSchema)]
struct FooSpec {
    #[validated(
        method = cel_validated,
        rule = Rule{rule: "self != 'illegal'".into(), message: Some(Message::Expression("'string cannot be illegal'".into())), reason: Some(Reason::FieldValueForbidden), ..Default::default()},
        rule = Rule{rule: "self != 'not legal'".into(), reason: Some(Reason::FieldValueInvalid), ..Default::default()}
    )]
    #[schemars(schema_with = "cel_validated")]
    field:
  1. A more macro-like implementation for Rule composition:
#[derive(Validated, JsonSchema)]
struct FooSpec {
    #[cel_validation(
        rule = "self.minReplicas <= self.replicas",
        message = "replicas should be greater than or equal to minReplicas.".into() // Is actually an enum Message
        reason = FieldValueInvalid // a Reason::FieldValueInvalid
    )]
    #[cel_validation(rule = "self.minReplicas <= self.replicas")] // Multiple
    #[schemars(schema_with = "FooSpecValidation::cel_validated")]
    field:
  1. Any of the above, but a structure is generated in a separate mod with an added JsonSchema derive, and serve as a template for manual implementation of JsonSchema for the #[derive(Validated)] struct, which is also generated.
#[derive(Validated)] // No jsonSchema - Validated implements it
struct FooSpec {
    #[validated(
        rule = Rule{rule: "self != 'illegal'".into(), message: Some(Message::Expression("'string cannot be illegal'".into())), reason: Some(Reason::FieldValueForbidden), ..Default::default()},
        rule = Rule{rule: "self != 'not legal'".into(), message: Some(“a message”.into()), reason: Some(Reason::FieldValueInvalid), ..Default::default()}
    )]
    // No need for schemars attribute - added automatically
    field:

Personally I like 3 most, but I noticed some issues with the lint, like complains to dead_code as it treats the generated structure and the user definition as the same. Disabling lint disables it for both. On the upside - this allows to add any modifications on top of the generated structure schema, leaving a room for growth (meanwhile the schemars 1.0 is still alpha :/ )

Other things tried:

  • Shemars Visitor trait. Problem: no way to generate additions of a Visitor structure to the processing list .with_visitor(…) at compile time.
  • Updating schemars to alpha and using extend or transform. Problem: clash with generated code for older version, no way to gauge the version based on user crate to allow opt-out of alpha v1.0.

@clux
Copy link
Member Author

clux commented Nov 27, 2024

i do like the idea to inject it automatically (3) the most. i put some questions on some specifics in the pr. dead_code stuff is commonly explicitly allowed in generated code for proc macros so that shouldn't be the biggest deal.

Updating schemars to alpha and using extend or transform. Problem: clash with generated code for older version, no way to gauge the version based on user crate to allow opt-out of alpha v1.0.

yeah, i'd probably want to wait until there's a new release here before forcing out anything new. what new does schemars alpha extend/transform you reference add to the table?

@Danil-Grigorev
Copy link
Member

Danil-Grigorev commented Nov 29, 2024

Once 1.0 is released, generation of the method will no longer be required, as #schemars(extend(“x-kubernetes-validations” = Rule{…})) will be available. People might still want to use #schemars(transform) ~= previously #schemars(schema_with) with generated methods, for more advanced schema rewrites, since existing schema object will be available in transform (unlike in schema_with).

However there are a couple of downfalls with just purely sticking with upstream schemars(extend):

  1. Not clear release date
  2. Multiple rules will be cramped under one key, x-kubernetes-validations is easy to misspell, while validate provides clear abstraction on top with rule separation, and type system integration, as the methods expect a Rule object. For a raw extend there will not be possible to use any From/Into methods of the Rule.
#[schemars(extend(“x-kubernetes-validations”: vec![Rule{}, Rule{}]))]
  1. There will be breaking changes, all existing schema_with implementations will have to be rewritten to transform. With the macro in-place we can soften the fall (sort of) by providing declarative syntax for the intended behavior, which will not be changed from the cel_validate macro POV.

Once the 1.0 is out, it will be easy enough to change the behavior of #[cel_validate] to optionally generate method, but use extend by default, while adding typing integration on top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
derive kube-derive proc_macro related discussions possibly more of a discussion piece than an issue question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

No branches or pull requests

4 participants