diff --git a/main.go b/main.go index 0876a50..31ad670 100644 --- a/main.go +++ b/main.go @@ -45,6 +45,11 @@ var ( once sync.Once tracer *libpack_tracing.TracingSetup shutdownManager *ShutdownManager + // pprofServer is the optional debug pprof HTTP server started by + // parseConfig when PPROF_PORT is set. It is stored here (rather than + // discarded) so main can register it with shutdownManager and Shutdown + // it cleanly instead of leaking the goroutine (see L2). + pprofServer *http.Server ) // Shutdown phases for components registered on shutdownManager. @@ -83,32 +88,78 @@ func getDetailsFromEnv[T any](key string, defaultValue T) T { } return any(envutil.Getenv(key, v)).(T) case int: - if val, ok := os.LookupEnv(prefixedKey); ok { - if intVal, err := strconv.Atoi(val); err == nil { + // An explicitly-empty value (VAR="") is treated as unset -- fall + // through to the next source instead of warning, since there is + // nothing invalid to parse, only a deliberately blank override. + if val, ok := os.LookupEnv(prefixedKey); ok && val != "" { + if intVal, err := strconv.Atoi(strings.TrimSpace(val)); err == nil { return any(intVal).(T) } + // C3: the prefixed var is set but unparsable - use the coded + // default. Do NOT fall through to the unprefixed name, or a + // stale/unrelated unprefixed value could silently win. + warnInvalidEnv(prefixedKey, val, v) + return any(v).(T) } - return any(envutil.GetInt(key, v)).(T) + if val, ok := os.LookupEnv(key); ok && val != "" { + if intVal, err := strconv.Atoi(strings.TrimSpace(val)); err == nil { + return any(intVal).(T) + } + // C2: envutil.GetInt silently returns 0 (not the default) when + // the value is present but non-numeric, because it discards the + // parse error. Parse it ourselves so we can warn and keep the + // coded default instead of silently accepting 0. + warnInvalidEnv(key, val, v) + return any(v).(T) + } + return any(v).(T) case bool: - if val, ok := os.LookupEnv(prefixedKey); ok { - boolVal := strings.ToLower(val) == "true" || val == "1" - return any(boolVal).(T) + // An explicitly-empty value (VAR="") is treated as unset -- fall + // through to the next source instead of warning, since there is + // nothing invalid to parse, only a deliberately blank override. + if val, ok := os.LookupEnv(prefixedKey); ok && val != "" { + if b, valid := parseBoolEnv(val); valid { + return any(b).(T) + } + // C13: the prefixed and unprefixed bool parsers must agree on + // the same truthy/falsy token sets (true/1/on/yes, + // false/0/off/no). On anything else, warn and use the default + // rather than silently disagreeing with the unprefixed path. + warnInvalidEnv(prefixedKey, val, v) + return any(v).(T) } - return any(envutil.GetBool(key, v)).(T) + if val, ok := os.LookupEnv(key); ok && val != "" { + if b, valid := parseBoolEnv(val); valid { + return any(b).(T) + } + warnInvalidEnv(key, val, v) + return any(v).(T) + } + return any(v).(T) case float64: // envutil has no GetFloat; handle the GMP_ prefixed and plain keys // directly. Previously float64 env values (CIRCUIT_FAILURE_RATIO, // CIRCUIT_BACKOFF_MULTIPLIER, RETRY_BUDGET_TOKENS_PER_SEC) fell into // default and were silently impossible to configure. - if val, ok := os.LookupEnv(prefixedKey); ok { - if f, err := strconv.ParseFloat(val, 64); err == nil { + // An explicitly-empty value (VAR="") is treated as unset -- fall + // through to the next source instead of warning, since there is + // nothing invalid to parse, only a deliberately blank override. + if val, ok := os.LookupEnv(prefixedKey); ok && val != "" { + if f, err := strconv.ParseFloat(strings.TrimSpace(val), 64); err == nil { return any(f).(T) } + // C3: prefixed var set but unparsable - use the default, do not + // fall through to the unprefixed name. + warnInvalidEnv(prefixedKey, val, v) + return any(v).(T) } - if val, ok := os.LookupEnv(key); ok { - if f, err := strconv.ParseFloat(val, 64); err == nil { + if val, ok := os.LookupEnv(key); ok && val != "" { + if f, err := strconv.ParseFloat(strings.TrimSpace(val), 64); err == nil { return any(f).(T) } + // C2: unprefixed var set but unparsable - warn and use default. + warnInvalidEnv(key, val, v) + return any(v).(T) } return defaultValue default: @@ -116,6 +167,46 @@ func getDetailsFromEnv[T any](key string, defaultValue T) T { } } +// warnInvalidEnv logs that an environment variable's value could not be +// parsed as its expected type and that the coded default is used instead. +// getDetailsFromEnv runs for many fields before cfg (and its Logger) exists, +// so this prefers cfg.Logger once available and falls back to a throwaway +// libpack_logging.Logger otherwise. +func warnInvalidEnv(varName, rawValue string, defaultValue any) { + msg := &libpack_logging.LogMessage{ + Message: "Invalid value for environment variable, using default", + Pairs: map[string]any{"var": varName, "value": rawValue, "default": defaultValue}, + } + + cfgMutex.RLock() + var logger *libpack_logging.Logger + if cfg != nil { + logger = cfg.Logger + } + cfgMutex.RUnlock() + + if logger == nil { + logger = libpack_logging.New() + } + logger.Warning(msg) +} + +// parseBoolEnv parses val using the same truthy/falsy token set gookit's +// envutil.GetBool accepts (true/1/on/yes and false/0/off/no), so the +// GMP_-prefixed and unprefixed bool paths in getDetailsFromEnv agree on the +// same input instead of the prefixed path only accepting "true"/"1" (C13). +// ok is false when val matches neither set. +func parseBoolEnv(val string) (parsed bool, ok bool) { + switch strings.ToLower(strings.TrimSpace(val)) { + case "true", "1", "on", "yes": + return true, true + case "false", "0", "off", "no": + return false, true + default: + return false, false + } +} + // validateJWTClaimPath validates JWT claim paths to prevent injection attacks func validateJWTClaimPath(path string) error { if path == "" { @@ -162,6 +253,10 @@ func parseConfig() { // Server configurations c.Server.PortGraphQL = getDetailsFromEnv("PORT_GRAPHQL", 8080) c.Server.PortMonitoring = getDetailsFromEnv("MONITORING_PORT", 9393) + // S6: bind address for the GraphQL proxy / admin API / monitoring + // listeners. Empty default preserves today's "bind all interfaces" + // behavior (":port"); consumers format as ":". + c.Server.BindAddress = getDetailsFromEnv("BIND_ADDRESS", "") c.Server.HostGraphQL = getDetailsFromEnv("HOST_GRAPHQL", "http://localhost/") c.Server.HostGraphQLReadOnly = getDetailsFromEnv("HOST_GRAPHQL_READONLY", "") // Client configurations @@ -228,6 +323,17 @@ func parseConfig() { // Logger setup c.Logger = libpack_logging.New().SetMinLogLevel(libpack_logging.GetLogLevel(c.LogLevel)). SetFieldName("timestamp", "ts").SetFieldName("message", "msg").SetShowCaller(false) + + // C1: a non-positive CACHE_TTL crashes the cache cleanup ticker + // (time.NewTicker panics on d <= 0). Clamp to the coded default before + // this value ever reaches cache init. + if c.Cache.CacheTTL <= 0 { + c.Logger.Warning(&libpack_logging.LogMessage{ + Message: "Invalid CACHE_TTL (must be positive), using default", + Pairs: map[string]any{"requested": c.Cache.CacheTTL, "default": 60}, + }) + c.Cache.CacheTTL = 60 + } // Health check c.Server.HealthcheckGraphQL = getDetailsFromEnv("HEALTHCHECK_GRAPHQL_URL", "") c.Client.GQLClient = graphql.NewConnection() @@ -345,12 +451,21 @@ func parseConfig() { c.CircuitBreaker.TripOn4xx = getDetailsFromEnv("CIRCUIT_TRIP_ON_4XX", false) // 4xx are usually client errors c.CircuitBreaker.BackoffMultiplier = getDetailsFromEnv("CIRCUIT_BACKOFF_MULTIPLIER", 1.0) // No backoff by default c.CircuitBreaker.MaxBackoffTimeout = getDetailsFromEnv("CIRCUIT_MAX_BACKOFF_TIMEOUT", 300) // 5 minutes max - // Initialize endpoint configs map - c.CircuitBreaker.EndpointConfigs = make(map[string]*EndpointCBConfig) // Retry budget configuration c.RetryBudget.Enable = getDetailsFromEnv("RETRY_BUDGET_ENABLE", true) c.RetryBudget.TokensPerSecond = getDetailsFromEnv("RETRY_BUDGET_TOKENS_PER_SEC", 10.0) + // C14: a non-positive refill rate means the token bucket never refills, + // permanently locking out retries once the initial burst is spent. + // Clamp to the coded default (10.0/s, consistent with the default above + // and with RETRY_BUDGET_MAX_TOKENS's coded default of 100 below). + if c.RetryBudget.TokensPerSecond <= 0 { + c.Logger.Warning(&libpack_logging.LogMessage{ + Message: "Invalid RETRY_BUDGET_TOKENS_PER_SEC (must be positive), using default", + Pairs: map[string]any{"requested": c.RetryBudget.TokensPerSecond, "default": 10.0}, + }) + c.RetryBudget.TokensPerSecond = 10.0 + } c.RetryBudget.MaxTokens = getDetailsFromEnv("RETRY_BUDGET_MAX_TOKENS", 100) // Request coalescing configuration @@ -374,16 +489,20 @@ func parseConfig() { c.Logger.Info(&libpack_logging.LogMessage{ Message: "pprof endpoint listening on " + addr, }) + // L2: keep the server so it can be registered with + // shutdownManager and Shutdown cleanly instead of leaking the + // goroutine for the life of the process. + srv := &http.Server{ + Addr: addr, + Handler: nil, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 120 * time.Second, + IdleTimeout: 120 * time.Second, + } + pprofServer = srv go func(listenAddr string) { - srv := &http.Server{ - Addr: listenAddr, - Handler: nil, - ReadHeaderTimeout: 5 * time.Second, - ReadTimeout: 30 * time.Second, - WriteTimeout: 120 * time.Second, - IdleTimeout: 120 * time.Second, - } - if err := srv.ListenAndServe(); err != nil { + if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { c.Logger.Error(&libpack_logging.LogMessage{ Message: "pprof endpoint failed", Pairs: map[string]any{"error": err.Error(), "addr": listenAddr}, @@ -663,6 +782,14 @@ func main() { // traffic-drain phase, before any of the backends above are closed. shutdownManager.RegisterComponentWithPhase("http-proxy", shutdownPhaseDrainTraffic, shutdownHTTPProxy) + // L2: register the optional pprof debug server (if PPROF_PORT was set) + // so it stops cleanly instead of leaking its listener goroutine forever. + if pprofServer != nil { + shutdownManager.RegisterComponent("pprof-server", func(ctx context.Context) error { + return pprofServer.Shutdown(ctx) + }) + } + // Close the Redis cache client's connection pool at shutdown (in-memory // caches are no-ops). Runs after the traffic drain so in-flight // CacheLookup/CacheStore calls do not hit a closed Redis client. diff --git a/main_test.go b/main_test.go index 589ee94..67d6a35 100644 --- a/main_test.go +++ b/main_test.go @@ -291,3 +291,206 @@ func (suite *Tests) TestIntrospectionEnvironmentConfig() { }) } } + +// TestGetDetailsFromEnv_InvalidValues covers C2 (an unparsable env value must +// fall back to the coded default, never a silent 0/false) and C13 (the +// GMP_-prefixed and unprefixed bool parsers must accept the same +// true/1/on/yes and false/0/off/no tokens, and agree on the same input). +// Uses t.Setenv so each subtest's environment is reset automatically. +func TestGetDetailsFromEnv_InvalidValues(t *testing.T) { + tests := []struct { + defaultValue any + expected any + name string + key string + envKey string + envValue string + }{ + // C2: unprefixed int/float, garbage value falls back to the coded + // default instead of envutil.GetInt's silent 0. + {name: "unprefixed int garbage falls back to default, not 0", key: "TEST_C2_INT", envKey: "TEST_C2_INT", envValue: "not-a-number", defaultValue: 42, expected: 42}, + {name: "unprefixed float garbage falls back to default", key: "TEST_C2_FLOAT", envKey: "TEST_C2_FLOAT", envValue: "not-a-number", defaultValue: 2.5, expected: 2.5}, + // Sanity: a valid unprefixed value still parses (default-preserving). + {name: "unprefixed int valid value still parses", key: "TEST_C2_INT_OK", envKey: "TEST_C2_INT_OK", envValue: "7", defaultValue: 42, expected: 7}, + + // C13: unprefixed bool accepts on/yes/off/no (baseline gookit + // behavior, pinned here so a future change cannot silently regress + // the parity the prefixed path is fixed to match). + {name: "unprefixed bool on", key: "TEST_C13_BOOL_ON", envKey: "TEST_C13_BOOL_ON", envValue: "on", defaultValue: false, expected: true}, + {name: "unprefixed bool off", key: "TEST_C13_BOOL_OFF", envKey: "TEST_C13_BOOL_OFF", envValue: "off", defaultValue: true, expected: false}, + {name: "unprefixed bool yes", key: "TEST_C13_BOOL_YES", envKey: "TEST_C13_BOOL_YES", envValue: "yes", defaultValue: false, expected: true}, + {name: "unprefixed bool no", key: "TEST_C13_BOOL_NO", envKey: "TEST_C13_BOOL_NO", envValue: "no", defaultValue: true, expected: false}, + + // C13: the prefixed path now accepts the same tokens (previously + // only "true"/"1" were accepted, diverging from the unprefixed + // path - security-relevant for e.g. GMP_BLOCK_SCHEMA_INTROSPECTION). + {name: "prefixed bool on", key: "TEST_C13_PFX_ON", envKey: "GMP_TEST_C13_PFX_ON", envValue: "on", defaultValue: false, expected: true}, + {name: "prefixed bool off", key: "TEST_C13_PFX_OFF", envKey: "GMP_TEST_C13_PFX_OFF", envValue: "off", defaultValue: true, expected: false}, + {name: "prefixed bool yes", key: "TEST_C13_PFX_YES", envKey: "GMP_TEST_C13_PFX_YES", envValue: "yes", defaultValue: false, expected: true}, + {name: "prefixed bool no", key: "TEST_C13_PFX_NO", envKey: "GMP_TEST_C13_PFX_NO", envValue: "no", defaultValue: true, expected: false}, + + // C13/C2: a garbage bool value on either side falls back to the + // coded default instead of silently becoming false. + {name: "prefixed bool garbage falls back to default", key: "TEST_C13_PFX_BAD", envKey: "GMP_TEST_C13_PFX_BAD", envValue: "maybe", defaultValue: true, expected: true}, + {name: "unprefixed bool garbage falls back to default", key: "TEST_C13_BAD", envKey: "TEST_C13_BAD", envValue: "maybe", defaultValue: true, expected: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(tt.envKey, tt.envValue) + + var result any + switch def := tt.defaultValue.(type) { + case int: + result = getDetailsFromEnv(tt.key, def) + case float64: + result = getDetailsFromEnv(tt.key, def) + case bool: + result = getDetailsFromEnv(tt.key, def) + default: + t.Fatalf("unsupported default value type %T", tt.defaultValue) + } + + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestGetDetailsFromEnv_PrefixedInvalidDoesNotFallThrough covers C3: when +// the GMP_-prefixed var is set but fails to parse, getDetailsFromEnv must +// use the coded default and must NOT fall through to a stale/unrelated +// unprefixed value of the same base key. +func TestGetDetailsFromEnv_PrefixedInvalidDoesNotFallThrough(t *testing.T) { + t.Run("int: prefixed invalid ignores stale unprefixed value", func(t *testing.T) { + t.Setenv("TEST_C3_INT", "999") // stale unprefixed value, must not win + t.Setenv("GMP_TEST_C3_INT", "not-a-number") // prefixed set but unparsable + result := getDetailsFromEnv("TEST_C3_INT", 42) + assert.Equal(t, 42, result) + }) + + t.Run("float64: prefixed invalid ignores stale unprefixed value", func(t *testing.T) { + t.Setenv("TEST_C3_FLOAT", "999.0") // stale unprefixed value, must not win + t.Setenv("GMP_TEST_C3_FLOAT", "not-a-number") // prefixed set but unparsable + result := getDetailsFromEnv("TEST_C3_FLOAT", 4.2) + assert.Equal(t, 4.2, result) + }) +} + +// TestGetDetailsFromEnv_EmptyValueTreatedAsUnset covers the polish fix: an +// explicitly-empty env var (VAR="") must fall back to the coded default, +// the same as an unset var, without emitting an invalid-value warning -- +// there is nothing invalid to parse, only a deliberately blank override. +// Uses t.Setenv so each subtest's environment is reset automatically. +func TestGetDetailsFromEnv_EmptyValueTreatedAsUnset(t *testing.T) { + t.Run("int: empty prefixed value falls back to default", func(t *testing.T) { + t.Setenv("GMP_TEST_EMPTY_INT", "") + result := getDetailsFromEnv("TEST_EMPTY_INT", 42) + assert.Equal(t, 42, result) + }) + + t.Run("int: empty unprefixed value falls back to default", func(t *testing.T) { + t.Setenv("TEST_EMPTY_INT_UNPFX", "") + result := getDetailsFromEnv("TEST_EMPTY_INT_UNPFX", 42) + assert.Equal(t, 42, result) + }) + + t.Run("float64: empty prefixed value falls back to default", func(t *testing.T) { + t.Setenv("GMP_TEST_EMPTY_FLOAT", "") + result := getDetailsFromEnv("TEST_EMPTY_FLOAT", 2.5) + assert.Equal(t, 2.5, result) + }) + + t.Run("bool: empty prefixed value falls back to default", func(t *testing.T) { + t.Setenv("GMP_TEST_EMPTY_BOOL", "") + result := getDetailsFromEnv("TEST_EMPTY_BOOL", true) + assert.Equal(t, true, result) + }) + + t.Run("int: empty prefixed value still falls through to a valid unprefixed value", func(t *testing.T) { + t.Setenv("GMP_TEST_EMPTY_FALLTHROUGH", "") + t.Setenv("TEST_EMPTY_FALLTHROUGH", "7") + result := getDetailsFromEnv("TEST_EMPTY_FALLTHROUGH", 42) + assert.Equal(t, 7, result) + }) +} + +// TestGetDetailsFromEnv_FloatTrimSpace covers the polish fix: the float64 +// path was missing the strings.TrimSpace that the int and bool paths +// already had, so a value like " 1.5 " failed to parse and silently fell +// back to the coded default instead of parsing to 1.5. +func TestGetDetailsFromEnv_FloatTrimSpace(t *testing.T) { + t.Run("prefixed float with surrounding whitespace parses", func(t *testing.T) { + t.Setenv("GMP_TEST_FLOAT_TRIM", " 1.5 ") + result := getDetailsFromEnv("TEST_FLOAT_TRIM", 2.5) + assert.Equal(t, 1.5, result) + }) + + t.Run("unprefixed float with surrounding whitespace parses", func(t *testing.T) { + t.Setenv("TEST_FLOAT_TRIM_UNPFX", " 1.5 ") + result := getDetailsFromEnv("TEST_FLOAT_TRIM_UNPFX", 2.5) + assert.Equal(t, 1.5, result) + }) +} + +// TestParseConfig_ClampsInvalidNumericEnv covers C1 (CACHE_TTL) and C14 +// (RETRY_BUDGET_TOKENS_PER_SEC): a non-positive value must clamp to the +// coded default instead of crashing the cache cleanup ticker (C1) or +// permanently locking out retry-budget token refills (C14). +func TestParseConfig_ClampsInvalidNumericEnv(t *testing.T) { + tests := []struct { + checkResult func(t *testing.T, c *config) + name string + envKey string + envValue string + }{ + { + name: "CACHE_TTL zero clamps to 60s default", + envKey: "CACHE_TTL", + envValue: "0", + checkResult: func(t *testing.T, c *config) { + assert.Equal(t, 60, c.Cache.CacheTTL) + }, + }, + { + name: "CACHE_TTL negative clamps to 60s default", + envKey: "CACHE_TTL", + envValue: "-5", + checkResult: func(t *testing.T, c *config) { + assert.Equal(t, 60, c.Cache.CacheTTL) + }, + }, + { + name: "RETRY_BUDGET_TOKENS_PER_SEC zero clamps to 10/s default", + envKey: "RETRY_BUDGET_TOKENS_PER_SEC", + envValue: "0", + checkResult: func(t *testing.T, c *config) { + assert.Equal(t, 10.0, c.RetryBudget.TokensPerSecond) + }, + }, + { + name: "RETRY_BUDGET_TOKENS_PER_SEC negative clamps to 10/s default", + envKey: "RETRY_BUDGET_TOKENS_PER_SEC", + envValue: "-1.5", + checkResult: func(t *testing.T, c *config) { + assert.Equal(t, 10.0, c.RetryBudget.TokensPerSecond) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(tt.envKey, tt.envValue) + + cfgMutex.Lock() + cfg = nil + cfgMutex.Unlock() + parseConfig() + + cfgMutex.RLock() + c := cfg + cfgMutex.RUnlock() + + tt.checkResult(t, c) + }) + } +} diff --git a/struct_config.go b/struct_config.go index 00c29f3..a9399bd 100644 --- a/struct_config.go +++ b/struct_config.go @@ -7,14 +7,6 @@ import ( "github.com/valyala/fasthttp" ) -// EndpointCBConfig holds per-endpoint circuit breaker configuration -type EndpointCBConfig struct { - MaxFailures int // Override max failures for this endpoint - FailureRatio float64 // Override failure ratio for this endpoint - Timeout int // Override timeout for this endpoint - Disabled bool // Disable circuit breaker for this endpoint -} - // config is a struct that holds the configuration of the application. // It includes settings for logging, monitoring, client connections, security, and server behavior. type config struct { @@ -69,6 +61,12 @@ type config struct { HostGraphQLReadOnly string HealthcheckGraphQL string AllowURLs []string // List of allowed URL paths for access control + // BindAddress is the host portion of every listener's bind address + // (GraphQL proxy, admin API, monitoring). Empty (the default) + // preserves the existing ":port" behavior of binding all + // interfaces; set it (e.g. "127.0.0.1") to restrict listeners to a + // single interface. See S6. + BindAddress string PortGraphQL int PortMonitoring int @@ -80,7 +78,6 @@ type config struct { PurgeOnCrawl bool } CircuitBreaker struct { - EndpointConfigs map[string]*EndpointCBConfig // Per-endpoint circuit breaker configurations ExcludedStatusCodes []int MaxFailures int FailureRatio float64