Infinispan Protostream Marshaller by pruivo · Pull Request #29474 · keycloak/keycloak

pruivo · 2024-05-13T10:37:38Z

I'm gathering earlier feedback and sharing how it will end up.
Pending tasks are:

Compute the final Protostream IDs. With Protostream IDs, it generates smaller payloads since it sends the IDs (Goole Protobuf varInt) instead of the message name.
Refactor events and predicates. I'm taking the opportunity to remove unused fields and duplicated code.
Migration from JBoss Marshalling to Infinispan Protostream
Re-add JBoss Marshalling to allow migration of sessions
Add clearing of caches on migration to KC25 - all entries in the caches will be gone
Documentation for the release notes

Out of scope: Existing entries in the session cache will be migrated in PR #29377

Migration

The migration path is under discussion and has the following assumptions:

Only user and client sessions will be migrated. All other data stored in distributed/replicated caches will be lost.
Embedded cache needs to have persistence configured. If only in-memory storage is configured, we assume that losing sessions during Keycloak major version upgrade is acceptable and expected.

The most appealing solution is to start a separate, non-clustered Infinispan CacheManager with JBoss-Marshalling configured (the CacheManager is limited to a single marshalled). This CacheManager will be used to read the old session data. Then, we have the following 2 scenarios

Persistent User Sessions feature is enabled. Using the JBoss-Marshalling CacheManager to read the sessions and store them in the database. The cache is clear at the end.
Without Persistent User Session feature, it reads the sessions from the JBoss-Marshalling CacheManager and writes them in the Protostream enabled CacheManager.

ryanemerson

@pruivo Do you plan to add the proto compatibility check capabilities as part of this PR or should we create an issue to add that in a subsequent PR? I would be happy with the latter given how large the current PR is.

stianst · 2024-05-15T10:32:47Z

Please do not merge until we decide if this should be included in Keycloak 25, or shortly after the release

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-05-29T18:24:56Z

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.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

pruivo · 2024-06-03T17:37:21Z

Rebased and added documentation to

Release notes
Migration guide

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-06-03T18:34:17Z

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.oauth.TokenIntrospectionTest#testUnsupportedToken

Keycloak CI - Store IT

java.lang.NullPointerException: Cannot invoke "com.fasterxml.jackson.databind.JsonNode.asBoolean()" because the return value of "com.fasterxml.jackson.databind.JsonNode.get(String)" is null
	at org.keycloak.testsuite.oauth.TokenIntrospectionTest.testUnsupportedToken(TokenIntrospectionTest.java:313)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
...

Report flaky test

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (ubuntu-latest - temurin - 17)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken

Keycloak CI - Base IT (6)

java.lang.NullPointerException: Cannot invoke "com.fasterxml.jackson.databind.JsonNode.asBoolean()" because the return value of "com.fasterxml.jackson.databind.JsonNode.get(String)" is null
	at org.keycloak.testsuite.oauth.TokenIntrospectionTest.testUnsupportedToken(TokenIntrospectionTest.java:313)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-06-04T08:56:15Z

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.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

ahus1

Thank you for the PR! Found a typo in the include for the release notes. Used the opportunity to remove some now unused code in the PersitentUserSessionProvider.

pruivo · 2024-06-11T14:58:03Z

Thank you for the PR! Found a typo in the include for the release notes. Used the opportunity to remove some now unused code in the PersitentUserSessionProvider.

I didn't remove that method because of the javadoc: This method is public so people can use it to build their custom migrations or re-import sessions when necessary in a future version of Keycloak.

Isn't useful anymore?

ahus1 · 2024-06-11T15:05:31Z

Isn't useful anymore?

With the change of the serialization format, it IMHO became less useful. But ok, let's keep it for now. I realize I only deleted half of it anyway :-(

ryanemerson

Great work @pruivo, just a few minor things I have noticed so far.

ryanemerson · 2024-06-11T15:50:21Z

None of the IDs defined above are used anywhere in the code according to Intellij.

The modules where they are used do not depend on this one. The comments tell where they are used.

pruivo · 2024-06-11T17:04:34Z

Added a new commit with @ryanemerson suggestions 👍

Closes keycloak#29394 Signed-off-by: Pedro Ruivo <pruivo@redhat.com> Co-authored-by: Alexander Schwartz <aschwart@redhat.com>

pruivo · 2024-06-13T08:27:06Z

Rebased and squashed the commits.

I suspect that CacheInitializer#loadSessions() is running concurrently with UserSessionProvider#migrate(). The migrate method will clear the cache and remove the InitializerState preventing nodes to start. I couldn't get any logs from GitHub Actions to confirm this.

I changed how the migration works and moved the code to DefaultInfinispanConnectionProvider to avoid code duplication. It no longers clears the in-memory data (assumes there isn't a mix of KC25 and KC26 servers in the same cluster) but only from persistence (which is the only place where the data is stored in binary).

cc @ahus1 @mhajas @ryanemerson

mhajas

Thank you very much @pruivo! This was quite a journey. Let's see what the rest of it will look like! It looks good to me.

pruivo force-pushed the t_protostream_ispn15 branch 2 times, most recently from 81b9388 to 81f1ba0 Compare May 14, 2024 13:17

ryanemerson reviewed May 14, 2024

View reviewed changes

Comment thread ...n/src/main/java/org/keycloak/models/cache/infinispan/authorization/events/ResourceEvent.java Resolved

Comment thread ...nispan/src/main/java/org/keycloak/models/cache/infinispan/events/TypedInvalidationEvent.java Resolved

mhajas self-requested a review May 14, 2024 14:27

pruivo force-pushed the t_protostream_ispn15 branch from 81f1ba0 to 70caa5f Compare May 14, 2024 15:41

keycloak-github-bot Bot added the flaky-test label May 14, 2024

keycloak-github-bot Bot mentioned this pull request May 14, 2024

Flaky test: org.keycloak.testsuite.forms.RecoveryAuthnCodesAuthenticatorTest#test03AuthenticateRecoveryAuthnCodes #26176

Closed

pruivo force-pushed the t_protostream_ispn15 branch 2 times, most recently from 8e8b859 to 25c5cb1 Compare May 14, 2024 20:29

stianst added the status/hold PR should not be merged. On hold for later. label May 15, 2024

pruivo force-pushed the t_protostream_ispn15 branch from 25c5cb1 to d3bf888 Compare May 15, 2024 13:01

This was referenced May 15, 2024

Flaky test: org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore #25204

Closed

Flaky test: org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile #25674

Closed

pruivo force-pushed the t_protostream_ispn15 branch from d3bf888 to b716502 Compare May 29, 2024 17:13

keycloak-github-bot Bot reviewed May 29, 2024

View reviewed changes

pruivo force-pushed the t_protostream_ispn15 branch from b716502 to c91033c Compare June 3, 2024 17:26

keycloak-github-bot Bot reviewed Jun 3, 2024

View reviewed changes

pruivo force-pushed the t_protostream_ispn15 branch from c91033c to 8d26723 Compare June 4, 2024 07:28

keycloak-github-bot Bot mentioned this pull request Jun 4, 2024

Flaky test: org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken #30111

Closed

keycloak-github-bot Bot reviewed Jun 4, 2024

View reviewed changes

ryanemerson reviewed Jun 4, 2024

View reviewed changes

Comment thread docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc Resolved

pruivo force-pushed the t_protostream_ispn15 branch 2 times, most recently from ac26393 to 8e8a6f7 Compare June 6, 2024 10:48

keycloak-github-bot Bot mentioned this pull request Jun 6, 2024

Flaky test: org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode #28952

Closed

ahus1 self-assigned this Jun 6, 2024

ahus1 previously approved these changes Jun 11, 2024

View reviewed changes

ahus1 enabled auto-merge (squash) June 11, 2024 14:46

ahus1 dismissed their stale review via b5d3a88 June 11, 2024 14:50

ahus1 previously approved these changes Jun 11, 2024

View reviewed changes

ahus1 dismissed their stale review via d2a1573 June 11, 2024 15:05

ahus1 force-pushed the t_protostream_ispn15 branch from b5d3a88 to d2a1573 Compare June 11, 2024 15:05

ahus1 previously approved these changes Jun 11, 2024

View reviewed changes

auto-merge was automatically disabled June 11, 2024 15:38
Head branch was pushed to by a user without write access

pruivo dismissed ahus1’s stale review via b9788fc June 11, 2024 15:38

ahus1 force-pushed the t_protostream_ispn15 branch from b9788fc to 652cb51 Compare June 11, 2024 15:48

ahus1 enabled auto-merge (squash) June 11, 2024 15:48

ahus1 previously approved these changes Jun 11, 2024

View reviewed changes

ryanemerson reviewed Jun 11, 2024

View reviewed changes

ahus1 dismissed their stale review via 8cced17 June 11, 2024 17:02

This was referenced Jun 11, 2024

Flaky test: org.keycloak.testsuite.model.user.UserModelTest#testAddRemoveUsersInTheSameGroupConcurrent #30264

Closed

Flaky test: org.keycloak.testsuite.model.DBLockTest#testTwoLocksCurrently #25794

Closed

auto-merge was automatically disabled June 11, 2024 17:04
Head branch was pushed to by a user without write access

keycloak-github-bot Bot mentioned this pull request Jun 11, 2024

Flaky test: org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled #30188

Closed

pruivo force-pushed the t_protostream_ispn15 branch from 16846bd to ed400fe Compare June 12, 2024 12:06

Infinispan Protostream Marshaller

25e3dfd

Closes keycloak#29394 Signed-off-by: Pedro Ruivo <pruivo@redhat.com> Co-authored-by: Alexander Schwartz <aschwart@redhat.com>

pruivo force-pushed the t_protostream_ispn15 branch from 3218aff to 25e3dfd Compare June 13, 2024 08:20

mhajas approved these changes Jun 13, 2024

View reviewed changes

mhajas merged commit 18a6c79 into keycloak:main Jun 13, 2024

pruivo deleted the t_protostream_ispn15 branch June 13, 2024 16:28

GeorgEchterling mentioned this pull request Oct 10, 2024

Client Scope updates are not replicated on a distributed keycloak setup in kubernetes #33731

Closed

2 tasks done

shawkins mentioned this pull request Aug 9, 2025

keycloak warns about: [ISPN000554] jboss-marshalling is deprecated and planned for removal #17362

Closed

Conversation

Uh oh!

Migration

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants