Skip to content

Commit 0822e3a

Browse files
mvanhorngaius-qi
mvanhorn
and
gaius-qi
authored
feat(manager): add ExternalRedis TLS support (closes #4734) (#4738)
* feat(manager): add ExternalRedis TLS support (closes #4734) Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * feat(scheduler): mirror external Redis TLS support Per @lavih's review on #4738. The scheduler hits the same external Redis as the manager (issue #4734 is specifically about helm-deployed external Redis), so the TLS plumbing must mirror across both. - scheduler/config: add RedisTLSClientConfig + RedisConfig.TLS field with the same shape as manager (CACert, Cert, Key, InsecureSkipVerify); validate the three required parameters when TLS is set. - scheduler/scheduler.go: build redis.UniversalOptions then attach a tls.Config built via go-connections/tlsconfig when TLS is configured. - scheduler/config/config_test.go: cover the three TLS validation branches. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * Address review feedback: relax Redis TLS validation for managed Redis @lavih asked to relax the validation so managed Redis deployments (AWS ElastiCache, Azure Cache for Redis, GCP Memorystore) can use server-side TLS only. The runtime side (manager/database/database.go, scheduler/scheduler.go) already tolerates empty cert fields via go-connections tlsconfig.Client; only the config validator was overly strict. Permitted shapes after this change: - InsecureSkipVerify=true (no certs required) - CACert only (server-side TLS, common for managed Redis) - CACert + Cert + Key (full mutual TLS, current behavior) Rejected: - Nothing set (caCert AND insecureSkipVerify both missing) - Cert without Key, or Key without Cert (mTLS half-config) Same validation block applies in both manager/config/config.go and scheduler/config/config.go. Test suites in both packages now cover all 6 cases (3 valid, 3 invalid). Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * style: remove comments and add blank lines for consistency Signed-off-by: Gaius <gaius.qi@gmail.com> --------- Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Signed-off-by: Gaius <gaius.qi@gmail.com> Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Gaius <gaius.qi@gmail.com>
1 parent 1db2e55 commit 0822e3a

7 files changed

Lines changed: 301 additions & 4 deletions

File tree

‎manager/config/config.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,24 @@ type RedisConfig struct {
267267
// manager's IP and port need to be exposed.
268268
// Note: Only a single Redis address is supported by the proxy.
269269
Proxy RedisProxyConfig `yaml:"proxy" mapstructure:"proxy"`
270+
271+
// Custom TLS client configuration for connecting to redis.
272+
TLS *RedisTLSClientConfig `yaml:"tls" mapstructure:"tls"`
273+
}
274+
275+
type RedisTLSClientConfig struct {
276+
// CACert is the file path of CA certificate for redis.
277+
CACert string `yaml:"caCert" mapstructure:"caCert"`
278+
279+
// Cert is the client certificate file path.
280+
Cert string `yaml:"cert" mapstructure:"cert"`
281+
282+
// Key is the client key file path.
283+
Key string `yaml:"key" mapstructure:"key"`
284+
285+
// InsecureSkipVerify controls whether a client verifies the
286+
// server's certificate chain and host name.
287+
InsecureSkipVerify bool `yaml:"insecureSkipVerify" mapstructure:"insecureSkipVerify"`
270288
}
271289

272290
type CacheConfig struct {
@@ -654,6 +672,19 @@ func (cfg *Config) Validate() error {
654672
}
655673
}
656674

675+
if cfg.Database.Redis.TLS != nil {
676+
tls := cfg.Database.Redis.TLS
677+
if !tls.InsecureSkipVerify {
678+
if tls.CACert == "" {
679+
return errors.New("redis tls requires parameter caCert or insecureSkipVerify")
680+
}
681+
682+
if (tls.Cert == "") != (tls.Key == "") {
683+
return errors.New("redis tls cert and key must be provided together")
684+
}
685+
}
686+
}
687+
657688
if cfg.Cache.Redis.TTL == 0 {
658689
return errors.New("redis requires parameter ttl")
659690
}

‎manager/config/config_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,112 @@ func TestConfig_Validate(t *testing.T) {
818818
assert.EqualError(err, "redis proxy requires parameter addr")
819819
},
820820
},
821+
{
822+
name: "redis tls allows insecureSkipVerify only",
823+
config: New(),
824+
mock: func(cfg *Config) {
825+
cfg.Auth.JWT = mockJWTConfig
826+
cfg.Database.Type = DatabaseTypeMysql
827+
cfg.Database.Mysql = mockMysqlConfig
828+
cfg.Database.Redis = mockRedisConfig
829+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
830+
InsecureSkipVerify: true,
831+
}
832+
},
833+
expect: func(t *testing.T, err error) {
834+
assert := assert.New(t)
835+
assert.NoError(err)
836+
},
837+
},
838+
{
839+
name: "redis tls allows caCert only",
840+
config: New(),
841+
mock: func(cfg *Config) {
842+
cfg.Auth.JWT = mockJWTConfig
843+
cfg.Database.Type = DatabaseTypeMysql
844+
cfg.Database.Mysql = mockMysqlConfig
845+
cfg.Database.Redis = mockRedisConfig
846+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
847+
CACert: "foo",
848+
}
849+
},
850+
expect: func(t *testing.T, err error) {
851+
assert := assert.New(t)
852+
assert.NoError(err)
853+
},
854+
},
855+
{
856+
name: "redis tls allows mutual tls",
857+
config: New(),
858+
mock: func(cfg *Config) {
859+
cfg.Auth.JWT = mockJWTConfig
860+
cfg.Database.Type = DatabaseTypeMysql
861+
cfg.Database.Mysql = mockMysqlConfig
862+
cfg.Database.Redis = mockRedisConfig
863+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
864+
CACert: "foo",
865+
Cert: "foo",
866+
Key: "foo",
867+
}
868+
},
869+
expect: func(t *testing.T, err error) {
870+
assert := assert.New(t)
871+
assert.NoError(err)
872+
},
873+
},
874+
{
875+
name: "redis tls requires caCert or insecureSkipVerify",
876+
config: New(),
877+
mock: func(cfg *Config) {
878+
cfg.Auth.JWT = mockJWTConfig
879+
cfg.Database.Type = DatabaseTypeMysql
880+
cfg.Database.Mysql = mockMysqlConfig
881+
cfg.Database.Redis = mockRedisConfig
882+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{}
883+
},
884+
expect: func(t *testing.T, err error) {
885+
assert := assert.New(t)
886+
assert.EqualError(err, "redis tls requires parameter caCert or insecureSkipVerify")
887+
},
888+
},
889+
{
890+
name: "redis tls cert and key must be paired",
891+
config: New(),
892+
mock: func(cfg *Config) {
893+
cfg.Auth.JWT = mockJWTConfig
894+
cfg.Database.Type = DatabaseTypeMysql
895+
cfg.Database.Mysql = mockMysqlConfig
896+
cfg.Database.Redis = mockRedisConfig
897+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
898+
CACert: "foo",
899+
Cert: "foo",
900+
Key: "",
901+
}
902+
},
903+
expect: func(t *testing.T, err error) {
904+
assert := assert.New(t)
905+
assert.EqualError(err, "redis tls cert and key must be provided together")
906+
},
907+
},
908+
{
909+
name: "redis tls cert and key must be paired (reverse)",
910+
config: New(),
911+
mock: func(cfg *Config) {
912+
cfg.Auth.JWT = mockJWTConfig
913+
cfg.Database.Type = DatabaseTypeMysql
914+
cfg.Database.Mysql = mockMysqlConfig
915+
cfg.Database.Redis = mockRedisConfig
916+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
917+
CACert: "foo",
918+
Cert: "",
919+
Key: "foo",
920+
}
921+
},
922+
expect: func(t *testing.T, err error) {
923+
assert := assert.New(t)
924+
assert.EqualError(err, "redis tls cert and key must be provided together")
925+
},
926+
},
821927
{
822928
name: "local requires parameter size",
823929
config: New(),

‎manager/database/database.go‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"os"
2424
"time"
2525

26+
"github.com/docker/go-connections/tlsconfig"
2627
caches "github.com/go-gorm/caches/v4"
2728
"github.com/redis/go-redis/v9"
2829
"gorm.io/gorm"
@@ -93,7 +94,7 @@ func New(cfg *config.Config) (*Database, error) {
9394
return nil, err
9495
}
9596

96-
rdb, err := pkgredis.NewRedis(&redis.UniversalOptions{
97+
redisOpts := &redis.UniversalOptions{
9798
Addrs: cfg.Database.Redis.Addrs,
9899
MasterName: cfg.Database.Redis.MasterName,
99100
DB: cfg.Database.Redis.DB,
@@ -103,7 +104,23 @@ func New(cfg *config.Config) (*Database, error) {
103104
SentinelPassword: cfg.Database.Redis.SentinelPassword,
104105
PoolSize: cfg.Database.Redis.PoolSize,
105106
PoolTimeout: cfg.Database.Redis.PoolTimeout,
106-
})
107+
}
108+
109+
if redisTLS := cfg.Database.Redis.TLS; redisTLS != nil {
110+
tlsCfg, err := tlsconfig.Client(tlsconfig.Options{
111+
CAFile: redisTLS.CACert,
112+
CertFile: redisTLS.Cert,
113+
KeyFile: redisTLS.Key,
114+
InsecureSkipVerify: redisTLS.InsecureSkipVerify,
115+
})
116+
if err != nil {
117+
return nil, err
118+
}
119+
120+
redisOpts.TLSConfig = tlsCfg
121+
}
122+
123+
rdb, err := pkgredis.NewRedis(redisOpts)
107124
if err != nil {
108125
return nil, err
109126
}

‎pkg/redis/redis.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ func NewRedis(cfg *redis.UniversalOptions) (redis.UniversalClient, error) {
8888
SentinelPassword: cfg.SentinelPassword,
8989
PoolSize: cfg.PoolSize,
9090
PoolTimeout: cfg.PoolTimeout,
91+
TLSConfig: cfg.TLSConfig,
9192
})
9293

9394
if err := client.Ping(context.Background()).Err(); err != nil {

‎scheduler/config/config.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,24 @@ type RedisConfig struct {
296296

297297
// BackendDB is backend database name.
298298
BackendDB int `yaml:"backendDB" mapstructure:"backendDB"`
299+
300+
// Custom TLS client configuration for connecting to redis.
301+
TLS *RedisTLSClientConfig `yaml:"tls" mapstructure:"tls"`
302+
}
303+
304+
type RedisTLSClientConfig struct {
305+
// CACert is the file path of CA certificate for redis.
306+
CACert string `yaml:"caCert" mapstructure:"caCert"`
307+
308+
// Cert is the client certificate file path.
309+
Cert string `yaml:"cert" mapstructure:"cert"`
310+
311+
// Key is the client key file path.
312+
Key string `yaml:"key" mapstructure:"key"`
313+
314+
// InsecureSkipVerify controls whether a client verifies the
315+
// server's certificate chain and host name.
316+
InsecureSkipVerify bool `yaml:"insecureSkipVerify" mapstructure:"insecureSkipVerify"`
299317
}
300318

301319
type MetricsConfig struct {
@@ -480,6 +498,19 @@ func (cfg *Config) Validate() error {
480498
return errors.New("redis requires parameter backendDB")
481499
}
482500

501+
if cfg.Database.Redis.TLS != nil {
502+
tls := cfg.Database.Redis.TLS
503+
if !tls.InsecureSkipVerify {
504+
if tls.CACert == "" {
505+
return errors.New("redis tls requires parameter caCert or insecureSkipVerify")
506+
}
507+
508+
if (tls.Cert == "") != (tls.Key == "") {
509+
return errors.New("redis tls cert and key must be provided together")
510+
}
511+
}
512+
}
513+
483514
if cfg.DynConfig.RefreshInterval <= 0 {
484515
return errors.New("dynconfig requires parameter refreshInterval")
485516
}

‎scheduler/config/config_test.go‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,100 @@ func TestConfig_Validate(t *testing.T) {
329329
assert.EqualError(err, "redis requires parameter backendDB")
330330
},
331331
},
332+
{
333+
name: "redis tls allows insecureSkipVerify only",
334+
config: New(),
335+
mock: func(cfg *Config) {
336+
cfg.Manager = mockManagerConfig
337+
cfg.Database.Redis = mockRedisConfig
338+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
339+
InsecureSkipVerify: true,
340+
}
341+
},
342+
expect: func(t *testing.T, err error) {
343+
assert := assert.New(t)
344+
assert.NoError(err)
345+
},
346+
},
347+
{
348+
name: "redis tls allows caCert only",
349+
config: New(),
350+
mock: func(cfg *Config) {
351+
cfg.Manager = mockManagerConfig
352+
cfg.Database.Redis = mockRedisConfig
353+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
354+
CACert: "foo",
355+
}
356+
},
357+
expect: func(t *testing.T, err error) {
358+
assert := assert.New(t)
359+
assert.NoError(err)
360+
},
361+
},
362+
{
363+
name: "redis tls allows mutual tls",
364+
config: New(),
365+
mock: func(cfg *Config) {
366+
cfg.Manager = mockManagerConfig
367+
cfg.Database.Redis = mockRedisConfig
368+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
369+
CACert: "foo",
370+
Cert: "foo",
371+
Key: "foo",
372+
}
373+
},
374+
expect: func(t *testing.T, err error) {
375+
assert := assert.New(t)
376+
assert.NoError(err)
377+
},
378+
},
379+
{
380+
name: "redis tls requires caCert or insecureSkipVerify",
381+
config: New(),
382+
mock: func(cfg *Config) {
383+
cfg.Manager = mockManagerConfig
384+
cfg.Database.Redis = mockRedisConfig
385+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{}
386+
},
387+
expect: func(t *testing.T, err error) {
388+
assert := assert.New(t)
389+
assert.EqualError(err, "redis tls requires parameter caCert or insecureSkipVerify")
390+
},
391+
},
392+
{
393+
name: "redis tls cert and key must be paired",
394+
config: New(),
395+
mock: func(cfg *Config) {
396+
cfg.Manager = mockManagerConfig
397+
cfg.Database.Redis = mockRedisConfig
398+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
399+
CACert: "foo",
400+
Cert: "foo",
401+
Key: "",
402+
}
403+
},
404+
expect: func(t *testing.T, err error) {
405+
assert := assert.New(t)
406+
assert.EqualError(err, "redis tls cert and key must be provided together")
407+
},
408+
},
409+
{
410+
name: "redis tls cert and key must be paired (reverse)",
411+
config: New(),
412+
mock: func(cfg *Config) {
413+
cfg.Manager = mockManagerConfig
414+
cfg.Database.Redis = mockRedisConfig
415+
cfg.Database.Redis.TLS = &RedisTLSClientConfig{
416+
CACert: "foo",
417+
Cert: "",
418+
Key: "foo",
419+
}
420+
},
421+
expect: func(t *testing.T, err error) {
422+
assert := assert.New(t)
423+
assert.EqualError(err, "redis tls cert and key must be provided together")
424+
},
425+
},
332426
{
333427
name: "scheduler requires parameter algorithm",
334428
config: New(),

‎scheduler/scheduler.go‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"strconv"
2525
"time"
2626

27+
"github.com/docker/go-connections/tlsconfig"
2728
"github.com/redis/go-redis/v9"
2829
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
2930
"google.golang.org/grpc"
@@ -118,7 +119,7 @@ func New(ctx context.Context, cfg *config.Config, d dfpath.Dfpath) (*Server, err
118119
// Initialize redis client.
119120
var rdb redis.UniversalClient
120121
if pkgredis.IsEnabled(cfg.Database.Redis.Addrs) {
121-
rdb, err = pkgredis.NewRedis(&redis.UniversalOptions{
122+
redisOpts := &redis.UniversalOptions{
122123
Addrs: cfg.Database.Redis.Addrs,
123124
MasterName: cfg.Database.Redis.MasterName,
124125
Username: cfg.Database.Redis.Username,
@@ -127,7 +128,23 @@ func New(ctx context.Context, cfg *config.Config, d dfpath.Dfpath) (*Server, err
127128
SentinelPassword: cfg.Database.Redis.SentinelPassword,
128129
PoolSize: cfg.Database.Redis.PoolSize,
129130
PoolTimeout: cfg.Database.Redis.PoolTimeout,
130-
})
131+
}
132+
133+
if redisTLS := cfg.Database.Redis.TLS; redisTLS != nil {
134+
tlsCfg, err := tlsconfig.Client(tlsconfig.Options{
135+
CAFile: redisTLS.CACert,
136+
CertFile: redisTLS.Cert,
137+
KeyFile: redisTLS.Key,
138+
InsecureSkipVerify: redisTLS.InsecureSkipVerify,
139+
})
140+
if err != nil {
141+
return nil, err
142+
}
143+
144+
redisOpts.TLSConfig = tlsCfg
145+
}
146+
147+
rdb, err = pkgredis.NewRedis(redisOpts)
131148
if err != nil {
132149
return nil, err
133150
}

0 commit comments

Comments
 (0)