envoyproxy / ratelimit

Go/gRPC service designed to enable generic rate limit scenarios from different types of applications.
Apache License 2.0
2.31k stars 453 forks source link

Using Redis in socket mode no longer works #133

Open moderation opened 4 years ago

moderation commented 4 years ago

I had a demo of ratelimit running in a Kubernetes cluster at the end of 2019. For performance reasons I had ratelimit connect to a Redis instance over a socket shared on a ConfigMap volume. Having the ratelimit -> Redis connection not use a network at all is great for performance.

Turns out https://github.com/envoyproxy/ratelimit/commit/3ec0f5f3a660ab3ce1f6e16f3823e18dd1ceebf7#diff-ab160e3a4ff5cb5fd488f666c5266fdd disabled the default Unix socket usage by hardcoding the use of TCP.

It is not possible to launch ratelimit with REDIS_SOCKET_TYPE=unix even though it is the 'default' value. See an example below:

env USE_STATSD=false LOG_LEVEL=debug REDIS_URL=/tmp/redis.sock REDIS_SOCKET_TYPE=unix RUNTIME_ROOT=examples/ratelimit/config/ RUNTIME_SUBDIRECTORY=. bin/ratelimit
WARN[0000] statsd is not in use
DEBU[0000] runtime changed. loading new snapshot at examples/ratelimit/config
DEBU[0000] runtime: processing examples/ratelimit/config
DEBU[0000] runtime: processing examples/ratelimit/config/config.yaml
DEBU[0000] runtime: adding key=config.yaml value=---
domain: mongo_cps
descriptors:
  - key: database
    value: users
    rate_limit:
      unit: second
      requests_per_unit: 500

  - key: database
    value: default
    rate_limit:
      unit: second
      requests_per_unit: 500
 uint=false
WARN[0000] connecting to redis on /tmp/redis.sock with pool size 10
panic: dial tcp: address /tmp/redis.sock: missing port in address

goroutine 1 [running]:
github.com/envoyproxy/ratelimit/src/redis.checkError(...)
        /home/moderation/Library/envoyproxy/ratelimit/src/redis/driver_impl.go:44
github.com/envoyproxy/ratelimit/src/redis.NewPoolImpl(0xca07e0, 0xc00010eb40, 0x0, 0x0, 0x0, 0xc00003a2ea, 0xf, 0xa, 0xc00011e080, 0xc000134000)
        /home/moderation/Library/envoyproxy/ratelimit/src/redis/driver_impl.go:98 +0x344
github.com/envoyproxy/ratelimit/src/service_cmd/runner.(*Runner).Run(0xc000213f68)
        /home/moderation/Library/envoyproxy/ratelimit/src/service_cmd/runner/runner.go:57 +0x317
main.main()
        /home/moderation/Library/envoyproxy/ratelimit/src/service_cmd/main.go:7 +0x43

The socket type setting at https://github.com/envoyproxy/ratelimit/blob/master/src/settings/settings.go#L22 is no longer used in the code base.

moderation commented 4 years ago

The following unblocks me but I'd suggest we need tests to ensure that default functionality can't be deleted without anyone noticing.

diff --git a/src/redis/driver_impl.go b/src/redis/driver_impl.go
index dbcf787..d50fb56 100644
--- a/src/redis/driver_impl.go
+++ b/src/redis/driver_impl.go
@@ -67,7 +67,7 @@ func (this *poolImpl) Put(c Connection) {
        }
 }

-func NewPoolImpl(scope stats.Scope, useTls bool, auth string, url string, poolSize int) Pool {
+func NewPoolImpl(scope stats.Scope, useTls bool, auth string, url string, poolSize int, socketType string) Pool {
        logger.Warnf("connecting to redis on %s with pool size %d", url, poolSize)
        df := func(network, addr string) (*redis.Client, error) {
                var conn net.Conn
@@ -75,7 +75,7 @@ func NewPoolImpl(scope stats.Scope, useTls bool, auth string, url string, poolSi
                if useTls {
                        conn, err = tls.Dial("tcp", addr, &tls.Config{})
                } else {
-                       conn, err = net.Dial("tcp", addr)
+                       conn, err = net.Dial(socketType, addr)
                }
                if err != nil {
                        return nil, err
diff --git a/src/service_cmd/runner/runner.go b/src/service_cmd/runner/runner.go
index ee9da4b..cd51408 100644
--- a/src/service_cmd/runner/runner.go
+++ b/src/service_cmd/runner/runner.go
@@ -51,10 +51,10 @@ func (runner *Runner) Run() {

        var perSecondPool redis.Pool
        if s.RedisPerSecond {
-               perSecondPool = redis.NewPoolImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize)
+               perSecondPool = redis.NewPoolImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisSocketType)
        }
        var otherPool redis.Pool
-       otherPool = redis.NewPoolImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisUrl, s.RedisPoolSize)
+       otherPool = redis.NewPoolImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisUrl, s.RedisPoolSize, s.RedisSocketType)

        service := ratelimit.NewService(
                srv.Runtime(),
stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

moderation commented 4 years ago

Not stale. Still broken. Will try to fix next week.