Cache separate vault clients for each namespace if necessary (#4706)

* Cache separate vault clients for each namespace if necessary

Signed-off-by: Christian Ciach <christian.ciach@gmail.com>

* Add unit tests for vault client caching

Signed-off-by: Christian Ciach <christian.ciach@gmail.com>

* Remove dead code

Signed-off-by: Christian Ciach <christian.ciach@gmail.com>

* Explicitly unset client cache when resetting cache in tests

Signed-off-by: Christian Ciach <christian.ciach@gmail.com>

---------

Signed-off-by: Christian Ciach <christian.ciach@gmail.com>

pkg/provider/vault/auth_test.go

@@ -137,7 +137,7 @@ func TestSetAuthNamespace(t *testing.T) {
 				t.Error(err.Error())
 			}
 
-			client, err := getVaultClient(prov, tc.args.store, cfg)
+			client, err := getVaultClient(prov, tc.args.store, cfg, "default")
 			if err != nil {
 				t.Errorf("vault.useAuthNamespace: failed to create client: %s", err.Error())
 			}

pkg/provider/vault/client_push.go

@@ -90,9 +90,6 @@ func (c *client) PushSecret(ctx context.Context, secret *corev1.Secret, data esv
 		return fmt.Errorf("error encoding vault secret: %w", err)
 	}
 	vaultSecretValue := bytes.TrimSpace(buf.Bytes())
-	if err != nil {
-		return fmt.Errorf("error marshaling vault secret: %w", err)
-	}
 	if bytes.Equal(vaultSecretValue, value) {
 		return nil
 	}

pkg/provider/vault/provider.go

@@ -50,6 +50,10 @@ const (
 	errCANamespace   = "missing namespace on caProvider secret"
 )
 
+const (
+	defaultCacheSize = 2 << 17
+)
+
 type Provider struct {
 	// NewVaultClient is a function that returns a new Vault client.
 	// This is used for testing to inject a fake client.
@@ -134,7 +138,7 @@ func (p *Provider) newClient(ctx context.Context, store esv1.GenericStore, kube
 		return nil, err
 	}
 
-	client, err := getVaultClient(p, store, cfg)
+	client, err := getVaultClient(p, store, cfg, namespace)
 	if err != nil {
 		return nil, fmt.Errorf(errVaultClient, err)
 	}
@@ -211,14 +215,21 @@ func (p *Provider) prepareConfig(ctx context.Context, kube kclient.Client, corev
 	return c, cfg, nil
 }
 
-func getVaultClient(p *Provider, store esv1.GenericStore, cfg *vault.Config) (util.Client, error) {
-	auth := store.GetSpec().Provider.Vault.Auth
+func getVaultClient(p *Provider, store esv1.GenericStore, cfg *vault.Config, namespace string) (util.Client, error) {
+	vaultProvider := store.GetSpec().Provider.Vault
+	auth := vaultProvider.Auth
 	isStaticToken := auth != nil && auth.TokenSecretRef != nil
 	useCache := enableCache && !isStaticToken
 
+	keyNamespace := store.GetObjectMeta().Namespace
+	// A single ClusterSecretStore may need to spawn separate vault clients for each namespace.
+	if store.GetTypeMeta().Kind == esv1.ClusterSecretStoreKind && namespace != "" && isReferentSpec(vaultProvider) {
+		keyNamespace = namespace
+	}
+
 	key := cache.Key{
 		Name:      store.GetObjectMeta().Name,
-		Namespace: store.GetObjectMeta().Namespace,
+		Namespace: keyNamespace,
 		Kind:      store.GetTypeMeta().Kind,
 	}
 	if useCache {
@@ -283,24 +294,25 @@ func isReferentSpec(prov *esv1.VaultProvider) bool {
 	return false
 }
 
+func initCache(size int) {
+	logger.Info("initializing vault cache", "size", size)
+	clientCache = cache.Must(size, func(client util.Client) {
+		err := revokeTokenIfValid(context.Background(), client)
+		if err != nil {
+			logger.Error(err, "unable to revoke cached token on eviction")
+		}
+	})
+}
+
 func init() {
 	var vaultTokenCacheSize int
 	fs := pflag.NewFlagSet("vault", pflag.ExitOnError)
 	fs.BoolVar(&enableCache, "experimental-enable-vault-token-cache", false, "Enable experimental 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, "experimental-vault-token-cache-size", 2<<17, "Maximum size of Vault token cache. When more tokens than Only used if --experimental-enable-vault-token-cache is set.")
-	lateInit := func() {
-		logger.Info("initializing vault cache", "size", vaultTokenCacheSize)
-		clientCache = cache.Must(vaultTokenCacheSize, func(client util.Client) {
-			err := revokeTokenIfValid(context.Background(), client)
-			if err != nil {
-				logger.Error(err, "unable to revoke cached token on eviction")
-			}
-		})
-	}
+	fs.IntVar(&vaultTokenCacheSize, "experimental-vault-token-cache-size", defaultCacheSize, "Maximum size of Vault token cache. When more tokens than Only used if --experimental-enable-vault-token-cache is set.")
 	feature.Register(feature.Feature{
 		Flags:      fs,
-		Initialize: lateInit,
+		Initialize: func() { initCache(vaultTokenCacheSize) },
 	})
 
 	esv1.Register(&Provider{

pkg/provider/vault/provider_test.go

@@ -707,3 +707,96 @@ func vaultTest(t *testing.T, _ string, tc testCase) {
 		t.Errorf("\n%s\nvault.New(...): -want error, +got error:\n%s", tc.reason, diff)
 	}
 }
+
+func TestCache(t *testing.T) {
+	t.Cleanup(resetCache)
+	enableCache = true
+	initCache(defaultCacheSize)
+
+	prov := &Provider{
+		NewVaultClient: fake.ClientWithLoginMock,
+	}
+
+	namespace := "default"
+
+	store := makeClusterSecretStore(func(s *esv1.SecretStore) {
+		s.Spec.Provider.Vault.Auth.Kubernetes.ServiceAccountRef = &esmeta.ServiceAccountSelector{
+			Name:      "vault-sa",
+			Namespace: &namespace, // fixed namespace!
+		}
+	})
+
+	// first request creates a new client:
+	c1, err := getVaultClient(prov, store, nil, namespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// seconds request should retrieve cached client instance:
+	c2, err := getVaultClient(prov, store, nil, namespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if c1 != c2 {
+		t.Fatal("Expected a cached client instance")
+	}
+
+	// third request should retrieve cached client instance even when using a different namespace,
+	// because the ClusterSecretStore references a ServiceAccount of a specific namespace:
+	c3, err := getVaultClient(prov, store, nil, "another-namespace")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if c3 != c1 {
+		t.Fatal("Expected a cached client instance")
+	}
+}
+
+func TestCacheWithReferentSpec(t *testing.T) {
+	t.Cleanup(resetCache)
+	enableCache = true
+	initCache(defaultCacheSize)
+
+	prov := &Provider{
+		NewVaultClient: fake.ClientWithLoginMock,
+	}
+
+	store := makeClusterSecretStore(func(s *esv1.SecretStore) {
+		s.Spec.Provider.Vault.Auth.Kubernetes.ServiceAccountRef = &esmeta.ServiceAccountSelector{
+			Name: "vault-sa",
+			// No fixed namespace!
+		}
+	})
+
+	// first request creates a new client:
+	c1, err := getVaultClient(prov, store, nil, "default")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// seconds request should retrieve cached client instance:
+	c2, err := getVaultClient(prov, store, nil, "default")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if c1 != c2 {
+		t.Fatal("Expected a cached client instance")
+	}
+
+	// third request should retrieve a new client instance,
+	// because the ServiceAccount namespace depends on the namespace of the referent:
+	c3, err := getVaultClient(prov, store, nil, "another-namespace")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if c3 == c1 {
+		t.Fatal("Expected a new client instance")
+	}
+}
+
+func resetCache() {
+	enableCache = false
+	clientCache = nil
+}