CRD Generator -- Stack overflow when using EnumMap type in Spec

[bequinn-hubspot]

Describe the bug

I'm seeing a stack overflow error in the CrdGenerator when defining a java.util.EnumMap as a private field. I believe the cause of this is due to the fact that the library is trying to introspect the Class<T> keyType private field that is contained within EnumMap. Within the Class type is a ConcurrentHashMap which has a bunch of circular references and that's where the generator gets into infinite recursion.

This effects multiple versions of the client.

I believe this is caused by part of the design of the CRD generator, where it introspects private fields as the means of generating schemas. The fields that should be added to the spec/status schemas are the ones that will be serialized by Jackson when sending a CR instance to the kube api. The default behavior in Jackson is to look at public, non-static, non-transient fields. This is also the behavior of the serializer that is contained within fabric8. Therefore, the CRD generator should follow the same visibility rules.

I don't think that private, static, or transient properties should be added to the schema, but rather the way schemas are generated should be inverted. First look at public, non-static, non-transient methods (getters) and then check to see if there's a matching private field to pull annotations from. I believe that is the behavior that is expected from jackson.

Fabric8 Kubernetes Client version

SNAPSHOT

Steps to reproduce

Add

public enum Foo { BAR }

private EnumMap<Foo, String> enumToStringMap;

to any Spec or Status. Add it to io.fabric8.crd.example.map.ContainingMapsSpec to create a testable reproduction of the issue.

Expected behavior

1. Stack overflow does not occur
2. Schema generation follows visibility rules of Jackson ObjectMapper

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.25.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

No response

Additional context

Stack trace for overflow

[ERROR] Tests run: 14, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 5.603 s <<< FAILURE! -- in io.fabric8.crd.generator.ParallelCRDGeneratorTest
[ERROR] io.fabric8.crd.generator.ParallelCRDGeneratorTest.mapPropertyShouldHaveCorrectValueType -- Time elapsed: 2.369 s <<< ERROR!
java.lang.StackOverflowError
        at java.base/java.util.stream.AbstractPipeline.wrapSink(AbstractPipeline.java:522)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at io.sundr.builder.BaseFluent.build(BaseFluent.java:58)
        at io.sundr.model.ClassRefFluentImpl.getArguments(ClassRefFluentImpl.java:227)
        at io.sundr.model.ClassRefBuilder.build(ClassRefBuilder.java:57)
        at io.sundr.model.ClassRefBuilder.build(ClassRefBuilder.java:7)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at io.sundr.builder.BaseFluent.build(BaseFluent.java:58)
        at io.sundr.model.ClassRefFluentImpl.getArguments(ClassRefFluentImpl.java:227)
        at io.sundr.model.ClassRefBuilder.build(ClassRefBuilder.java:57)
        at io.sundr.model.ClassRefBuilder.build(ClassRefBuilder.java:7)
        at io.sundr.model.PropertyFluentImpl.getTypeRef(PropertyFluentImpl.java:259)
...
io.fabric8.crd.generator.visitor.AnnotatedPropertyPathDetector.visitPropertiesClasses(AnnotatedPropertyPathDetector.java:91)
        at io.fabric8.crd.generator.visitor.AnnotatedPropertyPathDetector.visit(AnnotatedPropertyPathDetector.java:76)
        at io.fabric8.crd.generator.visitor.AnnotatedPropertyPathDetector.visit(AnnotatedPropertyPathDetector.java:34)
        at io.sundr.builder.Visitor.visit(Visitor.java:30)
        at io.sundr.builder.VisitorWiretap.visit(VisitorWiretap.java:79)
        at io.sundr.builder.BaseFluent.accept(BaseFluent.java:110)
        at io.sundr.builder.BaseFluent.accept(BaseFluent.java:139)
        at io.sundr.builder.BaseFluent.accept(BaseFluent.java:75)
        at io.fabric8.crd.generator.visitor.AnnotatedPropertyPathDetector.visitPropertiesClasses(AnnotatedPropertyPathDetector.java:91)
        at io.fabric8.crd.generator.visitor.AnnotatedPropertyPathDetector.visit(AnnotatedPropertyPathDetector.java:76)

[andreaTP]

Hi @bequinn-hubspot !
As a workaround you can mark the property with @JsonIgnore to exclude it explicitly.
Let me know if this works for you!

[bequinn-hubspot]

Hey @andreaTP thanks for the reply.

I don't think that will work because then the field will no longer be stored in kubernetes.

[andreaTP]

I'm from the phone and I will try to have a closer look tomorrow.
From what I see there is probably a bug cc. @metacosm .
You can still workaround with SchemaFrom or SchemaSwap, let me know how it goes...

[andreaTP]

@bequinn-hubspot thanks for your patience, I can reproduce the issue and the root cause is that EnumMap brings in a quite consistent part of the Java standard lib, up to ThreadLocals, ConcurrentHashMaps etc. (including many cyclic data structures).

The problem here is that the StackOverflow happens before the CRD generator logic kicks in AnnotatedPropertyPathDetector, so SchemaFrom and SchemaSwap are not going to help either.

We can exclude classes from the visiting algorithm, but it seems "artificial", and this situation can easily happen again.

Looking for another opinion, @metacosm @shawkins ?

[shawkins]

@andreaTP do you have a reproducer?

[andreaTP]

yup, the provided repro does the job: 30adf7b

[bequinn-hubspot]

Yes, I meant to include that I found it happens within AnnotatedPropertyPathDetector as well as the multi path detector.

[bequinn-hubspot]

Hey @andreaTP, I really appreciate how quickly you guys were able to take a look at this issue, thank you!

I cherry picked that commit from #5588 and I was able to confirm that it did fix our infinite recursion issue.

Unfortunately I did run into another separate, semi-related issue. In one of our classes we use guava's com.google.common.collect.ImmutableListMultimap which contains a circular reference. This throws the expected exception

        at io.fabric8.crd.generator.AbstractJsonSchema.resolveNestedClass(AbstractJsonSchema.java:618)
        at io.fabric8.crd.generator.AbstractJsonSchema.internalFromImpl(AbstractJsonSchema.java:599)
        at io.fabric8.crd.generator.AbstractJsonSchema.internalFromImpl(AbstractJsonSchema.java:259)

That would normally be the expected result of a circular reference, however there is a jackson GuavaModule which handles serializing the MultiMap implementation without the circular reference.

I think this may be a more general issue, where the class' shape isn't necessarily the same as the serialized object due to Jackson's support for modules, and the manipulation of serialization within those modules.

The reason this is semi related is because it's another place where the CRD generation code diverges from how jackson handles things. That said, let me know if this would best be discussed elsewhere.

[shawkins]

@bequinn-hubspot it would probably be best to separate this - there are quite a few places the crd logic diverges or doesn't handle things from jackson.

[bequinn-hubspot]

Ok, I'll open a separate issue