fix: apply TLS and auth config to Redis Sentinel connections#1015
Merged
collin-lee merged 3 commits intoenvoyproxy:mainfrom Dec 10, 2025
Merged
Conversation
collin-lee
reviewed
Dec 2, 2025
|
|
||
| mkRedisClient := func(addr string) redis.Client { | ||
| return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second) | ||
| return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "") |
Contributor
There was a problem hiding this comment.
Could we update to include unit tests?
`package redis_test
import (
"fmt"
"strings"
"testing"
"time"
"github.com/alicebob/miniredis/v2"
stats "github.com/lyft/gostats"
"github.com/stretchr/testify/assert"
"github.com/envoyproxy/ratelimit/src/redis"
)
func mustNewRedisServer() *miniredis.Miniredis {
srv, err := miniredis.Run()
if err != nil {
panic(err)
}
return srv
}
func expectPanicError(t *testing.T, f assert.PanicTestFunc) (result error) {
t.Helper()
defer func() {
panicResult := recover()
assert.NotNil(t, panicResult, "Expected a panic")
result = panicResult.(error)
}()
f()
return
}
func testNewClientImpl(t *testing.T, pipelineWindow time.Duration, pipelineLimit int) func(t *testing.T) {
return func(t *testing.T) {
redisAuth := "123"
statsStore := stats.NewStore(stats.NewNullSink(), false)
mkRedisClient := func(auth, addr string) redis.Client {
return redis.NewClientImpl(statsStore, false, auth, "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "")
}
t.Run("connection refused", func(t *testing.T) {
// It's possible there is a redis server listening on 6379 in ci environment, so
// use a random port.
panicErr := expectPanicError(t, func() { mkRedisClient("", "localhost:12345") })
assert.Contains(t, panicErr.Error(), "connection refused")
})
t.Run("ok", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
var client redis.Client
assert.NotPanics(t, func() {
client = mkRedisClient("", redisSrv.Addr())
})
assert.NotNil(t, client)
})
t.Run("auth fail", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
redisSrv.RequireAuth(redisAuth)
assert.PanicsWithError(t, "NOAUTH Authentication required.", func() {
mkRedisClient("", redisSrv.Addr())
})
})
t.Run("auth pass", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
redisSrv.RequireAuth(redisAuth)
assert.NotPanics(t, func() {
mkRedisClient(redisAuth, redisSrv.Addr())
})
})
t.Run("auth user pass", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
user, pass := "test-user", "test-pass"
redisSrv.RequireUserAuth(user, pass)
redisAuth := fmt.Sprintf("%s:%s", user, pass)
assert.NotPanics(t, func() {
mkRedisClient(redisAuth, redisSrv.Addr())
})
})
t.Run("auth user pass fail", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
user, pass := "test-user", "test-pass"
redisSrv.RequireUserAuth(user, pass)
redisAuth := fmt.Sprintf("%s:invalid-password", user)
assert.PanicsWithError(t, "WRONGPASS invalid username-password pair", func() {
mkRedisClient(redisAuth, redisSrv.Addr())
})
})
t.Run("ImplicitPipeliningEnabled() return expected value", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
client := mkRedisClient("", redisSrv.Addr())
if pipelineWindow == 0 && pipelineLimit == 0 {
assert.False(t, client.ImplicitPipeliningEnabled())
} else {
assert.True(t, client.ImplicitPipeliningEnabled())
}
})
}
}
func TestNewClientImpl(t testing.T) {
t.Run("ImplicitPipeliningEnabled", testNewClientImpl(t, 2time.Millisecond, 2))
t.Run("ImplicitPipeliningDisabled", testNewClientImpl(t, 0, 0))
}
func TestDoCmd(t *testing.T) {
statsStore := stats.NewStore(stats.NewNullSink(), false)
mkRedisClient := func(addr string) redis.Client {
return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, 0, 0, nil, false, nil, 10*time.Second, "")
}
t.Run("SETGET ok", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
client := mkRedisClient(redisSrv.Addr())
var res string
assert.Nil(t, client.DoCmd(nil, "SET", "foo", "bar"))
assert.Nil(t, client.DoCmd(&res, "GET", "foo"))
assert.Equal(t, "bar", res)
})
t.Run("INCRBY ok", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
client := mkRedisClient(redisSrv.Addr())
var res uint32
hits := uint32(1)
assert.Nil(t, client.DoCmd(&res, "INCRBY", "a", hits))
assert.Equal(t, hits, res)
assert.Nil(t, client.DoCmd(&res, "INCRBY", "a", hits))
assert.Equal(t, uint32(2), res)
})
t.Run("connection broken", func(t *testing.T) {
redisSrv := mustNewRedisServer()
client := mkRedisClient(redisSrv.Addr())
assert.Nil(t, client.DoCmd(nil, "SET", "foo", "bar"))
redisSrv.Close()
assert.EqualError(t, client.DoCmd(nil, "GET", "foo"), "EOF")
})
}
func testPipeDo(t *testing.T, pipelineWindow time.Duration, pipelineLimit int) func(t *testing.T) {
return func(t *testing.T) {
statsStore := stats.NewStore(stats.NewNullSink(), false)
mkRedisClient := func(addr string) redis.Client {
return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "")
}
t.Run("SETGET ok", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
client := mkRedisClient(redisSrv.Addr())
var res string
pipeline := redis.Pipeline{}
pipeline = client.PipeAppend(pipeline, nil, "SET", "foo", "bar")
pipeline = client.PipeAppend(pipeline, &res, "GET", "foo")
assert.Nil(t, client.PipeDo(pipeline))
assert.Equal(t, "bar", res)
})
t.Run("INCRBY ok", func(t *testing.T) {
redisSrv := mustNewRedisServer()
defer redisSrv.Close()
client := mkRedisClient(redisSrv.Addr())
var res uint32
hits := uint32(1)
assert.Nil(t, client.PipeDo(client.PipeAppend(redis.Pipeline{}, &res, "INCRBY", "a", hits)))
assert.Equal(t, hits, res)
assert.Nil(t, client.PipeDo(client.PipeAppend(redis.Pipeline{}, &res, "INCRBY", "a", hits)))
assert.Equal(t, uint32(2), res)
})
t.Run("connection broken", func(t *testing.T) {
redisSrv := mustNewRedisServer()
client := mkRedisClient(redisSrv.Addr())
assert.Nil(t, nil, client.PipeDo(client.PipeAppend(redis.Pipeline{}, nil, "SET", "foo", "bar")))
redisSrv.Close()
expectErrContainEOF := func(t *testing.T, err error) {
assert.NotNil(t, err)
assert.Contains(t, err.Error(), "EOF")
}
expectErrContainEOF(t, client.PipeDo(client.PipeAppend(redis.Pipeline{}, nil, "GET", "foo")))
})
}
}
func TestPipeDo(t testing.T) {
t.Run("ImplicitPipeliningEnabled", testPipeDo(t, 10time.Millisecond, 2))
t.Run("ImplicitPipeliningDisabled", testPipeDo(t, 0, 0))
}
func TestNewClientImplSentinel(t *testing.T) {
statsStore := stats.NewStore(stats.NewNullSink(), false)
mkSentinelClient := func(auth, sentinelAuth, url string, useTls bool, timeout time.Duration) redis.Client {
// Pass nil for tlsConfig - we can't test TLS without a real TLS server,
// but we can verify the code path is executed (logs will show TLS is enabled)
return redis.NewClientImpl(statsStore, useTls, auth, "tcp", "sentinel", url, 1, 0, 0, nil, false, nil, timeout, sentinelAuth)
}
t.Run("invalid url format - missing sentinel addresses", func(t *testing.T) {
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", "mymaster", false, 10*time.Second)
})
assert.Contains(t, panicErr.Error(), "Expected master name and a list of urls for the sentinels")
})
t.Run("invalid url format - only master name", func(t *testing.T) {
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", "mymaster,", false, 10*time.Second)
})
// Empty sentinel address causes "missing address" error from radix
assert.True(t,
containsAny(panicErr.Error(), []string{"Expected master name", "missing address"}),
"Expected format validation error, got: %s", panicErr.Error())
})
t.Run("connection refused - sentinel not available", func(t *testing.T) {
// Use a port that's unlikely to have a sentinel running
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", url, false, 1*time.Second)
})
// Should fail with connection error or timeout
assert.NotNil(t, panicErr)
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect"}),
"Expected connection error, got: %s", panicErr.Error())
})
t.Run("sentinel auth password only", func(t *testing.T) {
// This will fail to connect, but we're testing that sentinelAuth parameter is accepted
// The log output will show "enabling authentication to redis sentinel" which confirms the code path
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "sentinel-password", url, false, 1*time.Second)
})
// Should fail with connection error, not auth error (since we can't connect)
assert.NotNil(t, panicErr)
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect"}),
"Expected connection error, got: %s", panicErr.Error())
})
t.Run("sentinel auth user:password", func(t *testing.T) {
// This will fail to connect, but we're testing that sentinelAuth parameter with user:password format is accepted
// The log output will show "enabling authentication to redis sentinel on ... with user sentinel-user"
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "sentinel-user:sentinel-pass", url, false, 1*time.Second)
})
// Should fail with connection error, not auth error (since we can't connect)
assert.NotNil(t, panicErr)
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect"}),
"Expected connection error, got: %s", panicErr.Error())
})
t.Run("sentinel with timeout", func(t *testing.T) {
// Test that timeout parameter is used
url := "mymaster,localhost:12345"
start := time.Now()
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", url, false, 500*time.Millisecond)
})
duration := time.Since(start)
assert.NotNil(t, panicErr)
// Timeout should be respected (with some tolerance)
assert.True(t, duration < 2*time.Second, "Timeout should be respected, took %v", duration)
})
t.Run("sentinel with multiple addresses", func(t *testing.T) {
// Test that multiple sentinel addresses are accepted in URL format
url := "mymaster,localhost:12345,localhost:12346,localhost:12347"
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", url, false, 1*time.Second)
})
// Should fail with connection error, not format error
assert.NotNil(t, panicErr)
assert.NotContains(t, panicErr.Error(), "Expected master name")
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect"}),
"Expected connection error, got: %s", panicErr.Error())
})
t.Run("sentinel with redis auth but no sentinel auth", func(t *testing.T) {
// Test that redis auth and sentinel auth are separate
// redisAuth is for master/replica, sentinelAuth is for sentinel nodes
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("redis-password", "", url, false, 1*time.Second)
})
// Should fail with connection error (can't test auth without real sentinel)
assert.NotNil(t, panicErr)
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect"}),
"Expected connection error, got: %s", panicErr.Error())
})
t.Run("sentinel with TLS enabled", func(t *testing.T) {
// Test that TLS configuration is accepted (will fail to connect without real TLS server)
// The log output will show "enabling TLS to redis sentinel" which confirms the code path
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("", "", url, true, 1*time.Second)
})
// Should fail with connection/TLS error (can't test TLS without real TLS server)
assert.NotNil(t, panicErr)
// Error could be connection refused, TLS handshake failure, or timeout
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect", "tls", "handshake"}),
"Expected connection/TLS error, got: %s", panicErr.Error())
})
t.Run("sentinel with TLS and sentinel auth", func(t *testing.T) {
// Test that both TLS and sentinel auth can be configured together
// The log output will show both TLS and auth messages
url := "mymaster,localhost:12345"
panicErr := expectPanicError(t, func() {
mkSentinelClient("redis-password", "sentinel-password", url, true, 1*time.Second)
})
// Should fail with connection/TLS error (can't test without real servers)
assert.NotNil(t, panicErr)
assert.True(t,
containsAny(panicErr.Error(), []string{"connection refused", "timeout", "no such host", "connect", "tls", "handshake"}),
"Expected connection/TLS error, got: %s", panicErr.Error())
})
}
// Helper function to check if error message contains any of the given strings
func containsAny(s string, substrs []string) bool {
for _, substr := range substrs {
if strings.Contains(strings.ToLower(s), strings.ToLower(substr)) {
return true
}
}
return false
}
`
Contributor
Author
There was a problem hiding this comment.
Yes of course - Updating now
When using Redis Sentinel with TLS enabled, the client was failing to connect to Sentinel nodes because the TLS configuration was not being applied to the SentinelConnFunc. This caused "SSL wrong version number" errors and connection resets. This fix adds a sentinelDialFunc that properly applies: - TLS configuration (when REDIS_TLS=true) - Authentication settings (when REDIS_AUTH is set) - Connection timeout settings The fix mirrors the approach used for the main Redis connection dial function, ensuring consistent configuration across both Sentinel and data node connections. Fixes connection to Redis Sentinel over TLS. Signed-off-by: Stefan Kolesnikowicz <stefan@sandnetworks.com> Signed-off-by: stekole <stefan@sandnetworks.com>
When using Redis Sentinel with TLS enabled, the client was failing to connect to Sentinel nodes because the TLS configuration was not being applied to the SentinelConnFunc. This caused "SSL wrong version number" errors and connection resets. This fix adds a sentinelDialFunc that properly applies: - TLS configuration (when REDIS_TLS=true) - Authentication settings (when REDIS_AUTH is set) - Connection timeout settings The fix mirrors the approach used for the main Redis connection dial function, ensuring consistent configuration across both Sentinel and data node connections. Fixes connection to Redis Sentinel over TLS. Signed-off-by: Stefan Kolesnikowicz <stefan@sandnetworks.com> Signed-off-by: stekole <stefan@sandnetworks.com>
When using Redis Sentinel with TLS enabled, the client was failing to connect to Sentinel nodes because the TLS configuration was not being applied to the SentinelConnFunc. This caused "SSL wrong version number" errors and connection resets. This fix adds a sentinelDialFunc that properly applies: - TLS configuration (when REDIS_TLS=true) - Authentication settings (when REDIS_AUTH is set) - Connection timeout settings The fix mirrors the approach used for the main Redis connection dial function, ensuring consistent configuration across both Sentinel and data node connections. Fixes connection to Redis Sentinel over TLS. Signed-off-by: Stefan Kolesnikowicz <stefan@sandnetworks.com> Signed-off-by: stekole <stefan@sandnetworks.com>
stekole
force-pushed
the
fix-redis-sentinel-inital-connect-tls
branch
from
79b3d2a to
6dc495d
Compare
Contributor
Author
|
Resolved conflicts - @collin-lee any suggested follow-ups? |
collin-lee
approved these changes
Dec 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When using Redis Sentinel with TLS enabled, the client was failing to connect to Sentinel nodes because the TLS and auth configuration was not being applied to the SentinelConnFunc. This caused this Issue.
This fix adds a sentinelDialFunc that properly applies the following to the initial sentinel calls:
The fix mirrors the approach used for the main Redis connection dial function, ensuring consistent across both Sentinel and data node connections.