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.

Sign up for GitHub

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

Configurable Redis TLS Config through settings #289

Merged
merged 5 commits into from
Sep 9, 2021
Merged

Configurable Redis TLS Config through settings #289

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1eec7db
Configurable Redis TLS through settings
JavierR14 Aug 19, 2021
e182ecd
Fix failing integration test
JavierR14 Sep 8, 2021
af7220a
Add mention of configurable Redis TLS Config in README.
JavierR14 Sep 8, 2021
5b63d8c
Fix formatting of settings.go
JavierR14 Sep 8, 2021
22dbdea
Add TODO in settings.go and remove readme line
JavierR14 Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@ As well Ratelimit supports TLS connections and authentication. These can be conf
1. `REDIS_AUTH` & `REDIS_PERSECOND_AUTH`: set to `"password"` to enable authentication to the redis host.
1. `CACHE_KEY_PREFIX`: a string to prepend to all cache keys

Ratelimit also provides the ability to specify TLS settings via `RedisTlsConfig` in the [settings](https://github.com/envoyproxy/ratelimit/blob/master/src/settings/settings.go).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's not clear to me how this is used. Does someone have to instantiate this by code? Or can this be set via env variables?

/wait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone has to instantiate it by code. You shouldn't be setting a TLS config through env variables as it would require dealing with certs and key files and needs code. TLS configs should be configurable and this allows the client using ratelimit to configure it if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then I guess this isn't a general purpose feature so I'm not sure it's worth documenting like this. It would probably be better long term to allow the config to be read from a YAML file of some type. I would remove this from the README and put a TODO in the settings code to have that setting be out of box user configurable somehow.

/wait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've added a TODO line. I do think providing an out of the box user configurable setting is a good idea but ultimately will probably not be used much. Setting up a Redis TLS config will be very different from case to case and most of the time users will provide it through code.


## Redis type

Ratelimit supports different types of redis deployments:
Expand Down
4 changes: 2 additions & 2 deletions src/redis/cache_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freeca
var perSecondPool Client
if s.RedisPerSecond {
perSecondPool = NewClientImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, s.RedisPerSecondSocketType,
s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit)
s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit, s.RedisTlsConfig)
}
var otherPool Client
otherPool = NewClientImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisSocketType, s.RedisType, s.RedisUrl, s.RedisPoolSize,
s.RedisPipelineWindow, s.RedisPipelineLimit)
s.RedisPipelineWindow, s.RedisPipelineLimit, s.RedisTlsConfig)

return NewFixedRateLimitCacheImpl(
otherPool,
Expand Down
8 changes: 6 additions & 2 deletions src/redis/driver_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,18 @@ func checkError(err error) {
}

func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisType, url string, poolSize int,
pipelineWindow time.Duration, pipelineLimit int) Client {
pipelineWindow time.Duration, pipelineLimit int, tlsConfig *tls.Config) Client {
logger.Warnf("connecting to redis on %s with pool size %d", url, poolSize)

df := func(network, addr string) (radix.Conn, error) {
var dialOpts []radix.DialOpt

if useTls {
dialOpts = append(dialOpts, radix.DialUseTLS(&tls.Config{}))
if tlsConfig != nil {
dialOpts = append(dialOpts, radix.DialUseTLS(tlsConfig))
} else {
dialOpts = append(dialOpts, radix.DialUseTLS(&tls.Config{}))
}
}

if auth != "" {
Expand Down
3 changes: 3 additions & 0 deletions src/settings/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package settings

import (
"crypto/tls"
"time"

"github.com/kelseyhightower/envconfig"
Expand Down Expand Up @@ -48,6 +49,8 @@ type Settings struct {
RedisPoolSize int `envconfig:"REDIS_POOL_SIZE" default:"10"`
RedisAuth string `envconfig:"REDIS_AUTH" default:""`
RedisTls bool `envconfig:"REDIS_TLS" default:"false"`
RedisTlsConfig *tls.Config

// RedisPipelineWindow sets the duration after which internal pipelines will be flushed.
// If window is zero then implicit pipelining will be disabled. Radix use 150us for the
// default value, see https://github.com/mediocregopher/radix/blob/v3.5.1/pool.go#L278.
Expand Down
1 change: 1 addition & 0 deletions test/integration/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ func TestMultiNodeMemcache(t *testing.T) {

func testBasicConfigAuthTLS(perSecond bool, local_cache_size int) func(*testing.T) {
s := makeSimpleRedisSettings(16381, 16382, perSecond, local_cache_size)
s.RedisTlsConfig = nil
s.RedisAuth = "password123"
s.RedisTls = true
s.RedisPerSecondAuth = "password123"
Expand Down
2 changes: 1 addition & 1 deletion test/redis/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func BenchmarkParallelDoLimit(b *testing.B) {
return func(b *testing.B) {
statsStore := gostats.NewStore(gostats.NewNullSink(), false)
sm := stats.NewMockStatManager(statsStore)
client := redis.NewClientImpl(statsStore, false, "", "tcp", "single", "127.0.0.1:6379", poolSize, pipelineWindow, pipelineLimit)
client := redis.NewClientImpl(statsStore, false, "", "tcp", "single", "127.0.0.1:6379", poolSize, pipelineWindow, pipelineLimit, nil)
defer client.Close()

cache := redis.NewFixedRateLimitCacheImpl(client, nil, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), 10, nil, 0.8, "", sm)
Expand Down
6 changes: 3 additions & 3 deletions test/redis/driver_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func testNewClientImpl(t *testing.T, pipelineWindow time.Duration, pipelineLimit
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)
return redis.NewClientImpl(statsStore, false, auth, "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil)
}

t.Run("connection refused", func(t *testing.T) {
Expand Down Expand Up @@ -103,7 +103,7 @@ 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)
return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, 0, 0, nil)
}

t.Run("SETGET ok", func(t *testing.T) {
Expand Down Expand Up @@ -148,7 +148,7 @@ func testPipeDo(t *testing.T, pipelineWindow time.Duration, pipelineLimit int) f
statsStore := stats.NewStore(stats.NewNullSink(), false)

mkRedisClient := func(addr string) redis.Client {
return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit)
return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil)
}

t.Run("SETGET ok", func(t *testing.T) {
Expand Down