Use the mutex pool provided by k8s keymutex (#975)

The previous client cache locking scheme was not thread safe, and allocated more locks than are typically needed. This change replaces that approach by using the KeyMutex provided by the k8s utils package. Locks are now a pooled resource. Other fixes - update invalid Bitnami Helm chart repo for postgres. We should phasing out its use. - Bump TF Helm to latest version - demo: update the postgres chart URL - lint bats tests
hashicorp · Dec 10, 2024 · 3b75144 · 3b75144
1 parent 6ab056c
commit 3b75144
Show file tree

Hide file tree

Showing 21 changed files with 1,013 additions and 845 deletions.
diff --git a/chart/templates/_helpers.tpl b/chart/templates/_helpers.tpl
@@ -334,3 +334,14 @@ vaultAuthGlobalRef generates the default VaultAuth spec.vaultAuthGlobalRef.
 {{- $ret | toYaml | nindent 4 -}}
 {{- end -}}
 {{- end -}}
+
+{{/*
+clientCache numLocks
+*/}}
+{{- define "vso.clientCacheNumLocks" -}}
+{{- with .Values.controller.manager.clientCache -}}
+{{- if or .numLocks (eq .numLocks 0) -}}
+--client-cache-num-locks={{ .numLocks }}
+{{- end -}}
+{{- end -}}
+{{- end -}}
diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml
@@ -75,6 +75,9 @@ spec:
         {{- if .Values.controller.manager.clientCache.cacheSize }}
         - --client-cache-size={{ .Values.controller.manager.clientCache.cacheSize }}
         {{- end }}
+        {{- with include "vso.clientCacheNumLocks" . }}
+        - {{ . }}
+        {{- end }}
         {{- if .Values.controller.manager.maxConcurrentReconciles }}
         - --max-concurrent-reconciles={{ .Values.controller.manager.maxConcurrentReconciles }}
         {{- end }}

diff --git a/chart/values.yaml b/chart/values.yaml
@@ -267,6 +267,18 @@ controller:
       # @type: integer
       cacheSize:
 
+      # Defines the number of locks to use for the Vault client cache controller.
+      # May also be set via the `VSO_CLIENT_CACHE_NUM_LOCKS` environment variable.
+      #
+      # Setting this value less than 1 will cause the manager to set the number of locks equal
+      # to the number of logical CPUs of the run host.
+      #
+      # See the VSO help output for more information.
+      #
+      # default: 100
+      # @type: integer
+      numLocks:
+
       # StorageEncryption provides the necessary configuration to encrypt the client storage
       # cache within Kubernetes objects using (required) Vault Transit Engine.
       # This should only be configured when client cache persistence with encryption is enabled and

diff --git a/demo/infra/app/main.tf b/demo/infra/app/main.tf
@@ -5,7 +5,7 @@ terraform {
   required_providers {
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
     kubernetes = {
       source  = "hashicorp/kubernetes"

diff --git a/demo/infra/app/postgres.tf b/demo/infra/app/postgres.tf
@@ -8,8 +8,10 @@ resource "helm_release" "postgres" {
   wait             = true
   wait_for_jobs    = true
 
-  repository = "https://charts.bitnami.com/bitnami"
+  # ref: https://github.com/bitnami/charts/issues/30582#issuecomment-2494545610
+  repository = "oci://registry-1.docker.io/bitnamicharts"
   chart      = "postgresql"
+  version    = "16.2.2"
 
   set {
     name  = "auth.audit.logConnections"

diff --git a/internal/options/env.go b/internal/options/env.go
@@ -46,6 +46,9 @@ type VSOEnvOptions struct {
 
 	// GlobalVaultAuthOptions is VSO_GLOBAL_VAULT_AUTH_OPTIONS environment variable option
 	GlobalVaultAuthOptions []string `split_words:"true"`
+
+	// ClientCacheNumLocks is VSO_CLIENT_CACHE_NUM_LOCKS environment variable option
+	ClientCacheNumLocks *int `split_words:"true"`
 }
 
 // Parse environment variable options, prefixed with "VSO_"

diff --git a/internal/options/env_test.go b/internal/options/env_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"k8s.io/utils/ptr"
 )
 
 func TestParse(t *testing.T) {
@@ -33,19 +34,21 @@ func TestParse(t *testing.T) {
 				"VSO_BACKOFF_MULTIPLIER":             "2.5",
 				"VSO_GLOBAL_TRANSFORMATION_OPTIONS":  "gOpt1,gOpt2",
 				"VSO_GLOBAL_VAULT_AUTH_OPTIONS":      "vOpt1,vOpt2",
+				"VSO_CLIENT_CACHE_NUM_LOCKS":         "10",
 			},
 			wantOptions: VSOEnvOptions{
 				OutputFormat:                "json",
-				ClientCacheSize:             makeInt(t, 100),
+				ClientCacheSize:             ptr.To(100),
 				ClientCachePersistenceModel: "memory",
-				MaxConcurrentReconciles:     makeInt(t, 10),
+				MaxConcurrentReconciles:     ptr.To(10),
 				BackoffInitialInterval:      time.Second * 1,
 				BackoffMaxInterval:          time.Second * 60,
 				BackoffMaxElapsedTime:       time.Hour * 24,
 				BackoffRandomizationFactor:  0.5,
 				BackoffMultiplier:           2.5,
 				GlobalTransformationOptions: []string{"gOpt1", "gOpt2"},
 				GlobalVaultAuthOptions:      []string{"vOpt1", "vOpt2"},
+				ClientCacheNumLocks:         ptr.To(10),
 			},
 		},
 	}
@@ -61,8 +64,3 @@ func TestParse(t *testing.T) {
 		})
 	}
 }
-
-func makeInt(t *testing.T, i int) *int {
-	t.Helper()
-	return &i
-}
diff --git a/main.go b/main.go
@@ -158,6 +158,12 @@ func main() {
 	flag.IntVar(&cfc.ClientCacheSize, "client-cache-size", cfc.ClientCacheSize,
 		"Size of the in-memory LRU client cache. "+
 			"Also set from environment variable VSO_CLIENT_CACHE_SIZE.")
+	// update chart/values.yaml if changing the default value
+	flag.IntVar(&cfc.ClientCacheNumLocks, "client-cache-num-locks", 100,
+		"Number of locks to use for the client cache. "+
+			"Increasing this value may improve performance during Vault client creation, but requires more memory. "+
+			"When the value is <= 0 the number of locks will be set to the number of logical CPUs of the run host. "+
+			"Also set from environment variable VSO_CLIENT_CACHE_NUM_LOCKS.")
 	flag.StringVar(&clientCachePersistenceModel, "client-cache-persistence-model", defaultPersistenceModel,
 		fmt.Sprintf(
 			"The type of client cache persistence model that should be employed. "+
@@ -229,6 +235,9 @@ func main() {
 	if vsoEnvOptions.ClientCacheSize != nil {
 		cfc.ClientCacheSize = *vsoEnvOptions.ClientCacheSize
 	}
+	if vsoEnvOptions.ClientCacheNumLocks != nil {
+		cfc.ClientCacheNumLocks = *vsoEnvOptions.ClientCacheNumLocks
+	}
 	if vsoEnvOptions.ClientCachePersistenceModel != "" {
 		clientCachePersistenceModel = vsoEnvOptions.ClientCachePersistenceModel
 	}

diff --git a/test/integration/hcpvaultsecretsapp/terraform/main.tf b/test/integration/hcpvaultsecretsapp/terraform/main.tf
@@ -9,7 +9,7 @@ terraform {
 
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }

diff --git a/test/integration/infra/main.tf b/test/integration/infra/main.tf
@@ -5,7 +5,7 @@ terraform {
   required_providers {
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
     kubernetes = {
       source  = "hashicorp/kubernetes"

diff --git a/test/integration/infra/scale-testing/deployments/providers.tf b/test/integration/infra/scale-testing/deployments/providers.tf
@@ -6,7 +6,7 @@ terraform {
     }
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }

diff --git a/test/integration/operator/terraform/main.tf b/test/integration/operator/terraform/main.tf
@@ -5,7 +5,7 @@ terraform {
   required_providers {
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
     kubernetes = {
       source  = "hashicorp/kubernetes"

diff --git a/test/integration/revocation/terraform/main.tf b/test/integration/revocation/terraform/main.tf
@@ -15,7 +15,7 @@ terraform {
     }
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }

diff --git a/test/integration/vaultauthmethods/terraform/main.tf b/test/integration/vaultauthmethods/terraform/main.tf
@@ -13,7 +13,7 @@ terraform {
     }
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }

diff --git a/test/integration/vaultdynamicsecret/terraform/main.tf b/test/integration/vaultdynamicsecret/terraform/main.tf
@@ -5,7 +5,7 @@ terraform {
   required_providers {
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
     kubernetes = {
       source  = "hashicorp/kubernetes"

diff --git a/test/integration/vaultdynamicsecret/terraform/postgres.tf b/test/integration/vaultdynamicsecret/terraform/postgres.tf
@@ -8,8 +8,10 @@ resource "helm_release" "postgres" {
   wait             = true
   wait_for_jobs    = true
 
-  repository = "https://charts.bitnami.com/bitnami"
+  # ref: https://github.com/bitnami/charts/issues/30582#issuecomment-2494545610
+  repository = "oci://registry-1.docker.io/bitnamicharts"
   chart      = "postgresql"
+  version    = "16.2.2"
 }
 
 resource "kubernetes_namespace" "postgres" {

diff --git a/test/integration/vaultpkisecret/terraform/main.tf b/test/integration/vaultpkisecret/terraform/main.tf
@@ -13,7 +13,7 @@ terraform {
     }
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }

diff --git a/test/integration/vaultstaticsecret/terraform/main.tf b/test/integration/vaultstaticsecret/terraform/main.tf
@@ -13,7 +13,7 @@ terraform {
     }
     helm = {
       source  = "hashicorp/helm"
-      version = "2.13.1"
+      version = "2.16.1"
     }
   }
 }