Use `k8s_openapi::apimachinery::pkg::apis::meta::v1::Condition` instead of generating a new API

[aryan9600]

At the moment, kopium parses the CRD yaml and then generates Rust structs accordingly. While this works well for almost everything, it has a drawback related to the Condition API. The Condition API is a standard API which is consistent across all CRDs and has a definition in upstream Kubernetes. Hence, it would be great if instead of redefining the API for each CRD used in a project, we could detect the presence of .status.conditions in the CRD and use the Condition API from the k8s-openapi crate. This would enable controller devs to use a standard Condition type across their entire project rather than having to use different generated Condition types specific to a particular CRD.

[clux]

Yeah, that makes sense to me. Not sure how you best to do it in kopium atm - everything is kind of dynamically named and recursively analysed one step-at-a-time.

you could maybe do a pre-scan (before analysis) for the properties.status.properties.condition inside openAPIV3Schema and create some kind of deny-list for the analyzer so it can short-circuit (if you just do it without prior knowledge i suspect you'll struggle to correlate where you are).

[aryan9600]

Would it be acceptable to modify the analyzer itself? We could create a deny list which would prevent the generation of a new Condition API but we also need to modify the conditions field in the Status object to be of type k8s_openapi::apimachinery::pkg::apis::meta::v1::Condition. I took a quick scan of the code and what I could think of was:

• In analyze_(), check if the object's properties matches the Conditon API and exit early if it does.
• In extract_container(), inside the "array" match arm check if the key is "conditions" and if the value's items contains the Condition API fields. If they do, then set rust_type to k8s_openapi::apimachinery::pkg::apis::meta::v1::Condition.

Keen to hear your thoughts on this!

[clux]

yeah, something like that sounds reasonable. i would probably amend it slightly with:

• an extra function to call before extract_container to avoid stepping into extract_container to contain the special case (something like fn find_status_conditions(props) -> Option<Container> and when this gives you a Some, avoid extract_container from analyze_)
• maybe an extra sanity to only do the bypass when level == 1 (which i think it will be at after recursing down into .status)

[aryan9600]

an extra function to call before extract_container to avoid stepping into extract_container

sounds good!

maybe an extra sanity to only do the bypass when level == 1

this does sound reasonable, but it'd be better if we could skip this check, as it could be present anywhere in an object. for eg, in the Gateway API project, the Gateway CRD has two paths .status.conditions and .status.listeners[].conditions, both of metav1.Condition type (ref: https://github.com/kubernetes-sigs/gateway-api/blob/346e951245f290d281e29901c266d1b1df8e292f/apis/v1/gateway_types.go#L627, https://github.com/kubernetes-sigs/gateway-api/blob/346e951245f290d281e29901c266d1b1df8e292f/apis/v1/gateway_types.go#L872)

[clux]

but it'd be better if we could skip this check

that sounds reasonable. i am just worried about false-positives (particularly if we don't allow opting out to this type of detection), but I guess the answer might just be to be a little more careful in the detection function (check for properties on condition to actually make sure it's not just a struct with the same name).

[aryan9600]

yeah, i think we can provide a flag to enable/disable this behavior. the awkward thing would be having to pass the value down to analyze(), but i think that's something thats unavoidable.

[aryan9600]

upon writing some code, i think it'd be better if we introduced a new type and made all the functions in analyzer.rs methods of the new type:

struct Analyzer{
  use_k8s_openapi_conditions: bool
}

impl Analyzer{
  pub fn analyze()

  fn analyze_()

  fn extract_container()
  ...
}

imo, this is better than passing the boolean to analyze() as a parameter because:

• it seems out of place and semantically awkward.
• since everything's recursive, all functions will need to be modified accept this parameter.

it also provides a foundation for any similar configuration needs that might arise in the future.

[clux]

you could do something like that yeah, i also don't like passing a params struct around by ref everywhere here. just be aware it will require some shifting of the setup parts of unit tests to do that. probably easier if you lean on a Default derive somewhere.

you could also maybe pass a analyze::Config to analyze and that can create the Analyzer (which then does thing within it's impl internally)