From c3c8f5493206de00b0b39d6719ead960b0b13619 Mon Sep 17 00:00:00 2001 From: Pavel Okhlopkov Date: Wed, 20 May 2026 07:55:07 +0300 Subject: [PATCH 1/2] continue fixing passed config Signed-off-by: Pavel Okhlopkov --- pkg/shell-operator/bootstrap.go | 32 +++++- pkg/shell-operator/bootstrap_test.go | 139 +++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 3 deletions(-) create mode 100644 pkg/shell-operator/bootstrap_test.go diff --git a/pkg/shell-operator/bootstrap.go b/pkg/shell-operator/bootstrap.go index b7fccf4b..9f2d8bb2 100644 --- a/pkg/shell-operator/bootstrap.go +++ b/pkg/shell-operator/bootstrap.go @@ -115,7 +115,12 @@ 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"). @@ -123,8 +128,29 @@ func Init(ctx context.Context, cfg *app.Config, logger *log.Logger) (*ShellOpera // 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 +167,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 diff --git a/pkg/shell-operator/bootstrap_test.go b/pkg/shell-operator/bootstrap_test.go new file mode 100644 index 00000000..83eb25e3 --- /dev/null +++ b/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) +} From 206d0518b7c61ac4a58cc7172f73c0b19ae8d595 Mon Sep 17 00:00:00 2001 From: Pavel Okhlopkov Date: Wed, 20 May 2026 09:57:31 +0300 Subject: [PATCH 2/2] add apply from config values Signed-off-by: Pavel Okhlopkov --- pkg/app/README.md | 22 +++++++++++++++++++++- pkg/app/debug.go | 20 ++++++++++++++++++++ pkg/app/debug_test.go | 31 +++++++++++++++++++++++++++++-- pkg/app/flags.go | 2 +- pkg/shell-operator/bootstrap.go | 4 ++++ 5 files changed, 75 insertions(+), 4 deletions(-) diff --git a/pkg/app/README.md b/pkg/app/README.md index 13756fed..5e6f61f0 100644 --- a/pkg/app/README.md +++ b/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` diff --git a/pkg/app/debug.go b/pkg/app/debug.go index ed622f32..86856225 100644 --- a/pkg/app/debug.go +++ b/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. diff --git a/pkg/app/debug_test.go b/pkg/app/debug_test.go index d8331465..03d6c5df 100644 --- a/pkg/app/debug_test.go +++ b/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) +} diff --git a/pkg/app/flags.go b/pkg/app/flags.go index 8c174362..810dfcfe 100644 --- a/pkg/app/flags.go +++ b/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.") diff --git a/pkg/shell-operator/bootstrap.go b/pkg/shell-operator/bootstrap.go index 9f2d8bb2..eb2b4946 100644 --- a/pkg/shell-operator/bootstrap.go +++ b/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{