8000 Automatically connect to a writer instance of PostgreSQL by ahus1 · Pull Request #40384 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Automatically connect to a writer instance of PostgreSQL #40384

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2025
Merged

Automatically connect to a writer instance of PostgreSQL #40384

merged 1 commit into from
Jul 4, 2025

Conversation

ahus1
Copy link
Contributor
@ahus1 ahus1 commented Jun 10, 2025

Closes #40383

@ahus1 ahus1 self-assigned this Jun 10, 2025
@ahus1 ahus1 force-pushed the is-40383-connect-to-writer-instance-automatically branch from b94241e to cd5f5df Compare June 10, 2025 15:22
@ahus1
Copy link
Contributor Author
ahus1 commented Jun 10, 2025

@vmuzikar / @shawkins - I am trying to set a DB property by default similar to

keycloak/model/jpa/src/main/java/org/keycloak/connections/jpa/DefaultJpaConnectionProviderFactory.java

Lines 451 to 461 in 24910d9

private String addPostgreSQLKeywords(String jdbcUrl) {
if (!jdbcUrl.contains("targetServerType=")) {
if (jdbcUrl.contains("?")) {
jdbcUrl = jdbcUrl + "&";
} else {
jdbcUrl = jdbcUrl + "?";
}
jdbcUrl = jdbcUrl + "targetServerType=primary";
}
return jdbcUrl;
}

but I didn't find a similar spot in Quarkus. When someone passes in a JDBC URL, there is no way good way I found intercept this to add a suffix.

This also keep me wondering how much automation / magic should be done here. Still IMHO this is a very common reason why people struggle with the DB connection pools, as we've seen in support tickets.

Is there a different way for this?

cc: @sguilhen

@mabartos
Copy link
Contributor

@ahus1 It should be possible to use the transformer for the DB_URL_PROPERTIES property mapper. It'd always append the targetServerType to the URL props. However, there's an issue because users would not be able to override it, which is not the right way IMO. Maybe the best approach would be warn them (as you do) if they use the DB URL props with Postgres and the prop is missing.

@vmuzikar @shawkins Or any other opinion?

@shawkins
Copy link
Contributor

When someone passes in a JDBC URL, there is no way good way I found intercept this to add a suffix.

@mabartos @ahus1 there is general quarkus mechanism via: quarkus.datasource.jdbc.additional-jdbc-properties.targetServerType=primary - but setting that only for pg would require some new logic, or less ideal it would have a side effect (setting a system property) of the db-kind propertymapper.

@vmuzikar
Copy link
Contributor

Using quarkus.datasource.jdbc.additional-jdbc-properties.targetServerType seems like the right way to do. We'd have a hidden option for that in order to hook up a transformer that will look at the db-kind option.

@ahus1 ahus1 force-pushed the is-40383-connect-to-writer-instance-automatically branch from 411f85a to 24c1183 Compare June 20, 2025 18:51
@ahus1
Copy link
Contributor Author
ahus1 commented Jun 20, 2025

I have a Quarkus Unit test KeycloakPathConfigurationTest#testAuthEndpointAvailable failing with

SRCFG00014: The config property quarkus.datasource.jdbc.additional-jdbc-properties.targetServerType is required but it could not be found in any config source

I did some debugging but without success. I would be happy for a pointer from the @keycloak/cloud-native team.

@shawkins
Copy link
Contributor

I did some debugging but without success. I would be happy for a pointer from the @keycloak/cloud-native team.

This is due to it being a quarkus test - the test logic is performing a quarkus build, not our build command.

Then because the option has a default - and it is currently forced to by this code

keycloak/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java

Line 149 in f4d5fa6

defaultValue = Optional.of((T) Boolean.FALSE);
- the logic in PropertyMappingInterceptor will advertise the property name to the name iterator. This is needed so that quarkus knows about it when doing it's own property mapping.

From there it does not have a value because it will map to a null value.

Not sure if there's a great way to inhibit the quarkus build, or not contribute our interceptors / configsources which are coming in from classpath mechanisms.

@ahus1 ahus1 force-pushed the is-40383-connect-to-writer-instance-automatically branch from afe12dd to ac7e1ff Compare June 26, 2025 13:52
@ahus1
Copy link
Contributor Author
ahus1 commented Jun 26, 2025

@shawkins, thank you for the pointer. I changed the property to a String without a default, and then that one unit test passed. Let's wait for the full CI results.

@ahus1 ahus1 marked this pull request as ready for review June 26, 2025 14:59
@ahus1 ahus1 requested review from a team as code owners June 26, 2025 14:59
@ahus1
Copy link
Contributor Author
ahus1 commented Jun 26, 2025

Build is now green, looking for a @keycloak/cloud-native team member to review when you have the time. Thanks!

vmuzikar
vmuzikar requested changes Jun 27, 2025
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahus1 Thanks for the PR.

mabartos
mabartos reviewed Jun 27, 2025
View reviewed changes
Copy link
Contributor
@mabartos mabartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few things to improve it.

mabartos
mabartos reviewed Jun 27, 2025
View reviewed changes
@ahus1 ahus1 force-pushed the is-40383-connect-to-writer-instance-automatically branch from 2d7c5cb to d406837 Compare July 2, 2025 19:33
@ahus1
Copy link
Contributor Author
ahus1 commented Jul 2, 2025

@mabartos - your review comments should now be addressed. Please re-review when you have the time.

@ahus1 ahus1 requested a review from mabartos July 2, 2025 19:34
keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jul 2, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cluster.SessionFailoverClusterTest#sessionFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

mabartos
mabartos reviewed Jul 3, 2025
View reviewed changes
Copy link
Contributor
@mabartos mabartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahus1 LGTM in general, just a few "polishing" comments to have it fine :))

@ahus1 @vmuzikar @mabartos
Automatically connect to a writer instance of PostgreSQL
bb71905 
Closes keycloak#40383

Co-authored-by: Václav Muzikář <vaclav@muzikari.cz>
Co-authored-by: Martin Bartoš <mabartos@redhat.com>
Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
@ahus1 ahus1 force-pushed the is-40383-connect-to-writer-instance-automatically branch from 2da429b to bb71905 Compare July 4, 2025 11:58
@ahus1
Copy link
Contributor Author
ahus1 commented Jul 4, 2025

@mabartos - your comments have been addressed. Please re-review. Thanks!

@ahus1 ahus1 requested a review from mabartos July 4, 2025 11:58
mabartos
mabartos approved these changes Jul 4, 2025
View reviewed changes
Copy link
Contributor
@mabartos mabartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahus1 Nice, thanks! LGTM

@mabartos
Copy link
Contributor
mabartos commented Jul 4, 2025

@vmuzikar Do you want to do a final look?

vmuzikar
vmuzikar approved these changes Jul 4, 2025
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @ahus1!

@vmuzikar vmuzikar merged commit 05d0c34 into keycloak:main Jul 4, 2025
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@vmuzikar vmuzikar vmuzikar approved these changes

@mabartos mabartos mabartos approved these changes

Assignees

@ahus1 ahus1

Labels
flaky-test team/cloud-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KC should connect to a writer instance of PostgreSQL automatically
4 participants
@ahus1 @mabartos @shawkins @vmuzikar
0