Optional enums with comments emit invalid CRDs

[NickLarsenNZ]

Current and expected behavior

Current Behaviour

Using kube-3.0.0, adding a comment to an Optional Enum field like so:

pub struct ListenerClassSpec {
    /// With a doc-comment here, the CRD becomes invalid.
    pub service_external_traffic_policy: Option<KubernetesTrafficPolicy>,
}

/// Doc comment
#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, PartialEq, Eq, strum::Display)]
pub enum KubernetesTrafficPolicy {
    /// Doc comment
    Cluster,

    /// Doc comment
    Local,
}

...results in an invalid CRD (snippet):

              serviceExternalTrafficPolicy:
                anyOf:
                - description: Doc comment
                  enum:
                  - Cluster
                  - Local
                  type: string
                - enum:
                  - null
                  nullable: true
                description: With a doc-comment here, the CRD becomes invalid.

When applied to Kubernetes, it errors with the same thing from a previous issue:

❯ kubectl apply -f crates/stackable-operator/crds/ListenerClass.yaml
The CustomResourceDefinition "listenerclasses.listeners.stackable.tech" is invalid:
* spec.validation.openAPIV3Schema.properties[spec].properties[serviceExternalTrafficPolicy].anyOf[0].description: Forbidden: must be empty to be structural
* spec.validation.openAPIV3Schema.properties[spec].properties[serviceExternalTrafficPolicy].anyOf[0].type: Forbidden: must be empty to be structural
* spec.validation.openAPIV3Schema.properties[spec].properties[serviceExternalTrafficPolicy].anyOf[1].nullable: Forbidden: must be false to be structural
* spec.validation.openAPIV3Schema.properties[spec].properties[serviceExternalTrafficPolicy].type: Required value: must not be empty for specified object fields

Expected Behaviour

Adding a comment should still produce a valid CRD as it used to (pre: kube 1.0.0).

Possible solution

Use the transformer implementation from the original unmerged PR.

Additional context

No response

Environment

N/A

Configuration and features

k8s-openapi = { version = "0.27.0", default-features = false, features = ["schemars", "v1_35"] }
kube = { version = "3.0.0", default-features = false, features = ["client", "jsonpatch", "runtime", "derive", "admission", "rustls-tls", "ring"] }

YAML

No response

Affected crates

kube-core, kube-derive

Would you like to work on fixing this bug?

yes

[doxxx93]

I dug into this and found the issue.

Root Cause

In OptionalEnum transformer (kube-core/src/schema.rs:381):

let Some(obj) = schema.as_object_mut().filter(|o| o.len() == 1) else {
    return;
};

This only runs when the schema has exactly 1 key. But when a field has a doc comment, schemars adds description, so we get 2 keys (description + anyOf) and the transform gets skipped.

Fix

Just remove the o.len() == 1 check:

let Some(obj) = schema.as_object_mut() else {
    return;
};

The other conditions are already specific enough to only match Option patterns.

Tested locally - all existing tests pass, no regressions.

Happy to submit a PR if you'd like. It's just a 1-line fix + test case.

[NickLarsenNZ]

@doxxx93 there is already and PR, and it is the same change from my other unmerged PR that existed before the last PR fixing schemas.

[doxxx93]

Thanks for the context and your work on #1839

I've looked through both #1839 and #1853 to understand how we got here. Given the earlier feedback about keeping PRs focused, I've submitted #1908 as a minimal fix for now.

The module restructuring ideas from #1839 and #1907 still look valuable - happy to revisit that as a follow-up once the bug fix lands.

[NickLarsenNZ]

Thanks for the context and your work on #1839

Appreciated.

I've submitted #1908 as a minimal fix for now.

I also indicated on this issue (via the template) that I'd like to work on the issue.

I understand the desire to do absolute minimal fixes, that is how #1853 ended up getting merged, and here we are still fixing things that are already fixed in #1839.

I'd rather not be restricted to doing shortest-path-to-fix changes, which prevent refactoring. At least in this case where things have been broken and untested since kube ~1.0.0.

If the maintainers do want contributors to only do minimal changes and avoid refactors (eg: leaving those to the maintenance team), that is fine, but it needs to be said loud and clear otherwise we waste time contributing, and end up duplicating work like you had done.

The module restructuring ideas from #1839 and #1907 still look valuable - happy to revisit that as a follow-up once the bug fix lands.

I'm happy to drop those commits in this PR. schema.rs is getting rather big and it seems a good time to do it now instead of raising a second PR (separate PRs have an overhead) just to do that. Of course, if that is how the maintainers would like it done, I won't argue, but there is no clear guidance on that. Maybe something to add to CONTRIBUTING.md.

Could be worth a comment from @clux since he was across my other PR from which this one is derived.

[doxxx93]

You're right. I should've looked at #1907 more carefully before opening #1908. I'll close mine.

To be clear, I'm not saying "no refactoring allowed." Just that focusing on the fix makes it easier for others to review. But that's a preference, not a rule.

Sorry for the confusion, and thanks for your patience.

[NickLarsenNZ]

Thanks @doxxx93.

Will see what @clux comes back with, because perhaps he prefers your PR. We'll see.

[clux]

I'd rather not be restricted to doing shortest-path-to-fix changes, which prevent refactoring. At least in this case where things have been broken and untested since kube ~1.0.0.

If the maintainers do want contributors to only do minimal changes and avoid refactors (eg: leaving those to the maintenance team), that is fine, but it needs to be said loud and clear otherwise we waste time contributing, and end up duplicating work like you had done.

fair. will try to codify it in #1910

(i do think all maintainers on most projects want this. if you are proposing big changes to a file, while simultaneously moving things, it's much harder to review because github does not show what has moved so we have to keep two tabs open and cross reference manually.)

[clux]

after a bit to think (and a call with stackable) I think i also agree with @doxxx93 .

The simpler fix in #1908 is preferable to me as well. I am happy to merge that change first, and then (as discussed with stackable);

1. merge in tests from stackable (fix: kube-core::Schema hoisting logic #1839 or fix: Generate valid CRDs for fields with Optional enums and doc-comments #1907) that currently pass
2. raise bugs and fix any other issues that crop op (if there are tests we cannot merge yet from the original pr fix: kube-core::Schema hoisting logic #1839)
3. iteratively; goto 2

you happy with that @doxxx93 ?

[clux]

Merged #1908. Reopening this for now to leave some room for the tests to make it in.

Will wait for a test PR and some feedback there, but am otherwise appy to do a patch release whenever 😄 @NickLarsenNZ

[NickLarsenNZ]

Thanks @clux.

I have raised a PR with additional passing tests: #1921 (which came from my original PR).

The remaining tests (not included in this new PR) fail, so I will raise an issue describing why (and this one can be closed).