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

Generate schema for CRD v1 #348

Merged
merged 3 commits into from
Dec 22, 2020
Merged

Generate schema for CRD v1 #348

merged 3 commits into from
Dec 22, 2020

Conversation

kazk
Copy link
Member

@kazk kazk commented Dec 18, 2020

Schema is generated with schemars.

Also fixed the location of subresources and additionalPrinterColumns.

Closes #264


For deriving CRD v1, the spec struct must include schemars::JsonSchema. The schemas for subresources are currently not generated, but it should be possible to add the status subresource by requiring the status struct to have schemars::JsonSchema and extracting the generated schema similar to the spec. If there is a status subresource, its struct must include JsonSchema as well.

I'm not sure how to handle the new schemars dependency.

@kazk kazk force-pushed the fix-crd-v1-schema branch 2 times, most recently from fa706cb to 2aeee34 Compare December 19, 2020 00:33
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

IIRC Kubernetes requires its own weird dialect of JSON Schemas. In particular, it has some weird requirements around tagged unions (which we'd generally use rich enums for in Rust).

Can schemars generate schemas that are compliant with that? If so, what special options are required for that?

kube-derive/src/custom_resource.rs Show resolved Hide resolved
kube-derive/src/custom_resource.rs Outdated Show resolved Hide resolved
@kazk
Copy link
Member Author

kazk commented Dec 19, 2020

@teozkr Thanks for reviewing. I just pushed a fix to support nested struct/enums and that also includes the status subresource automatically if present.

IIRC Kubernetes requires its own weird dialect of JSON Schemas. In particular, it has some weird requirements around tagged unions (which we'd generally use rich enums for in Rust).

Yes, they use Open API v3 (dialect of JSON Schemas) with additional restrictions.

I wasn't aware of the requirements around tagged unions. I'll try adding some.

Can schemars generate schemas that are compliant with that? If so, what special options are required for that?

schemars support Open API v3 through a setting and this uses that. Also, Kubernetes disallows definitions so I'm using an option to inline all the schemas. Usually schemars defines schema to reference under definitions for each type. The original implementation was extracting the schema of the spec struct with schema.definitions.get("FooSpec").unwrap(), but I found the option to inline all.

@kazk
Copy link
Member Author

kazk commented Dec 19, 2020

2. for each field in an object and each item in an array which is specified within any of allOf, anyOf, oneOf or not, the schema also specifies the field/item outside of those logical junctors (compare example 1 and 2).

BAD

allOf:
- properties:
    foo:
      ...

GOOD

properties:
  foo:
    ...
allOf:
- properties:
    foo:
      ...

😕


For the following spec (attributes omitted for brevity):

pub struct FooSpec {
    value: FooEnum,
}
#[serde(untagged)]
pub enum FooEnum {
    Foo { foo: String },
    Bar { bar: String },
}

The generated schema currently looks like the following:

openAPIV3Schema:
  type: object
  properties:
    spec:
      properties:
        value:
          anyOf:
            - properties:
                foo:
                  type: string
              required:
                - foo
              type: object
            - properties:
                bar:
                  type: string
              required:
                - bar
              type: object
      required:
        - value
      type: object
  required:
    - spec

Can schemars generate schemas that are compliant with that? If so, what special options are required for that?

I'm not sure what's valid in this case, but I think we can transform the schema by implementing a Visitor and using with_visitor:

let gen = schemars::gen::SchemaSettings::openapi3()
    .with(|s| {
        s.inline_subschemas = true;
    })
    .with_visitor(KubeVisitor::new())
    .into_generator();

@kazk
Copy link
Member Author

kazk commented Dec 19, 2020

@clux
Copy link
Member

clux commented Dec 19, 2020

This is huge @kazk . Thank you so much for doing this. 🥳

Using schemars here is a very sensible solution that avoids homebrewing this too much, and the library has sensible serde re-use policies for attributes. As a tiny negative it does look like this prevents us from doing validation constraints such as minimum and maximum, but starting to think it's maybe more transparent for rust to control the validation fully where possible.

There are a couple of things I would like to have tests for and clarify before we release this though:

  • default and default_with serde attrs should correspond with crd defaulting rules
  • if Optional fields are nullable, do they require explicit serialization of the null value for k8s to null them out? If so, we ought to at least document that it's a bit scary to put #[serde(skip_serializing_if = "Option::is_none")] on them.
  • if a user puts a timestamp in a struct (timestamp: DateTime<Utc>,) requiring chrono with serde feature, how would this interact with kube-derive (which pulls in schemars without the chrono feature), maybe we should just pull in chrono feature always (it's always in k8s-openapi's dependency tree anyway)

Of course; I am happy to take this into master without further changes right now. This is a massive improvement, so thank you again. But if you are able to help a little bit with the general user experience, that would be super helpful :-)

@kazk
Copy link
Member Author

kazk commented Dec 19, 2020

@clux Thanks for reviewing. Yeah, this definitely need more polishing. Also, we need to ensure generating a structural schema for more complex specs (should be a separate PR). But api.create(&PostParams::default(), &Foo::crd()).await? for simple FooSpec should work now and I'm happy to be able to give something back :)

As a tiny negative it does look like this prevents us from doing validation constraints such as minimum and maximum

There's an issue open and the author wrote:

Alternatively, I could just add more options to #[schemars(...)] attributes to allow setting more properties on the generated schema

GREsau/schemars#12 (comment)

So maybe it can be supported relatively easily.


I'll look into the points you brought up later. It's getting late here.

examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
@kazk
Copy link
Member Author

kazk commented Dec 21, 2020

I did some research on generating more complex schemas and it turns out controller-gen (used by kubebuilder) doesn't support it either yet.

So, it might not be limiting as I thought. (But I also think that Rust code is more likely to need this.)


I'll finish the examples/tests either tonight or tomorrow.

examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
examples/crd_derive_schema.rs Outdated Show resolved Hide resolved
@kazk kazk force-pushed the fix-crd-v1-schema branch 2 times, most recently from 188b8b0 to 01e3a3c Compare December 21, 2020 10:33
@kazk kazk requested a review from clux December 21, 2020 10:34
Schema is generated with `schemars`.

`inline_subschemas` option is used to avoid definitions in schema.
`meta_schema` is set to `None` to prevent including `$schema` which is
not allowed by Kubernetes.

This support nested structs and enums, but does not support structural
schemas for some of the more complex spec types.
The schema of the status subresource is included if present.

Also fixed the location of `subresources` and `additionalPrinterColumns`.

Closes kube-rs#264
@kazk
Copy link
Member Author

kazk commented Dec 21, 2020

@clux I cleaned up the commits and it should be ready for merge. Please let me know if I missed anything or if there's something you'd like to be fixed.

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.

This is absolutely awesome. Tests seem to cover things well, I'll add them to the circleci config so that it sticks around.

Left a small request for chrono feature, I think it will "just work", but if it's in any way complicated, just say so, and I'll merge this as is!

examples/crd_derive_schema.rs Show resolved Hide resolved
examples/crd_derive_schema.rs Show resolved Hide resolved
examples/crd_derive_schema.rs Show resolved Hide resolved
examples/crd_derive_schema.rs Show resolved Hide resolved
@clux
Copy link
Member

clux commented Dec 21, 2020

Looking good. Ready to merge? :-)

@kazk
Copy link
Member Author

kazk commented Dec 22, 2020

Yeah :)

@clux clux merged commit 474e07b into kube-rs:master Dec 22, 2020
@kazk kazk deleted the fix-crd-v1-schema branch December 22, 2020 00:03
@clux
Copy link
Member

clux commented Dec 22, 2020

Thanks again for all of this. I'm planning to get a release out on Wednesday with some release notes for this. I'll post when it's done.

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