quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.6k stars 2.63k forks source link

Kubernetes client/mock unable to register kotlin module 3.2.4 #35338

Open robp94 opened 1 year ago

robp94 commented 1 year ago

Describe the bug

In quarkus 3.2.x the registration process for object mapper customization changed. https://github.com/quarkusio/quarkus/pull/33767 Previously we used the static variant, which worked. Now I tried both variants listed here but none of them works. https://quarkus.io/guides/kubernetes-client#customizing-and-overriding

Reproducer https://github.com/robp94/quarkus-kubernetes-client-kotlin

2 Branches:

Error happens while testing in line 45 of TenantServiceControllerTest

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

ubuntu 22.04

Output of java -version

17

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.4

Build tool (ie. output of mvnw --version or gradlew --version)

maven

Additional information

No response

quarkus-bot[bot] commented 1 year ago

/cc @Sgitario (kubernetes), @evanchooly (kotlin), @geoand (kotlin,kubernetes), @iocanel (kubernetes)

geoand commented 1 year ago

cc @manusa

manusa commented 1 year ago

Hi @robp94,

I notice that you're registering the Kotlin module here: https://github.com/robp94/quarkus-kubernetes-client-kotlin/blob/cbdceb01490759d05d2bbf0d0b9f19f1124d79db/src/main/kotlin/test/RegisterCustomModuleCustomizer.kt

@Singleton
class RegisterCustomModuleCustomizer : ObjectMapperCustomizer {
    override fun customize(mapper: ObjectMapper) {
        mapper.registerModule(KotlinModule.Builder().enable(KotlinFeature.NullToEmptyCollection).enable(KotlinFeature.NullToEmptyMap).build())
    }
}

Note that the documentation states that you need to either provide an ObjectMapper where the singleton has been declared using the @KubernetesClientObjectMapper, or customize the one generated by Quarkus implementing the KubernetesClientObjectMapperCustomizer interface.

Unless I missed something, you're reproducer is not following any of these approaches.

I've never done kotlin, but I assume you need to change the RegisterCustomModuleCustomizer to

class RegisterCustomModuleCustomizer : KubernetesClientObjectMapperCustomizer {

so that the Kubernetes Client exclusive ObjectMapper gets customized, or implement both interfaces so that the generic ObjectMapper and the one for the client get customized with the module.

manusa commented 1 year ago

OK, I just saw that these changes are there in the 3.2.4 branch

manusa commented 1 year ago

OK, I'm more or less following now. The issue comes up only when using the MockServer, right? In production code, everything works as expected?

robp94 commented 1 year ago

Actually, I haven't tested it in production yet, since most of our test for the specific service failed.

But I just changed the test slightly. And used the normal client. There it seems to work.

See new commit to 3.2.4

manusa commented 1 year ago

:)) I was just testing that

manusa commented 1 year ago

So yes, changing the test to use the Quarkus-provided KubernetesClient:

    @Inject
    lateinit var client: KubernetesClient
    //fun getTenantServiceNullable() {...
        client.resource(tenantService).inNamespace(tenant).create()
    //...}

does the trick.

@geoand I'm not sure what's the Quarkus recommended approach for this. My assumption is that this should be the way. Are we recommending users anywhere to do mockServer.getClient() to perform any Kubernetes preparation operation? In this case there's no easy fix, since the (upstream) MockServer instantiates its own KubernetesClient that has no notion of the Quarkus-customized ObjectMapper.

So the fix would be to change the documentation (if such thing is documented).

geoand commented 1 year ago

I don't remember at all what we recommend and I'm on a phone now, so docs are hard to look at

manusa commented 1 year ago

https://github.com/quarkusio/quarkus/blob/2f85dad0cf741423188e6a194f59b0d75c1f54c2/docs/src/main/asciidoc/kubernetes-client.adoc#L172-L174

manusa commented 1 year ago

I created #35342

The further change would be to modify the KubernetesServerTestResource so that the injected KubernetesServer.getClient provides the client that Quarkus injects instad of the one that the MockServer creates.

However, I'm not really sure how to do this or if it's even possible.

robp94 commented 1 year ago

I just changed our tests to use the normal client, not the mock one. With this, our tests are green. Maybe change the docs and add a warning until the new issue is fixed?

Furthermore, the migration guide for 3.2 did not mention changes to json handling of kubernetes client, only other changes to kubernetes. Not sure how big a breaking change needs to be, to be listed there.