Disable cache store only if a remote store is used #24774

ahus1 · 2023-11-15T14:41:22Z

Disclaimer: The primary purpose of this is to not stand in the way of people experimenting with persistence in Infinispan's XML. This hasn't been performance tested, and is not a recommended setup. More changes are necessary to make this fully tested and supported, and we might look into different options to make persistence of sessions fully supported which might be incompatible from this one. It is not ideal (and can be problematic) as it uses one additional connection pool per cache, where it should probably reuse Quarkus' connection pool.

The following change will disable the writethrough/readthrough only when a remote store is used. Using a persistence and a remote store at the same time is not supported.

Closes #10803
Closes #24766

ahus1 · 2023-11-15T14:50:23Z

I've tested this with the following configuration. This is still experimental and not supported, as it uses for example a separate connection pool for each cache. Ideally it should re-use Quarkus' connection pool. Further changes will be necessary to support this in a better way.

<?xml version="1.0" encoding="UTF-8"?>

<infinispan
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:schemaLocation="urn:infinispan:config:14.0 http://www.infinispan.org/schemas/infinispan-config-14.xsd urn:infinispan:config:store:jdbc:14.0 https://infinispan.org/schemas/infinispan-cachestore-jdbc-config-14.0.xsd"
        xmlns="urn:infinispan:config:14.0">

    <cache-container name="keycloak">
        <transport lock-timeout="60000" />
        <local-cache name="realms">
            <encoding>
                <key media-type="application/x-java-object"/>
                <value media-type="application/x-java-object"/>
            </encoding>
            <memory max-count="10000"/>
        </local-cache>
        <local-cache name="users">
            <encoding>
                <key media-type="application/x-java-object"/>
                <value media-type="application/x-java-object"/>
            </encoding>
            <memory max-count="10000"/>
        </local-cache>
        <distributed-cache name="sessions" owners="2">
            <expiration />
            <!-- TODO: Keycloak should ensure to use either remote store or a persistence -->
            <persistence passivation="false">
                <!-- consider preload to be true or false? customer decision -->
                <string-keyed-jdbc-store
                        xmlns="urn:infinispan:config:store:jdbc:14.0"
                        dialect="POSTGRES"
                        shared="true"
                        segmented="true"
                        purge="false"
                        preload="true">
                    <!-- TODO: inject the JDBC connection from Quarkus -->
                    <connection-pool
                            connection-url="jdbc:postgresql://localhost:5432/keycloak?ApplicationName=keycloak-ispn"
                            username="keycloak"
                            password="pass"
                            driver="org.postgresql.Driver"/>
                    <string-keyed-table create-on-start="true" fetch-size="10000" prefix="ispn">
                        <id-column name="id" type="VARCHAR(255)" />
                        <data-column name="datum" type="BYTEA" />
                        <timestamp-column name="timestamp" type="BIGINT" />
                        <segment-column name="segment" type="BIGINT" />
                    </string-keyed-table>
                </string-keyed-jdbc-store>
            </persistence>
        </distributed-cache>
        <distributed-cache name="clientSessions" owners="2">
            <expiration />
            <persistence passivation="false">
                <string-keyed-jdbc-store
                        xmlns="urn:infinispan:config:store:jdbc:14.0"
                        dialect="POSTGRES"
                        shared="true"
                        segmented="true"
                        purge="false"
                        preload="false">
                    <connection-pool
                            connection-url="jdbc:postgresql://localhost:5432/keycloak?ApplicationName=keycloak-ispn"
                            username="keycloak"
                            password="pass"
                            driver="org.postgresql.Driver"/>
                    <string-keyed-table create-on-start="true" fetch-size="10000" prefix="ispn">
                        <id-column name="id" type="VARCHAR(255)" />
                        <data-column name="datum" type="BYTEA" />
                        <timestamp-column name="timestamp" type="BIGINT" />
                        <segment-column name="segment" type="BIGINT" />
                    </string-keyed-table>
                </string-keyed-jdbc-store>
            </persistence>
        </distributed-cache>
        <distributed-cache name="authenticationSessions" owners="2">
            <expiration lifespan="-1"/>
        </distributed-cache>
        <distributed-cache name="offlineSessions" owners="2">
            <expiration lifespan="-1"/>
        </distributed-cache>
        <distributed-cache name="offlineClientSessions" owners="2">
            <expiration lifespan="-1"/>
        </distributed-cache>
        <distributed-cache name="loginFailures" owners="2">
            <expiration lifespan="-1"/>
        </distributed-cache>
        <local-cache name="authorization">
            <encoding>
                <key media-type="application/x-java-object"/>
                <value media-type="application/x-java-object"/>
            </encoding>
            <memory max-count="10000"/>
        </local-cache>
        <replicated-cache name="work">
            <expiration lifespan="-1"/>
        </replicated-cache>
        <local-cache name="keys">
            <encoding>
                <key media-type="application/x-java-object"/>
                <value media-type="application/x-java-object"/>
            </encoding>
            <expiration max-idle="3600000"/>
            <memory max-count="1000"/>
        </local-cache>
        <distributed-cache name="actionTokens" owners="2">
            <encoding>
                <key media-type="application/x-java-object"/>
                <value media-type="application/x-java-object"/>
            </encoding>
            <expiration max-idle="-1" lifespan="-1" interval="300000"/>
            <memory max-count="-1"/>
        </distributed-cache>
    </cache-container>
</infinispan>

Two additional JARs are needed:

curl -f https://repo1.maven.org/maven2/org/infinispan/infinispan-cachestore-jdbc/14.0.19.Final/infinispan-cachestore-jdbc-14.0.19.Final.jar -o providers/infinispan-cachestore-jdbc-14.0.19.Final.jar
curl -f https://repo1.maven.org/maven2/org/infinispan/infinispan-cachestore-jdbc-common-jakarta/14.0.19.Final/infinispan-cachestore-jdbc-common-jakarta-14.0.19.Final.jar -o providers/infinispan-cachestore-jdbc-common-jakarta-14.0.19.Final.jar

This can then be started as follows:

bin/kc.sh --verbose start --http-enabled true --hostname-strict false --db postgres --db-password pass --db-username keycloak --cache-config-file cache-ispn-db.xml

Smoketests I performed: The user session of a logged in user survives a Keycloak restart. Listing of user sessions in the Keycloak UI works as well.

ghost

Unreported flaky test detected, please review

ghost · 2023-11-15T15:48:45Z

Unreported flaky test detected

If the below 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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

ghost · 2023-11-15T16:47:00Z

Unreported flaky test detected

If the below 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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

ghost

Unreported flaky test detected, please review

martin-kanis

@ahus1 Thanks for the improvement. Looks good to me.

ahus1 · 2023-11-20T10:53:14Z

I've updated my Infinispan XML above to ensure that sessions are preloaded. This is required for listing sessions of a user / broker.

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

Closes keycloak#10803 Closes keycloak#24766 Signed-off-by: Alexander Schwartz <aschwart@redhat.com> Co-authored-by: daviddelannoy <16318239+daviddelannoy@users.noreply.github.com>

ahus1 · 2023-11-20T13:54:33Z

@martin-kanis - I've updated the PR after the comment of @daviddelannoy - could you please re-approve the change?

daviddelannoy · 2023-11-20T14:16:15Z

I've updated my Infinispan XML above to ensure that sessions are preloaded. This is required for listing sessions of a user / broker.

@ahus1 you have updated to preload=true for the sessions cache only, but not for clientSessions cache. Is it on purpose ? Is it enough to preload user sessions to be able to list them ?

mhajas

Thank you @ahus1, I like this change. I agree we should not limit users that want to use persistence.

mhajas · 2023-11-20T17:13:18Z

I've updated my Infinispan XML above to ensure that sessions are preloaded. This is required for listing sessions of a user / broker.

@ahus1 you have updated to preload=true for the sessions cache only, but not for clientSessions cache. Is it on purpose ? Is it enough to preload user sessions to be able to list them ?

Thanks for bringing it up @daviddelannoy! I checked the code and if the reason for preloading entries is only listing entities it should be ok to preload user sessions only as we are not listing client sessions anywhere. We usually load client sessions only for a specific user session that contains references to the client sessions. @ahus1 Was not preloading client sessions intended or it was only a mistake?

ahus1 · 2023-11-20T17:22:17Z

@ahus1 Was not preloading client sessions intended or it was only a mistake?

This was only by mistake and lack of knowledge. Good to see you fixing this.

daviddelannoy · 2023-11-20T17:28:04Z

Thanks a lot for your help on this subject @ahus1 , @mhajas and @martin-kanis, it is really appreciated.

Issue #10803 can be closed too and that's a very good news !

ahus1 · 2023-11-20T17:49:59Z

Thank you to everyone who participated in this. I'm now merging this. To change the docs and the behavior of Keycloak, please consider creating pull requests. Thank you for @miroRucka for creating the first PR here.

At the same time, the the store team at Keycloak might be focusing on other issues like performance, bugs and new features, so our time might be limited. FYI: PRs usually get more attention than feature requests.

miroRucka · 2023-11-21T08:10:50Z

Thank you @ahus1 for promptly addressing the issue, we will test it with the recommended setting.

ahus1 · 2023-11-21T12:05:38Z

As this has been merged yesterday, it is part of tonights nightly Keycloak build for everyone to try.

https://github.com/keycloak/keycloak/releases/tag/nightly

daviddelannoy · 2023-11-21T17:25:45Z

@ahus1 it works fine for me with nightly build !

ahus1 · 2023-11-22T09:00:23Z

@daviddelannoy - thank you for testing this, this is always appreciated!

ahus1 self-assigned this Nov 15, 2023

This was referenced Nov 15, 2023

The fix solves the problem when integrating on the infinispan jdbc pe… #24248

Closed

Persist session in JDBC store without using external infinispan cluster #10803

Closed

ghost added the flaky-test label Nov 15, 2023

This was referenced Nov 15, 2023

Flaky test: org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createGroup #24337

Closed

Flaky test: org.keycloak.testsuite.webauthn.account.WebAuthnSigningInTest#availableAuthenticatorsAfterRemove #21785

Closed

ghost reviewed Nov 15, 2023

View reviewed changes

martin-kanis self-requested a review November 16, 2023 10:41

ahus1 marked this pull request as ready for review November 16, 2023 16:56

ahus1 requested a review from a team November 16, 2023 16:56

ahus1 requested a review from a team as a code owner November 16, 2023 16:56

ghost added the team/store label Nov 16, 2023

daviddelannoy mentioned this pull request Nov 17, 2023

fix(#61) Haproxy database ispn upgrade 22.0.5 thomasdarimont/keycloak-project-example#62

Open

martin-kanis approved these changes Nov 20, 2023

View reviewed changes

daviddelannoy reviewed Nov 20, 2023

View reviewed changes

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/CacheDecorators.java Outdated Show resolved Hide resolved

ahus1 force-pushed the is-10803-conditional-no-store-no-load-on-remote-store branch 2 times, most recently from 1b9453a to 72a799b Compare November 20, 2023 13:47

Disable cache store and load only if a remote store is used

2b437b7

Closes keycloak#10803 Closes keycloak#24766 Signed-off-by: Alexander Schwartz <aschwart@redhat.com> Co-authored-by: daviddelannoy <16318239+daviddelannoy@users.noreply.github.com>

ahus1 force-pushed the is-10803-conditional-no-store-no-load-on-remote-store branch from 72a799b to 2b437b7 Compare November 20, 2023 13:48

ghost mentioned this pull request Nov 20, 2023

Flaky test: org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordLink #21422

Closed

mhajas changed the title ~~Disable cache store and load only if a remote store is used~~ Disable cache store only if a remote store is used Nov 20, 2023

mhajas approved these changes Nov 20, 2023

View reviewed changes

ahus1 merged commit a45934a into keycloak:main Nov 20, 2023
69 checks passed

ahus1 deleted the is-10803-conditional-no-store-no-load-on-remote-store branch March 4, 2024 10:01

daviddelannoy mentioned this pull request Apr 3, 2024

Valid refresh token gives error #14040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable cache store only if a remote store is used #24774

Disable cache store only if a remote store is used #24774

ahus1 commented Nov 15, 2023 •

edited

Loading

ahus1 commented Nov 15, 2023 •

edited

Loading

ghost left a comment

ghost commented Nov 15, 2023

ghost commented Nov 15, 2023

ghost left a comment

martin-kanis left a comment

ahus1 commented Nov 20, 2023

ahus1 commented Nov 20, 2023

daviddelannoy commented Nov 20, 2023

mhajas left a comment

mhajas commented Nov 20, 2023

ahus1 commented Nov 20, 2023

daviddelannoy commented Nov 20, 2023

ahus1 commented Nov 20, 2023

miroRucka commented Nov 21, 2023

ahus1 commented Nov 21, 2023

daviddelannoy commented Nov 21, 2023

ahus1 commented Nov 22, 2023

Disable cache store only if a remote store is used #24774

Disable cache store only if a remote store is used #24774

Conversation

ahus1 commented Nov 15, 2023 • edited Loading

ahus1 commented Nov 15, 2023 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

ghost commented Nov 15, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

ghost commented Nov 15, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

ghost left a comment

Choose a reason for hiding this comment

martin-kanis left a comment

Choose a reason for hiding this comment

ahus1 commented Nov 20, 2023

ahus1 commented Nov 20, 2023

daviddelannoy commented Nov 20, 2023

mhajas left a comment

Choose a reason for hiding this comment

mhajas commented Nov 20, 2023

ahus1 commented Nov 20, 2023

daviddelannoy commented Nov 20, 2023

ahus1 commented Nov 20, 2023

miroRucka commented Nov 21, 2023

ahus1 commented Nov 21, 2023

daviddelannoy commented Nov 21, 2023

ahus1 commented Nov 22, 2023

ahus1 commented Nov 15, 2023 •

edited

Loading

ahus1 commented Nov 15, 2023 •

edited

Loading