using kubernetesresource as the raw type

[shawkins]

Description

This is an alternative to #4409 that uses KubernetesResource as the raw type, rather than object.

There are a couple of nuances.

• for a little better backwards compatibility for models using Map<String, Object> it would be nice if RawExtension implemented Map<String, Object> - however that causes a problem with sundrio and results in build errors. So the usage would need to be RawExtension.fromMap(...) https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:kubernetes-client:raw_kuberesource?expand=1#diff-97d49d0a6ea3655ffb6a17863e03105b6a4822eade197fcf217ba5594d33d38dR73 - or new RawExtension(map), which would also be useful if RawExtension is added to the array of buildable resources.

• There are also sundrio build errors if all generated types remain KubernetesResources, so these changes see if the resource has a kind field to be considered a KubernetesResources. If it does not, then we'll just use serializable instead. That does not affect deserialization as those resources can't be deserialized by the KubernetesDeserializer. However this change touches quite a few generated classes and could have other considerations around the usage of the KuberentesResource interface.

To minimize the changes / breakage I'll back out the changes to migrate from the Map<String, Object> to KuberentesResource and save those for a major release.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[shawkins]

@manusa This commit shows a minimum amount of changes / breakage needed to non-generated files. The commit to the generated will be quite large, so I'm leaving that out for now - I'm also not able to generate all the extensions. For example knative fails with:

# knative.dev/pkg/changeset
vendor/knative.dev/pkg/changeset/commit.go:57:24: info.Settings undefined (type *debug.BuildInfo has no field or method Settings)
make: *** [Makefile:28: gobuild] Error 2

Not sure yet what is going on there. After seeing which ones currently are actually using the Map<String, Object> for raw, I can further update the generate.go files.

To recap these changes are:

• Change the default mapping of raw from HasMetadata to KubernetesResource. Unlike the Object mapping change, this preserves the builder methods like NamedExtensionFluent.withNewXXXExtension. This corrects the Serialization logic when building a list of results, and allows for things that aren't HasMetadata to be used in raw locations.
• Introduces a kubernetes RawExtension, which is a replacement for the openshift RawExtension, that is used when deserializing a value that has no kind/apiVersion. Not included yet in these changes is adding RawExtension to the Buildable array - that would allow buildable methods like withNewRawExtensionXXX and we may also want to add a Map based constructor as well for convenience. It might be good for RawExtension to implement Map<String, Object>, but that seems to cause problems for sundrio.
• To satisfy sundrio objects that have no kind are no longer HasMetadata, but instead just serializable. This does not affect what buildable methods are available, nor deserialization. Not sure who else may be looking for this marker interface.

What they don't do:

• Change the existing raw mappings. Map<String, Object> and the openshift RawExtension still exist. All existing code should work except for a couple of narrow cases - like with KubernetesList / KubernetesBaseList typing, but we could workaround that more such as changing the constructors to use List<? extends KubernetesResource>.

What could be done differently:

• As Marc suggested all generated classes could extend RawExtension and the mapping could be changed from KubernetesResource to RawExtension. However this is also a potentially breaking change - at the very least requiring users to regenerate or update custom model classes to the in hierarchy.

Migrating away from directly supporting map based on these changes would be a breaking change - the user would be expected to use RawExtension instead. The biggest usability issue there is that the getter will become typed as KubernetesResource rather than Map.

[shawkins]

Thanks @rohanKanojia it turned out to be the go version. knative requires 1.18+.

Updated to pare back where KubernetesResource is used - many of the locations where the object model specifies raw can really only have typed values. Since we don't differentiate between typed and HasMetadata, the most appropriate mapping is to still use HasMetadata. This was the conclusion for BaseKubernetesList - it also applies to Template (you can create a Template with an untyped raw value, but you can't do anything with it).

So to do this more thoroughly we'd need to evaluate on a case by case basis if the raw mapping actually accept untyped values, like NamedExtension. Presumably all the places that were using Map and the previous RawExtension class, the answer is yes. Unfortunately there's nothing to indicate this ahead of time or that would be easy to capture in the generation logic - it would seemingly take special case handling like was being done for the image raw extension.

This is obviously more involved that I had intended, so I'll probably take one more pass over it to see how to further minimize things.

[shawkins]

@manusa @rohanKanojia A quick recap of the changes:

• There are places that were mapping json and other types to Object and JsonNode, these have not changed
• The places using Map<String, Object> have not changed
• By default we were mapping the RawExtension type to HasMetadata. This has been changed to KubernetesResource - that retains the builder methods and allows for non-HasMetadata values. The existing deserialization logic was converting those circumstances to a GenericKubernetesResource - without metadata or type information. With these changes we'll deserialize instead to a new RawExtension (just a Map wrapper) class that is a KubernetesResource.
• HasMetadata is still used as the default mapping for core. This simplifies the handling of the List related types - which we determined should remain HasMetadata.
• Because core defaults to HasMetadata, WatchEvent and NamedExtension raw properties are specifically overriden to KubernetesResource.
• Special handling in the schemagen logic for image raw handling was removed. Instead in the generator specific generate.go files we specify overrides as needed to existing types.
• The openshift RawExtension is still in use for the image metadata, just deprecated - it extends the kuberentes RawExtension.
• clusteroperatorstatus.extension needs to be further overriden. As a KubernetesResource it causes build issues with sundrio. It's mapped to RawExtension instead.

So to handle json / raw, we have in total:

• JsonNode - see camelk integrationspec flows and other places
• Object - tekton json type
• Map<String, Object> - for RawExtension in several of the OpenShift and all of the extension modules
• kuberentes RawExtension - this is new, used so that the KubernetesDeserializer can return a non-HasMetadata value that is still a KubernetesResource.
• openshift RawExtension - used only for on image property, will be replaced with the kubernetes RawExtension
• KuberentesResource - this had limited use before, just in WatchEvent. Now is the preferred mapping - but on a case by case base we may still want to use HasMetadata.
• HasMetadata - still appropriate anywhere we want to restrict the raw to only typed values, such as Template, BaseKubernetesList, etc.

Quite a mess...

[manusa]

It seems there was an accidental commit (7746090 - #4160) of the vendor directory that you are reverting now.

It would be better if we separated that reversion to a separate commit or even a different PR. Then we can rebase this one so that the changes are clearer.

[shawkins]

@andreaTP @manusa this introduces an AnyType class that is a base for IntOrString and RawExtension (which is still a KubernetesResource). Alternatively we could leave IntOrString alone and just introduce AnyType, but also make it a KubernetesResource.

For now it is just an Object wrapper with some javadocs over the expected getValue types. Are there additional helper methods that we would want?

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

48.3% Coverage
0.0% Duplication

[andreaTP]

@shawkins thanks a lot for taking care of this, appreciated!

Regarding having IntOrString that extends AnyType I don't find in the spec any hint, I would not do it at first, but it's more of a gut feeling other than having any proof that it's the correct encoding, so not fussy on it.

Regarding helper functions I think that a from/to JsonNode would be a good start to cover a bunch of use cases.

[shawkins]

Regarding having IntOrString that extends AnyType I don't find in the spec any hint, I would not do it at first, but it's more of a gut feeling other than having any proof that it's the correct encoding, so not fussy on it.

It's related more to the Java implementation rather than any explicit schema hierarchy. AnyType is simply and object wrapper - which is what IntOrString is as well. IntOrString is just type restrictions layered on AnyType.

Regarding helper functions I think that a from/to JsonNode would be a good start to cover a bunch of use cases.

Without additional restructuring I think those functions would need to go in same module as Serialization - so that we can reuse the same mapping logic.

Converting from JsonNode should be unnecessary - any object, including JsonNode, is a valid value. That can be called out in the JavaDocs.

For converting to JsonNode that would currently be Serialization.jsonMapper().valueToTree(object) - but we may not want to encourage that static usage as we'll ideally encapsulate that better.

[manusa]

This reverts #4263 / #4206

Are we sure we want to proceed with this change? In which cases are we going to use our Serialization tooling to unmarshal an "invalid"/incomplete resource?

[shawkins]

This reverts #4263 / #4206

The main qualm addressed with throwing an exception with 4263 was not returning an untyped GenericKubernetesResource, which is also HasMetadata. Here we're returning a RawExtension - so there's no ambiguity.

[manusa]

🤔

I feel that the following methods are still ambiguous:

public static <T> T unmarshal(InputStream is) {
// ...
public static <T> T unmarshal(String str) {

We probably need to clean up this class (other PR/Issue).

I see value in a Serialization.unmarshall field that can return any instance of HasMetadata, list of HasMetadata (as a YAML multidocument 😉), or maybe even KubernetesList, but only restricted for these types.

The current behavior for the referenced methods is sort of unpredictable.

[shawkins]

I feel that the following methods are still ambiguous:

That's a different level of ambiguity. What I'm mostly trying to do is remove special case handling. If we have no type specified, then RawExtension makes just as much sense as throwing an exception - but allows us to remove the specialized check in Serialization.

I see value in a Serialization.unmarshall field that can return any instance of HasMetadata, list of HasMetadata, or maybe even KubernetesList, but only restricted for these types.

That seems fine to add - Serialization.unmarshallHasMetadata or another signature with a hasmetadata flag.

Related to this - a yaml list of HasMetadata is currently problematic. The root unmarshall method conversion from yaml only accounts for an object root - an array will fail with a class cast.