FrankChen021 commented on code in PR #19666:
URL: https://github.com/apache/druid/pull/19666#discussion_r3543861394
##########
extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheFactory.java:
##########
@@ -100,15 +90,25 @@ public static Cache create(final RedisCacheConfig config)
return new RedisStandaloneCache(
new JedisPool(
poolConfig,
- config.getHost(),
- config.getPort(),
- config.getTimeout().getMillisecondsAsInt(), //connection timeout
and read timeout
- config.getPassword() == null ? null :
config.getPassword().getPassword(),
- config.getDatabase(),
- null
+ new HostAndPort(config.getHost(), config.getPort()),
+ buildClientConfig(config)
),
config
);
}
}
+
+ private static JedisClientConfig buildClientConfig(RedisCacheConfig config)
+ {
+ return DefaultJedisClientConfig
+ .builder()
+ .connectionTimeoutMillis(config.getTimeout().getMillisecondsAsInt())
+ .socketTimeoutMillis(config.getTimeout().getMillisecondsAsInt())
+ .password(config.getPassword() == null ? null :
config.getPassword().getPassword())
+ // database applies to standalone only; Redis Cluster supports
database 0 only, so this
+ // is a no-op in cluster mode (the default is 0).
+ .database(config.getDatabase())
Review Comment:
[P2] Do not pass the standalone database index to Redis Cluster
Cluster mode now uses the shared client config, but this builder always
copies `druid.cache.database`. Jedis 7.0.0 initializes a connection by reading
the configured database and issuing `SELECT` whenever it is greater than 0, and
Redis Cluster rejects selecting non-zero databases. Before this refactor the
cluster constructors never received Druid's standalone database setting, so a
cluster config that also carries `druid.cache.database=1` can now fail during
cluster initialization. Build a cluster-specific client config that leaves the
database at 0.
##########
extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheFactory.java:
##########
@@ -100,15 +90,25 @@ public static Cache create(final RedisCacheConfig config)
return new RedisStandaloneCache(
new JedisPool(
poolConfig,
- config.getHost(),
- config.getPort(),
- config.getTimeout().getMillisecondsAsInt(), //connection timeout
and read timeout
- config.getPassword() == null ? null :
config.getPassword().getPassword(),
- config.getDatabase(),
- null
+ new HostAndPort(config.getHost(), config.getPort()),
+ buildClientConfig(config)
),
config
);
}
}
+
+ private static JedisClientConfig buildClientConfig(RedisCacheConfig config)
+ {
+ return DefaultJedisClientConfig
+ .builder()
+ .connectionTimeoutMillis(config.getTimeout().getMillisecondsAsInt())
+ .socketTimeoutMillis(config.getTimeout().getMillisecondsAsInt())
+ .password(config.getPassword() == null ? null :
config.getPassword().getPassword())
+ // database applies to standalone only; Redis Cluster supports
database 0 only, so this
+ // is a no-op in cluster mode (the default is 0).
+ .database(config.getDatabase())
+ .ssl(config.getEnableTls())
Review Comment:
[P2] Verify Redis hostnames when TLS is enabled
Setting only `.ssl(true)` encrypts the socket but does not enable hostname
verification. In Jedis 7.0.0 the default socket factory only verifies the host
when a `HostnameVerifier` is supplied, or when endpoint-identification
parameters are supplied through `SSLParameters`; this config sets neither. As
added, `druid.cache.enableTls=true` can accept any certificate trusted by the
JVM regardless of the Redis hostname, so a trusted-but-wrong certificate can
impersonate the cache endpoint. Configure endpoint identification or expose an
explicit verification/skip option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
