test-lib: reach Redis through a cluster client with optional TLS - #2207
yaroslavmokflmg wants to merge 4 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (email.equals(jedis.get(key))) { | ||
| return key.split(":pwdreset:")[1]; | ||
| } | ||
| try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) { |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] clientConfig() throws before the JedisCluster try-with-resources can close it, but the SSLSocketFactory/context objects it partially builds are never leaked; however new JedisCluster(...) is constructed with the config as an argument evaluated before entering try-with-resources, so if clientConfig()
In getResetToken, new JedisCluster(RedisConfig.getClusterNodes(), clientConfig()) is evaluated as the resource expression of the try-with-resources statement. If JedisCluster's constructor throws partway through establishing cluster connections (e.g. after opening a socket to node 0 but failing on node 1's handshake), the partially-constructed object is never assigned to the try-with-resources variable and therefore never has close() called on it, leaking any sockets it already opened. This is a real behavior change from the previous per-node Jedis loop, where each Jedis instance was independently opened and closed. Consider constructing the JedisCluster in a preceding statement and only entering the try block once construction succeeds, or accept the leak risk is bounded since JVM/OS will eventually reclaim the sockets on GC/process exit.
Evidence
try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) {
🤖 Prompt for AI agents
In openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java around line 34, address this code-review finding: clientConfig() throws before the JedisCluster try-with-resources can close it, but the SSLSocketFactory/context objects it partially builds are never leaked; however new JedisCluster(...) is constructed with the config as an argument evaluated before entering try-with-resources, so if clientConfig().
In getResetToken, `new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())` is evaluated as the resource expression of the try-with-resources statement. If JedisCluster's constructor throws partway through establishing cluster connections (e.g. after opening a socket to node 0 but failing on node 1's handshake), the partially-constructed object is never assigned to the try-with-resources variable and therefore never has close() called on it, leaking any sockets it already opened. This is a real behavior change from the previous per-node Jedis loop, where each Jedis instance was independently opened and closed. Consider constructing the JedisCluster in a preceding statement and only entering the try block once construction succeeds, or accept the leak risk is bounded since JVM/OS will eventually reclaim the sockets on GC/process exit.
The flagged code:
```
try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) {
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 35 — react 👍/👎 to teach the reviewer
e547674 to
4c62a4f
Compare
Memorystore publishes one discovery endpoint instead of per-pod addresses, so probing seed nodes one at a time no longer reaches the node that owns the slot. Every pwdreset key carries the same hash tag, and Jedis routes a cluster SCAN by the slot of the MATCH pattern, so a cluster client lands on that node directly; the batch is then read with one MGET rather than a GET per key. TLS is enabled only when a CA is published - from the service config or REDIS_SERVER_CA - and the CA is loaded into a trust store of its own, because a managed Redis signs with a private CA the JVM has never seen. With no CA the client connects in plain text exactly as before, so OSS installations running an in-cluster Redis are untouched. Failures still return null, which is the contract callers poll against, but the cause is now logged with its stack: a TLS or routing mistake used to be indistinguishable from a miss.
Auth and TLS are independent, so build the client config incrementally instead of branching on the CA alone.
Dev runs IAM auth, where the credential is a short-lived token the client has to fetch itself. An environment variable cannot carry that, so the TLS support stays and the token wiring goes.
The cluster hands out an access token that expires within the hour, so the credentials go in through a Supplier that Jedis calls per connection rather than a string resolved once. Identity comes from Workload Identity, so a pod needs no key material. Opt-in, like TLS: with REDIS_IAM_AUTH unset no credentials are sent and an in-cluster Redis is reached exactly as before.
8e39059 to
c534ba5
Compare
🦩 Flamingo Code Review2 finding(s) — 0 action required · 2 recommended · 0 informational Mode: advisory · Rules cited: Inline comments: 2 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-17 12:08 UTC · updated 2026-09-17 12:08 UTC · workflow run |
| try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) { | ||
| ScanParams scanParams = new ScanParams().match(pattern).count(100); | ||
| String cursor = ScanParams.SCAN_POINTER_START; | ||
| do { | ||
| ScanResult<String> scanResult = cluster.scan(cursor, scanParams); | ||
| List<String> keys = scanResult.getResult(); | ||
| if (!keys.isEmpty()) { | ||
| // One slot for the whole batch, so this is a single round trip rather than a GET per key. | ||
| List<String> emails = cluster.mget(keys.toArray(new String[0])); | ||
| for (int i = 0; i < keys.size(); i++) { | ||
| if (email.equals(emails.get(i))) { | ||
| return keys.get(i).split(":pwdreset:")[1]; | ||
| } | ||
| } | ||
| cursor = scanResult.getCursor(); | ||
| } while (!cursor.equals(ScanParams.SCAN_POINTER_START)); | ||
| } catch (Exception e) { | ||
| // Node unreachable or does not own the slot — try the next seed. | ||
| } | ||
| } | ||
| cursor = scanResult.getCursor(); | ||
| } while (!cursor.equals(ScanParams.SCAN_POINTER_START)); | ||
| } catch (Exception e) { | ||
| log.warn("Reading the password-reset token from Redis failed", e); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] OFJAVA-018 Broad catch(Exception e) around getResetToken swallows and reclassifies all failures as a null miss
The rewritten getResetToken wraps the entire cluster connection, SCAN and MGET pipeline in a single catch(Exception e) that logs at WARN and returns null. This means genuine bugs (e.g. IllegalStateException thrown by iamCredentials() or sslSocketFactory() for misconfiguration) are logged identically to a transient 'key not found' condition, and the caller (which polls this method) cannot distinguish 'not yet available' from 'misconfigured/broken'. Per OFJAVA-018/016, catching should be limited to what can be meaningfully handled (or at minimum the config-time exceptions from clientConfig() should be treated differently from scan-time exceptions), otherwise a persistent misconfiguration (e.g. bad CA PEM) manifests only as a poll timeout with a buried WARN log rather than a clear failure.
Evidence
try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) {
ScanParams scanParams = new ScanParams().match(pattern).count(100);
String cursor = ScanParams.SCAN_POINTER_START;
do {
ScanResult<String> scanResult = cluster.scan(cursor, scanParams);
List<String> keys = scanResult.getResult();
if (!keys.isEmpty()) {
// One slot for the whole batch, so this is a single round trip rather than a GET per key.
List<String> emails = cluster.mget(keys.toArray(new String[0]));
for (int i = 0; i < keys.size(); i++) {
if (email.equals(emails.get(i))) {
return keys.get(i).split(":pwdreset:")[1];
🤖 Prompt for AI agents
In openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java around lines 45-66, address this code-review finding: Broad catch(Exception e) around getResetToken swallows and reclassifies all failures as a null miss.
The rewritten getResetToken wraps the entire cluster connection, SCAN and MGET pipeline in a single catch(Exception e) that logs at WARN and returns null. This means genuine bugs (e.g. IllegalStateException thrown by iamCredentials() or sslSocketFactory() for misconfiguration) are logged identically to a transient 'key not found' condition, and the caller (which polls this method) cannot distinguish 'not yet available' from 'misconfigured/broken'. Per OFJAVA-018/016, catching should be limited to what can be meaningfully handled (or at minimum the config-time exceptions from clientConfig() should be treated differently from scan-time exceptions), otherwise a persistent misconfiguration (e.g. bad CA PEM) manifests only as a poll timeout with a buried WARN log rather than a clear failure.
The flagged code:
```
try (JedisCluster cluster = new JedisCluster(RedisConfig.getClusterNodes(), clientConfig())) {
ScanParams scanParams = new ScanParams().match(pattern).count(100);
String cursor = ScanParams.SCAN_POINTER_START;
do {
ScanResult<String> scanResult = cluster.scan(cursor, scanParams);
List<String> keys = scanResult.getResult();
if (!keys.isEmpty()) {
// One slot for the whole batch, so this is a single round trip rather than a GET per key.
List<String> emails = cluster.mget(keys.toArray(new String[0]));
for (int i = 0; i < keys.size(); i++) {
if (email.equals(emails.get(i))) {
return keys.get(i).split(":pwdreset:")[1];
}
}
}
cursor = scanResult.getCursor();
} while (!cursor.equals(ScanParams.SCAN_POINTER_START));
} catch (Exception e) {
log.warn("Reading the password-reset token from Redis failed", e);
}
return null;
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 45 — react 👍/👎 to teach the reviewer
| private static RedisCredentials iamCredentials() { | ||
| try { | ||
| GoogleCredentials credentials = GoogleCredentials.getApplicationDefault() | ||
| .createScoped("https://www.googleapis.com/auth/cloud-platform"); | ||
| credentials.refreshIfExpired(); | ||
| return new DefaultRedisCredentials(null, credentials.getAccessToken().getTokenValue()); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException("Could not obtain an access token for Redis IAM auth", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] IAM access-token refresh discards refreshed credential state on every call, defeating token caching
iamCredentials() calls GoogleCredentials.getApplicationDefault() fresh on every invocation (which happens once per new JedisCluster, i.e. potentially many times when getResetToken is polled in a loop by a caller). GoogleCredentials.getApplicationDefault() performs metadata-server/ADC discovery each time rather than reusing a cached credentials object, and refreshIfExpired() on a brand-new instance will nearly always trigger a full token fetch since a fresh instance has no cached token. This turns every poll iteration into an extra call to the GCE metadata server or STS endpoint, which is wasteful and can hit rate limits under a tight polling loop from a test harness. The credentials object should be created once (e.g. cached in a static field) and only refreshIfExpired() called per connection.
Evidence
private static RedisCredentials iamCredentials() {
try {
GoogleCredentials credentials = GoogleCredentials.getApplicationDefault()
.createScoped("https://www.googleapis.com/auth/cloud-platform");
credentials.refreshIfExpired();
return new DefaultRedisCredentials(null, credentials.getAccessToken().getTokenValue());
} catch (IOException e) {
throw new IllegalStateException("Could not obtain an access token for Redis IAM auth", e);
}
}
🤖 Prompt for AI agents
In openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java around lines 96-105, address this code-review finding: IAM access-token refresh discards refreshed credential state on every call, defeating token caching.
iamCredentials() calls GoogleCredentials.getApplicationDefault() fresh on every invocation (which happens once per new JedisCluster, i.e. potentially many times when getResetToken is polled in a loop by a caller). GoogleCredentials.getApplicationDefault() performs metadata-server/ADC discovery each time rather than reusing a cached credentials object, and refreshIfExpired() on a brand-new instance will nearly always trigger a full token fetch since a fresh instance has no cached token. This turns every poll iteration into an extra call to the GCE metadata server or STS endpoint, which is wasteful and can hit rate limits under a tight polling loop from a test harness. The credentials object should be created once (e.g. cached in a static field) and only refreshIfExpired() called per connection.
The flagged code:
```
private static RedisCredentials iamCredentials() {
try {
GoogleCredentials credentials = GoogleCredentials.getApplicationDefault()
.createScoped("https://www.googleapis.com/auth/cloud-platform");
credentials.refreshIfExpired();
return new DefaultRedisCredentials(null, credentials.getAccessToken().getTokenValue());
} catch (IOException e) {
throw new IllegalStateException("Could not obtain an access token for Redis IAM auth", e);
}
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 40 — react 👍/👎 to teach the reviewer
1 participant
Why
The shared cluster is moving Redis from the in-cluster
redis-clusterchart to Memorystore forRedis Cluster. Two things break for this library:
walking a list of seeds and running a node-local
SCANno longer lands on the node that ownsthe slot;
new Jedis(host)cannot do.How it works now
Redis.getResetTokenusesJedisCluster. This is safe precisely because the keys arehash-tagged (
of:{<tenant>}:pwdreset:<token>): Jedis routes a clusterSCANby the slot of theMATCH pattern — and rejects a pattern without a tag — so the scan goes straight to the node that
owns every pwdreset key. The batch is then read with a single
MGETinstead of aGETper key,which the shared slot makes legal.
TLS is opt-in. The CA arrives from the service config (
test.redis.ca) or fromREDIS_SERVER_CA, and is loaded into a trust store of its own. With no CA published the clientconnects in plain text exactly as before, so OSS installations on an in-cluster Redis are
untouched.
Contract kept
Failures still return
null— key absent, cluster unreachable, wrong tenant prefix. Callers pollthis method, so a failure has to look like a miss. What changed is that the cause is now logged
with its stack;
PKIX path building failedis otherwise invisible and reads as "the user neverreceived a reset mail".
A malformed CA fails fast with a named error rather than an empty trust store and an opaque
handshake failure later.
Files
test/config/RedisConfig.javaREDIS_SERVER_CAfallbacktest/data/redis/Redis.javaMGETbatch read, optional TLS, logged failuresVerified against Jedis 7.2.0 sources
JedisCluster(Set<HostAndPort>, JedisClientConfig)exists,UnifiedJedisisAutoCloseable, andClusterCommandObjects.scanenforces the hash-tag rule this change relies on.