[Cache] phpredis: Added full TLS support for RedisCluster #40882
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Sure. I think I can come up with tests. The issue this is addressing is that when you're using redis cluster, and getHosts gets called, it makes a new Redis object for each redis cluster node and runs commands intended just for that node via this secondary connection. The problem with that, is that when you call _masters on a RedisCluster, it does not indicate if tls is used or not. The secondary connections will be setup without tls. I've looked over the phpredis code and it does not expose in any way if its using tls or not. The easy fix for this is to just reuse the connections that RedisCluster already has to each node, using the method described in the phpredis documentation. RedisClusterNodeProxy provides a structure to encapsulate the method calls to \RedisCluster using the same interface that \Redis presents. |
|
Rebased against 4.4 |
|
Very welcome! |
|
@nicolas-grekas What about psalm? I'm not sure what its upset about. |
|
psalm is drunk here, it can be ignored :) |
|
What is remaining? |
|
Can someone approve workflows? |
|
I switched the default value for ssl back to null. I start seeing client disconnects / timeouts when the value provided is an empty array. In the C++ code its default null. I think the presence of the array at all forces the driver into ssl mode. |
|
Thank you @jackthomasatl. |
…(jackthomasatl) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Cache] phpredis: Added full TLS support for RedisCluster | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | n/a This Pr bridges the gap for full TLS support when using phpredis driver implementation of TLS. Adds the 'ssl' options array for cache configuration when using RedisCluster https://www.php.net/manual/en/context.ssl.php Switches directed node commands from using individual \Redis connections to using the recommended implementation from the phpredis documentation: https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands This pr will enable compatibility with Amazon ElastiCache redis cluster mode using In Transit encryption (TLS) using the phpredis driver, Supports tagging & binary data types. Commits ------- a1e0408 [Cache] phpredis: Added full TLS support for RedisCluster
|
(dunno why github displays this has having no commits, but the merge is confirmed in f8518ca) |
|
Ah, I did an empty merge commit trying to trigger the test suite to run again, it was stuck. |
This Pr bridges the gap for full TLS support when using phpredis driver implementation of TLS.
Adds the 'ssl' options array for cache configuration when using RedisCluster
https://www.php.net/manual/en/context.ssl.php
Switches directed node commands from using individual \Redis connections to using the recommended implementation from the phpredis documentation:
https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands
This pr will enable compatibility with Amazon ElastiCache redis cluster mode using In Transit encryption (TLS) using the phpredis driver, Supports tagging & binary data types.