Support redis cluster

[zfanta]

When ThrottlerStorageRedisService get an instance of Redis.Cluster, the instance meets the last else, then it tries connect default host(127.0.0.0) and port(6379)

nestjs-throttler-storage-redis/src/throttler-storage-redis.service.ts

Lines 14 to 23 in 80aef95

	constructor(redisOrOptions?: Redis.Redis | Redis.RedisOptions | string, scanCount?: number) {
	this.scanCount = typeof scanCount === 'undefined' ? 1000 : scanCount;
	if (redisOrOptions instanceof Redis) {
	this.redis = redisOrOptions;
	} else if (typeof redisOrOptions === 'string') {
	this.redis = new Redis(redisOrOptions as string);
	this.disconnectRequired = true;
	} else {
	this.redis = new Redis(redisOrOptions);

[kkoomen]

@zfanta I've never met the use case to use the Redis.Cluster, nor have I experience in that. Would you say it's basically adjusting the if-statement like so?

if (redisOrOptions instanceof Redis || redisOrOptions instanceof Redis.Cluster) { 
  this.redis = redisOrOptions; 
}

[mikeulvila]

For anyone else that comes across this looking to use with a cluster, SCAN command is not supported for a cluster as it iterates the set of keys for a single Redis database.

[n10000k]

So does this storage package support Redis.Cluster? @kkoomen @mikeulvila thanks in advance

[kkoomen]

@mikeulvila so how would this be implemented? I'm fine if people want support for clusters, but I have no clue about how to implement it as I have no experience with it.

[n10000k]

@kkoomen ok so I got this working, when you use a cluster keyPrefix {login-throttle} using Redis.Cluster however I ended up making my own lib for this. Will leave this here for anyone who needs this in the future:

  public constructor(nodes: ClusterNode[], options?: ClusterOptions, scanCount?: number) {
    this.scanCount = typeof scanCount === 'undefined' ? 1000 : scanCount;
    this.redis = new Redis.Cluster(nodes, options);
    this.disconnectRequired = true;
  }

return {
        ttl: rateLimitConfig.ttl,
        limit: rateLimitConfig.limit,
        storage: new ThrottlerStorageRedisService([
          redisRateLimitConfig.host,
        ], {
          enableReadyCheck: false,
          retryDelayOnClusterDown: 300,
          retryDelayOnFailover: 1000,
          retryDelayOnTryAgain: 3000,
          slotsRefreshTimeout: 10000,
          redisOptions: {
            keyPrefix: '{login-throttler}',
          },
        }),
      };

For this package I would suggest maybe separating the cluster part out into it's own class instead of doing them both together as you need cluster options etc which might be application to people in the future.

[kkoomen]

@uncodable we can combine your example like so:

NOTE: this is just an idea, haven't tested it yet.

return {
  ttl: rateLimitConfig.ttl,
  limit: rateLimitConfig.limit,
  storage: new ThrottlerStorageRedisClusterService(
    [redisRateLimitConfig.host],
    {
      enableReadyCheck: false,
      retryDelayOnClusterDown: 300,
      retryDelayOnFailover: 1000,
      retryDelayOnTryAgain: 3000,
      slotsRefreshTimeout: 10000,
      redisOptions: {
        keyPrefix: '{login-throttler}',
      },
    }
  }),
};

and then the code will be changed like so:

diff --git a/src/throttler-storage-redis-cluster.service.ts b/src/throttler-storage-redis-cluster.service.ts
new file mode 100644
index 0000000..f53a62e
--- /dev/null
+++ b/src/throttler-storage-redis-cluster.service.ts
@@ -0,0 +1,10 @@
+import { Injectable } from '@nestjs/common';
+import Redis, { ClusterNode, ClusterOptions } from 'ioredis';
+import { ThrottlerStorageRedisService } from './throttler-storage-redis.service';
+
+@Injectable()
+export class ThrottlerStorageRedisClusterService extends ThrottlerStorageRedisService {
+  constructor(nodes: ClusterNode[], options?: ClusterOptions, scanCount?: number) {
+    super(new Redis.Cluster(nodes, options), scanCount);
+  }
+}
diff --git a/src/throttler-storage-redis.interface.ts b/src/throttler-storage-redis.interface.ts
index 353df80..fd746b7 100644
--- a/src/throttler-storage-redis.interface.ts
+++ b/src/throttler-storage-redis.interface.ts
@@ -1,10 +1,10 @@
-import Redis from 'ioredis';
+import Redis, { Cluster } from 'ioredis';
 
 export interface ThrottlerStorageRedis {
   /**
    * The redis instance.
    */
-  redis: Redis;
+  redis: Redis | Cluster;
 
   /**
    * The amount of items that redis should return for each scan.
diff --git a/src/throttler-storage-redis.service.ts b/src/throttler-storage-redis.service.ts
index a5f554a..42554a7 100644
--- a/src/throttler-storage-redis.service.ts
+++ b/src/throttler-storage-redis.service.ts
@@ -1,20 +1,21 @@
 import { Injectable, OnModuleDestroy } from '@nestjs/common';
-import Redis, { RedisOptions } from 'ioredis';
+import Redis, { Cluster, RedisOptions } from 'ioredis';
 import { ThrottlerStorageRedis } from './throttler-storage-redis.interface';
 
 @Injectable()
 export class ThrottlerStorageRedisService implements ThrottlerStorageRedis, OnModuleDestroy {
-  redis: Redis;
+  redis: Redis | Cluster;
   disconnectRequired?: boolean;
   scanCount: number;
 
   constructor(redis?: Redis, scanCount?: number);
   constructor(options?: RedisOptions, scanCount?: number);
+  constructor(cluster?: Cluster, scanCount?: number);
   constructor(url?: string, scanCount?: number);
-  constructor(redisOrOptions?: Redis | RedisOptions | string, scanCount?: number) {
+  constructor(redisOrOptions?: Redis | RedisOptions | Cluster | string, scanCount?: number) {
     this.scanCount = typeof scanCount === 'undefined' ? 1000 : scanCount;
 
-    if (redisOrOptions instanceof Redis) {
+    if (redisOrOptions instanceof Redis || redisOrOptions instanceof Cluster) {
       this.redis = redisOrOptions;
     } else if (typeof redisOrOptions === 'string') {
       this.redis = new Redis(redisOrOptions as string);

Would this work for you and others? Or do you have any other suggestions?

[mpx2m]

Any planning time to finish the feature of supporting cluster mode?

[kkoomen]

@mpx2m Thanks for reminding me, as I'm very busy. I can test the above suggestion I did anytime soon, so I'll try to work on it.

[kkoomen]

@mpx2m since I don't have a proper cluster setup, do you? if so, can you test the patch provided above? Because if that already fixes it, I can simply apply that, merge and be done with this, which would speed things up a lot if I can get some support from the community regarding testing things.

[vjunjoced]

I would like to know if there is any advance with this integration ?

[kkoomen]

@zfanta @mikeulvila @n10000k @mpx2m See #1038 for a possible implementation for the Redis Clusters. Can you all test this and see if this works? Any feedback on the PR #1038 should be commented inside the PR itself, but not here. Thanks.

[mpx2m]

@zfanta I implemented the throttler functionality in the service mesh now, not test the code yet.

A docker redis cluster should be enough to test it.

[kkoomen]

#1041 fixed a good performance issue, I've merged the code into #1038 but not sure if there's anything more needed in #1038. If anyone could test the changes in #1038 with 2 or more redis clusters, that'd be great.

[kkoomen]

I just merged #1038 with proper support for redis clusters along with automated tests. Version v0.2.1 and above contains full support for clusters and has been published to NPM here. If there are any problems, feel free to comment here.

Enjoy.