quarkiverse / quarkus-vault

Quarkus HashiCorp Vault extension
Apache License 2.0
19 stars 26 forks source link

Removing testcontainers dependencies from mutable-jar causes errors during augmentation #12

Closed pedroigor closed 2 years ago

pedroigor commented 2 years ago

Hi,

We are using quarkus-vault extension in Keycloak to enable Hashicorp to our users.

Right now, we are reviewing the libraries that are shipped within our distribution to not only make the distribution size smaller but also reduce the vulnerability surface by removing any unwanted library.

Differently from other extensions we are using, looks like the extension is trying to load classes related to testcoinrainers libraries and producing an error when dev services is disabled:

[ Failed to load steps from class io.quarkus.vault.deployment.DevServicesVaultProcessor: org/testcontainers/vault/VaultContainer: org.testcontainers.vault.VaultContainer

This happens when building the project but also when re-augmenting a mutable jar.

I would like to know if there is something we can do to completely avoid loading testcontainers related classes when dev services are disabled as well as not breaking re-augmentation if those classes are missing from classpath.

pedroigor commented 2 years ago

I've tried to find a possible workaround, but I failed. For instance, remove any static init code that could end up loading the missing classes from testcontainers.

gsmet commented 2 years ago

/cc @stuartwdouglas

pedroigor commented 2 years ago

Thanks @gsmet.

The relevant part of the stacktrace should be:

Caused by: java.lang.NoClassDefFoundError: org/testcontainers/vault/VaultContainer
    at java.lang.ClassLoader.defineClass1 (Native Method)
    at java.lang.ClassLoader.defineClass (ClassLoader.java:1017)
    at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass (QuarkusClassLoader.java:445)
    at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass (QuarkusClassLoader.java:405)
    at java.lang.Class.getDeclaredMethods0 (Native Method)
    at java.lang.Class.privateGetDeclaredMethods (Class.java:3166)
    at java.lang.Class.getDeclaredMethods (Class.java:2309)
    at io.quarkus.deployment.ExtensionLoader.getMethods (ExtensionLoader.java:931)
    at io.quarkus.deployment.ExtensionLoader.loadStepsFromClass (ExtensionLoader.java:479)
    at io.quarkus.deployment.ExtensionLoader.loadStepsFrom (ExtensionLoader.java:197)

Using Quarkus 2.5.1.Final.

pedroigor commented 2 years ago

@stuartwdouglas Would make sense to have a type-level annotation to indicate if a class should be skipped when loading build steps?

pedroigor commented 2 years ago

I was able to work around the issue by changing the extension like that https://github.com/pedroigor/quarkus/tree/vault-devservice-reflection.

You are probably the best person to ask why that works :) It seems to avoid loading classes when doing method reflection.

kdubb commented 2 years ago

The build step is annotated with this:

@BuildStep(onlyIfNot = IsNormal.class, onlyIf = { GlobalDevServicesConfig.Enabled.class })

I would assume that would stop it from being loaded when devservices is globally disabled.

pedroigor commented 2 years ago

Yeah, me too. And I have dev services globally disabled by setting quarkus.devservices.enabled=false.

However, I think the problem here is before evaluating if the build step conditions are met. When using reflection to load build steps from classes (as per stack trace).

kdubb commented 2 years ago

@pedroigor After looking at ExtensionLoader, you are correct.

Specifically because it loads all the methods of the classes and attempts to enumerate the methods to locate required classes.

Vault has a method to start the devservices container: https://github.com/quarkiverse/quarkus-vault/blob/a2294978ffb0f3f18c36cb6edd7f8bd581c70c1a/deployment/src/main/java/io/quarkus/vault/deployment/DevServicesVaultProcessor.java#L123

The startContainer returns a VaultContainer which is what's causing your issue, it sees this type during enumeration and tries to load it.

The solution you attempted works because instead of returning a VaultContainer you are returning a Supplier<VaultContainer> which, when you take into account Java's type erasure, is really just a Supplier<?>; hence the method signature no longer references VaultContainer directly and never. tries to load it.

I think the correct solution is to fix ExtensionLoader itself. I'm fairly certain other plugins probably suffer a similar fate. I know the Redis extension has a similar methodology.

Offhand, it seems like it should be ignoring private methods. Alternatively, it could ignore class loading issues when iterating the methods. Alas, I think that's for others to decide. If it cannot be fixed in ExtensionLoader at least we have a solution; just return the the VaultContainer in a wrapper that's always available.

kdubb commented 2 years ago

Added issue to Quarkus for fixing ExtensionLoader

stuartwdouglas commented 2 years ago

So this library is not actually loaded when running, it can only be loaded when doing a re-augmentation. The reason why it is present is that Quarkus does not have a split between the dev/test mode classpath, and the classpath used for building production applications, so dev mode features will also be present in mutable jar.

Any attempt to remove libraries like this is always going to be hacky and fragile without a full dev/test mode split, which is basically an extra module per extension and a lot more complexity.

pedroigor commented 2 years ago

Thanks, @kdubb. I had a wrong assumption that classes wouldn't be loaded like that when loading methods through reflection.

pedroigor commented 2 years ago

Any attempt to remove libraries like this is always going to be hacky and fragile without a full dev/test mode split, which is basically an extra module per extension and a lot more complexity.

I understand how fragile this can be now and a good example is the behavior reported here. We would like to reduce our dist size and that is why we are trying to remove libraries like that.

Can we expect a middle-ground solution that makes this less fragile and more consistent across different extensions? Or the best thing to do here is "fix" the quarkus-vault extension?

vsevel commented 2 years ago

Is this issue still relevant? Is there anything we can/should do?

pedroigor commented 2 years ago

@vsevel Thanks. It is but I'm not sure if fixing here is going to solve the root problem. I think we can close for now and if needed, we can re-open and resume discussions.