fix: error on missing secret key when using vals (#2496)

* fix: error on missing secret key when using vals Add HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP environment variable to control whether vals should fail when a referenced key does not exist in the secret map. Previously, when a secret reference like ref+vault://path#/nonexistent-key pointed to a non-existent key, vals would silently return an empty string without error. This could lead to deployments with missing configuration. Default behavior remains backward compatible (returns empty string). Set HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP=true to enable strict mode. Fixes #1563 Signed-off-by: yxxhero <aiopsclub@163.com> * refactor: extract buildValsOptions helper and improve tests - Extract buildValsOptions() to make vals configuration testable - Use t.Setenv instead of manual env save/restore in tests - Test actual vals.Options output including FailOnMissingKeyInMap Addresses PR review comments on #2496 Signed-off-by: yxxhero <aiopsclub@163.com> * fix: use strconv.ParseBool and make tests hermetic - Use strconv.ParseBool for FailOnMissingKeyInMap parsing to support common boolean values like 'TRUE', '1', '0', etc. - Always set env vars explicitly in tests (even to empty string) to prevent flaky tests when env vars are set externally - Add test cases for various boolean formats Signed-off-by: yxxhero <aiopsclub@163.com> * docs: add documentation for vals-related environment variables Add documentation for: - HELMFILE_AWS_SDK_LOG_LEVEL: configure AWS SDK logging for vals - HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP: enable strict mode for secret refs Signed-off-by: yxxhero <aiopsclub@163.com> * fix: improve error handling and case-insensitive comparison - buildValsOptions now returns error for invalid boolean values instead of silently defaulting to false - Use strings.EqualFold for case-insensitive 'off' comparison to handle OFF, Off, etc. - Add test cases for invalid boolean and uppercase OFF - Update docs to mention case-insensitive and error behavior Signed-off-by: yxxhero <aiopsclub@163.com> * fix: normalize log level and improve singleton initialization - Normalize AWS log level 'off' to lowercase for true case-insensitivity - Replace sync.Once with mutex to allow recovery from config errors - Update tests to expect normalized 'off' value - Update docs to clarify when error is raised Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com>
2026-03-24 09:42:54 +08:00 · 2026-03-24 09:42:54 +08:00 · 472e8c7a2d
parent 5c67cbcd6a
commit 472e8c7a2d
4 changed files with 260 additions and 122 deletions
--- a/docs/index.md
+++ b/docs/index.md
@ -593,6 +593,8 @@ Helmfile uses some OS environment variables to override default behaviour:
 * `HELMFILE_FILE_PATH` - specify the path to the helmfile.yaml file
 * `HELMFILE_INTERACTIVE` - enable interactive mode, expecting `true` lower case. The same as `--interactive` CLI flag
 * `HELMFILE_RENDER_YAML` - force helmfile.yaml to be rendered as a Go template regardless of file extension, expecting `true` lower case. Useful for migrating from v0 to v1 without renaming files to `.gotmpl`
+* `HELMFILE_AWS_SDK_LOG_LEVEL` - configure AWS SDK logging level for vals library. Valid values: `off` (default, secure, case-insensitive), `minimal`, `standard`, `verbose`, or custom comma-separated values like `request,response`. See issue #2270 for details
+* `HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP` - enable strict mode for vals secret references. When set to `true` (or any value accepted by Go's `strconv.ParseBool` like `TRUE`, `1`), vals will fail when a referenced key does not exist in the secret map. Invalid values will cause an error when vals is initialized (when secret refs are first evaluated). Default is `false` (when unset or empty) for backward compatibility. See issue #1563 for details

 ## CLI Reference

--- a/pkg/envvar/const.go
+++ b/pkg/envvar/const.go
@ -28,4 +28,10 @@ const (
 	// Can be overridden by AWS_SDK_GO_LOG_LEVEL environment variable
 	// See issue #2270 and vals PR #893
 	AWSSDKLogLevel = "HELMFILE_AWS_SDK_LOG_LEVEL"
+
+	// ValsFailOnMissingKeyInMap controls whether vals should fail when a key is missing in a map.
+	// When set to "true", vals returns an error if a referenced key does not exist in the secret map.
+	// Default is false for backward compatibility (returns empty string for missing keys).
+	// See issue #1563
+	ValsFailOnMissingKeyInMap = "HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP"
 )
--- a/pkg/plugins/vals.go
+++ b/pkg/plugins/vals.go
@ -1,8 +1,10 @@
 package plugins

 import (
+	"fmt"
 	"io"
 	"os"
+	"strconv"
 	"strings"
 	"sync"

@ -17,46 +19,82 @@ const (
 )

 var instance *vals.Runtime
-var once sync.Once
+var mu sync.Mutex
+
+func buildValsOptions() (vals.Options, error) {
+	// Configure AWS SDK logging via HELMFILE_AWS_SDK_LOG_LEVEL environment variable
+	// Default: "off" to prevent sensitive information (tokens, auth headers) from being exposed
+	// See issue #2270 and vals PR helmfile/vals#893
+	//
+	// Valid values:
+	// - "off" (default): No AWS SDK logging - secure, prevents credential leakage
+	// - "minimal": Log retries only - minimal debugging info
+	// - "standard": Log retries + requests - moderate debugging (previous default)
+	// - "verbose": Log everything - full debugging (requests, responses, bodies, signing)
+	// - Custom: Comma-separated values like "request,response"
+	//
+	// Note: AWS_SDK_GO_LOG_LEVEL environment variable always takes precedence over this setting
+	// Note: Case-insensitive for known values like "off", "OFF", "Off"
+	logLevel := strings.TrimSpace(os.Getenv(envvar.AWSSDKLogLevel))
+
+	// Configure fail on missing key behavior
+	// Default to false for backward compatibility
+	// Set HELMFILE_VALS_FAIL_ON_MISSING_KEY_IN_MAP=true to enable strict mode
+	// Supports common boolean values: "true", "TRUE", "1", etc.
+	// See issue #1563
+	envVal := strings.TrimSpace(os.Getenv(envvar.ValsFailOnMissingKeyInMap))
+	var failOnMissingKey bool
+	if envVal != "" {
+		var err error
+		failOnMissingKey, err = strconv.ParseBool(envVal)
+		if err != nil {
+			return vals.Options{}, fmt.Errorf("invalid value for %s: %q (must be a valid boolean)", envvar.ValsFailOnMissingKeyInMap, envVal)
+		}
+	}
+
+	// Default to "off" for security if not specified
+	if logLevel == "" {
+		logLevel = "off"
+	}
+
+	// Normalize known values to lowercase for case-insensitive handling
+	if strings.EqualFold(logLevel, "off") {
+		logLevel = "off"
+	}
+
+	opts := vals.Options{
+		CacheSize:             valsCacheSize,
+		FailOnMissingKeyInMap: failOnMissingKey,
+		AWSLogLevel:           logLevel,
+	}
+
+	// Also suppress vals' own internal logging unless user wants verbose output
+	// This prevents vals' log messages (separate from AWS SDK logs) from exposing credentials
+	if logLevel == "off" {
+		opts.LogOutput = io.Discard
+	}
+	// For other levels, allow vals to log to default output for debugging
+
+	return opts, nil
+}

 func ValsInstance() (*vals.Runtime, error) {
-	var err error
-	once.Do(func() {
-		// Configure AWS SDK logging via HELMFILE_AWS_SDK_LOG_LEVEL environment variable
-		// Default: "off" to prevent sensitive information (tokens, auth headers) from being exposed
-		// See issue #2270 and vals PR helmfile/vals#893
-		//
-		// Valid values:
-		// - "off" (default): No AWS SDK logging - secure, prevents credential leakage
-		// - "minimal": Log retries only - minimal debugging info
-		// - "standard": Log retries + requests - moderate debugging (previous default)
-		// - "verbose": Log everything - full debugging (requests, responses, bodies, signing)
-		// - Custom: Comma-separated values like "request,response"
-		//
-		// Note: AWS_SDK_GO_LOG_LEVEL environment variable always takes precedence over this setting
-		logLevel := strings.TrimSpace(os.Getenv(envvar.AWSSDKLogLevel))
+	mu.Lock()
+	defer mu.Unlock()

-		opts := vals.Options{
-			CacheSize: valsCacheSize,
-		}
+	if instance != nil {
+		return instance, nil
+	}

-		// Default to "off" for security if not specified
-		if logLevel == "" {
-			logLevel = "off"
-		}
+	opts, err := buildValsOptions()
+	if err != nil {
+		return nil, err
+	}

-		// Set AWS SDK log level for vals library
-		opts.AWSLogLevel = logLevel
+	instance, err = vals.New(opts)
+	if err != nil {
+		return nil, err
+	}

-		// Also suppress vals' own internal logging unless user wants verbose output
-		// This prevents vals' log messages (separate from AWS SDK logs) from exposing credentials
-		if logLevel == "off" {
-			opts.LogOutput = io.Discard
-		}
-		// For other levels, allow vals to log to default output for debugging
-
-		instance, err = vals.New(opts)
-	})
-
-	return instance, err
+	return instance, nil
 }
--- a/pkg/plugins/vals_test.go
+++ b/pkg/plugins/vals_test.go
@ -2,11 +2,11 @@ package plugins

 import (
 	"io"
-	"os"
-	"strings"
 	"testing"

 	"github.com/helmfile/vals"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"

 	"github.com/helmfile/helmfile/pkg/envvar"
 )
@ -25,19 +25,165 @@ func TestValsInstance(t *testing.T) {
 	}
 }

-// TestAWSSDKLogLevelConfiguration tests the AWS SDK log level configuration logic
+func TestBuildValsOptions(t *testing.T) {
+	tests := []struct {
+		name                       string
+		awsLogLevel                string
+		failOnMissingKey           string
+		expectedLogLevel           string
+		expectedFailOnMissingKey   bool
+		expectedLogOutputDiscarded bool
+		expectError                bool
+	}{
+		{
+			name:                       "defaults",
+			awsLogLevel:                "",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "explicit failOnMissingKey true",
+			awsLogLevel:                "",
+			failOnMissingKey:           "true",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   true,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "failOnMissingKey false",
+			awsLogLevel:                "",
+			failOnMissingKey:           "false",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "failOnMissingKey with whitespace",
+			awsLogLevel:                "",
+			failOnMissingKey:           "  true  ",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   true,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "failOnMissingKey uppercase TRUE",
+			awsLogLevel:                "",
+			failOnMissingKey:           "TRUE",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   true,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "failOnMissingKey numeric 1",
+			awsLogLevel:                "",
+			failOnMissingKey:           "1",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   true,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "failOnMissingKey numeric 0",
+			awsLogLevel:                "",
+			failOnMissingKey:           "0",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:             "failOnMissingKey invalid value",
+			awsLogLevel:      "",
+			failOnMissingKey: "invalid",
+			expectError:      true,
+		},
+		{
+			name:                       "aws log level verbose",
+			awsLogLevel:                "verbose",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "verbose",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: false,
+		},
+		{
+			name:                       "aws log level with whitespace",
+			awsLogLevel:                "  minimal  ",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "minimal",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: false,
+		},
+		{
+			name:                       "aws log level OFF uppercase",
+			awsLogLevel:                "OFF",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "aws log level Off mixed case",
+			awsLogLevel:                "Off",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "aws log level Off mixed case",
+			awsLogLevel:                "Off",
+			failOnMissingKey:           "",
+			expectedLogLevel:           "off",
+			expectedFailOnMissingKey:   false,
+			expectedLogOutputDiscarded: true,
+		},
+		{
+			name:                       "both options set",
+			awsLogLevel:                "standard",
+			failOnMissingKey:           "true",
+			expectedLogLevel:           "standard",
+			expectedFailOnMissingKey:   true,
+			expectedLogOutputDiscarded: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envvar.AWSSDKLogLevel, tt.awsLogLevel)
+			t.Setenv(envvar.ValsFailOnMissingKeyInMap, tt.failOnMissingKey)
+
+			opts, err := buildValsOptions()
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), envvar.ValsFailOnMissingKeyInMap)
+				return
+			}
+
+			require.NoError(t, err)
+
+			assert.Equal(t, tt.expectedLogLevel, opts.AWSLogLevel)
+			assert.Equal(t, tt.expectedFailOnMissingKey, opts.FailOnMissingKeyInMap)
+			assert.Equal(t, valsCacheSize, opts.CacheSize)
+
+			isDiscarded := opts.LogOutput == io.Discard
+			assert.Equal(t, tt.expectedLogOutputDiscarded, isDiscarded)
+		})
+	}
+}
+
 func TestAWSSDKLogLevelConfiguration(t *testing.T) {
 	tests := []struct {
 		name              string
 		envValue          string
 		expectedLogLevel  string
-		expectedLogOutput bool // true if LogOutput should be io.Discard
+		expectedLogOutput bool
 	}{
 		{
 			name:              "no env var defaults to off",
 			envValue:          "",
 			expectedLogLevel:  "off",
-			expectedLogOutput: true, // LogOutput should be io.Discard
+			expectedLogOutput: true,
 		},
 		{
 			name:              "explicit off",
@ -45,11 +191,17 @@ func TestAWSSDKLogLevelConfiguration(t *testing.T) {
 			expectedLogLevel:  "off",
 			expectedLogOutput: true,
 		},
+		{
+			name:              "OFF uppercase",
+			envValue:          "OFF",
+			expectedLogLevel:  "off",
+			expectedLogOutput: true,
+		},
 		{
 			name:              "minimal logging",
 			envValue:          "minimal",
 			expectedLogLevel:  "minimal",
-			expectedLogOutput: false, // LogOutput should NOT be io.Discard
+			expectedLogOutput: false,
 		},
 		{
 			name:              "standard logging",
@ -73,94 +225,34 @@ func TestAWSSDKLogLevelConfiguration(t *testing.T) {

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// Note: This test verifies the configuration logic, not the actual vals.New() call
-			// since ValsInstance() uses sync.Once and can only be initialized once per test run.
+			t.Setenv(envvar.AWSSDKLogLevel, tt.envValue)

-			// Simulate the logic from ValsInstance()
-			var logLevel string
-			if tt.envValue != "" {
-				logLevel = strings.TrimSpace(tt.envValue)
-			}
+			opts, err := buildValsOptions()
+			require.NoError(t, err)

-			// Default to "off" for security if not specified
-			if logLevel == "" {
-				logLevel = "off"
-			}
+			assert.Equal(t, tt.expectedLogLevel, opts.AWSLogLevel)

-			// Verify expected log level
-			if logLevel != tt.expectedLogLevel {
-				t.Errorf("Expected log level %q, got %q", tt.expectedLogLevel, logLevel)
-			}
-
-			// Verify LogOutput configuration logic
-			opts := vals.Options{
-				CacheSize: valsCacheSize,
-			}
-			opts.AWSLogLevel = logLevel
-
-			// Verify LogOutput is set to io.Discard only when level is "off"
-			if tt.expectedLogOutput {
-				opts.LogOutput = io.Discard
-				if opts.LogOutput != io.Discard {
-					t.Error("Expected LogOutput to be io.Discard for 'off' level")
-				}
-			}
+			isDiscarded := opts.LogOutput == io.Discard
+			assert.Equal(t, tt.expectedLogOutput, isDiscarded)
 		})
 	}
 }

-// TestEnvironmentVariableReading verifies that the HELMFILE_AWS_SDK_LOG_LEVEL env var is read correctly
-func TestEnvironmentVariableReading(t *testing.T) {
-	tests := []struct {
-		name          string
-		envValue      string
-		expectedValue string
-	}{
-		{
-			name:          "empty defaults to off",
-			envValue:      "",
-			expectedValue: "off",
-		},
-		{
-			name:          "whitespace trimmed",
-			envValue:      "  minimal  ",
-			expectedValue: "minimal",
-		},
-		{
-			name:          "standard value preserved",
-			envValue:      "standard",
-			expectedValue: "standard",
-		},
-	}
+func TestBuildValsOptionsIntegration(t *testing.T) {
+	t.Run("valid configuration produces working vals options", func(t *testing.T) {
+		t.Setenv(envvar.AWSSDKLogLevel, "off")
+		t.Setenv(envvar.ValsFailOnMissingKeyInMap, "true")

-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Save and restore env var
-			original := os.Getenv(envvar.AWSSDKLogLevel)
-			defer func() {
-				if original == "" {
-					os.Unsetenv(envvar.AWSSDKLogLevel)
-				} else {
-					os.Setenv(envvar.AWSSDKLogLevel, original)
-				}
-			}()
+		opts, err := buildValsOptions()
+		require.NoError(t, err)

-			// Set test env var
-			if tt.envValue == "" {
-				os.Unsetenv(envvar.AWSSDKLogLevel)
-			} else {
-				os.Setenv(envvar.AWSSDKLogLevel, tt.envValue)
-			}
+		assert.Equal(t, valsCacheSize, opts.CacheSize)
+		assert.Equal(t, "off", opts.AWSLogLevel)
+		assert.True(t, opts.FailOnMissingKeyInMap)
+		assert.Equal(t, io.Discard, opts.LogOutput)

-			// Read and process like ValsInstance() does
-			logLevel := strings.TrimSpace(os.Getenv(envvar.AWSSDKLogLevel))
-			if logLevel == "" {
-				logLevel = "off"
-			}
-
-			if logLevel != tt.expectedValue {
-				t.Errorf("Expected %q, got %q", tt.expectedValue, logLevel)
-			}
-		})
-	}
+		rt, err := vals.New(opts)
+		require.NoError(t, err)
+		assert.NotNil(t, rt)
+	})
 }