fix(vault): only apply deprecated token-cache flags when explicitly set (#6827)

Signed-off-by: Marko Filipovic <marko.filipovic@caralegal.eu>
Co-authored-by: Marko Filipovic <marko.filipovic@caralegal.eu>
Co-authored-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>

providers/v1/vault/provider.go

@@ -323,53 +323,76 @@ func initCache(size int) {
 	})
 }
 
-func init() {
-	var (
-		vaultTokenCacheSize     int
-		experimentalEnableCache bool
-		experimentalCacheSize   int
-	)
+// vaultCacheFlags holds the token cache flag values. Keeping them in a struct
+// lets the registration and the resolution be covered by a test without going
+// through init() and the package level state it writes.
+type vaultCacheFlags struct {
+	enable    bool
+	size      int
+	expEnable bool
+	expSize   int
+}
 
-	fs := pflag.NewFlagSet("vault", pflag.ExitOnError)
+// registerVaultCacheFlags declares the token cache flags on a new FlagSet.
+// Both deprecated experimental flags carry the same defaults as the flags that
+// replace them, so their value alone cannot tell an explicit setting apart from
+// an untouched default. resolveVaultCacheConfig uses fs.Changed for that.
+func registerVaultCacheFlags(errorHandling pflag.ErrorHandling) (*pflag.FlagSet, *vaultCacheFlags) {
+	flags := &vaultCacheFlags{}
+	fs := pflag.NewFlagSet("vault", errorHandling)
 	fs.BoolVar(
-		&enableCache,
+		&flags.enable,
 		"enable-vault-token-cache",
 		false,
 		"Enable Vault token cache. External secrets will reuse the Vault token without creating a new one on each request.",
 	)
 	// max. 265k vault leases with 30bytes each ~= 7MB
 	fs.IntVar(
-		&vaultTokenCacheSize,
+		&flags.size,
 		"vault-token-cache-size",
 		defaultCacheSize,
 		"Maximum size of Vault token cache. Only used if --enable-vault-token-cache is set.",
 	)
 	fs.BoolVar(
-		&experimentalEnableCache,
+		&flags.expEnable,
 		"experimental-enable-vault-token-cache",
 		false,
 		"Enable Vault token cache. External secrets will reuse the Vault token without creating a new one on each request.",
 	)
 	// max. 265k vault leases with 30bytes each ~= 7MB
 	fs.IntVar(
-		&experimentalCacheSize,
+		&flags.expSize,
 		"experimental-vault-token-cache-size",
 		defaultCacheSize,
 		"Maximum size of Vault token cache. Only used if --experimental-enable-vault-token-cache is set.",
 	)
+	return fs, flags
+}
+
+// resolveVaultCacheConfig returns the token cache settings to apply. A deprecated
+// experimental flag only takes effect when it was explicitly passed, otherwise its
+// default would overwrite the supported flag on every start.
+func resolveVaultCacheConfig(fs *pflag.FlagSet, flags *vaultCacheFlags) (bool, int) {
+	enable, size := flags.enable, flags.size
+	if fs.Changed("experimental-enable-vault-token-cache") {
+		logger.Info("DEPRECATION WARNING: --experimental-enable-vault-token-cache is deprecated. Please use --enable-vault-token-cache instead. This flag will be removed in a future release.")
+		enable = flags.expEnable
+	}
+	if fs.Changed("experimental-vault-token-cache-size") {
+		logger.Info("DEPRECATION WARNING: --experimental-vault-token-cache-size is deprecated. Please use --vault-token-cache-size instead. This flag will be removed in a future release.")
+		size = flags.expSize
+	}
+	return enable, size
+}
+
+func init() {
+	fs, flags := registerVaultCacheFlags(pflag.ExitOnError)
 	feature.Register(feature.Feature{
 		Flags: fs,
 		Initialize: func() {
-			// Check for deprecated experimental flags and warn users
-			if experimentalEnableCache {
-				logger.Info("DEPRECATION WARNING: --experimental-enable-vault-token-cache is deprecated. Please use --enable-vault-token-cache instead. This flag will be removed in a future release.")
-				enableCache = true
-			}
-			if experimentalCacheSize > 0 {
-				logger.Info("DEPRECATION WARNING: --experimental-vault-token-cache-size is deprecated. Please use --vault-token-cache-size instead. This flag will be removed in a future release.")
-				vaultTokenCacheSize = experimentalCacheSize
-			}
-			initCache(vaultTokenCacheSize)
+			enable, size := resolveVaultCacheConfig(fs, flags)
+			enableCache = enable
+			initCache(size)
 		},
 	})
 }

providers/v1/vault/provider_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	vault "github.com/hashicorp/vault/api"
+	"github.com/spf13/pflag"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -942,3 +943,77 @@ func TestValidateTokenExpiry(t *testing.T) {
 		}
 	})
 }
+
+func TestRegisterVaultCacheFlagsDefaults(t *testing.T) {
+	_, flags := registerVaultCacheFlags(pflag.ContinueOnError)
+
+	// The deprecated size flag shares the non-zero default of the flag that
+	// replaces it. That is why resolveVaultCacheConfig has to look at
+	// fs.Changed instead of the value.
+	if flags.size != defaultCacheSize {
+		t.Errorf("vault-token-cache-size default = %d, want %d", flags.size, defaultCacheSize)
+	}
+	if flags.expSize != defaultCacheSize {
+		t.Errorf("experimental-vault-token-cache-size default = %d, want %d", flags.expSize, defaultCacheSize)
+	}
+	if flags.enable || flags.expEnable {
+		t.Errorf("cache enable flags should default to false, got enable=%v expEnable=%v", flags.enable, flags.expEnable)
+	}
+}
+
+func TestResolveVaultCacheConfig(t *testing.T) {
+	cases := map[string]struct {
+		reason     string
+		args       []string
+		wantEnable bool
+		wantSize   int
+	}{
+		"NoFlagSet": {
+			reason:     "Should keep the supported defaults when no flag is passed",
+			args:       []string{},
+			wantEnable: false,
+			wantSize:   defaultCacheSize,
+		},
+		"SupportedFlagsOnly": {
+			reason:     "Should honor --vault-token-cache-size, which the deprecated default used to overwrite",
+			args:       []string{"--enable-vault-token-cache", "--vault-token-cache-size=4096"},
+			wantEnable: true,
+			wantSize:   4096,
+		},
+		"DeprecatedFlagsOnly": {
+			reason:     "Should apply the deprecated flags when they are explicitly passed",
+			args:       []string{"--experimental-enable-vault-token-cache", "--experimental-vault-token-cache-size=8192"},
+			wantEnable: true,
+			wantSize:   8192,
+		},
+		"BothSizeFlagsSet": {
+			reason:     "Should let the deprecated size win, which is the existing precedence",
+			args:       []string{"--vault-token-cache-size=4096", "--experimental-vault-token-cache-size=8192"},
+			wantEnable: false,
+			wantSize:   8192,
+		},
+		"DeprecatedEnableExplicitlyFalse": {
+			reason:     "Should honor an explicit false instead of forcing the cache on",
+			args:       []string{"--enable-vault-token-cache", "--experimental-enable-vault-token-cache=false"},
+			wantEnable: false,
+			wantSize:   defaultCacheSize,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			fs, flags := registerVaultCacheFlags(pflag.ContinueOnError)
+			if err := fs.Parse(tc.args); err != nil {
+				t.Fatalf("%s: unexpected parse error: %v", tc.reason, err)
+			}
+
+			gotEnable, gotSize := resolveVaultCacheConfig(fs, flags)
+			if gotEnable != tc.wantEnable {
+				t.Errorf("%s: enable = %v, want %v", tc.reason, gotEnable, tc.wantEnable)
+			}
+			if gotSize != tc.wantSize {
+				t.Errorf("%s: size = %d, want %d", tc.reason, gotSize, tc.wantSize)
+			}
+		})
+	}
+}