[shell-operator] continue fixing passed config

pkg/app/README.md

@@ -16,7 +16,7 @@ The implementation lives in three files:
 |---|---|
 | `app_config.go` | `Config` struct, `NewConfig()` (hardcoded defaults), `ParseEnv()` (env layer), `Validate()` |
 | `flags.go` | `BindFlags()` — registers cobra/pflag CLI flags whose defaults are the *already-merged* `cfg` values |
-| `debug.go` | Package-level `DebugUnixSocket` global used by debug sub-commands |
+| `debug.go` | Package-level `DebugUnixSocket` global used by debug sub-commands, plus `ApplyConfig(cfg)` to copy cfg into it |
 
 ## How it works
 
@@ -331,6 +331,26 @@ admission/conversion webhooks), use `shell_operator.Init(ctx, cfg, logger)`;
 it now internally delegates to `AssembleCommonOperatorFromConfig` so the
 behaviour stays identical.
 
+### Forcing `cfg` into legacy globals
+
+Some debug sub-commands (`queue`, `hook`, `config`) bind their
+`--debug-unix-socket` flag to the package-level `app.DebugUnixSocket` global
+instead of `cfg`, because they don't go through the `start` command path. To
+guarantee that an outer-program-supplied `cfg` overrides this global, call
+`app.ApplyConfig(cfg)` once after you've built `cfg`:
+
+```go
+cfg := shapp.NewConfig()
+cfg.Debug.UnixSocket = "/run/myapp/debug.socket"
+shapp.ApplyConfig(cfg) // DebugUnixSocket now equals cfg.Debug.UnixSocket
+```
+
+`shell_operator.Init` and `BindFlags` already call `ApplyConfig` internally,
+so most callers don't need this. Call it explicitly only when you bypass
+both — e.g. you build `cfg` yourself and invoke debug sub-commands directly
+without `Init`/`BindFlags`. The function is idempotent and a `nil` cfg is a
+no-op.
+
 ## Adding a new parameter
 
 1. **Add a field** to the appropriate settings struct in `app_config.go`

pkg/app/debug.go

@@ -10,6 +10,26 @@ import (
 // For the start command, cfg.Debug.UnixSocket is preferred; see flags.go.
 var DebugUnixSocket = "/var/run/shell-operator/debug.socket"
 
+// ApplyConfig copies values from cfg into package-level globals that pre-date
+// the Config struct and therefore cannot be populated by caarlos0/env (today
+// just DebugUnixSocket).
+//
+// Call it whenever cfg may have been built outside the CLI flow — most
+// importantly when an outer program (e.g. addon-operator) assembles its own
+// *Config and hands it to shell-operator without going through BindFlags.
+// After this call, debug sub-commands that bind --debug-unix-socket against
+// the global will see cfg.Debug.UnixSocket as their default.
+//
+// A nil cfg is a no-op so callers don't need to guard. ApplyConfig is also
+// invoked from BindFlags and shell_operator.Init, so most users get the
+// override for free; calling it again is safe and idempotent.
+func ApplyConfig(cfg *Config) {
+	if cfg == nil {
+		return
+	}
+	DebugUnixSocket = cfg.Debug.UnixSocket
+}
+
 // DefineDebugUnixSocketFlag registers the --debug-unix-socket flag on cmd,
 // binding it to the DebugUnixSocket global. Called by debug sub-commands that
 // need to locate the operator's debug socket.

pkg/app/debug_test.go

@@ -1,9 +1,9 @@
 package app
 
 import (
-"testing"
+	"testing"
 
-"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestDebugKeepTempFiles_DefaultIsFalse(t *testing.T) {
@@ -21,3 +21,30 @@ func TestDebugKeepTempFilesIsBool(t *testing.T) {
 	cfg.Debug.KeepTempFiles = false
 	assert.False(t, cfg.Debug.KeepTempFiles)
 }
+
+// TestApplyConfig_OverridesDebugUnixSocket guarantees that handing a *Config
+// built by an outer program to ApplyConfig replaces the package-level
+// DebugUnixSocket global, even when BindFlags is never called.
+func TestApplyConfig_OverridesDebugUnixSocket(t *testing.T) {
+	prev := DebugUnixSocket
+	t.Cleanup(func() { DebugUnixSocket = prev })
+
+	cfg := NewConfig()
+	cfg.Debug.UnixSocket = "/run/outer-program/debug.socket"
+
+	ApplyConfig(cfg)
+
+	assert.Equal(t, "/run/outer-program/debug.socket", DebugUnixSocket)
+}
+
+// TestApplyConfig_NilIsNoop documents that ApplyConfig tolerates a nil cfg
+// so callers don't need to guard at every call site.
+func TestApplyConfig_NilIsNoop(t *testing.T) {
+	prev := DebugUnixSocket
+	t.Cleanup(func() { DebugUnixSocket = prev })
+
+	DebugUnixSocket = "/run/sentinel.socket"
+	ApplyConfig(nil)
+
+	assert.Equal(t, "/run/sentinel.socket", DebugUnixSocket)
+}

pkg/app/flags.go

@@ -117,7 +117,7 @@ func bindLogFlags(cfg *Config, cmd *cobra.Command) {
 func bindDebugFlags(cfg *Config, rootCmd *cobra.Command, cmd *cobra.Command) {
 	// Sync the package-level global so debug sub-commands (queue, hook, etc.)
 	// that bind to DebugUnixSocket get the env/default value.
-	DebugUnixSocket = cfg.Debug.UnixSocket
+	ApplyConfig(cfg)
 
 	f := cmd.Flags()
 	f.StringVar(&cfg.Debug.UnixSocket, "debug-unix-socket", cfg.Debug.UnixSocket, "A path to a unix socket for a debug endpoint. Can be set with $DEBUG_UNIX_SOCKET.")

pkg/shell-operator/bootstrap.go

@@ -26,6 +26,10 @@ import (
 // a ShellOperator instance with all dependencies.
 // cfg must already have all configuration sources merged (NewConfig → ParseEnv → BindFlags → flags parsed).
 func Init(ctx context.Context, cfg *app.Config, logger *log.Logger) (*ShellOperator, error) {
+	// Propagate cfg into package-level globals (e.g. DebugUnixSocket) so
+	// library consumers that skip BindFlags still get them overridden from cfg.
+	app.ApplyConfig(cfg)
+
 	// Initialize webhook settings from merged configuration.
 	admission.InitFromSettings(admission.WebhookSettings{
 		Settings: webhookserver.Settings{
@@ -115,16 +119,42 @@ func Init(ctx context.Context, cfg *app.Config, logger *log.Logger) (*ShellOpera
 // example, addon-operator builds its own and embeds shell-operator).
 //
 // It derives the HTTP server address, the main and object-patcher
-// KubeClientConfigs from cfg and delegates to AssembleCommonOperator.
+// KubeClientConfigs from cfg and delegates to AssembleCommonOperator. The
+// derivation reads only the supplied *app.Config — no environment variables
+// are consulted on this path, so the values you put into cfg are the values
+// shell-operator uses. See kubeClientConfigsFromAppConfig for the exact
+// field mapping.
+//
 // kubeEventsManagerLabels are the metric labels for the kube-events manager;
 // each embedder typically passes its own (e.g. addon-operator adds "module"
 // and "kind", shell-operator passes "hook"/"binding"/"queue").
 //
 // Pass a nil cfg to fall back to zero-valued KubeClientConfig (in-cluster
 // defaults) — useful for tests.
 func (op *ShellOperator) AssembleCommonOperatorFromConfig(cfg *app.Config, kubeEventsManagerLabels []string) error {
+	listenAddress, listenPort := listenAddrFromAppConfig(cfg)
+	mainKubeCfg, patcherKubeCfg := kubeClientConfigsFromAppConfig(cfg)
+	return op.AssembleCommonOperator(listenAddress, listenPort, kubeEventsManagerLabels, mainKubeCfg, patcherKubeCfg)
+}
+
+// listenAddrFromAppConfig returns the HTTP server listen address/port from cfg
+// or empty strings when cfg is nil. Extracted as a helper so unit tests can
+// assert that no environment variable is consulted during derivation.
+func listenAddrFromAppConfig(cfg *app.Config) (string, string) {
+	if cfg == nil {
+		return "", ""
+	}
+	return cfg.App.ListenAddress, cfg.App.ListenPort
+}
+
+// kubeClientConfigsFromAppConfig derives the main and object-patcher
+// KubeClientConfigs from an *app.Config. The function is pure: it does not
+// touch the process environment, so any value present in cfg is used as-is
+// and library consumers can rely on env vars never overriding their config.
+// A nil cfg yields two zero KubeClientConfig values (in-cluster defaults).
+func kubeClientConfigsFromAppConfig(cfg *app.Config) (KubeClientConfig, KubeClientConfig) {
 	if cfg == nil {
-		return op.AssembleCommonOperator("", "", kubeEventsManagerLabels, KubeClientConfig{}, KubeClientConfig{})
+		return KubeClientConfig{}, KubeClientConfig{}
 	}
 	mainKubeCfg := KubeClientConfig{
 		Context:      cfg.Kube.Context,
@@ -141,7 +171,7 @@ func (op *ShellOperator) AssembleCommonOperatorFromConfig(cfg *app.Config, kubeE
 		Timeout:      cfg.ObjectPatcher.KubeClientTimeout,
 		MetricPrefix: "object_patcher_",
 	}
-	return op.AssembleCommonOperator(cfg.App.ListenAddress, cfg.App.ListenPort, kubeEventsManagerLabels, mainKubeCfg, patcherKubeCfg)
+	return mainKubeCfg, patcherKubeCfg
 }
 
 // AssembleCommonOperator instantiates common dependencies used by both

pkg/shell-operator/bootstrap_test.go

@@ -0,0 +1,139 @@
+package shell_operator
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/flant/shell-operator/pkg/app"
+)
+
+// TestKubeClientConfigsFromAppConfig_DerivedFromConfig is a basic check that
+// the per-client KubeClientConfigs are built straight from the supplied
+// *app.Config (including the special "object_patcher_" metric prefix for the
+// patcher client).
+func TestKubeClientConfigsFromAppConfig_DerivedFromConfig(t *testing.T) {
+	cfg := &app.Config{
+		App: app.AppSettings{
+			ListenAddress:           "127.0.0.1",
+			ListenPort:              "9000",
+			PrometheusMetricsPrefix: "embedded_",
+		},
+		Kube: app.KubeSettings{
+			Context:     "explicit-ctx",
+			Config:      "/explicit/kubeconfig",
+			ClientQPS:   42,
+			ClientBurst: 84,
+		},
+		ObjectPatcher: app.ObjectPatcherSettings{
+			KubeClientQPS:     11,
+			KubeClientBurst:   22,
+			KubeClientTimeout: 7 * time.Second,
+		},
+	}
+
+	addr, port := listenAddrFromAppConfig(cfg)
+	main, patcher := kubeClientConfigsFromAppConfig(cfg)
+
+	assert.Equal(t, "127.0.0.1", addr)
+	assert.Equal(t, "9000", port)
+
+	assert.Equal(t, "explicit-ctx", main.Context)
+	assert.Equal(t, "/explicit/kubeconfig", main.Config)
+	assert.InDelta(t, float32(42), main.QPS, 0.001)
+	assert.Equal(t, 84, main.Burst)
+	assert.Equal(t, time.Duration(0), main.Timeout)
+	assert.Equal(t, "embedded_", main.MetricPrefix)
+
+	assert.Equal(t, "explicit-ctx", patcher.Context)
+	assert.Equal(t, "/explicit/kubeconfig", patcher.Config)
+	assert.InDelta(t, float32(11), patcher.QPS, 0.001)
+	assert.Equal(t, 22, patcher.Burst)
+	assert.Equal(t, 7*time.Second, patcher.Timeout)
+	assert.Equal(t, "object_patcher_", patcher.MetricPrefix)
+}
+
+// TestKubeClientConfigsFromAppConfig_NilCfg verifies the nil-cfg fallback
+// yields zero KubeClientConfig values (which klient treats as in-cluster
+// defaults) without consulting the environment.
+func TestKubeClientConfigsFromAppConfig_NilCfg(t *testing.T) {
+	t.Setenv("KUBE_CONTEXT", "env-ctx")
+	t.Setenv("SHELL_OPERATOR_LISTEN_PORT", "7777")
+
+	addr, port := listenAddrFromAppConfig(nil)
+	main, patcher := kubeClientConfigsFromAppConfig(nil)
+
+	assert.Empty(t, addr)
+	assert.Empty(t, port)
+	assert.Equal(t, KubeClientConfig{}, main)
+	assert.Equal(t, KubeClientConfig{}, patcher)
+}
+
+// TestAssembleFromConfig_EnvDoesNotOverrideConfig is the regression test that
+// guards the library contract: when the consumer hands us an *app.Config,
+// environment variables MUST NOT silently override its values. We set env
+// vars that conflict with every field used by the assembly path and verify
+// the derived listen address, KubeClient settings and metric prefix all come
+// from cfg instead.
+func TestAssembleFromConfig_EnvDoesNotOverrideConfig(t *testing.T) {
+	// Set env vars that, if mistakenly consulted, would change every
+	// derived value.
+	t.Setenv("SHELL_OPERATOR_LISTEN_ADDRESS", "9.9.9.9")
+	t.Setenv("SHELL_OPERATOR_LISTEN_PORT", "9999")
+	t.Setenv("SHELL_OPERATOR_PROMETHEUS_METRICS_PREFIX", "env_prefix_")
+	t.Setenv("KUBE_CONTEXT", "env-ctx")
+	t.Setenv("KUBE_CONFIG", "/env/kubeconfig")
+	t.Setenv("KUBE_CLIENT_QPS", "999")
+	t.Setenv("KUBE_CLIENT_BURST", "888")
+	t.Setenv("OBJECT_PATCHER_KUBE_CLIENT_QPS", "777")
+	t.Setenv("OBJECT_PATCHER_KUBE_CLIENT_BURST", "666")
+	t.Setenv("OBJECT_PATCHER_KUBE_CLIENT_TIMEOUT", "55s")
+
+	cfg := &app.Config{
+		App: app.AppSettings{
+			ListenAddress:           "127.0.0.1",
+			ListenPort:              "9000",
+			PrometheusMetricsPrefix: "lib_prefix_",
+		},
+		Kube: app.KubeSettings{
+			Context:     "lib-ctx",
+			Config:      "/lib/kubeconfig",
+			ClientQPS:   1,
+			ClientBurst: 2,
+		},
+		ObjectPatcher: app.ObjectPatcherSettings{
+			KubeClientQPS:     3,
+			KubeClientBurst:   4,
+			KubeClientTimeout: 5 * time.Second,
+		},
+	}
+
+	addr, port := listenAddrFromAppConfig(cfg)
+	main, patcher := kubeClientConfigsFromAppConfig(cfg)
+
+	// Listen address/port must come from cfg, not env.
+	assert.Equal(t, "127.0.0.1", addr)
+	assert.Equal(t, "9000", port)
+
+	// Main client values come from cfg.Kube / cfg.App.
+	assert.Equal(t, "lib-ctx", main.Context)
+	assert.Equal(t, "/lib/kubeconfig", main.Config)
+	assert.InDelta(t, float32(1), main.QPS, 0.001)
+	assert.Equal(t, 2, main.Burst)
+	assert.Equal(t, "lib_prefix_", main.MetricPrefix)
+
+	// Patcher client values come from cfg.ObjectPatcher.
+	assert.Equal(t, "lib-ctx", patcher.Context)
+	assert.Equal(t, "/lib/kubeconfig", patcher.Config)
+	assert.InDelta(t, float32(3), patcher.QPS, 0.001)
+	assert.Equal(t, 4, patcher.Burst)
+	assert.Equal(t, 5*time.Second, patcher.Timeout)
+	assert.Equal(t, "object_patcher_", patcher.MetricPrefix)
+
+	// And the source-of-truth cfg itself is untouched by the call.
+	assert.Equal(t, "127.0.0.1", cfg.App.ListenAddress)
+	assert.Equal(t, "9000", cfg.App.ListenPort)
+	assert.Equal(t, "lib-ctx", cfg.Kube.Context)
+	assert.InDelta(t, float32(1), cfg.Kube.ClientQPS, 0.001)
+}