kube-rs / kube

Rust Kubernetes client and controller runtime
https://kube.rs
Apache License 2.0
2.79k stars 291 forks source link

`kubectl explain` can't explain fields in CRDs that contain `Option<T>`'s #1078

Open Dav1dde opened 1 year ago

Dav1dde commented 1 year ago

Current and expected behavior

The following definition:

    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub namespace_resource_blacklist: Option<Vec<k8s_argocd::AppProjectNamespaceResourceBlacklist>>,
    #[serde(default, skip_serializing_if = "Vec::is_empty")]
    pub namespace_resource_blacklist_additional:
        Vec<k8s_argocd::AppProjectNamespaceResourceBlacklist>,

Generates the CRD:

                namespaceResourceWhitelist:
                  items:
                    properties:
                      group:
                        type: string
                      kind:
                        type: string
                    required:
                      - group
                      - kind
                    type: object
                  nullable: true
                  type: array
                namespaceResourceWhitelistAdditional:
                  items:
                    properties:
                      group:
                        type: string
                      kind:
                        type: string
                    required:
                      - group
                      - kind
                    type: object
                  type: array

(Note: the only difference is the nullable: true field)

Using kubectl explain we get this (descriptions etc. omitted):

   namespaceResourceWhitelist    <>
   namespaceResourceWhitelistAdditional    <[]Object>

Further inspecting the fields.

Without Option:

└─ % k explain TenantApplication.spec.namespaceResourceWhitelistAdditional
KIND:     TenantApplication
VERSION:  yggdrasil/v1alpha1

RESOURCE: namespaceResourceWhitelistAdditional <[]Object>

DESCRIPTION:
<...snip>

FIELDS:
   group    <string> -required-

   kind    <string> -required-

With Option:

└─ % k explain TenantApplication.spec.namespaceResourceWhitelist
KIND:     TenantApplication
VERSION:  yggdrasil/v1alpha1

DESCRIPTION:
<...snip>

The type information is completely lost.

Possible solution

Omitting the nullable: true field from the Schema makes it explorable via kubectl explain.

This is obviously a major change potentially affecting every CRD generated with kube.

Additional context

The problem seems to be described here:

If nullable: true is set, we drop type, nullable, items and properties because OpenAPI v2 is not able to express nullable. To avoid kubectl to reject good objects, this is necessary.

The nullable behaviour described here, seems to indicate that this should be compatible with serde if we just never emit the nullable field.

Environment

└─ % k version --short
Client Version: v1.25.3
Kustomize Version: v4.5.7
Server Version: v1.20.15

Configuration and features

k8s-openapi = { version = "0.16", features = ["v1_25"] }
kube = "0.75"
# kube = { path = "../../../workspaces/rust/kube/kube" }

Affected crates

kube-derive

Would you like to work on fixing this bug?

yes

Dav1dde commented 1 year ago

Currently testing this workaround in my setup:

// Removes all `nullable: true` occurences from the CRD.
//
// See https://github.com/kube-rs/kube/issues/1078
fn remove_nullable(crd: &str) -> String {
    lazy_static::lazy_static! {
        static ref RE: Regex = Regex::new(r"\s*nullable: true").unwrap();
    }

    RE.replace_all(crd, "").into_owned()
}

fn print_crd<T: CustomResourceExt>() -> eyre::Result<()> {
    let crd = serde_yaml::to_string(&T::crd())?;
    let crd = remove_nullable(&crd);
    print!("{crd}");
    Ok(())
}
clux commented 1 year ago

Work to fix this was reverted in 0.82.1 due to unintended consequences. Users that want to fix this should opt-in to non_nullable behaviour (and we can probably offer a kube-derive attr for it like #[kube(non_nullable)]).

On the other hand, it is also possible that upstream (i.e. kubectl) should handle legit schemas Kubernetes actually support better.