From 51a41e239c7c5d0c1150530e2db3c4b30b75d69b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 9 Aug 2024 10:29:31 -0700 Subject: [PATCH 1/2] Allow Redis password to be specified in Workhorse URL Previously the Workhorse config allowed a Redis URL to be used, but the password component had to be specified inside the `Password` config field. However, it's common for the URL to contain both the username and password, so we should allow that. The `Password` field takes precedence if both are specified, however. Changelog: changed --- workhorse/internal/redis/redis.go | 4 ++- workhorse/internal/redis/redis_test.go | 42 ++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/workhorse/internal/redis/redis.go b/workhorse/internal/redis/redis.go index b4262f3a4593f9..68fa8829d82b11 100644 --- a/workhorse/internal/redis/redis.go +++ b/workhorse/internal/redis/redis.go @@ -176,7 +176,9 @@ func configureRedis(cfg *config.RedisConfig) (*redis.Client, error) { } opt.DB = getOrDefault(cfg.DB, 0) - opt.Password = cfg.Password + if cfg.Password != "" { + opt.Password = cfg.Password + } opt.PoolSize = getOrDefault(cfg.MaxActive, defaultMaxActive) opt.MaxIdleConns = getOrDefault(cfg.MaxIdle, defaultMaxIdle) diff --git a/workhorse/internal/redis/redis_test.go b/workhorse/internal/redis/redis_test.go index ab258c524b820a..3cf66a12203160 100644 --- a/workhorse/internal/redis/redis_test.go +++ b/workhorse/internal/redis/redis_test.go @@ -2,6 +2,7 @@ package redis import ( "context" + "fmt" "net" "sync/atomic" "testing" @@ -48,7 +49,11 @@ func TestConfigureConfigWithoutRedis(t *testing.T) { func TestConfigureValidConfigX(t *testing.T) { testCases := []struct { - scheme string + scheme string + username string + urlPassword string + redisPassword string + expectedPassword string }{ { scheme: "redis", @@ -59,6 +64,24 @@ func TestConfigureValidConfigX(t *testing.T) { { scheme: "tcp", }, + { + scheme: "redis", + username: "redis-user", + urlPassword: "redis-password", + expectedPassword: "redis-password", + }, + { + scheme: "redis", + redisPassword: "override-password", + expectedPassword: "override-password", + }, + { + scheme: "redis", + username: "redis-user", + urlPassword: "redis-password", + redisPassword: "override-password", + expectedPassword: "override-password", + }, } for _, tc := range testCases { @@ -66,8 +89,18 @@ func TestConfigureValidConfigX(t *testing.T) { connectReceived := atomic.Value{} a := mockRedisServer(t, &connectReceived) - parsedURL := helper.URLMustParse(tc.scheme + "://" + a) - redisCfg := &config.RedisConfig{URL: config.TomlURL{URL: *parsedURL}} + var u string + if tc.username != "" || tc.urlPassword != "" { + u = fmt.Sprintf("%s://%s:%s@%s", tc.scheme, tc.username, tc.urlPassword, a) + } else { + u = fmt.Sprintf("%s://%s", tc.scheme, a) + } + + parsedURL := helper.URLMustParse(u) + redisCfg := &config.RedisConfig{ + URL: config.TomlURL{URL: *parsedURL}, + Password: tc.redisPassword, + } cfg := &config.Config{Redis: redisCfg} rdb, err := Configure(cfg) @@ -75,6 +108,9 @@ func TestConfigureValidConfigX(t *testing.T) { defer rdb.Close() require.NotNil(t, rdb.Conn(), "Pool should not be nil") + opt := rdb.Options() + require.Equal(t, tc.username, opt.Username) + require.Equal(t, tc.expectedPassword, opt.Password) // goredis initialise connections lazily rdb.Ping(context.Background()) -- GitLab From af78cbed7c4b744d5d86645830a364b7a5ef8321 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 9 Aug 2024 10:41:02 -0700 Subject: [PATCH 2/2] Fix lint errors with Workhorse redis_test.go --- workhorse/_support/lint_last_known_acceptable_go1.21.txt | 4 ---- workhorse/_support/lint_last_known_acceptable_go1.22.txt | 4 ---- workhorse/internal/redis/redis_test.go | 9 +++++---- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/workhorse/_support/lint_last_known_acceptable_go1.21.txt b/workhorse/_support/lint_last_known_acceptable_go1.21.txt index 0911364d46f6d6..89c077e70f338d 100644 --- a/workhorse/_support/lint_last_known_acceptable_go1.21.txt +++ b/workhorse/_support/lint_last_known_acceptable_go1.21.txt @@ -181,10 +181,6 @@ internal/redis/redis.go:18:2: blank-imports: a blank import should be only in a internal/redis/redis.go:23:24: var-declaration: should omit type error from declaration of var errSentinelMasterAddr; it will be inferred from the right-hand side (revive) internal/redis/redis.go:25:2: exported: exported var TotalConnections should have comment or be unexported (revive) internal/redis/redis.go:110:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic) -internal/redis/redis_test.go:24:2: error-nil: use require.NoError (testifylint) -internal/redis/redis_test.go:29:3: error-nil: use require.NoError (testifylint) -internal/redis/redis_test.go:79:15: `initialise` is a misspelling of `initialize` (misspell) -internal/redis/redis_test.go:119:15: `initialise` is a misspelling of `initialize` (misspell) internal/senddata/contentprocessor/contentprocessor.go:136:35: response body must be closed (bodyclose) internal/sendfile/sendfile_test.go:180:34: response body must be closed (bodyclose) internal/sendurl/sendurl.go:100: Function 'Inject' has too many statements (51 > 40) (funlen) diff --git a/workhorse/_support/lint_last_known_acceptable_go1.22.txt b/workhorse/_support/lint_last_known_acceptable_go1.22.txt index 98734858876c62..b623020d97b36b 100644 --- a/workhorse/_support/lint_last_known_acceptable_go1.22.txt +++ b/workhorse/_support/lint_last_known_acceptable_go1.22.txt @@ -181,10 +181,6 @@ internal/redis/redis.go:18:2: blank-imports: a blank import should be only in a internal/redis/redis.go:23:24: var-declaration: should omit type error from declaration of var errSentinelMasterAddr; it will be inferred from the right-hand side (revive) internal/redis/redis.go:25:2: exported: exported var TotalConnections should have comment or be unexported (revive) internal/redis/redis.go:110:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic) -internal/redis/redis_test.go:24:2: error-nil: use require.NoError (testifylint) -internal/redis/redis_test.go:29:3: error-nil: use require.NoError (testifylint) -internal/redis/redis_test.go:79:15: `initialise` is a misspelling of `initialize` (misspell) -internal/redis/redis_test.go:119:15: `initialise` is a misspelling of `initialize` (misspell) internal/senddata/contentprocessor/contentprocessor.go:136:35: response body must be closed (bodyclose) internal/sendfile/sendfile_test.go:180:34: response body must be closed (bodyclose) internal/sendurl/sendurl.go:100: Function 'Inject' has too many statements (51 > 40) (funlen) diff --git a/workhorse/internal/redis/redis_test.go b/workhorse/internal/redis/redis_test.go index 3cf66a12203160..88dc73aed6623d 100644 --- a/workhorse/internal/redis/redis_test.go +++ b/workhorse/internal/redis/redis_test.go @@ -7,6 +7,7 @@ import ( "sync/atomic" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab/workhorse/internal/config" @@ -22,12 +23,12 @@ const ( func mockRedisServer(t *testing.T, connectReceived *atomic.Value) string { ln, err := net.Listen("tcp", "127.0.0.1:0") - require.Nil(t, err) + require.NoError(t, err) go func() { defer ln.Close() conn, err := ln.Accept() - require.Nil(t, err) + assert.NoError(t, err) connectReceived.Store(true) conn.Write([]byte("OK\n")) }() @@ -112,7 +113,7 @@ func TestConfigureValidConfigX(t *testing.T) { require.Equal(t, tc.username, opt.Username) require.Equal(t, tc.expectedPassword, opt.Password) - // goredis initialise connections lazily + // goredis initialize connections lazily rdb.Ping(context.Background()) require.True(t, connectReceived.Load().(bool)) }) @@ -152,7 +153,7 @@ func TestConnectToSentinel(t *testing.T) { require.NotNil(t, rdb.Conn(), "Pool should not be nil") - // goredis initialise connections lazily + // goredis initialize connections lazily rdb.Ping(context.Background()) require.True(t, connectReceived.Load().(bool)) }) -- GitLab