From 38c435efdca658f08a463808d2b3abfb4d4aa67b Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Mon, 27 Jul 2026 17:37:56 -0500 Subject: [PATCH] config: apply environment overrides before unmarshal --- pkg/config/config.go | 3 +- pkg/config/config_test.go | 11 +-- pkg/config/environment.go | 9 +- pkg/config/environment_test.go | 153 +++++++++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 10 deletions(-) create mode 100644 pkg/config/environment_test.go diff --git a/pkg/config/config.go b/pkg/config/config.go index 92d9d422..0b4d581c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -361,12 +361,13 @@ func (c *Config) Load(ctx context.Context) error { } } + c.mergeEnvVars() + err := c.FileConfig.Unmarshal(c) if err != nil { return err } - c.mergeEnvVars() return c.expandOSPathFlagValues() } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 9c632a53..89c59dfb 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -766,15 +766,10 @@ func TestValidateSetInput(t *testing.T) { } func TestEnvironmentHelpers(t *testing.T) { - t.Setenv("GNMIC_API_SERVER_ADDRESS", ":9999") t.Setenv("GNMIC_OUTPUTS_FILE_TYPE", "file") t.Setenv("OTHER_VAR", "ignored") got := envToMap() - apiServer := got["api"].(map[string]any)["server"].(map[string]any) - if apiServer["address"] != ":9999" { - t.Fatalf("envToMap api-server address = %#v", apiServer) - } outputs := got["outputs"].(map[string]any)["file"].(map[string]any) if outputs["type"] != "file" { t.Fatalf("envToMap output type = %#v", outputs) @@ -1316,11 +1311,11 @@ func TestExpandOSPathFlagValuesAndMergeEnv(t *testing.T) { t.Fatalf("tls-ca = %q, want %q", got, file) } - t.Setenv("GNMIC_FORMAT", "event") + t.Setenv("GNMIC_OUTPUTS_FILE_TYPE", "file") c = New() c.mergeEnvVars() - if got := c.FileConfig.GetString("format"); got != "event" { - t.Fatalf("mergeEnvVars format=%q", got) + if got := c.FileConfig.GetString("outputs/file/type"); got != "file" { + t.Fatalf("mergeEnvVars outputs/file/type=%q", got) } } diff --git a/pkg/config/environment.go b/pkg/config/environment.go index c0aecdcc..62c8ba8e 100644 --- a/pkg/config/environment.go +++ b/pkg/config/environment.go @@ -28,7 +28,14 @@ func envToMap() map[string]any { } pair[0] = strings.ToLower(strings.TrimPrefix(pair[0], envPrefix+"_")) items := strings.Split(pair[0], "_") - mergeMap(m, items, pair[1]) + + // Only free-form configuration sections need underscore-separated + // environment variables materialized as nested maps. Known struct + // fields are handled by Viper's AutomaticEnv and key replacer. + switch items[0] { + case "clustering", "outputs", "inputs", "processors", "loader", "actions": + mergeMap(m, items, pair[1]) + } } return m } diff --git a/pkg/config/environment_test.go b/pkg/config/environment_test.go new file mode 100644 index 00000000..d8429db6 --- /dev/null +++ b/pkg/config/environment_test.go @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/pflag" +) + +func TestEnvToMapSplitsOnlyFreeFormSections(t *testing.T) { + sections := []string{ + "clustering", + "outputs", + "inputs", + "processors", + "loader", + "actions", + } + + for _, section := range sections { + t.Setenv( + "GNMIC_"+strings.ToUpper(section)+"_TEST_VALUE", + section, + ) + } + + // These are known struct-backed fields and must be handled by Viper, + // not interpreted as nested maps by envToMap. + t.Setenv("GNMIC_CLUSTER_NAME", "collector-cluster") + t.Setenv("GNMIC_LOG_FILE", "/tmp/gnmic-env.log") + + got := envToMap() + + for _, section := range sections { + level1, ok := got[section].(map[string]any) + if !ok { + t.Fatalf( + "envToMap()[%q] = %T, want map[string]any", + section, + got[section], + ) + } + + level2, ok := level1["test"].(map[string]any) + if !ok { + t.Fatalf( + "envToMap()[%q][test] = %T, want map[string]any", + section, + level1["test"], + ) + } + + if value, want := level2["value"], section; value != want { + t.Errorf( + "envToMap()[%q][test][value] = %v, want %q", + section, + value, + want, + ) + } + } + + if _, ok := got["cluster"]; ok { + t.Errorf( + "envToMap() contains cluster = %v; known cluster-name field should be excluded", + got["cluster"], + ) + } + + if _, ok := got["log"]; ok { + t.Errorf( + "envToMap() contains log = %v; known log-file field should be excluded", + got["log"], + ) + } +} + +func TestLoadEnvironmentOverridesConfig(t *testing.T) { + t.Setenv("GNMIC_CLUSTER_NAME", "collector-cluster") + t.Setenv("GNMIC_LOG_FILE", "/tmp/gnmic-env.log") + t.Setenv( + "GNMIC_CLUSTERING_LOCKER_ADDRESS", + "consul.example:8500", + ) + + configFile := filepath.Join(t.TempDir(), "gnmic.yaml") + if err := os.WriteFile( + configFile, + []byte(`log-file: /tmp/gnmic-file.log +clustering: + locker: + type: consul + address: file.example:8500 +`), + 0o600, + ); err != nil { + t.Fatalf("os.WriteFile() failed: %v", err) + } + + cfg := New() + cfg.CfgFile = configFile + + // Global flags are bound before Config.Load in the actual application. + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String("cluster-name", "default-cluster", "") + flags.String("log-file", "", "") + + for _, name := range []string{"cluster-name", "log-file"} { + if err := cfg.FileConfig.BindPFlag( + name, + flags.Lookup(name), + ); err != nil { + t.Fatalf("BindPFlag(%q) failed: %v", name, err) + } + } + + if err := cfg.Load(context.Background()); err != nil { + t.Fatalf("Config.Load() failed: %v", err) + } + + if got, want := cfg.ClusterName, "collector-cluster"; got != want { + t.Errorf("ClusterName = %q, want %q", got, want) + } + + if got, want := cfg.LogFile, "/tmp/gnmic-env.log"; got != want { + t.Errorf("LogFile = %q, want %q", got, want) + } + + if cfg.Clustering == nil { + t.Fatal("Clustering is nil, want configuration populated") + } + + if got, want := cfg.Clustering.Locker["type"], "consul"; got != want { + t.Errorf( + "Clustering.Locker[type] = %v, want %q", + got, + want, + ) + } + + if got, want := cfg.Clustering.Locker["address"], "consul.example:8500"; got != want { + t.Errorf( + "Clustering.Locker[address] = %v, want %q", + got, + want, + ) + } +}