eSignet Go Performance: Fix yaml-overrides-env

esignet-service/data/deployment.yaml

@@ -9,6 +9,9 @@
 # read from this file; they're documented here for operators anyway.
 identifier: "${NAMESPACE}"
 port: 8080
+# Private Prometheus scrape port, not routed through the public ingress.
+# Override with METRICS_PORT (env wins over this value).
+metrics_port: 9090
 issuer: "${MOSIP_ESIGNET_HOST}"
 data_dir: "${DATA_DIR}"
 
@@ -45,14 +48,31 @@ redis:
   # REDIS_DIAL_TIMEOUT_SECS, REDIS_READ_TIMEOUT_SECS, REDIS_WRITE_TIMEOUT_SECS,
   # REDIS_POOL_TIMEOUT_SECS) is env-var-driven only and cannot be set here —
   # see the Redis doc comment in internal/config/redis.go.
+  #
+  # At startup the service logs one "redis pool config resolved" INFO line
+  # giving each setting's effective value and a "<name>Source" field naming
+  # where it came from (env / default). An env var that is set but unusable
+  # (unparseable, or non-positive where a positive value is required) is
+  # reported as an "ignoring invalid config env var" WARN naming the rejected
+  # value and the value actually used — it is never discarded silently.
 
 db:
   dsn: "${DATABASE_URL}"
   # Pool tuning: env var (DB_MAX_OPEN_CONNS, DB_MAX_IDLE_CONNS,
   # DB_CONN_MAX_LIFETIME_SECS, DB_CONN_MAX_IDLE_TIME_SECS) > these values >
   # compiled-in default (see loadDB in internal/config/db.go). Only an env
   # var of "0" opts *_lifetime_secs out to "no limit"; 0 here is treated as
-  # "not set".
+  # "not set", and a negative env var is rejected as invalid.
+  #
+  # The env var always wins over the values below — it is never silently
+  # shadowed by them. At startup the service logs one "db pool config
+  # resolved" INFO line giving each setting's effective value and a
+  # "<name>Source" field naming where it came from (env / yaml / default), so
+  # "did my env var take effect?" is answered by the log rather than by
+  # guesswork. An env var that is set but unusable (unparseable, or
+  # non-positive where a positive value is required) is reported as an
+  # "ignoring invalid config env var" WARN naming the rejected value and the
+  # value actually used — it is never discarded silently.
   pool:
     max_open_conns: 25
     max_idle_conns: 5       # kept well below max_open_conns: pgxpool eagerly

esignet-service/internal/config/app.go

@@ -299,7 +299,12 @@ func applyDefaults(cfg *AppConfig) {
 	cfg.Port = envIntOrConfigOrDefault("PORT", cfg.Port, defaultPort)
 	cfg.Issuer = envOrConfigOrDefault("MOSIP_ESIGNET_HOST", cfg.Issuer, fmt.Sprintf("http://localhost:%d", cfg.Port))
 	cfg.DataDir = envOrConfigOrDefault("DATA_DIR", cfg.DataDir, defaultDataDir)
-	cfg.MetricsPort = envIntOrDefault("METRICS_PORT", defaultMetricsPort)
+	// Threads cfg.MetricsPort (the decoded `metrics_port:` value) through like
+	// cfg.Port above. Previously this read only the env var and the compiled
+	// default, so a metrics_port set in deployment.yaml parsed cleanly and was
+	// then silently discarded — the mirror image of the yaml-shadows-env trap
+	// in issue #2498, and the same "dead config" surprise for an operator.
+	cfg.MetricsPort = envIntOrConfigOrDefault("METRICS_PORT", cfg.MetricsPort, defaultMetricsPort)
 	cfg.DB = loadDB(cfg.DB)
 
 	cfg.PProfConfig = PProfConfig{
@@ -685,41 +690,122 @@ func envOrConfigOrDefault(key, fromYAML, fallback string) string {
 	return fallback
 }
 
-// configOrDefault is the shared yaml/fallback tail for envIntOrConfigOrDefault
-// and envIntOrConfigOrDefaultAllowEnvZero: fromYAML wins if positive,
-// otherwise fallback. A yaml value of 0 (or an omitted field, which decodes
-// to the same zero value) can't be distinguished from "not configured", so
-// neither variant can honor an explicit yaml 0 the way they can an explicit
-// env var 0.
-func configOrDefault(fromYAML, fallback int) int {
+// configSource identifies which tier of the env > yaml > default precedence
+// chain supplied a resolved setting's effective value. Reported alongside the
+// value at startup so an operator can tell "my env var took effect" apart
+// from "my env var was ignored and yaml won" without reading source.
+type configSource string
+
+const (
+	sourceEnv     configSource = "env"
+	sourceYAML    configSource = "yaml"
+	sourceDefault configSource = "default"
+)
+
+// warnIgnoredEnvVar reports an env var that was set but could not be used,
+// naming the rejected value alongside the value and source actually taking
+// effect — so "why didn't my env var change anything" is answered by the log
+// rather than by reading source. Mirrors envPositiveInt's existing warning.
+func warnIgnoredEnvVar(key, raw string, using int, src configSource) {
+	applog.GetLogger().Warn(context.Background(),
+		"ignoring invalid config env var; falling back to lower-precedence value",
+		applog.String("key", key),
+		applog.String("value", raw),
+		applog.Int("usingValue", using),
+		applog.String("usingSource", string(src)))
+}
+
+// resolvedSetting pairs a setting's effective value with the tier that
+// supplied it, for reporting via logResolvedSettings.
+type resolvedSetting struct {
+	name  string
+	value int
+	src   configSource
+}
+
+// logResolvedSettings emits one INFO line reporting each setting's effective
+// value alongside a "<name>Source" field naming the tier it came from. The
+// suffix convention lives here rather than at each call site so a dozen
+// settings across db.go and redis.go can't drift apart.
+func logResolvedSettings(msg string, settings ...resolvedSetting) {
+	fields := make([]applog.Field, 0, len(settings)*2)
+	for _, s := range settings {
+		fields = append(fields,
+			applog.Int(s.name, s.value),
+			applog.String(s.name+"Source", string(s.src)))
+	}
+	applog.GetLogger().Info(context.Background(), msg, fields...)
+}
+
+// configOrDefaultSourced is the shared yaml/fallback tail for the resolvers
+// below: fromYAML wins if positive, otherwise fallback. A yaml value of 0 (or
+// an omitted field, which decodes to the same zero value) can't be
+// distinguished from "not configured", so no variant can honor an explicit
+// yaml 0 the way they can an explicit env var 0.
+func configOrDefaultSourced(fromYAML, fallback int) (int, configSource) {
 	if fromYAML > 0 {
-		return fromYAML
+		return fromYAML, sourceYAML
 	}
-	return fallback
+	return fallback, sourceDefault
+}
+
+// configOrDefault is configOrDefaultSourced without the source.
+func configOrDefault(fromYAML, fallback int) int {
+	v, _ := configOrDefaultSourced(fromYAML, fallback)
+	return v
 }
 
-// envIntOrConfigOrDefault is envOrConfigOrDefault for int settings. A
-// zero/negative value at any tier is treated as "not set" and falls through
-// to the next tier — this deliberately can't express an env-var-only "0 =
-// no limit" opt-out; use envIntOrConfigOrDefaultAllowEnvZero for those.
+// envIntOrConfigOrDefaultSourced is envOrConfigOrDefault for int settings,
+// additionally reporting which tier supplied the value. A zero/negative value
+// at any tier is treated as "not set" and falls through to the next tier —
+// this deliberately can't express an env-var-only "0 = no limit" opt-out; use
+// envIntOrConfigOrDefaultAllowEnvZeroSourced for those.
+//
+// An env var that is set but unusable (unparseable, or non-positive where
+// positive is required) is warned about rather than silently discarded: that
+// silence is the "dead config" trap this variant exists to close.
+func envIntOrConfigOrDefaultSourced(key string, fromYAML, fallback int) (int, configSource) {
+	raw := strings.TrimSpace(os.Getenv(key))
+	if raw == "" {
+		return configOrDefaultSourced(fromYAML, fallback)
+	}
+	if n, err := strconv.Atoi(raw); err == nil && n > 0 {
+		return n, sourceEnv
+	}
+	v, src := configOrDefaultSourced(fromYAML, fallback)
+	warnIgnoredEnvVar(key, raw, v, src)
+	return v, src
+}
+
+// envIntOrConfigOrDefault is envIntOrConfigOrDefaultSourced without the source.
 func envIntOrConfigOrDefault(key string, fromYAML, fallback int) int {
-	if v := envIntOrDefault(key, 0); v > 0 {
-		return v
+	v, _ := envIntOrConfigOrDefaultSourced(key, fromYAML, fallback)
+	return v
+}
+
+// envIntOrConfigOrDefaultAllowEnvZeroSourced is like
+// envIntOrConfigOrDefaultSourced, but an explicitly-set env var of "0" is
+// honored as-is rather than treated as "not set". Used for settings where 0
+// is a documented env-var-only opt-out (e.g. "0 = no limit" for a connection
+// lifetime). Negative values remain invalid and are warned about.
+func envIntOrConfigOrDefaultAllowEnvZeroSourced(key string, fromYAML, fallback int) (int, configSource) {
+	raw := strings.TrimSpace(os.Getenv(key))
+	if raw == "" {
+		return configOrDefaultSourced(fromYAML, fallback)
 	}
-	return configOrDefault(fromYAML, fallback)
+	if n, err := strconv.Atoi(raw); err == nil && n >= 0 {
+		return n, sourceEnv
+	}
+	v, src := configOrDefaultSourced(fromYAML, fallback)
+	warnIgnoredEnvVar(key, raw, v, src)
+	return v, src
 }
 
-// envIntOrConfigOrDefaultAllowEnvZero is like envIntOrConfigOrDefault, but an
-// explicitly-set env var of "0" is honored as-is rather than treated as
-// "not set". Used for settings where 0 is a documented env-var-only opt-out
-// (e.g. "0 = no limit" for a connection lifetime).
+// envIntOrConfigOrDefaultAllowEnvZero is
+// envIntOrConfigOrDefaultAllowEnvZeroSourced without the source.
 func envIntOrConfigOrDefaultAllowEnvZero(key string, fromYAML, fallback int) int {
-	if raw := os.Getenv(key); raw != "" {
-		if n, err := strconv.Atoi(raw); err == nil {
-			return n
-		}
-	}
-	return configOrDefault(fromYAML, fallback)
+	v, _ := envIntOrConfigOrDefaultAllowEnvZeroSourced(key, fromYAML, fallback)
+	return v
 }
 
 func envBool(key string) bool {

esignet-service/internal/config/app_test.go

@@ -934,3 +934,125 @@ func (ts *AppConfigTestSuite) TestEnvBool() {
 		})
 	}
 }
+
+// The env > yaml > default chain is the tuning surface operators actually
+// use, so these lock down both the value it resolves to and the source it
+// reports — the source is what the startup "… pool config resolved" log line
+// turns into an answer for "did my env var take effect?" (issue #2498).
+func TestEnvIntOrConfigOrDefaultSourced(t *testing.T) {
+	const key = "ESIGNET_TEST_SOURCED_INT"
+
+	cases := []struct {
+		name     string
+		env      string
+		fromYAML int
+		fallback int
+		wantVal  int
+		wantSrc  configSource
+	}{
+		{"env wins over yaml and default", "50", 15, 25, 50, sourceEnv},
+		{"env wins with surrounding whitespace", "  50  ", 15, 25, 50, sourceEnv},
+		{"yaml wins when env unset", "", 15, 25, 15, sourceYAML},
+		{"default when neither set", "", 0, 25, 25, sourceDefault},
+		// The "dead config" trap from issue #2498: each of these is an env var
+		// the operator believes they set. It must resolve to the lower tier
+		// *and report that tier*, never masquerade as an env-sourced value.
+		{"unparseable env falls back to yaml", "abc", 15, 25, 15, sourceYAML},
+		{"unparseable env falls back to default", "abc", 0, 25, 25, sourceDefault},
+		{"zero env falls back to yaml", "0", 15, 25, 15, sourceYAML},
+		{"negative env falls back to yaml", "-5", 15, 25, 15, sourceYAML},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv(key, tc.env)
+
+			gotVal, gotSrc := envIntOrConfigOrDefaultSourced(key, tc.fromYAML, tc.fallback)
+
+			require.Equal(t, tc.wantVal, gotVal)
+			require.Equal(t, tc.wantSrc, gotSrc)
+			// The source-less wrapper must stay in lockstep; ~40 existing call
+			// sites still go through it.
+			require.Equal(t, tc.wantVal, envIntOrConfigOrDefault(key, tc.fromYAML, tc.fallback))
+		})
+	}
+}
+
+func TestEnvIntOrConfigOrDefaultAllowEnvZeroSourced(t *testing.T) {
+	const key = "ESIGNET_TEST_SOURCED_INT_ZERO"
+
+	cases := []struct {
+		name     string
+		env      string
+		fromYAML int
+		fallback int
+		wantVal  int
+		wantSrc  configSource
+	}{
+		// Unlike the variant above, an explicit "0" is a documented opt-out
+		// ("no limit"), not a "not set" signal.
+		{"explicit zero env is honored", "0", 1800, 1800, 0, sourceEnv},
+		{"positive env wins", "600", 1800, 1800, 600, sourceEnv},
+		{"yaml wins when env unset", "", 900, 1800, 900, sourceYAML},
+		{"default when neither set", "", 0, 1800, 1800, sourceDefault},
+		{"unparseable env falls back to yaml", "abc", 900, 1800, 900, sourceYAML},
+		{"negative env falls back to default", "-5", 0, 1800, 1800, sourceDefault},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv(key, tc.env)
+
+			gotVal, gotSrc := envIntOrConfigOrDefaultAllowEnvZeroSourced(key, tc.fromYAML, tc.fallback)
+
+			require.Equal(t, tc.wantVal, gotVal)
+			require.Equal(t, tc.wantSrc, gotSrc)
+			require.Equal(t, tc.wantVal, envIntOrConfigOrDefaultAllowEnvZero(key, tc.fromYAML, tc.fallback))
+		})
+	}
+}
+
+func TestConfigOrDefaultSourced(t *testing.T) {
+	v, src := configOrDefaultSourced(15, 25)
+	require.Equal(t, 15, v)
+	require.Equal(t, sourceYAML, src)
+
+	v, src = configOrDefaultSourced(0, 25)
+	require.Equal(t, 25, v)
+	require.Equal(t, sourceDefault, src)
+
+	// A yaml 0 is indistinguishable from an omitted field, so it can never be
+	// an opt-out the way an explicit env "0" can.
+	require.Equal(t, 25, configOrDefault(0, 25))
+	require.Equal(t, 15, configOrDefault(15, 25))
+}
+
+// metrics_port is a yaml-decodable field (AppConfig.MetricsPort), and
+// KnownFields(true) means setting it in deployment.yaml parses cleanly — so a
+// value set there must actually take effect. Sibling `port` on the line above
+// honors yaml via envIntOrConfigOrDefault; metrics_port must not silently
+// diverge from it (issue #2498, acceptance criterion 4).
+func TestApplyDefaults_MetricsPortHonorsYAML(t *testing.T) {
+	t.Setenv("METRICS_PORT", "")
+
+	cfg := &AppConfig{MetricsPort: 9091}
+	applyDefaults(cfg)
+
+	require.Equal(t, 9091, cfg.MetricsPort)
+}
+
+func TestApplyDefaults_MetricsPortEnvWinsOverYAML(t *testing.T) {
+	t.Setenv("METRICS_PORT", "9099")
+
+	cfg := &AppConfig{MetricsPort: 9091}
+	applyDefaults(cfg)
+
+	require.Equal(t, 9099, cfg.MetricsPort)
+}
+
+func TestApplyDefaults_MetricsPortFallsBackToDefault(t *testing.T) {
+	t.Setenv("METRICS_PORT", "")
+
+	cfg := &AppConfig{}
+	applyDefaults(cfg)
+
+	require.Equal(t, defaultMetricsPort, cfg.MetricsPort)
+}

esignet-service/internal/config/db.go

@@ -145,15 +145,29 @@ func resolveDBDSN(yamlDSN string) string {
 func loadDB(yamlDB DB) DB {
 	dsn := resolveDBDSN(yamlDB.DSN)
 
-	maxOpen := clampPositiveInt32("DB_MAX_OPEN_CONNS", envIntOrConfigOrDefault("DB_MAX_OPEN_CONNS", yamlDB.Pool.MaxOpenConns, defaultDBMaxOpenConns))
-	maxIdle := clampPositiveInt32("DB_MAX_IDLE_CONNS", envIntOrConfigOrDefault("DB_MAX_IDLE_CONNS", yamlDB.Pool.MaxIdleConns, defaultDBMaxIdleConns))
+	maxOpenRaw, maxOpenSrc := envIntOrConfigOrDefaultSourced("DB_MAX_OPEN_CONNS", yamlDB.Pool.MaxOpenConns, defaultDBMaxOpenConns)
+	maxOpen := clampPositiveInt32("DB_MAX_OPEN_CONNS", maxOpenRaw)
+	maxIdleRaw, maxIdleSrc := envIntOrConfigOrDefaultSourced("DB_MAX_IDLE_CONNS", yamlDB.Pool.MaxIdleConns, defaultDBMaxIdleConns)
+	maxIdle := clampPositiveInt32("DB_MAX_IDLE_CONNS", maxIdleRaw)
 	// An explicit env var of "0" means "no limit" — same convention as
 	// database/sql and Redis. See effectiveMaxConnLifetime in Open() for why
 	// pgxpool needs this translated rather than passed through. A yaml value
 	// of 0 (or an omitted field, which decodes to the same zero value) can't
 	// be distinguished from "not configured", so only the env var can opt out.
-	lifetimeSecs := clampDurationSecs("DB_CONN_MAX_LIFETIME_SECS", envIntOrConfigOrDefaultAllowEnvZero("DB_CONN_MAX_LIFETIME_SECS", yamlDB.Pool.ConnMaxLifetimeSecs, defaultDBConnMaxLifetimeSecs))
-	idleSecs := clampDurationSecs("DB_CONN_MAX_IDLE_TIME_SECS", envIntOrConfigOrDefault("DB_CONN_MAX_IDLE_TIME_SECS", yamlDB.Pool.ConnMaxIdleTimeSecs, defaultDBConnMaxIdleTimeSecs))
+	lifetimeRaw, lifetimeSrc := envIntOrConfigOrDefaultAllowEnvZeroSourced("DB_CONN_MAX_LIFETIME_SECS", yamlDB.Pool.ConnMaxLifetimeSecs, defaultDBConnMaxLifetimeSecs)
+	lifetimeSecs := clampDurationSecs("DB_CONN_MAX_LIFETIME_SECS", lifetimeRaw)
+	idleRaw, idleSrc := envIntOrConfigOrDefaultSourced("DB_CONN_MAX_IDLE_TIME_SECS", yamlDB.Pool.ConnMaxIdleTimeSecs, defaultDBConnMaxIdleTimeSecs)
+	idleSecs := clampDurationSecs("DB_CONN_MAX_IDLE_TIME_SECS", idleRaw)
+
+	// Report the effective value *and* which tier supplied it. Logging only
+	// the values (as the "postgres connected" line in cmd/esignet/main.go
+	// does) leaves an operator unable to tell an env var that took effect
+	// from one that was ignored in favour of deployment.yaml.
+	logResolvedSettings("db pool config resolved",
+		resolvedSetting{"maxOpenConns", maxOpen, maxOpenSrc},
+		resolvedSetting{"maxIdleConns", maxIdle, maxIdleSrc},
+		resolvedSetting{"connMaxLifetimeSecs", lifetimeSecs, lifetimeSrc},
+		resolvedSetting{"connMaxIdleTimeSecs", idleSecs, idleSrc})
 
 	return DB{
 		DSN: dsn,