Fully addressing static deserialization issues

[shawkins]

Description

Builds on #4651 to show where these changes are headed. This is in a rough state, so it's just representative and not fully working.

To recap - clone, convertValue, and unmarshal with a type that transitively references KubernetesDeserializer are all relying on static KubernetesDeserializer state. Specifying a non-KubernetesResource type like Map, should be fine - however, it still seems like a good idea to aggregate all of the calls onto the new class rather than leaving some static calls.

The other Serialization methods rely on static objectmappers, at least the yaml mapper was identified as unnecessarily holding state (more on that below).

This change attempts to remove the static KubernetesDeserializer state and make more about the ObjectMapper injectable. We will do this by introducing a KubernetesSerialization class to represent state-fully how to do our serialization operations. With a few other tweaks we can give full control over what classes are known - of course still offering the classloader based loading mechanism by default.

The idea is that we'll associate a KubernetesSerialization with the client via the KubernetesClientBuilder. That will then get passed around appropriately. Instead of the KubernetesDeserializer holding a static mapping, there will instead be an instance held by KubernetesSerialization, which is associated with its objectmapper for use when a KubernetesDeserializer is requested.

Other thoughts:

• this is unfortunately a lot of work / rewiring, and at least some of the impetus to do all of this goes away if we don't allow for the programmatic registration of known types.
• Since we took responsibility for yaml parsing, we can do the same thing for yaml dumping and eliminate the need for the yaml mapper and the method. The only difference I can see in generation is that the string literal values won't be quoted by default.
• Serialization and KubernetesSerialization methods have a lot of duplication - I just didn't get around to making Serialization fully use a static KubernetesSerialization instance. That instance is obtainable for now from Serialization.getKubernetesSerialization() for convenience in tests - that will get moved later for clarity.
• If you try to deserialize a class that references KubernetesDeserializer on your own outside of using KubernetesSerialization or Serialization, then that will construct a KubernetesDeserializer for each call - that will work (as long as you aren't registering other classes) but will be a performance hit.

@manusa @rohanKanojia @metacosm let me know if this seems worth following through on.

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]

Rebased and pulled out unrelated changes. This is much closer to being fully functional.

General notes on these changes:

• Attempt to consolidate serdes calls as much as possible - avoiding direct mapper usage.
• We talked about this in one of the meeting, it removes any usage of a Yaml mapper. Instead there's direct snake usage for the toYaml function. To mostly match the previous output it does require some customization.
• What Serialization static methods should not be used?
  • Anything that assumes the usage of the KubernetesDeserializer. This is primarily typeless unmarshal calls.
• What Serialization static methods can still be used?
  • mainly toJson and toYaml, although they will be available off of KubernetesSerialization as well.
• What Serialization static methods are a grey area?
  • Directly providing a json and a yaml mapper, it seems best to deprecate these as they expose quite a wide public api.
  • even with the typed unmarshal calls it's possible to provide KubernetesResource as the type either directly or transitively, which will then need to use a KubernetesDeserializer
  • clone has the same transitive problem

What still needs work:

• Serialization.getKubernetesSerialization() for convenience in tests - that will get moved later for clarity. There are a couple of non-test places that use Serialization.getKubernetesSerialization which need to use the proper one instead. For example we can change the itemstore interface, or require the user to provide the appropriate kubernetesserialization in the constructor
• How available to make the kubernetesserialization instances - it probably should be available from the Client interface.
• Serialization and KubernetesSerialization methods still have duplication
• If you try to deserialize a class that references KubernetesDeserializer on your own outside of using KubernetesSerialization or Serialization, then that will construct a KubernetesDeserializer for each call - that will work (as long as you aren't registering other classes) but will be a performance hit. Longer term we can consider disallowing this such that you are required to use a path with a known KubernetesDeserializer.
• If we want to provide full control, move the KubernetesDeserializer to a public api location and supply it to the KubernetesSerialization constructor. It currently is fully encapsulated.

What this will allow us to do:

• Not hold a bunch of classes / classloaders statically - which should improve test and other scenarios that may need a live reload
• Improve / provide better controls over class loading / resolving scenarios - the current mechanism is based upon generating META-INF files, which is particular to the kubernetes-model-generator logic, but is otherwise a hand exercise. We can easily modify things so that the ServiceLoader scan is optional and give full control to the caller.
• We may also want to rethink the 1-1 relationship of a class to apiversion/kind - Add support for specifying the API Group and API Version of Custom Resource dyamically #4876 - that will require changes in the KubernetesDeserializer (which we can think about exposing in a better way) and possibly the client.

cc @manusa @andreaTP @metacosm

[shawkins]

This is a little closer.

The KubernetesDeserializer / class loading mechanism are still not exposed, but that should be straight-forward. Also the KubernetesSerialization constructor that takes an objectmapper could be refined - the logic that modifies the objectmapper could be moved to a separate method to be more usable with a quarkus ObjectMapperCustomizer.

For now the Serialization class remains in use as it would be painful to fully refactor some of the other static api methods. At the very least we'll still have another static objectmapper in Serialization.

and for now another derived one in the patchutils (which may be able to be removed, I'll look at that some more). The patchMapper has been removed as well - we could also choose to move patchutil methods onto KubernetesSerialization.

Other than that everything should be using the objectmapper provided to KubernetesSerialization. The methods on KubernetesSerialization try to encapsulate access to the objectmapper, but that can be relaxed if need be. The observation was that our existing code was using quite a few different ways to do parsing or serialization.

The third bullet point above remains - it's TBD on how to handle the direct usage of a non-modified objectmapper and KubernetesDeserializer. For now the static state has been removed from KubernetesDeserializer, but that means that each instance will need to redo the classloading.

cc @manusa @andreaTP @metacosm @rohanKanojia

[sonarqubecloud]

SonarCloud Quality Gate failed.   

1 Bug
0 Vulnerabilities
1 Security Hotspot
19 Code Smells

58.4% Coverage
0.0% Duplication

[manusa]

Hi @shawkins,
not sure if this PR deserves a changelog or despite the big refactor it's transparent to users