fix #4206: ensuring null won't be returned from kubernetesdeserializer

[shawkins]

Description

Fix #4206

This is to address #4206. We cannot throw an exception as it appears that the Config parsing has embedded HasMetadata (extensions) references, which lack type information. So instead we'll return a generic resource - that should ensure round-trip parse/serialization is also correct.

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

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[shawkins]

@manusa @rohanKanojia based upon our meeting we agree that mapping raw to HasMetadata is not good. It also creates a potentially odd set of builder methods as sundrio will assume the only valid types are the concrete types in the module and in the BuildableReferences. Raw should instead be any valid structured content.

My motivation to use the GenericKubernetesResource however was that it was the easiest way to preserve this content. if you run something like:

    IngressControllerSpec spec = new IngressControllerSpec();
    GenericKubernetesResource resource = new GenericKubernetesResource();
    resource.setAdditionalProperty("foo", "bar");
    spec.setUnsupportedConfigOverrides(resource);
    System.out.println(Serialization.asYaml(spec)); // contains unsupported
    IngressControllerSpec parsed = Serialization.unmarshal(Serialization.asYaml(spec), IngressControllerSpec.class);
    System.out.println(Serialization.asYaml(parsed)); // does not contain unsupported

You'll see that the unsupportedConfigOverrides are lost when they lack a kind/apiVersion.

That should be occurring in all such places like NamedExtension.extension, which seems to already exist in the Configs used in our test cases.

So if we don't want to use GenericKubernetesResource to conform to the existing mapping, then we'll have to change the generator / mapping logic.

As for the original issue #4206 - that could be handled separately for now just by considering the top-level object only, but it will of course get fixed if we generally handle the raw case - the Serialization.unmarshall would return a map of maps or whatever we think is appropriate for representing raw content when the root object lacks kind/apiVersion information.

[manusa]

base upon our meeting we agree that mapping raw to HasMetadata is not good. It also creates a potentially odd set of builder methods as sundrio will assume the only valid types are the concrete types in the module and in the BuildableReferences. Raw should instead be any valid structured content.

Right now the mapper has inconsistent handling for this, which is the worst part.

So in general, the following applies:

kubernetes-client/kubernetes-model-generator/pkg/schemagen/generate.go

Lines 180 to 181 in 296ec4f

	case "RawExtension":
	return BasePackage + ".HasMetadata"

But you can also find:

kubernetes-client/extensions/open-cluster-management/generator-agent/cmd/generate/generate.go

Line 63 in 9f1a569

reflect.TypeOf(runtime.RawExtension{}): "java.util.Map<String, Object>",

kubernetes-client/kubernetes-model-generator/openshift-model-machine/cmd/generate/generate.go

Line 73 in a1b6863

reflect.TypeOf(runtime.RawExtension{}): "java.util.Map<String, Object>",

and a few more...

And as I write this I'm recalling conversations around this topic and the specific mapping of rawExtension to Map precisely to avoid this problem. (#2073 #2335)

There also seems to be an internal issue about this d511cc8

[metacosm]

LGTM apart from typo (see comment).

[shawkins]

@manusa @metacosm here's an update to a hybrid / tactical solution. The deserializer will always return a generic, but we'll make a top level check to ensure that only valid generics are returned - which will satisfy #4206. This will allow for the HasMetadata mapping of raw to continue to work - then for a major release, or whenever we're comfortable with the breaking change, the mappings can be fixed.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

83.3% Coverage
0.0% Duplication