Parsing kube config file without reflection. Fixes #1591

[ricardozanini]

Fixes #1591

Work in Progress: this PR has been opened to discuss the implementation details with manteiners

Hi @geoand and @iocanel! As promised, here's the PR with an implementation suggestion to remove reflection completely from the configuration use case.

I've tested this feature across your unit tests, IT and inside the native image. Seems solid, but I intend to write some more tests.

Before keep going, I'd like to hear from you if this approach seems fine or if you'd like to take another one. With this new approach I was able to run the authentication and config features from kubernetes client inside a native image without registering any class for reflection.

I understand that's not the ideal world and in the future we could auto-generate those "mappers" with a maven plugin during build time.

Let me know your thoughts.

Cheers!

[centos-ci]

Can one of the admins verify this patch?

[rohanKanojia]

ok to test

[geoand]

Thanks @ricardozanini !

I'll let @iocanel comment since he is the expert here

[iocanel]

Why do you replace jackson with a custom snakeyaml + custom deserializers?
Is this really needed?
Why is jackson an issue only on Config and not when desrializing responses from the api server?

[ricardozanini]

Hi @iocanel! I replaced only the Config model because this way we can leverage from the configuration and authentication features from the client without using reflection. The idea is to replace with a custom mapper the responses as well.

In our project, having the client handle the authentication features, we can leverage from the OkHttpClient to query the API and deserialize using another options (like json path for example).

I see three options to handle this deserialization issue when working with Quarkus in a native image:

1. Register the entire model using GraalVM SDK
2. Register with Quarkus (could be an extension)
3. Use a custom mapper

This PR tackles the option 3 since the use case is only to parse the kube yaml files and it seemed a simple approach at first. Those mappers could be generated during build time, like the way you guys are doing with the domain model.

[iocanel]

So, my undersntanding is that long term we will go with option 3 for everything.
Untill, then can't we handle Config using 1?

I want to avoid introducing a mapping mechanism just for handling a single class.

[geoand]

Can we perhaps just add a GraalVM json file for registering reflection with GraalVM as @iocanel mentions? Wouldn't that be an easier first step?
Also, that file could contain other GraalVM specific configurations, could it not?

[ricardozanini]

@iocanel a long term option should be 3 if we decide to drop reflection usage at all. You're right to deny introducing a custom mapper for a single class, I'd say the same. This is just a proposal for a mapper that we could build for the entire client. I could work on it.

@geoand manually registering the entire domain model is the easiest solution and it's the approach that you took for the quarkusio/quarkus#2910 PR, right? Yeah, I guess that this option is a great first step and we can go ahead with it! For this PR, though, I'd say to introduce the option 3 for the entire client.

So, the question is: should we create our custom (generated) mapper for the long term or we stick with manual reflection registration provided by Quarkus/SubtrateVM? Having the custom mapper won't exclude the Quarkus extension, since there's still the unsupported feature in subtrateVM regarding the EC Keys with Bouncy Castle.

[geoand]

I would say that making incremental improvements on reducing reflection in this project will be great down the line. That said, I think @iocanel has some thoughts on how to make that easier based on some other he has done.

The Quarkus PR does register everything in the Kubernetes Model for reflection, but with a twist :). It doesn't force everything to end up in the native binary, but instructs GraalVM to include all methods and fields for only those model classes that are actually used in the application that includes the it.

[ricardozanini]

So I guess that we can close this PR since @iocanel is already working on something? I worked around the Config parsing stuff just setting KUBERNETES_AUTH_TRYKUBECONFIG to false. And then mapped the JSON response as simple Maps to avoid reflection of the domain classes. Worked, but now I guess with the extension I'll drop this work around.

[geoand]

Let's what and see what the man himself will say :).
I know the work in question was done on another project so I am not sure if it can be of immediate use here.

In any case, thanks a lot of looking into this!

[iocanel]

Since it's possible to use the client in native mode by registering the model in graal I wouldn't want to rush this.

From my point of view the main issue, is that we are adding a lot of code just to get rid of using jackson for a single resource.

I'd rather find a solution that would somehow code generate this.
A possible solution (which is not battle tested though), would be to use the @pojo annotation from sundrio, which allows conversion of annotations/interface to pojos and also supports generating adapters from maps (pretty much what this code does).

[ricardozanini]

@iocanel I agree with you, I'm closing this PR for now. Many thanks!