kube-derive: Disable option_nullable for CRD generation

[Dav1dde]

Fixes: #1078

Check the related issue for discussion.

See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting-and-nullable for what this option does.

[clux]

I am curious to see if this affects options when running patches. Have you tried experimenting with the crd_derive_schema example? It has a bunch of nullable tests (and i don't think it actually runs on CI atm, but probably should).

[Dav1dde]

I am curious to see if this affects options when running patches. Have you tried experimenting with the crd_derive_schema example? It has a bunch of nullable tests (and i don't think it actually runs on CI atm, but probably should).

I have yet to look into the (failing) tests and examples, I mainly opened this for initial discussion, I'll look into it when I get a bit more time.

But this change fixed my schema and by the looks of it didn't break anything.

[Dav1dde]

@clux I forgot about this PR, rebased it. Looks like tests are passing, windows is failing due to missing OpenSSL and coverage failed but all tests passed?

I've been running this change in 2 controllers for a while now and didn't have any issues.

[clux]

Reading up on this, yes, it seems like it's the right thing to set it to false. Rust convention is generally to treat null the same as None which is almost always deserialized from both null and missing.

[codecov]

Codecov Report

Merging #1079 (044392f) into main (36dcdaa) will increase coverage by 0.04%.
The diff coverage is 93.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
+ Coverage   73.47%   73.52%   +0.04%     
==========================================
  Files          68       68              
  Lines        5354     5364      +10     
==========================================
+ Hits         3934     3944      +10     
  Misses       1420     1420              

Impacted Files	Coverage Δ
kube-derive/src/custom_resource.rs	13.83% <ø> (ø)
kube-derive/tests/crd_schema_test.rs	97.50% <93.33%> (-2.50%)	⬇️

... and 1 file with indirect coverage changes

[clux]

One other minor point; since by default we are taking out nullable: true everywhere, if for some reason people want to have it included, we could showcase how to do that in that in the referenced crd_schema_test.

[Dav1dde]

One other minor point; since by default we are taking out nullable: true everywhere, if for some reason people want to have it included, we could showcase how to do that in that in the referenced crd_schema_test.

I've added a struct Nullable<T>(T) example, is that what you were thinking of?

[clux]

I've added a struct Nullable<T>(T) example, is that what you were thinking of?

yeah, that looks great. Completeness of options in tests so at least people can figure out how to do it, even though we think it's not a common use case.

[Dav1dde]

That's what I get for adding last second docs, because what can go wrong with docs...

json atom at path ".spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.nullable.description" is missing from rhs

json atom at path ".spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.timestamp.description" is missing from rhs

[clux]

Ran this through some more controllers at $dayjob and noticing that this PR can cause actually issues when you do a server side apply patch on a crd struct with naked options :(

Example:

api.patch_status(&name, &serverside, &status_struct)

where status struct had:

struct MyStatus {
  pub last_success: Option<Attempt>
}

with an unset last_succes in the patch. this caused a null on the wire which was rejected by the apiserver because of the null (after nullable got removed):

    0: ApiError: Notification.SOMECRD "X" is invalid: status.SOMECRD.last_success: Invalid value: "null": status.notifications.x.last_success in body must be of type object: "null": Invalid (ErrorResponse { status: "Failure", message: "Notification.SOMECRD \"X\" is invalid: status.notifications.x.last_success: Invalid value: \"null\": status.notifications.x.last_success in body must be of type object: \"null\"", reason: "Invalid", code: 422 })
    1: Notification.SOMECRD "x" is invalid: status.notifications.x.last_success: Invalid value: "null": 

this can be fixed by avoiding serializing explicit nulls:

struct MyStatus {
+  #[serde(skip_serializing_if = "Option::is_none")]
    pub last_success: Option<Attempt>
}

which is more inline with how most kubernetes structs work, but unfortunately a little more breaking than intended.

[Dav1dde]

Mh, that's not good.

Not sure what we can do, maybe can hack something togethere where option becomes anyOf { type, null }, if Kubernetes even allows that.

I think marking it not as nullable is still the correct thing, but it clashing with serde's default behaviour is quite unfortunate. Shall I open a revert PR?

[clux]

Not sure. Many Kubernetes resources already implement that skip behaviour (e.g. k8s-openapi#PodSpec), or kopium generated, so it's mostly down to individual CRDs in the wild.

We could just focus on telling people to use skip_serializing_if = "Option::if_none" in general on option types because that is generally the most compatible thing with server side apply, and more in-line with how Kubernetes envisions apply configurations (everything optional types) should work with server-side apply.