Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid obtaining Vertx from CDI #109

Merged
merged 2 commits into from
Feb 8, 2023
Merged

Avoid obtaining Vertx from CDI #109

merged 2 commits into from
Feb 8, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Feb 6, 2023

This is needed because the VertxVaultClient can potentially be used inside a Synthetic bean and in such cases
obtaining Vertx which is itself a synthetic bean is not possible

Fixes: #78

This is needed because the VertxVaultClient can potentially
be used inside a Synthetic bean and in such cases
obtaining Vertx which is itself a synthetic bean is
not possible

Fixes: quarkiverse#78
@kdubb
Copy link
Contributor

kdubb commented Feb 6, 2023

@geoand I'm not sure this does exactly what the previous code was doing. I need to look at this more in depth and report back.

The goal with Shared/Private is to ensure the system provided VertX (aka Shared) is used whenever it is available but for config related queries we might need to create a temporary (aka Private) VertX instance to query Vault before the system provides one.

@kdubb
Copy link
Contributor

kdubb commented Feb 6, 2023

I'm realizing that createSharedVaultClient is not the best name for this function. It should at least be createSharedVaultClientIfAvailable.

@geoand
Copy link
Contributor Author

geoand commented Feb 7, 2023

Right, that's also what my PR does, doesn't it?

@kdubb
Copy link
Contributor

kdubb commented Feb 7, 2023

This is a @Dependent producer. Previously it was not always creating a new client but only used as a way to lookup the best available client (shared or private). Now it is creating a new client for each injection point.

@geoand
Copy link
Contributor Author

geoand commented Feb 8, 2023

Okay, so how would you like to handle it?

@kdubb
Copy link
Contributor

kdubb commented Feb 8, 2023

I think the following works. Basically the same code just uses VertxRecorder.getVerx() as the flag for shared vs. private and then requests it directly in the constructor instead of injecting it.

@Produces
@Dependent
public static VertxVaultClient createSharedVaultClient() {
    Annotation clientType;
    if (VertxRecorder.getVertx() != null) {
        clientType = Shared.Literal.INSTANCE;
    } else {
        clientType = Private.Literal.INSTANCE;
    }
    return Arc.container().select(VertxVaultClient.class, clientType).get();
}
public SharedVertxVaultClient(VaultConfigHolder vaultConfigHolder, TlsConfig tlsConfig) {
    super(vaultConfigHolder.getVaultBootstrapConfig().url.orElseThrow(() -> new VaultException("no vault url provided")),
            vaultConfigHolder.getVaultBootstrapConfig().enterprise.namespace,
            vaultConfigHolder.getVaultBootstrapConfig().readTimeout);
    Vertx vertx = Vertx.newInstance(VertxRecorder.getVertx());
    this.webClient.set(createHttpClient(vertx, vaultConfigHolder.getVaultBootstrapConfig(), tlsConfig));
}

@kdubb
Copy link
Contributor

kdubb commented Feb 8, 2023

I tested this locally. I can push the changes to the PR branch.

@geoand
Copy link
Contributor Author

geoand commented Feb 8, 2023

Sounds good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault DB credentials provider with reactive-pg-client
2 participants