Cross-site validation for lazy loading of offline sessions & Switch d… by martin-kanis · Pull Request #9414 · keycloak/keycloak

martin-kanis · 2022-01-06T15:17:54Z

…efault offline sessions to lazy loaded

This PR add test scenarios to model testsuite which will test offline sessions lazy loading (in Cross DC environment) and make offline sessions to be lazy loaded by default.

Closes #9409
Closes #9410

martin-kanis · 2022-01-17T15:00:22Z

Documentation + release notes PR keycloak/keycloak-documentation#1362

martin-kanis · 2022-01-17T15:18:28Z

Pipeline run:
https://master-jenkins.com/job/universal-test-pipeline-server/1167 No regression found. I got suspicious about oracle19cRAC / org.keycloak.testsuite.oauth.BackchannelLogoutTest.postBackchannelLogoutNestedBrokeringRevokeOfflineSessions but that doesn't seem to be related to my changes. The test passed in re-run https://master-jenkins.com/job/universal-test-pipeline-server/1172

martin-kanis · 2022-01-18T16:35:21Z

Link to GH discussion

mhajas

Thank you for the PR @martin-kanis! I added few comments/questions

mhajas

Thank you for the changes @martin-kanis. This looks good to me, I added one more idea though. Feel free to scratch it if you don't think it makes sense or it isn't possible. I am approving as it works this way too.

mhajas · 2022-01-31T12:05:33Z

One more idea to this, I am not sure this could work this way, but I believe it is worth trying since it could have better performance.

Suggested change

@NamedQuery(name="findClientSessionsClientIds", query="SELECT clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider" +

" FROM PersistentClientSessionEntity clientSess INNER JOIN PersistentUserSessionEntity sess ON clientSess.userSessionId = sess.userSessionId " +

" WHERE sess.offline = :offline AND sess.realmId = :realmId")

@NamedQuery(name="findClientSessionsClientIds", query="SELECT clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider, count(clientSess)" +

" FROM PersistentClientSessionEntity clientSess INNER JOIN PersistentUserSessionEntity sess ON clientSess.userSessionId = sess.userSessionId " +

" WHERE sess.offline = :offline AND sess.realmId = :realmId" +

"GROUP BY clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider")

This way we could get rid of this:

closing(query.getResultStream()) .map(row -> { String clientId = row[0].toString(); if (clientId.equals(PersistentClientSessionEntity.EXTERNAL)) { final String externalClientId = row[1].toString(); final String clientStorageProvider = row[2].toString(); clientId = new StorageId(clientStorageProvider, externalClientId).getId(); } return clientId; }) .sorted() .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));

and replace it with something like this:

closing(query.getResultStream()) .collect(Collectors.toMap(row -> { String clientId = row[0].toString(); if (clientId.equals(PersistentClientSessionEntity.EXTERNAL)) { final String externalClientId = row[1].toString(); final String clientStorageProvider = row[2].toString(); clientId = new StorageId(clientStorageProvider, externalClientId).getId(); } return clientId; }, row -> (Long) row[3])); // Not sure about the exact type of row[3] so I am not sure this will work

WDYT?

I think it might work. I'll double check it with some tests. But what about ordering? Do we care about order of the entries in the resulting map by final clientId? If yes, we probably need ordering in Java.

Hmm, this is a good question, I completely missed it. The previous method used HashMap for returning the result and HashMap does not preserve order so I believe the order is not needed. Also, the current order would be a little bit different as we are using StorageId that is producing id in the form f:......:....... which wasn't there before. Maybe we could get another opinion on this, whether some order is needed @hmlnarik? WDYT?

In case some order is needed, I believe it would be possible to use something like that: ORDER BY clientSess.clientId, clientSess.clientStorageProvider.

Yeah, we probably don't have to care about ordering as the return type is HashMap. The method is called only from AdminRealmResource where the map is again transformed and return type is Stream<Map<String, String>>.
I also extended one test in ClientStorageTest to test the query more.

mhajas

Thanks Martin, the changes look great!

…efault offline sessions to lazy loaded

martin-kanis · 2022-02-01T19:27:43Z

https://master-jenkins.com/job/universal-test-pipeline-server/1187
https://master-jenkins.com/job/universal-test-pipeline-adapters/863
There are some failures in DB matrix but I don't see them related to my changes in queries. There are no recent runs which could be compared to this run.

The adapter failure is suspicious:

13:43:01,844 WARN  [org.keycloak.events] (default task-6) type=REFRESH_TOKEN_ERROR, realmId=test, clientId=offline-client, userId=b32f301d-bf0f-4b36-bd47-516120a0d595, ipAddress=127.0.0.1, error=invalid_token, grant_type=refresh_token, refresh_token_type=Offline, refresh_token_id=ad219500-a74f-41f4-850b-81a2f061cfdf, client_auth_method=client-secret
2022-02-01 14:43:03  13:43:01,848 ERROR [org.keycloak.adapters.RefreshableKeycloakSecurityContext] (http-127.0.0.1:8280-1) Refresh token failure status: 400 {"error":"invalid_grant","error_description":"Offline user session not found"}

But it can be also found in an older run https://master-jenkins.com/job/universal-test-pipeline-adapters/857/execution/node/932/log/?consoleFull so it's probably not a regression.

hmlnarik · 2022-02-03T14:59:58Z

Rerun pipelines:

hmlnarik

Well done, @martin-kanis !

martin-kanis force-pushed the 9409 branch 4 times, most recently from a4a4478 to b244987 Compare January 11, 2022 16:55

hmlnarik added area/storage Indicates an issue that touches storage (change in data layout or data manipulation) area/infinispan labels Jan 13, 2022

hmlnarik self-assigned this Jan 13, 2022

martin-kanis force-pushed the 9409 branch 2 times, most recently from 9c00184 to 4eb70f7 Compare January 14, 2022 09:24

martin-kanis mentioned this pull request Jan 17, 2022

Switch default offline sessions to lazy loaded keycloak/keycloak-documentation#1362

Merged

hmlnarik requested a review from mhajas January 17, 2022 20:21

hmlnarik added this to the 17.0.0 milestone Jan 17, 2022

hmlnarik added the priority/important Must be worked on very soon label Jan 17, 2022

hmlnarik reviewed Jan 17, 2022

View reviewed changes

Comment thread ...pan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java Outdated

hmlnarik reviewed Jan 17, 2022

View reviewed changes

Comment thread model/jpa/src/main/java/org/keycloak/models/jpa/session/PersistentUserSessionEntity.java Outdated

martin-kanis force-pushed the 9409 branch from 4eb70f7 to 018b03d Compare January 18, 2022 08:35

mhajas reviewed Jan 20, 2022

View reviewed changes

martin-kanis force-pushed the 9409 branch 4 times, most recently from 6b64cd5 to 1c01acf Compare January 31, 2022 08:58

mhajas previously approved these changes Jan 31, 2022

View reviewed changes

martin-kanis dismissed mhajas’s stale review via 8c74fbb January 31, 2022 18:42

martin-kanis force-pushed the 9409 branch 4 times, most recently from 68a6b7e to 83150a8 Compare February 1, 2022 06:58

mhajas previously approved these changes Feb 1, 2022

View reviewed changes

Cross-site validation for lazy loading of offline sessions & Switch d…

50c7078

…efault offline sessions to lazy loaded

martin-kanis dismissed mhajas’s stale review via 50c7078 February 1, 2022 10:41

martin-kanis force-pushed the 9409 branch from 83150a8 to 50c7078 Compare February 1, 2022 10:41

mhajas approved these changes Feb 1, 2022

View reviewed changes

hmlnarik approved these changes Feb 3, 2022

View reviewed changes

hmlnarik merged commit 0471ec4 into keycloak:main Feb 3, 2022

stianst mentioned this pull request Feb 4, 2022

Disable pre-loading offline sessions by default #9135

Closed

-        @NamedQuery(name="findClientSessionsClientIds", query="SELECT clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider" +
-                " FROM PersistentClientSessionEntity clientSess INNER JOIN PersistentUserSessionEntity sess ON clientSess.userSessionId = sess.userSessionId " +
-                " WHERE sess.offline = :offline AND sess.realmId = :realmId")
+        @NamedQuery(name="findClientSessionsClientIds", query="SELECT clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider, count(clientSess)" +
+                " FROM PersistentClientSessionEntity clientSess INNER JOIN PersistentUserSessionEntity sess ON clientSess.userSessionId = sess.userSessionId " +
+                " WHERE sess.offline = :offline AND sess.realmId = :realmId" +
+                "GROUP BY clientSess.clientId, clientSess.externalClientId, clientSess.clientStorageProvider")

Conversation

martin-kanis commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-kanis commented Jan 17, 2022

Uh oh!

martin-kanis commented Jan 17, 2022

Uh oh!

Uh oh!

Uh oh!

martin-kanis commented Jan 18, 2022

Uh oh!

mhajas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhajas left a comment

Choose a reason for hiding this comment

Uh oh!

mhajas Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

martin-kanis Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

mhajas Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

martin-kanis Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

mhajas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-kanis commented Feb 1, 2022

Uh oh!

hmlnarik commented Feb 3, 2022

Uh oh!

hmlnarik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martin-kanis commented Jan 6, 2022 •

edited

Loading

mhajas left a comment •

edited

Loading