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

Adapt CRD Generator v2 to new approach to KubernetesResource #6335

Open
manusa opened this issue Sep 5, 2024 · 3 comments
Open

Adapt CRD Generator v2 to new approach to KubernetesResource #6335

manusa opened this issue Sep 5, 2024 · 3 comments
Labels
component/crd-generator Related to the CRD generator

Comments

@manusa
Copy link
Member

manusa commented Sep 5, 2024

Description

Part of #6616
Part of #6130

Originally posted by @shawkins in #6333 (comment)

Just wanted to talk broadly on the usage of AnyType, RawExtension, GenericKuberenetesResource, and what is being done here with object and the KubernetesDeserializer.

  • AnyType - can be a replacement for where we had previously been using JsonNode and Map types, but is not currently directly used in our models. Also serves as a base type for RawExtension and IntOrString. Is not directly needed by the KubernetesDeserializer.
  • RawExtension - provides a way to bridge between KubernetesResource and any content. This is due to the previous mapping of kube's raw extension type to KubernetesResource. Generally not expected to appear directly in user object models. Used directly by the KubernetesDeserializer when a KubernetesResource is not an object or has no api/version / kind information. The kube docs imply that raw extension only allows for complete kube resources, but the schema is not actually restricted to that.
  • GenericKubernetesResource - generally provides a way to represent kube resource for which there is no class definition. Like RawExtension, but restricts to HasMetadata and not deserialize to the real type - only generic.

v2 CRD Mappings:

type mapping
AnyType untyped property that preserves unknown
RawExtension maps to raw
KubernetesResource maps to object - this is probably not correct, if the field is exactly > KubernetesResource it should map to raw instead
GenericKubernetesResource / HasMetadata maps to raw

It's clear that these mappings are not bi-directional (I haven't check if the logic knows about AnyType), so round-tripping can result in a different object model.

With this pr we're mapping kube's raw extension to Object directly with the KubernetesDeserializer. If we're good with this approach we can consider deprecating the RawExtension class and recommending instead that users with custom resources take the same approach. The one thing that will look bad about this is KubernetesDeserializer is in an internal package. We'll also need to update the CRD generator to understand this convention. Also if user for whatever reason a user puts an untyped value in the Object field, it will be deserialized as a RawExtension - which doesn't work well if we want to put that on the path to deprecation.

We should also update the CRD generation logic to understand this convention, and consider making a similar change to the java generator.

AnyType could similarly be deprecated if we consider its usage synonomous with Object without the KuberenetsDeserializer - currently that will just map to an object property without preserve uknown.

Tasks

  • Move KubernetesDeserializer (and derivatives) out of the internal package
  • ...
@manusa manusa added the component/crd-generator Related to the CRD generator label Sep 5, 2024
@manusa
Copy link
Member Author

manusa commented Sep 11, 2024

Hi @shawkins,

Let's continue the discussion about sundrio/sundrio#482 (comment) here.

Do you think we should revert to using private KubernetesResource for untyped/raw fields.

I don't dislike using Object but maybe it has downsides to it (plus the additional work).

Thoughts? shall we revert and annotate KuberntesResource? shall we continue with the new approach?

@shawkins
Copy link
Contributor

Thoughts? shall we revert and annotate KuberntesResource? shall we continue with the new approach?

I'm open to using Object given that we're talking about the 7.0 release. It will be a breaking change for some - those who were able to have appropriate descendent methods, or who are expecting a KuberentesResource return type. Those aren't too hard to correct.

My main concerns are about completeness and handling the evolution away from KubernetesResource - so we don't end up with duplicate conventions for very long.

The only thing we lose, other than some of the fluent methods, is a bit of the principled nature of KubernetesResource - I think KubernetesResource was understood to be a little wider than HasMetadata and required some special knowledge to use / expect RawExtension. When something is typed as Object that won't be as clear - you'd need to see the deserializer and/or have some propogation of the javadocs, which is currently missing for the builder / fluents.

Let's see if we can round out the remaining changes / migration path and discuss it at the next community meeting.

@manusa
Copy link
Member Author

manusa commented Sep 12, 2024

I see your point.

I'd say that from a client-go/kubernetes-api/openshift-api parity standing point Object is probably better suited because it fits most of the model types I've seen (that are then translated as unreferenced object types or empty property nested objects in the OpenAPI/Swagger spec).

From a user experience (UX) perspective, it might be much better to keep using KubernetesResource as it sets clear expectations (just like you said).
As I see it, being able to use the IgnoreDescendants annotation I think we should continue with the former approach (preserve RawExtension et al)

Let's see if we can round out the remaining changes / migration path and discuss it at the next community meeting.

Agreed. Reverting to the former approach is just a matter of changing a few lines and re-running the model generation.
We have time to discuss and properly analyze this before committing to one of the described approaches.

Other links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/crd-generator Related to the CRD generator
Projects
None yet
Development

No branches or pull requests

2 participants