Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fix flushDb (cache:clear) for redis clusters #36281

Merged
merged 1 commit into from
Feb 17, 2021
Merged

[8.x] Fix flushDb (cache:clear) for redis clusters #36281

merged 1 commit into from
Feb 17, 2021

Conversation

willbrowningme
Copy link
Contributor

@willbrowningme willbrowningme commented Feb 16, 2021

When running php artisan cache:clear with a redis cluster as the cache driver, the flushDb() function is currently unable to run on each master if you are using a password or TLS scheme - #35180

For redis clusters an empty $config array is passed through to the constructor of vendor\laravel\framework\src\Illuminate\Redis\Connections\PhpRedisConnection.php causing authentication to fail.

However, even if you update vendor\laravel\framework\src\Illuminate\Redis\Connectors\PhpRedisConnector.php to pass through the cluster password it still fails because the current $redis = tap(new Redis)->connect($host, $port); does not connect to each master node using the correct TLS scheme or any stream context such as local_cert, local_pk, cafile or verify_peer_name.

This pull request fixes the issue by using directed node commands as described here - https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands

It uses the already authenticated RedisCluster connection with correct scheme and context to direct the flushdb command to each master node to succesfully clear the entire cache.

When running `php artisan cache:clear` with a redis cluster, the flushDb() function is currently unable to run on each master if you are using a password or TLS scheme - #35180

For redis clusters an empty `$config` array is passed through to the constructor of `vendor\laravel\framework\src\Illuminate\Redis\Connections\PhpRedisConnection.php` causing authentication to fail.

However, even if you update `vendor\laravel\framework\src\Illuminate\Redis\Connectors\PhpRedisConnector.php` to pass through the cluster password it still fails because `$redis = tap(new Redis)->connect($host, $port);` does not connect to each master node using the correct TLS scheme or any steam context such as `local_cert`, `local_pk`, `cafile` or `verify_peer_name`.

This pull request fixes the issue by using `Directed node commands` as described here - https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands

It uses the already authenticated `RedisCluster` connection with correct scheme and context to direct the `flushdb` command to each master node to succesfully clear the cache.
@driesvints driesvints changed the title Fix flushDb (cache:clear) for redis clusters [8.x] Fix flushDb (cache:clear) for redis clusters Feb 16, 2021
@taylorotwell
Copy link
Member

I will probably need someone actually using Redis Cluster to confirm this.

@taylorotwell
Copy link
Member

Are you using any particular Docker setup to test Redis Cluster?

@taylorotwell
Copy link
Member

Got a cluster up and running with Docker and this does indeed seem to not break anything at least.

@taylorotwell taylorotwell merged commit e66fb52 into laravel:8.x Feb 17, 2021
@willbrowningme
Copy link
Contributor Author

I've tested on Homestead running 6 instances 3 masters, each with 1 replica (slave).

I've also tested on an Ubuntu staging server running 20.04, using self signed certs and TLSv1.2.

Thanks for the merge!

@J5Dev
Copy link
Contributor

J5Dev commented Mar 19, 2021

Sorry to comment on an old MR, but I have found this after we have come across the same issue when using PHPRedis, however we are on laravel/framework v6.20.19.

I have verified that the issue is the exact same, and changing the PhpRedisConnection@flushdb to the above submitted code, does indeed fix the issue.

@taylor or @willbrowningme ... Can this fix be back-ported into the V6, given it is still LTS and receiving support fixes?
If not, is there an alternative that can be suggested?

Thank you.

@driesvints
Copy link
Member

@J5Dev maybe try a PR to see if Taylor would accept it?

@J5Dev
Copy link
Contributor

J5Dev commented Mar 19, 2021

@J5Dev maybe try a PR to see if Taylor would accept it?

No problems, will put one together now.

@JasonXup6
Copy link

@J5Dev maybe try a PR to see if Taylor would accept it?

No problems, will put one together now.

Could you make PR to laravel:7.x , we also facing this isssue.
It's wold be helpful a lot, thanks in advance.

@driesvints
Copy link
Member

Laravel 7 isn't maintained anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants