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

Using Redis in socket mode no longer works #133

Open
moderation opened this issue Apr 12, 2020 · 3 comments
Open

Using Redis in socket mode no longer works #133

moderation opened this issue Apr 12, 2020 · 3 comments

Comments

@moderation
Copy link

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 3ec0f5f#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
Copy link
Author

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
Copy link

stale bot commented May 13, 2020

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.

@stale stale bot added the stale label May 13, 2020
@moderation
Copy link
Author

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

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

No branches or pull requests

2 participants