Support for clusters in RedisHealthCheck

[adamkirk]

What is the issue?

When using a redis cluster the RedisHealthCheck fails. This is due to the fact that a RedisCluster connection requires an argument (which it uses to determine which host in the cluster to ping). An ErrorException is thrown with the following message: RedisCluster::ping() expects at least 1 parameter, 0 given.

Steps To Reproduce

Steps to reproduce the behavior:

1. You'll need a redis cluster, using redis' built-in clustering mechanism, rather than replicated nodes (i.e. sentinel)
2. Configure a laravel app to connect to this cluster (snippet 1 below)
3. Add the RedisHealthCheck to the healthcheck configuration (snippet 2 below)
4. Hit the healthcheck endpoint
5. The redis check should report a problem with the following ErrorException: RedisCluster::ping() expects at least 1 parameter, 0 given (screenshot below)

Snippet 1

// config/database.php
    'redis' => [
        
        'client' => 'phpredis',

        'clusters' => [
            'options' => [
                'cluster'    => 'redis',
                'password' => env('REDIS_PASSWORD', null),
                'parameters' => [
                    'scheme'   => env('REDIS_SCHEME', 'tcp'),
                ]
            ],
            'default' => [
                [
                    'host'     => env('REDIS_HOST', '127.0.0.1'),
                    'password' => env('REDIS_PASSWORD', null),
                    'port'     => env('REDIS_PORT', 6379),
                    'database' => '0',
                ],
            ],
        ],
    ],

Snippet 2

config/healthcheck.php

    'checks' => [
        UKFast\HealthCheck\Checks\RedisHealthCheck::class,
    ],

Screenshot from postman:

Expected behaviour

The healthcheck should check whether we're using a cluster connection, and ping each master node.

Possible fixes

I've implemented a custom check in our system to get around this, which checks whether the redis connection (from the facade) is a cluster connection, and if so pings each master node in turn. It may need to account for other scenarios, but I think this will work with most laravel/Lumen installations out of the box.

A couple of considerations to make this more widely applicable:

• Configuration for this check, could allow the user to choose specific nodes from the configuration.
• Limit the check to a number of nodes
• investigate required support for predis (I don't think predis supports clusters at all but i'm not 100%)

<?php

use Exception;
use UKFast\HealthCheck\HealthCheck;
use Illuminate\Support\Facades\Redis;
use Illuminate\Redis\Connections\PhpRedisClusterConnection;

class RedisHealthCheck extends HealthCheck
{
    protected $name = 'redis';

    public function status()
    {
        try {
            if ($this->isUsingPhpRedis()) {
                $this->handlePhpRedisPing();
            } else {
                // Think this is all we can do for predis?
                Redis::ping();
            }
        } catch (Exception $e) {
            return $this->problem('Failed to connect to redis', [
                'exception' => $this->exceptionContext($e),
            ]);
        }
        return $this->okay();
    }

    protected function isUsingPhpRedis()
    {
        return config('database.redis.client') == 'phpredis';
    }

    protected function handlePhpRedisPing()
    {
        $redis = Redis::connection();

        if ($redis instanceof PhpRedisClusterConnection) {
            foreach ($redis->_masters() as $master) {
                Redis::ping($master);
            }

            return;
        }

        $redis->ping();
    }
}

I can create a pull request for this, but wanted to make sure I went through an issue first :)

Additional context

~

[Gman98ish]

@adamkirk Thanks for the super detailed bug report. The proposed fix looks fine to me so happy to approve the PR

[adamkirk]

@Gman98ish Awesome! PR has been opened :)

[Gman98ish]

@adamkirk that's been merged and tagged at v1.13.1

[adamkirk]

@adamkirk that's been merged and tagged at v1.13.1

Thankyou very much! I'll get ours updated :)