Skip to content

Commit 1850612

Browse files
Fix ClassCastException on user-session lookups in Redis cluster mode (p2-inc#77)
UnifiedJedis.pipelined() returns a ClusterPipeline in cluster mode, a sibling of Pipeline (both extend AbstractPipeline). The (Pipeline) cast in getUserSessionsStreamByIndexKey threw ClassCastException, which is swallowed, so by-index user-session lookups silently returned an empty stream in cluster mode. Program to AbstractPipeline instead, matching the existing pattern in RedisChangelogTransaction. Adds RedisClusterUserSessionProviderModelTest (single-node Valkey cluster) reproducing the failure and verifying the fix.
1 parent 39de112 commit 1850612

2 files changed

Lines changed: 239 additions & 2 deletions

File tree

src/main/java/io/phasetwo/keycloak/redis/userSession/RedisUserSessionProvider.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
import org.keycloak.device.DeviceActivityManager;
2323
import org.keycloak.models.*;
2424
import org.keycloak.models.utils.KeycloakModelUtils;
25-
import redis.clients.jedis.Pipeline;
25+
import redis.clients.jedis.AbstractPipeline;
2626
import redis.clients.jedis.Response;
2727
import redis.clients.jedis.UnifiedJedis;
2828

@@ -262,7 +262,7 @@ private Stream<RedisUserSessionAdapter> getUserSessionsStreamByIndexKey(
262262
private Stream<RedisUserSessionAdapter> getUserSessionsStreamByIndexKey(
263263
String[] indexKeys, RealmModel realm, boolean offline) {
264264
log.tracef("[redis] SMEMBERS %s", indexKeys);
265-
try (Pipeline pipeline = (Pipeline) jedis.pipelined()) {
265+
try (AbstractPipeline pipeline = jedis.pipelined()) {
266266
List<Response<Set<String>>> responses = Lists.newArrayList();
267267

268268
for (String indexKey : indexKeys) {
Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
/*
2+
* Copyright 2023 IT-Systemhaus der Bundesagentur fuer Arbeit
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.phasetwo.keycloak.redis.testsuite.session;
18+
19+
import static io.phasetwo.keycloak.redis.testsuite.session.SessionTestUtils.createClients;
20+
import static org.hamcrest.MatcherAssert.assertThat;
21+
import static org.hamcrest.Matchers.contains;
22+
import static org.hamcrest.Matchers.hasSize;
23+
24+
import io.phasetwo.keycloak.redis.RedisHashCas;
25+
import io.phasetwo.keycloak.redis.connection.RedisMode;
26+
import io.phasetwo.keycloak.redis.testsuite.KeycloakModelTest;
27+
import io.phasetwo.keycloak.redis.userSession.RedisUserSessionProvider;
28+
import java.net.ServerSocket;
29+
import java.util.List;
30+
import java.util.Set;
31+
import java.util.stream.Collectors;
32+
import org.junit.AfterClass;
33+
import org.junit.Assert;
34+
import org.junit.BeforeClass;
35+
import org.junit.Test;
36+
import org.keycloak.models.Constants;
37+
import org.keycloak.models.RealmModel;
38+
import org.keycloak.models.UserModel;
39+
import org.keycloak.models.UserSessionModel;
40+
import org.testcontainers.containers.Container;
41+
import org.testcontainers.containers.FixedHostPortGenericContainer;
42+
import org.testcontainers.containers.wait.strategy.Wait;
43+
import redis.clients.jedis.DefaultJedisClientConfig;
44+
import redis.clients.jedis.HostAndPort;
45+
import redis.clients.jedis.JedisClientConfig;
46+
import redis.clients.jedis.RedisClusterClient;
47+
import redis.clients.jedis.UnifiedJedis;
48+
49+
/**
50+
* Reproduces (RED) and proves the fix (GREEN) for the Redis <b>cluster-mode</b> pipeline {@code
51+
* ClassCastException}.
52+
*
53+
* <p>In cluster mode {@code UnifiedJedis.pipelined()} returns a {@link
54+
* redis.clients.jedis.ClusterPipeline}, a sibling of {@link redis.clients.jedis.Pipeline} (both
55+
* extend {@code AbstractPipeline}). The pre-fix code in {@code
56+
* RedisUserSessionProvider.getUserSessionsStreamByIndexKey} cast the pipeline to {@code Pipeline},
57+
* throwing a {@code ClassCastException} that is caught and swallowed. The visible effect: every
58+
* by-index user-session lookup silently returns an empty stream in cluster mode (get-sessions-by-
59+
* user, broker lookups, active-client-session stats, ...), while single-node/standalone mode is
60+
* unaffected because it yields a real {@code Pipeline}.
61+
*
62+
* <p>This test drives the <em>real</em> {@link RedisUserSessionProvider} against a genuine
63+
* cluster-mode Jedis client, so {@code .pipelined()} really returns a {@code ClusterPipeline}. It
64+
* writes a user session through the provider (the write path already handles cluster mode) and then
65+
* reads it back via {@link RedisUserSessionProvider#getUserSessionsStream(RealmModel, UserModel)},
66+
* which routes through the buggy {@code getUserSessionsStreamByIndexKey} pipeline path.
67+
*
68+
* <p><b>Cluster strategy — deterministic single-node cluster.</b> Multi-node Redis clusters over
69+
* Testcontainers are notoriously flaky because the topology returned by {@code CLUSTER SLOTS}
70+
* advertises internal addresses/ports that do not match the randomly mapped host ports (MOVED
71+
* redirects then fail). We instead run a <em>single</em> Valkey node with {@code --cluster-enabled
72+
* yes}, bind it to a known fixed host port, and advertise that exact port via {@code
73+
* --cluster-announce-ip 127.0.0.1 --cluster-announce-port <port>}. All 16384 slots are assigned to
74+
* the one node, so {@code CLUSTER SLOTS} returns {@code 127.0.0.1:<port>} — an address the client
75+
* can always reach — and there are no cross-node MOVED redirects. A single-node cluster still
76+
* causes {@code UnifiedJedis.pipelined()} to return a {@code ClusterPipeline}, which is all that is
77+
* required to exercise the bug, making this the most robust (non-flaky) option.
78+
*/
79+
@SuppressWarnings("deprecation")
80+
public class RedisClusterUserSessionProviderModelTest extends KeycloakModelTest {
81+
82+
private static FixedHostPortGenericContainer<?> clusterContainer;
83+
private static UnifiedJedis clusterJedis;
84+
85+
private String realmId;
86+
87+
@BeforeClass
88+
public static void startCluster() throws Exception {
89+
int port = findFreePort();
90+
91+
clusterContainer =
92+
new FixedHostPortGenericContainer<>("valkey/valkey:8.1.5")
93+
.withFixedExposedPort(port, 6379)
94+
.withCommand(
95+
"valkey-server",
96+
"--port",
97+
"6379",
98+
"--cluster-enabled",
99+
"yes",
100+
"--cluster-node-timeout",
101+
"5000",
102+
"--appendonly",
103+
"no",
104+
"--protected-mode",
105+
"no",
106+
"--cluster-announce-ip",
107+
"127.0.0.1",
108+
"--cluster-announce-port",
109+
String.valueOf(port))
110+
.waitingFor(Wait.forLogMessage(".*Ready to accept connections.*", 1));
111+
clusterContainer.start();
112+
113+
// Assign every hash slot to the single node so the cluster reaches the 'ok' state.
114+
clusterContainer.execInContainer("valkey-cli", "cluster", "addslotsrange", "0", "16383");
115+
116+
// Wait for the cluster to report a healthy state before connecting.
117+
boolean ready = false;
118+
for (int i = 0; i < 40 && !ready; i++) {
119+
Container.ExecResult info = clusterContainer.execInContainer("valkey-cli", "cluster", "info");
120+
ready = info.getStdout().contains("cluster_state:ok");
121+
if (!ready) {
122+
Thread.sleep(500);
123+
}
124+
}
125+
if (!ready) {
126+
throw new IllegalStateException("Valkey cluster did not reach state 'ok' in time");
127+
}
128+
129+
JedisClientConfig clientConfig =
130+
DefaultJedisClientConfig.builder()
131+
.connectionTimeoutMillis(5000)
132+
.socketTimeoutMillis(5000)
133+
.build();
134+
clusterJedis =
135+
RedisClusterClient.builder()
136+
.nodes(Set.of(new HostAndPort("127.0.0.1", port)))
137+
.clientConfig(clientConfig)
138+
.build();
139+
140+
// Sanity check: topology is discovered and pipelining yields a genuine ClusterPipeline.
141+
Assert.assertTrue(
142+
"Expected a cluster-mode client whose pipeline is a ClusterPipeline",
143+
clusterJedis.pipelined() instanceof redis.clients.jedis.ClusterPipeline);
144+
145+
// Load the CAS Lua script onto the cluster node used by the provider's write path.
146+
RedisHashCas.initialize(clusterJedis);
147+
}
148+
149+
private static int findFreePort() throws Exception {
150+
try (ServerSocket socket = new ServerSocket(0)) {
151+
socket.setReuseAddress(true);
152+
return socket.getLocalPort();
153+
}
154+
}
155+
156+
@AfterClass
157+
public static void stopCluster() {
158+
if (clusterJedis != null) {
159+
clusterJedis.close();
160+
clusterJedis = null;
161+
}
162+
if (clusterContainer != null) {
163+
clusterContainer.stop();
164+
clusterContainer = null;
165+
}
166+
}
167+
168+
@Override
169+
public void createEnvironment(org.keycloak.models.KeycloakSession s) {
170+
RealmModel realm = createRealm(s, "test-cluster");
171+
s.getContext().setRealm(realm);
172+
173+
realm.setOfflineSessionIdleTimeout(Constants.DEFAULT_OFFLINE_SESSION_IDLE_TIMEOUT);
174+
realm.setDefaultRole(
175+
s.roles().addRealmRole(realm, Constants.DEFAULT_ROLES_ROLE_PREFIX + "-" + realm.getName()));
176+
realm.setSsoSessionIdleTimeout(1800);
177+
realm.setSsoSessionMaxLifespan(36000);
178+
realm.setClientSessionIdleTimeout(500);
179+
this.realmId = realm.getId();
180+
181+
s.users().addUser(realm, "user1").setEmail("user1@localhost");
182+
183+
createClients(s, realm);
184+
}
185+
186+
@Override
187+
public void cleanEnvironment(org.keycloak.models.KeycloakSession s) {
188+
RealmModel realm = s.realms().getRealm(realmId);
189+
s.getContext().setRealm(realm);
190+
s.realms().removeRealm(realmId);
191+
}
192+
193+
/**
194+
* Creates a user session through a real cluster-mode {@link RedisUserSessionProvider}, then reads
195+
* it back by user. The read routes through {@code getUserSessionsStreamByIndexKey}, whose
196+
* pipeline cast is the bug under test.
197+
*
198+
* <p>GREEN (with the fix): the session is found. RED (fix stashed): the swallowed {@code
199+
* ClassCastException} makes the lookup return an empty stream and the {@code hasSize(1)}
200+
* assertion fails.
201+
*/
202+
@Test
203+
public void testGetUserSessionsByUserInClusterMode() {
204+
// Write a user session through the provider; flushes to Redis on transaction commit.
205+
String sessionId =
206+
withRealm(
207+
realmId,
208+
(s, realm) -> {
209+
UserModel user = s.users().getUserByUsername(realm, "user1");
210+
RedisUserSessionProvider provider =
211+
new RedisUserSessionProvider(s, clusterJedis, RedisMode.CLUSTER);
212+
UserSessionModel userSession =
213+
provider.createUserSession(
214+
realm, user, "user1", "127.0.0.1", "form", true, null, null);
215+
return userSession.getId();
216+
});
217+
218+
// Read the session back by user in a fresh transaction / fresh provider.
219+
withRealm(
220+
realmId,
221+
(s, realm) -> {
222+
UserModel user = s.users().getUserByUsername(realm, "user1");
223+
RedisUserSessionProvider provider =
224+
new RedisUserSessionProvider(s, clusterJedis, RedisMode.CLUSTER);
225+
226+
List<String> foundIds =
227+
provider
228+
.getUserSessionsStream(realm, user)
229+
.map(UserSessionModel::getId)
230+
.collect(Collectors.toList());
231+
232+
assertThat(foundIds, hasSize(1));
233+
assertThat(foundIds, contains(sessionId));
234+
return null;
235+
});
236+
}
237+
}

0 commit comments

Comments
 (0)