Reactive Datasource: support CredentialsProvider changing values

[tsegismont]

Instead of:

• creating pgConnectOptions after calling the CredentialsProvider on startup
• using static pool configuration

We:

• use dynamic pool configuration
• call the Credentials provider every time a new connection must be created

This PR allows to support use cases like password rotation.

See also https://issues.redhat.com/projects/QUARKUS/issues/QUARKUS-2995

[tsegismont]

The first commit applies changes to the Reactive Pg Client extension only. I'll update the other reactive SQL client extensions after we've discussed the proposal.

[tsegismont]

@vietj can you please take a look (dynamic pool config implementation)

@cescoffier @geoand I've made changes so that the credentials provider is invoked on a worker thread, is that fine? The user might be doing blocking calls in the implementation.

[geoand]

I've made changes so that the credentials provider is invoked on a worker thread, is that fine? The user might be doing blocking calls in the implementation

Have you seen such use cases or users mentioning this?

[tsegismont]

Have you seen such use cases or users mentioning this?

I looked into this because of https://issues.redhat.com/projects/QUARKUS/issues/QUARKUS-2995

I haven't tried myself or seen reproducers. I'm trying to think about uses cases, like password rotation. The changing credentials must come from the disk or a remote system? If the lookup is made synchronously in the credentials provider, the event loop will be blocked.

We have two options, I think:

• acknowledging the CredentialsProvider contract is synchronous and invoking it on a worker thread
• invoking it on the event loop anyway and then we shall:
  • document it
  • recommend users to perform the new credentials lookup in a scheduled task

[geoand]

I guess we should have it on the worker thread for the time being and if the need arised, look into supporting the @Blockling / @NonBlocking annotations

[cescoffier]

The call to the credential provider must happen on a worker thread unfortunately (wrong design).

[cescoffier]

The idea looks good.

The credential provider must be called on a worker thread, as it may block.

[ingmarfjolla]

hello, i just verified that this fix seems to be working with this reproducer here (quarkus version 3.0.0.Alpha4): https://github.com/ingmarfjolla/quarkus-reactive. Thanks for working on this!

cc @senthilredhat @raffaelespazzoli

[ingmarfjolla]

the only thing i noticed is that the credential manager (vault in my case) seems to be called multiple times on 7 different event loop threads on startup, so the logs look something like :

2023-03-16 11:41:51,471 WARN [io.quarkus.vault.runtime.TimeLimitedBase] (vert.x-eventloop-thread-1) mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s {}  
2023-03-16 11:41:51,475 WARN [io.quarkus.vault.runtime.TimeLimitedBase] (vert.x-eventloop-thread-1) mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s {}  
2023-03-16 11:41:51,489 WARN [io.quarkus.vault.runtime.TimeLimitedBase] (vert.x-eventloop-thread-2) mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s {}  
2023-03-16 11:41:51,493 WARN [io.quarkus.vault.runtime.TimeLimitedBase] (vert.x-eventloop-thread-2) mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s {}  
...
2023-03-16 11:41:51,576 WARN [io.quarkus.vault.runtime.TimeLimitedBase] (vert.x-eventloop-thread-7) mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s mydbrole (database/creds) lease duration 60s is smaller than the renew grace period 3600s {}  

[tsegismont]

The idea looks good.

Thank you @cescoffier

The credential provider must be called on a worker thread, as it may block.

👍

the only thing i noticed is that the credential manager (vault in my case) seems to be called multiple times on 7 different event loop threads on startup

@ingmarfjolla The crendentials provider can be invoked twice on startup by the extension, and then the default 5 max connections may be created immediately if you setup the database (perhaps with Hibernate Reactive) ?

A good way to check would be to log a fake error (new Exception()) in order to see the stack trace.

[tsegismont]

@cescoffier @geoand about credentials provider API: I took a look at the Vault extension implementation:

            return vaultDynamicCredentialsManager.getDynamicCredentials(DATABASE_DEFAULT_MOUNT, DEFAULT_REQUEST_PATH,
                    config.databaseCredentialsRole.get()).await().indefinitely();

Too bad the contract is synchronous because the implementation uses non blocking-APIs.

Do you think it is worth tracking in a separate issue? In the meantime, we definitely have to invoke this on a worker thread.

[geoand]

Yeah, I think we should open an issue about providing a new SPI

[cescoffier]

Yes, when I found out about the synchronous and blocking nature of the credential provider it was too late :-(. So, yes, please open an issue to switch to a non-blocking API.

[tsegismont]

The PR is ready for review

[tsegismont]

The failure looks unrelated

[geoand]

Yeah, totally unrelated

[kdubb]

🎉

[tsegismont]

The test failure looks unrelated

[geoand]

@kdubb what was the outcome of your experiment vs this?

[kdubb]

@tsegismont Is using the culmination of my original PR's against the vert.x sql client.

[geoand]

Excellent. @cescoffier are you still good with the latest version of the PR?

[cescoffier]

Yes, looks good to me.

[kdubb]

Is there any chance of this being back ported to 2.x? idle-timeout is the solution to many issues and discussions in the vault extension. It would be nice to have the functionality in both series if possible.

[geoand]

Is there any chance of this being back ported to 2.x

-1 as we really don't want to backport features to 2.x

[geoand]

Can we get one last rebase please so we can make sure CI is fine?

[tsegismont]

Can we get one last rebase please so we can make sure CI is fine?

Done

[geoand]

Thanks!

[quarkus-bot]

Failing Jobs - Building 3e5afbf

Status	Name	Step	Failures	Logs	Raw logs
⌛	Native Tests - Security1	Build	⚠️ Check →	Logs	Raw logs

[tsegismont]

@geoand this time it seems the build failed because one job ran out of time.

[geoand]

Yeah, I doubt it's related. @sberyozkin can you confirm?

[tsegismont]

@cescoffier @geoand can we go ahead and merge the PR?

[tsegismont]

Thanks @geoand !

[geoand]

Thank you for this!