Use group attribute for KV version and add DB engine support (flyteor…

…g#539) * Continue supporting config kvversion as default arg Signed-off-by: Tim Bauer <[email protected]> * Remove obsolete test Signed-off-by: Tim Bauer <[email protected]> * Add deprecation information Signed-off-by: Tim Bauer <[email protected]> * Remove comment Signed-off-by: Tim Bauer <[email protected]> * Remove comments Signed-off-by: Tim Bauer <[email protected]> --------- Signed-off-by: Tim Bauer <[email protected]>
eapolinario · May 11, 2023 · 9a4ea00 · 9a4ea00
1 parent f4cadb0
commit 9a4ea00
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 46 deletions.
diff --git a/pkg/webhook/config/config.go b/pkg/webhook/config/config.go
@@ -114,7 +114,7 @@ type GCPSecretManagerConfig struct {
 
 type VaultSecretManagerConfig struct {
 	Role        string            `json:"role" pflag:",Specifies the vault role to use"`
-	KVVersion   KVVersion         `json:"kvVersion" pflag:"-,The KV Engine Version. Defaults to 2. Use 1 for unversioned secrets. Refer to - https://www.vaultproject.io/docs/secrets/kv#kv-secrets-engine."`
+	KVVersion   KVVersion         `json:"kvVersion" pflag:"-,DEPRECATED! Use the GroupVersion field of the Secret request instead. The KV Engine Version. Defaults to 2. Use 1 for unversioned secrets. Refer to - https://www.vaultproject.io/docs/secrets/kv#kv-secrets-engine."`
 	Annotations map[string]string `json:"annotations" pflag:"-,Annotation to be added to user task pod. The annotation can also be used to override default annotations added by Flyte. Useful to customize Vault integration (https://developer.hashicorp.com/vault/docs/platform/k8s/injector/annotations)"`
 }
 

diff --git a/pkg/webhook/utils.go b/pkg/webhook/utils.go
@@ -123,30 +123,44 @@ func AppendVolume(volumes []corev1.Volume, volume corev1.Volume) []corev1.Volume
 	return append(volumes, volume)
 }
 
-func CreateVaultAnnotationsForSecret(secret *core.Secret, kvversion config.KVVersion) (map[string]string, error) {
+func CreateVaultAnnotationsForSecret(secret *core.Secret, kvversion config.KVVersion) map[string]string {
 	// Creates three grouped annotations "agent-inject-secret", "agent-inject-file" and "agent-inject-template"
 	// for a given secret request and KV engine version. The annotations respectively handle: 1. retrieving the
 	// secret from a vault path specified in secret.Group, 2. storing it in a file named after secret.Group/secret.Key
 	// and 3. creating a template that retrieves only secret.Key from the multiple k:v pairs present in a vault secret.
 	id := string(uuid.NewUUID())
 
+	secretVaultAnnotations := map[string]string{
+		fmt.Sprintf("vault.hashicorp.com/agent-inject-secret-%s", id): secret.Group,
+		fmt.Sprintf("vault.hashicorp.com/agent-inject-file-%s", id):   fmt.Sprintf("%s/%s", secret.Group, secret.Key),
+	}
+
 	// Set the consul template language query depending on the KV Secrets Engine version.
 	// Version 1 stores plain k:v pairs under .Data, version 2 supports versioned secrets
 	// and wraps the k:v pairs into an additional subfield.
 	var query string
-	if kvversion == config.KVVersion1 {
+	switch secret.GroupVersion {
+	case "kv1":
 		query = ".Data"
-	} else if kvversion == config.KVVersion2 {
+	case "kv2":
 		query = ".Data.data"
-	} else {
-		err := fmt.Errorf("unsupported KV Version %v, supported versions are 1 and 2", kvversion)
-		return nil, err
+	case "db":
+		// For the database secrets engine backend we do not want to use the templating
+	default:
+		// Deprecated: The config setting KVVersion is deprecated and will be removed in a future release.
+		// You should instead use the GroupVersion field in the secret definition.
+		// Support using the legacy KVVersion config if GroupVersion is not set
+		switch kvversion {
+		case config.KVVersion1:
+			query = ".Data"
+		case config.KVVersion2:
+			query = ".Data.data"
+		}
 	}
-	template := fmt.Sprintf(`{{- with secret "%s" -}}{{ %s.%s }}{{- end -}}`, secret.Group, query, secret.Key)
-	secretVaultAnnotations := map[string]string{
-		fmt.Sprintf("vault.hashicorp.com/agent-inject-secret-%s", id):   secret.Group,
-		fmt.Sprintf("vault.hashicorp.com/agent-inject-file-%s", id):     fmt.Sprintf("%s/%s", secret.Group, secret.Key),
-		fmt.Sprintf("vault.hashicorp.com/agent-inject-template-%s", id): template,
+	if query != "" {
+		template := fmt.Sprintf(`{{- with secret "%s" -}}{{ %s.%s }}{{- end -}}`, secret.Group, query, secret.Key)
+		secretVaultAnnotations[fmt.Sprintf("vault.hashicorp.com/agent-inject-template-%s", id)] = template
+
 	}
-	return secretVaultAnnotations, nil
+	return secretVaultAnnotations
 }
diff --git a/pkg/webhook/vault_secret_manager.go b/pkg/webhook/vault_secret_manager.go
@@ -68,11 +68,7 @@ func (i VaultSecretManagerInjector) Inject(ctx context.Context, secret *coreIdl.
 			"vault.hashicorp.com/agent-pre-populate-only": "true",
 		}
 
-		secretVaultAnnotations, err := CreateVaultAnnotationsForSecret(secret, i.cfg.KVVersion)
-		// Creating annotations can break with an unsupported KVVersion
-		if err != nil {
-			return p, false, err
-		}
+		secretVaultAnnotations := CreateVaultAnnotationsForSecret(secret, i.cfg.KVVersion)
 
 		p.ObjectMeta.Annotations = utils.UnionMaps(secretVaultAnnotations, commonVaultAnnotations, i.cfg.Annotations, p.ObjectMeta.Annotations)
 

diff --git a/pkg/webhook/vault_secret_manager_test.go b/pkg/webhook/vault_secret_manager_test.go
@@ -45,7 +45,6 @@ func RetrieveUUID(annotations map[string]string) string {
 }
 
 func ExpectedKVv1(uuid string) *corev1.Pod {
-	// Injects uuid into expected output for KV v1 secrets
 	expected := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
@@ -64,7 +63,6 @@ func ExpectedKVv1(uuid string) *corev1.Pod {
 }
 
 func ExpectedKVv2(uuid string) *corev1.Pod {
-	// Injects uuid into expected output for KV v2 secrets
 	expected := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
@@ -82,8 +80,7 @@ func ExpectedKVv2(uuid string) *corev1.Pod {
 	return expected
 }
 
-func ExpectedKVv3(uuid string) *corev1.Pod {
-	// Injects uuid into expected output for KV v2 secrets
+func ExpectedExtraAnnotation(uuid string) *corev1.Pod {
 	expected := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
@@ -102,8 +99,7 @@ func ExpectedKVv3(uuid string) *corev1.Pod {
 	return expected
 }
 
-func ExpectedKVv4(uuid string) *corev1.Pod {
-	// Injects uuid into expected output for KV v2 secrets
+func ExpectedExistingRoleAnnotation(uuid string) *corev1.Pod {
 	expected := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
@@ -121,8 +117,7 @@ func ExpectedKVv4(uuid string) *corev1.Pod {
 	return expected
 }
 
-func ExpectedKVv5(uuid string) *corev1.Pod {
-	// Injects uuid into expected output for KV v2 secrets
+func ExpectedConfigAnnotation(uuid string) *corev1.Pod {
 	expected := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
@@ -140,6 +135,23 @@ func ExpectedKVv5(uuid string) *corev1.Pod {
 	return expected
 }
 
+func ExpectedDB(uuid string) *corev1.Pod {
+	expected := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				"vault.hashicorp.com/agent-inject":                              "true",
+				"vault.hashicorp.com/secret-volume-path":                        "/etc/flyte/secrets",
+				"vault.hashicorp.com/role":                                      "flyte",
+				"vault.hashicorp.com/agent-pre-populate-only":                   "true",
+				fmt.Sprintf("vault.hashicorp.com/agent-inject-secret-%s", uuid): "foo",
+				fmt.Sprintf("vault.hashicorp.com/agent-inject-file-%s", uuid):   "foo/bar",
+			},
+		},
+		Spec: PodSpec,
+	}
+	return expected
+}
+
 func NewInputPod(annotations map[string]string) *corev1.Pod {
 	// Need to create a new Pod for every test since annotations are otherwise appended to original reference object
 	p := &corev1.Pod{
@@ -176,70 +188,108 @@ func TestVaultSecretManagerInjector_Inject(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "KVv1 Secret",
+			name: "KVv1 Secret Group Version argument overwrites config",
 			args: args{
-				cfg:    config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion1},
-				secret: inputSecret,
-				p:      NewInputPod(map[string]string{}),
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2},
+				secret: &coreIdl.Secret{
+					Group:        "foo",
+					Key:          "bar",
+					GroupVersion: "kv1",
+				},
+				p: NewInputPod(map[string]string{}),
 			},
 			want:    ExpectedKVv1,
 			wantErr: false,
 		},
 		{
-			name: "KVv2 Secret",
+			name: "KVv2 Secret Group Version argument overwrites config",
 			args: args{
-				cfg:    config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2},
-				secret: inputSecret,
-				p:      NewInputPod(map[string]string{}),
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion1},
+				secret: &coreIdl.Secret{
+					Group:        "foo",
+					Key:          "bar",
+					GroupVersion: "kv2",
+				},
+				p: NewInputPod(map[string]string{}),
 			},
 			want:    ExpectedKVv2,
 			wantErr: false,
 		},
 		{
-			name: "KVv3 Secret - extra annotations",
+			name: "Extra annotations from config are added",
 			args: args{
 				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2, Annotations: map[string]string{
 					"vault.hashicorp.com/auth-config-type": "gce",
 				}},
 				secret: inputSecret,
 				p:      NewInputPod(map[string]string{}),
 			},
-			want:    ExpectedKVv3,
+			want:    ExpectedExtraAnnotation,
 			wantErr: false,
 		},
 		{
-			name: "KVv4 Secret - user override annotation",
+			name: "Already present annotation is not overwritten",
 			args: args{
 				cfg:    config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2, Annotations: map[string]string{}},
 				secret: inputSecret,
 				p: NewInputPod(map[string]string{
 					"vault.hashicorp.com/role": "my-role",
 				}),
 			},
-			want:    ExpectedKVv4,
+			want:    ExpectedExistingRoleAnnotation,
 			wantErr: false,
 		},
 		{
-			name: "KVv5 Secret - system override annotation",
+			name: "Config annotation overwrites system default annotation",
 			args: args{
 				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2, Annotations: map[string]string{
 					"vault.hashicorp.com/agent-pre-populate-only": "false", // override vault.hashicorp.com/agent-pre-populate-only
 				}},
 				secret: inputSecret,
 				p:      NewInputPod(map[string]string{}),
 			},
-			want:    ExpectedKVv5,
+			want:    ExpectedConfigAnnotation,
 			wantErr: false,
 		},
 		{
-			name: "Unsupported KV version",
+			name: "DB Secret backend enginge is supported",
 			args: args{
-				cfg:    config.VaultSecretManagerConfig{Role: "flyte", KVVersion: 3},
-				secret: inputSecret,
-				p:      NewInputPod(map[string]string{}),
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion1},
+				secret: &coreIdl.Secret{
+					Group:        "foo",
+					Key:          "bar",
+					GroupVersion: "db",
+				},
+				p: NewInputPod(map[string]string{}),
 			},
-			want:    nil,
-			wantErr: true,
+			want:    ExpectedDB,
+			wantErr: false,
+		},
+		{
+			name: "Legacy config option V1 is still supported",
+			args: args{
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion1},
+				secret: &coreIdl.Secret{
+					Group: "foo",
+					Key:   "bar",
+				},
+				p: NewInputPod(map[string]string{}),
+			},
+			want:    ExpectedKVv1,
+			wantErr: false,
+		},
+		{
+			name: "Legacy config option V2 is still supported",
+			args: args{
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2},
+				secret: &coreIdl.Secret{
+					Group: "foo",
+					Key:   "bar",
+				},
+				p: NewInputPod(map[string]string{}),
+			},
+			want:    ExpectedKVv2,
+			wantErr: false,
 		},
 	}