Implement CompatibilityMetadataProvider for Cache CLI args #41163

ryanemerson · 2025-07-15T11:11:54Z

Requires #41027

A limitation with the current implementation is that if a default value is not set for a CLI arg then explicitly setting the value will be classes as "incompatible" even though the semantics are the same. For example, the default cache configuration uses the "jdbc-ping" stack, however --cache-stack jdbc-ping is not set. So if the user executes the update command with --cache-stack jdbc-ping explicitly configured then a RECREATE exit code will be returned by the UpdateCompatibility command.

Given that we state the following in the docs, ^ limitation could be considered acceptable:

Ensure that all configuration options, whether set via environment variables or CLI arguments, are included when running this command.

keycloak-github-bot · 2025-07-15T12:02:02Z

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.federation.kerberos.KerberosLdapCrossRealmTrustTest#test03SpnegoLoginUsernamePassword

Keycloak CI - Base IT (5)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.11.0-1018-azure', java.version: '21.0.7'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

org.keycloak.testsuite.federation.ldap.LDAPReadOnlyTest#testReadOnlyUserGetsPermanentlyLocked

Keycloak CI - Base IT (5)

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at org.junit.Assert.assertFalse(Assert.java:75)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

pruivo

Overall, it looks good to me. I left some suggestions anticipating some future work around the SPI options.

pruivo · 2025-07-16T10:52:22Z

...finispan/src/main/java/org/keycloak/compatibility/AbstractCompatibilityMetadataProvider.java

+    public AbstractCompatibilityMetadataProvider(String spi, boolean enabled) {
+        this.spi = spi;
+        this.enabled = enabled;
+        this.config = enabled ? Config.scope(spi, "default") : null;


Not all providers have the ID default. I would add the providerId as a parameter to make this class future-proof.

For example, this factory has SPI optoins and the provider ID is not default:

keycloak/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProviderFactory.java

Line 313 in 9322d71

return InfinispanUtils.EMBEDDED_PROVIDER_ID;

Sure. I was undecided about this as we don't require it yet, but no harm to future proof.

pruivo · 2025-07-16T10:54:17Z

...finispan/src/main/java/org/keycloak/compatibility/AbstractCompatibilityMetadataProvider.java

+
+    @Override
+    public Map<String, String> metadata() {
+        if (!enabled)


I would remove the enabled field and create an abstract method isEnabled(Config.Scope).

The provider factory API has the Config.Scope as a parameter; it can use an SPI option/configuration to decide if it is enabled or not.

keycloak/server-spi-private/src/main/java/org/keycloak/provider/EnvironmentDependentProviderFactory.java

Lines 30 to 36 in 3e0a185

/**

* Check if the provider is supported and should be available based on the provider configuration.

*

* @param config the provider configuration

* @return {@code true} if the provider is supported. Otherwise, {@code false}.

*/

boolean isSupported(Config.Scope config);

pruivo · 2025-07-16T10:55:06Z

...pan/src/main/java/org/keycloak/infinispan/compatibility/CachingEmbeddedMetadataProvider.java

+    @Override
+    public Map<String, String> meta() {
+        return Map.of(
+              "persistence", Boolean.toString(MultiSiteUtils.isPersistentSessionsEnabled()),


It can be removed as the Feature is already tracked.

Good point, I hadn't considered that

pruivo · 2025-07-16T10:56:17Z

...ispan/src/main/java/org/keycloak/infinispan/compatibility/CachingRemoteMetadataProvider.java

+
+    @Override
+    public Map<String, String> meta() {
+        return Map.of("persistence", Boolean.toString(MultiSiteUtils.isPersistentSessionsEnabled()));


hmm, I think we can remove it here too.

pruivo · 2025-07-16T10:58:31Z

...finispan/src/main/java/org/keycloak/compatibility/AbstractCompatibilityMetadataProvider.java

+        return spi;
+    }
+
+    protected Map<String, String> meta() {


nitpicking

Suggested change

protected Map<String, String> meta() {

protected Map<String, String> customMetadata() {

pruivo · 2025-07-16T11:02:08Z

...finispan/src/main/java/org/keycloak/compatibility/AbstractCompatibilityMetadataProvider.java

+
+    @Override
+    public String getId() {
+        return spi;


I'm not sure if this should be a combination of SPI and the Provider ID. For example, InfinispanUserSessionProviderFactory and RemoteUserSessionProviderFactory both implement the same SPI.
But let's leave it as is for now.

Closes keycloak#41138 Signed-off-by: Ryan Emerson <remerson@redhat.com>

…ider Signed-off-by: Ryan Emerson <remerson@redhat.com>

Signed-off-by: Ryan Emerson <remerson@redhat.com>

…ompatibilityMetadataProvider Signed-off-by: Ryan Emerson <remerson@redhat.com>

Signed-off-by: Ryan Emerson <remerson@redhat.com>

ryanemerson · 2025-07-16T15:36:26Z

Thanks for the review @pruivo, I believe I have addressed all comments.

ahus1

Approving based on previous reviews

keycloak-github-bot bot added the flaky-test label Jul 15, 2025

keycloak-github-bot bot reviewed Jul 15, 2025

View reviewed changes

pruivo reviewed Jul 16, 2025

View reviewed changes

ryanemerson added 6 commits July 16, 2025 15:59

Implement CompatibilityMetadataProvider for Cache CLI args

c1c4558

Closes keycloak#41138 Signed-off-by: Ryan Emerson <remerson@redhat.com>

Add Provider id as a ctor param for AbstractCompatibilityMetadataProv…

5d184f0

…ider Signed-off-by: Ryan Emerson <remerson@redhat.com>

Add isEnabled method to AbstractCompatibilityMetadataProvider

ff900a6

Signed-off-by: Ryan Emerson <remerson@redhat.com>

s/meta/customMeta

57231a5

Signed-off-by: Ryan Emerson <remerson@redhat.com>

Remove 'persistence' element from metadata as now tracked by FeatureC…

e49e3c4

…ompatibilityMetadataProvider Signed-off-by: Ryan Emerson <remerson@redhat.com>

Add test for adding a new Feature

a23a7e5

Signed-off-by: Ryan Emerson <remerson@redhat.com>

ryanemerson force-pushed the 41138/cache_cli_option_compatibility_metadata_provider branch from 2e4811a to a23a7e5 Compare July 16, 2025 15:36

ryanemerson marked this pull request as ready for review July 16, 2025 15:36

ryanemerson requested review from a team as code owners July 16, 2025 15:36

keycloak-github-bot bot added team/cloud-native team/sre labels Jul 16, 2025

ahus1 requested a review from pruivo July 16, 2025 16:02

pruivo approved these changes Jul 16, 2025

View reviewed changes

ahus1 approved these changes Jul 16, 2025

View reviewed changes

ahus1 merged commit 4bb0230 into keycloak:main Jul 16, 2025
81 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement CompatibilityMetadataProvider for Cache CLI args #41163

Implement CompatibilityMetadataProvider for Cache CLI args #41163

Uh oh!

ryanemerson commented Jul 15, 2025 •

edited

Loading

Uh oh!

keycloak-github-bot bot commented Jul 15, 2025

Uh oh!

keycloak-github-bot bot left a comment

Uh oh!

pruivo left a comment

Uh oh!

pruivo Jul 16, 2025

Uh oh!

ryanemerson Jul 16, 2025

Uh oh!

pruivo Jul 16, 2025

Uh oh!

pruivo Jul 16, 2025

Uh oh!

ryanemerson Jul 16, 2025

Uh oh!

pruivo Jul 16, 2025

Uh oh!

pruivo Jul 16, 2025

Uh oh!

pruivo Jul 16, 2025

Uh oh!

ryanemerson commented Jul 16, 2025

Uh oh!

ahus1 left a comment

Uh oh!

Uh oh!

Uh oh!

	/**
	* Check if the provider is supported and should be available based on the provider configuration.
	*
	* @param config the provider configuration
	* @return {@code true} if the provider is supported. Otherwise, {@code false}.
	*/
	boolean isSupported(Config.Scope config);

	protected Map<String, String> meta() {
	protected Map<String, String> customMetadata() {

Implement CompatibilityMetadataProvider for Cache CLI args #41163

Implement CompatibilityMetadataProvider for Cache CLI args #41163

Uh oh!

Conversation

ryanemerson commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keycloak-github-bot bot commented Jul 15, 2025

Unreported flaky test detected

org.keycloak.testsuite.federation.kerberos.KerberosLdapCrossRealmTrustTest#test03SpnegoLoginUsernamePassword

org.keycloak.testsuite.federation.ldap.LDAPReadOnlyTest#testReadOnlyUserGetsPermanentlyLocked

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

pruivo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanemerson commented Jul 16, 2025

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanemerson commented Jul 15, 2025 •

edited

Loading