JAMES-4072 Support Redis TLS/SSL connection

[hungphan227]

No description provided.

[chibenwa]

That is a rather big changeset and I would like that we asks ourselves if we could deliver something shorter and more concise. I would suggest investing time (at least a halth day) to try to refactor this.

[hungphan227]

Broke to smaller commits

[Arsnael]

I honeestly quickly stopped reading... I bet like @chibenwa that there should be a much simpler way. I don't really see why the need of duplicated configuration depending if tls or not, I would believe there might be a way to just have one ssl conf for all topologies, like any other implementation we have?

Maybe we should do a MOB around this

[Arsnael]

So I had an other check on this... Can't we try to do something similar to RabbitMQ ssl configuration for example?

For example, don't try to figure out if ssl or not depending on the URI, have just a conf for that (ssl.enabled)

We should have in RedisConfiguration a subclass regrouping ssl conf, like SSLConfiguration

looking at rabbitmq conf for example:

| ssl.enabled
| Is using ssl enabled
Optional boolean, defaults to false

| ssl.validation.strategy
| Configure the validation strategy used for rabbitmq connections. Possible values are default, ignore and override.
Optional string, defaults to using systemwide ssl configuration

| ssl.truststore
| Points to the truststore (PKCS12) used for verifying rabbitmq connection. If configured then "ssl.truststore.password" must also be configured,
Optional string, defaults to systemwide truststore. "ssl.validation.strategy: override" must be configured if you want to use this

| ssl.truststore.password
| Configure the truststore password. If configured then "ssl.truststore" must also be configured,
Optional string, defaults to empty string. "ssl.validation.strategy: override" must be configured if you want to use this

| ssl.hostname.verifier
| Configure host name verification. Possible options are default and accept_any_hostname
Optional string, defaults to subject alternative name host verifier

| ssl.keystore
| Points to the keystore(PKCS12) used for client certificate authentication. If configured then "ssl.keystore.password" must also be configured,
Optional string, defaults to empty string

| ssl.keystore.password
| Configure the keystore password. If configured then "ssl.keystore" must also be configured,
Optional string, defaults to empty string

I don't know if all of those are necessary but maybe. It seems this PR is expecting keystore for some reason. Not automatically, it depends how ssl is configured on Redis side (cf hostname.verifier and validation.strategy that could bypass those mechanisms for example, and I would bet SSLOptions is the way)

Depending on how your SSLConfiguration ends up, you might inject different things in your redis client then. No need to modify all confs that much.

=> #307 rabbitmq ssl PR

I really think we could do something closer to that, let's keep doing things in a similar fashion

[Arsnael]

Maybe as well having something like a RedisClientFactory that would build the redis client depending if it's cluster, replica, ssl, etc... could help regroup the RedisClient build amongs the two factories we have? (RedisRateLimiterFactory and RedisEventBusClientFactory)

[chibenwa]

Maybe as well having something like a RedisClientFactory

Looks like a good idea to me!

[Arsnael]

Much better, thanks for the refactoring! Still a few comments

[quantranhong1999]

Overall good to me.
+1 for the way commits have been refactored.