From 8bda07655ce9765b4b9a503b09bf9c692cf5744c Mon Sep 17 00:00:00 2001 From: Rajendra kadam Date: Fri, 5 Jun 2026 16:49:15 +0530 Subject: [PATCH] Extract Redis initialization out of runServeCmd (#46830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts the Redis pool and the cached_mysql / mysqlredis datastore wrappers out of `runServeCmd` and into a new `cmd/fleet/redis.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742). Continues the path toward `serve.go` >60% coverage per the discussion on #33370. Three functions come out of the inline block: - `initRedis` — builds the Redis pool, wraps the datastore with `cached_mysql.New`, and applies `mysqlredis.New` with the license-enforced host limit and host-cache options. Returns the pool, the fully wrapped `fleet.Datastore`, and the outermost `*mysqlredis.Datastore` (a few callers need the concrete type). - `buildRedisPoolConfig` — translates `config.RedisConfig` into the `redis.PoolConfig`, including the `redis://` scheme strip. - `validateRedisConfig` — encodes the host-cache invariant: `HostCacheEnabled` requires `HostCacheTTL > 0`. Returns an error so the caller (or in this case `initRedis` via `initFatal`) can refuse boot without that decision being buried inside a pure builder. Behavior is preserved — `runServeCmd` calls these in the same order with the same arguments, the host-cache validation still aborts startup when violated, and the full `cmd/fleet` suite passes against MySQL + Redis. `initRedis` returns early after `initFatal` so it's safe when the caller's `initFatal` doesn't terminate (the case in tests). Following the precedent established on #46742, the caller also has a loud `initFatal` + `return` guard against a nil pool (covers the same nilaway flow we hit on the datastore slice). On test scope: `TestValidateRedisConfig` covers all four combinations of `HostCacheEnabled` and `HostCacheTTL` — that's the real boot/refuse-to-boot decision. `TestBuildRedisPoolConfigStripsScheme` pins the `redis://` scheme-strip contract for Render-style URIs. I didn't add a `buildRedisPoolConfig` field-mapping matrix or an `initRedis` happy-path unit test: the former would just re-state the struct literal, and the latter needs a real Redis pool (the smoke boot exercises it end-to-end instead). This completes the four named init-block extractions on this issue. If further coverage gains are needed beyond what these have already moved, the next conversation is whether to test `runServeCmd` directly via the injected `initFatal`. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change ## Summary by CodeRabbit * **Refactor** * Consolidated Redis initialization and datastore wrapping into a dedicated helper; startup now validates the Redis pool and handles initialization failures explicitly. * **Tests** * Added unit tests for Redis address handling and host-cache TTL validation to ensure config behavior is enforced. --- cmd/fleet/redis.go | 120 ++++++++++++++++++++++++++++++++++++++++ cmd/fleet/redis_test.go | 71 ++++++++++++++++++++++++ cmd/fleet/serve.go | 61 ++------------------ 3 files changed, 197 insertions(+), 55 deletions(-) create mode 100644 cmd/fleet/redis.go create mode 100644 cmd/fleet/redis_test.go diff --git a/cmd/fleet/redis.go b/cmd/fleet/redis.go new file mode 100644 index 0000000000..9d9b85f293 --- /dev/null +++ b/cmd/fleet/redis.go @@ -0,0 +1,120 @@ +package main + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + + "github.com/fleetdm/fleet/v4/server/config" + "github.com/fleetdm/fleet/v4/server/datastore/cached_mysql" + "github.com/fleetdm/fleet/v4/server/datastore/mysqlredis" + "github.com/fleetdm/fleet/v4/server/datastore/redis" + "github.com/fleetdm/fleet/v4/server/fleet" +) + +// buildRedisPoolConfig translates the Fleet Redis config into the redis +// package's PoolConfig. The address has its "redis://" scheme stripped so +// providers that publish a full URI (e.g. Render's managed Redis) work +// without a separate config knob. +func buildRedisPoolConfig(cfg config.RedisConfig) redis.PoolConfig { + return redis.PoolConfig{ + // Strip the Redis URI scheme if it's present. Scheme docs are at: + // https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml + // In the future, we could support the full Redis URI if needed + // (including username, password, database, etc.) + Server: strings.TrimPrefix(cfg.Address, "redis://"), + Username: cfg.Username, + Password: cfg.Password, + Database: cfg.Database, + UseTLS: cfg.UseTLS, + Region: cfg.Region, + CacheName: cfg.CacheName, + StsAssumeRoleArn: cfg.StsAssumeRoleArn, + StsExternalID: cfg.StsExternalID, + ConnTimeout: cfg.ConnectTimeout, + KeepAlive: cfg.KeepAlive, + ConnectRetryAttempts: cfg.ConnectRetryAttempts, + ClusterFollowRedirections: cfg.ClusterFollowRedirections, + ClusterReadFromReplica: cfg.ClusterReadFromReplica, + TLSCert: cfg.TLSCert, + TLSKey: cfg.TLSKey, + TLSCA: cfg.TLSCA, + TLSServerName: cfg.TLSServerName, + TLSHandshakeTimeout: cfg.TLSHandshakeTimeout, + MaxIdleConns: cfg.MaxIdleConns, + MaxOpenConns: cfg.MaxOpenConns, + ConnMaxLifetime: cfg.ConnMaxLifetime, + IdleTimeout: cfg.IdleTimeout, + ConnWaitTimeout: cfg.ConnWaitTimeout, + WriteTimeout: cfg.WriteTimeout, + ReadTimeout: cfg.ReadTimeout, + } +} + +// validateRedisConfig returns a non-nil error when the Redis host-cache +// configuration is inconsistent (enabled without a positive TTL). It +// encodes the boot/refuse-to-boot rule for the cache configuration so the +// decision can be unit-tested without spinning up Redis. +func validateRedisConfig(cfg config.RedisConfig) error { + if cfg.HostCacheEnabled && cfg.HostCacheTTL <= 0 { + return fmt.Errorf("redis.host_cache_ttl must be > 0 when redis.host_cache_enabled is true (got %s)", cfg.HostCacheTTL) + } + return nil +} + +// initRedis brings up the Redis pool and the two datastore wrappers that +// depend on it: cached_mysql (in-memory caching layer over the datastore) +// and mysqlredis (Redis-backed host lookup and license-enforced host +// limit). Failures go through initFatal. Returns nil values on the +// failure path so the function is safe when initFatal does not terminate +// (e.g., tests using a recorder). +// +// The returned fleet.Datastore is the fully wrapped chain (mysqlredis → +// cached_mysql → input ds); the returned *mysqlredis.Datastore is the +// outermost wrapper, which a few callers need by concrete type. +func initRedis( + ctx context.Context, + cfg config.FleetConfig, + license *fleet.LicenseInfo, + ds fleet.Datastore, + logger *slog.Logger, + initFatal func(err error, msg string), +) (fleet.RedisPool, fleet.Datastore, *mysqlredis.Datastore) { + if license == nil { + initFatal(errors.New("license was nil"), "initialize Redis") + return nil, nil, nil + } + + // Validate cheap local config before dialing Redis: surfaces a + // host-cache config error as itself, not as a connectivity failure, + // and avoids opening a pool that would be discarded if initFatal is + // swapped (e.g., a test recorder) and execution continues. + if err := validateRedisConfig(cfg.Redis); err != nil { + initFatal(err, "validate host cache configuration") + return nil, nil, nil + } + + redisPool, err := redis.NewPool(buildRedisPoolConfig(cfg.Redis)) + if err != nil { + initFatal(err, "initialize Redis") + return nil, nil, nil + } + logger.InfoContext(ctx, "redis initialized", "component", "redis", "mode", redisPool.Mode()) + + wrappedDS := cached_mysql.New(ds) + + var dsOpts []mysqlredis.Option + if license.DeviceCount > 0 && cfg.License.EnforceHostLimit { + dsOpts = append(dsOpts, mysqlredis.WithEnforcedHostLimit(license.DeviceCount)) + } + if cfg.Redis.HostCacheEnabled { + dsOpts = append(dsOpts, mysqlredis.WithHostCache(cfg.Redis.HostCacheTTL)) + logger.InfoContext(ctx, "host lookup redis cache enabled", + "component", "mysqlredis", "ttl", cfg.Redis.HostCacheTTL) + } + + redisWrapperDS := mysqlredis.New(wrappedDS, redisPool, dsOpts...) + return redisPool, redisWrapperDS, redisWrapperDS +} diff --git a/cmd/fleet/redis_test.go b/cmd/fleet/redis_test.go new file mode 100644 index 0000000000..12a66aeda4 --- /dev/null +++ b/cmd/fleet/redis_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "testing" + "time" + + "github.com/fleetdm/fleet/v4/server/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateRedisConfig(t *testing.T) { + for _, tc := range []struct { + name string + cfg config.RedisConfig + wantErr bool + wantSub string + }{ + { + name: "host cache disabled with zero ttl is ok", + cfg: config.RedisConfig{HostCacheEnabled: false, HostCacheTTL: 0}, + }, + { + name: "host cache disabled with negative ttl is ok", + cfg: config.RedisConfig{HostCacheEnabled: false, HostCacheTTL: -1 * time.Second}, + }, + { + name: "host cache enabled with positive ttl is ok", + cfg: config.RedisConfig{HostCacheEnabled: true, HostCacheTTL: 5 * time.Minute}, + }, + { + name: "host cache enabled with zero ttl is rejected", + cfg: config.RedisConfig{HostCacheEnabled: true, HostCacheTTL: 0}, + wantErr: true, + wantSub: "host_cache_ttl must be > 0", + }, + { + name: "host cache enabled with negative ttl is rejected", + cfg: config.RedisConfig{HostCacheEnabled: true, HostCacheTTL: -1 * time.Second}, + wantErr: true, + wantSub: "host_cache_ttl must be > 0", + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateRedisConfig(tc.cfg) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantSub) + return + } + assert.NoError(t, err) + }) + } +} + +func TestBuildRedisPoolConfigStripsScheme(t *testing.T) { + for _, tc := range []struct { + name string + input string + want string + }{ + {name: "scheme stripped", input: "redis://example.com:6379", want: "example.com:6379"}, + {name: "no scheme passes through", input: "example.com:6379", want: "example.com:6379"}, + {name: "empty passes through", input: "", want: ""}, + } { + t.Run(tc.name, func(t *testing.T) { + got := buildRedisPoolConfig(config.RedisConfig{Address: tc.input}) + assert.Equal(t, tc.want, got.Server) + }) + } +} diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index b647979e35..ffec1d1722 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -52,7 +52,6 @@ import ( "github.com/fleetdm/fleet/v4/server/contexts/installersize" licensectx "github.com/fleetdm/fleet/v4/server/contexts/license" "github.com/fleetdm/fleet/v4/server/cron" - "github.com/fleetdm/fleet/v4/server/datastore/cached_mysql" "github.com/fleetdm/fleet/v4/server/datastore/failing" "github.com/fleetdm/fleet/v4/server/datastore/filesystem" "github.com/fleetdm/fleet/v4/server/datastore/mysql" @@ -276,61 +275,13 @@ func runServeCmd(cmd *cobra.Command, configManager configpkg.Manager, debug, dev } } - // Strip the Redis URI scheme if it's present. Scheme docs are at: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml - // This allows us to use Render's Redis service in render.yaml, including the free tier. - // In the future, we could support the full Redis URI if needed (including username, password, database, etc.) - redisAddress := strings.TrimPrefix(config.Redis.Address, "redis://") - redisPool, err := redis.NewPool(redis.PoolConfig{ - Server: redisAddress, - Username: config.Redis.Username, - Password: config.Redis.Password, - Database: config.Redis.Database, - UseTLS: config.Redis.UseTLS, - Region: config.Redis.Region, - CacheName: config.Redis.CacheName, - StsAssumeRoleArn: config.Redis.StsAssumeRoleArn, - StsExternalID: config.Redis.StsExternalID, - ConnTimeout: config.Redis.ConnectTimeout, - KeepAlive: config.Redis.KeepAlive, - ConnectRetryAttempts: config.Redis.ConnectRetryAttempts, - ClusterFollowRedirections: config.Redis.ClusterFollowRedirections, - ClusterReadFromReplica: config.Redis.ClusterReadFromReplica, - TLSCert: config.Redis.TLSCert, - TLSKey: config.Redis.TLSKey, - TLSCA: config.Redis.TLSCA, - TLSServerName: config.Redis.TLSServerName, - TLSHandshakeTimeout: config.Redis.TLSHandshakeTimeout, - MaxIdleConns: config.Redis.MaxIdleConns, - MaxOpenConns: config.Redis.MaxOpenConns, - ConnMaxLifetime: config.Redis.ConnMaxLifetime, - IdleTimeout: config.Redis.IdleTimeout, - ConnWaitTimeout: config.Redis.ConnWaitTimeout, - WriteTimeout: config.Redis.WriteTimeout, - ReadTimeout: config.Redis.ReadTimeout, - }) - if err != nil { - initFatal(err, "initialize Redis") + var redisPool fleet.RedisPool + var redisWrapperDS *mysqlredis.Datastore + redisPool, ds, redisWrapperDS = initRedis(cmd.Context(), config, license, ds, logger, initFatal) + if redisPool == nil { + initFatal(errors.New("redis pool was nil after initialization"), "initialize Redis") + return } - logger.InfoContext(cmd.Context(), "redis initialized", "component", "redis", "mode", redisPool.Mode()) - - ds = cached_mysql.New(ds) - var dsOpts []mysqlredis.Option - if license.DeviceCount > 0 && config.License.EnforceHostLimit { - dsOpts = append(dsOpts, mysqlredis.WithEnforcedHostLimit(license.DeviceCount)) - } - if config.Redis.HostCacheEnabled { - if config.Redis.HostCacheTTL <= 0 { - initFatal( - fmt.Errorf("redis.host_cache_ttl must be > 0 when redis.host_cache_enabled is true (got %s)", config.Redis.HostCacheTTL), - "validate host cache configuration", - ) - } - dsOpts = append(dsOpts, mysqlredis.WithHostCache(config.Redis.HostCacheTTL)) - logger.InfoContext(cmd.Context(), "host lookup redis cache enabled", - "component", "mysqlredis", "ttl", config.Redis.HostCacheTTL) - } - redisWrapperDS := mysqlredis.New(ds, redisPool, dsOpts...) - ds = redisWrapperDS resultStore := pubsub.NewRedisQueryResults(redisPool, config.Redis.DuplicateResults, logger.With("component", "query-results"),