Implement ability to specify additional/override annotations when using Vault Secret Manager #556

pradithya · 2023-04-17T08:40:43Z

TL;DR

Add annotations configuration to the Vault Secret Manager to provide better flexibility when integrating with Vault.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

When the Vault secret manager is enabled, flyte-pod-webhook is responsible for adding vault-specific annotation to the task pod that requests secret variables. These annotations are used to configure the Vault Agent behavior when retrieving and populating the secret from Vault. As of now, the annotation provided by flyte-pod-webhook is limited to the one implemented in vault_secret_manager.go.

This PR allows Flyte administrator to add additional or override default annotations providing better flexibility to customize Vault agent behavior. For example, the following configuration will set the Vault agent authentication type to use GCP and to immediately exit if the authentication failed.

      vaultSecretManager:
        kvVersion: "2"
        role: flyte
        annotations:
          vault.hashicorp.com/auth-config-type : "gce"
          vault.hashicorp.com/auth-type : "gcp"
          vault.hashicorp.com/agent-auto-auth-exit-on-err : "true"

Tracking Issue

NA

Follow-up issue

NA

pkg/webhook/vault_secret_manager.go

codecov · 2023-04-17T08:48:54Z

Codecov Report

Merging #556 (60927aa) into master (e4ca252) will increase coverage by 0.42%.
The diff coverage is 100.00%.

❗ Current head 60927aa differs from pull request most recent head dcf3113. Consider uploading reports for the commit dcf3113 to get more accurate results

kumare3 · 2023-04-17T13:32:33Z

Cc @EngHabu / @hamersaw

hamersaw · 2023-04-17T16:10:00Z

pkg/webhook/config/config.go

-	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."`
+	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."`
+	ExtraAnnotations map[string]string `json:"extraAnnotations" pflag:"-,Additional annotation to be added to the pod. Useful to further customize Vault integration (https://developer.hashicorp.com/vault/docs/platform/k8s/injector/annotations)"`


Is the term extraAnnotations more descriptive than annotations? I think as long as we are explicit in the docs about which annotations Flyte automatically injects we can simplify this terminology.

Make sense, I think annotations is the better term since it not only allows Flyte administrator to add extra annotations but also overrides the default annotation created by Flyte.

hamersaw · 2023-04-17T16:10:55Z

pkg/webhook/vault_secret_manager_test.go

+		{
+			name: "KVv3 Secret - with extra annotations",
+			args: args{
+				cfg: config.VaultSecretManagerConfig{Role: "flyte", KVVersion: config.KVVersion2, ExtraAnnotations: map[string]string{


Can you add some more examples here to cover the various override scenarios?

Added cases where users provide their annotation via pod template annotations and system override annotation.

hamersaw

This is great! I think the only point of contention is ensuring users understand the default vault-specific annotations that Flyte sets so they don't assume this extraAnnotations argument needs to contain something already being set. Does it make sense to update these docs as part of this change as well?

…secret manager Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

…rg#554) Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

* removed barrier logic Signed-off-by: Daniel Rammer <daniel@union.ai> * deprecated TransitionTypeBarrier Signed-off-by: Daniel Rammer <daniel@union.ai> * removed barrier tests Signed-off-by: Daniel Rammer <daniel@union.ai> * bumping flyteplugins Signed-off-by: Daniel Rammer <daniel@union.ai> --------- Signed-off-by: Daniel Rammer <daniel@union.ai> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

…org#553) * Check for TerminateExecution error and eat Precondition status Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * lint Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> --------- Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

…t_extra_annotations

pradithya · 2023-04-18T02:20:48Z

Does it make sense to update these docs as part of this change as well?

Yes, will add a follow-up PR to update the Vault-related user guide once the PR is merged.

hamersaw

Minor nit, otherwise looks great.

pkg/webhook/vault_secret_manager.go

Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

bimtauer · 2023-05-10T12:33:18Z

Discovered this now when making some updates to my original contribution. I love this feature and its great to know that someone out there is also using flyte with vault :) One thing: I will rename the tests - you introduced a bunch of new ones named KVv3, KVv4 and so on. This was meant to represent the vault kv secret backend version to be tested. To my knowledge there are only 2.

kumare3 · 2023-05-10T13:45:45Z

Tbh @bimtauer i atleast know 3/4 companies that use vault

…ng Vault Secret Manager (flyteorg#556) * Implement ability to specify additional annotations when using Vault secret manager Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * Infer GOOS and GOARCH from environment (flyteorg#552) Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * fix makefile to read variables from environment and overrides (flyteorg#554) Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * Remove BarrierTick (flyteorg#545) * removed barrier logic Signed-off-by: Daniel Rammer <daniel@union.ai> * deprecated TransitionTypeBarrier Signed-off-by: Daniel Rammer <daniel@union.ai> * removed barrier tests Signed-off-by: Daniel Rammer <daniel@union.ai> * bumping flyteplugins Signed-off-by: Daniel Rammer <daniel@union.ai> --------- Signed-off-by: Daniel Rammer <daniel@union.ai> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * Check for TerminateExecution error and eat Precondition status (flyteorg#553) * Check for TerminateExecution error and eat Precondition status Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * lint Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> --------- Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * Rename to annotation Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> * Inline merging annotations Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> --------- Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com> Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Daniel Rammer <daniel@union.ai> Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> Co-authored-by: Jeev B <jeevb@users.noreply.github.com> Co-authored-by: Dan Rammer <daniel@union.ai> Co-authored-by: Haytham Abuelfutuh <haytham@afutuh.com>

pradithya requested review from kumare3, EngHabu and hamersaw as code owners April 17, 2023 08:40

pradithya force-pushed the vault_extra_annotations branch from 0ebca7f to 2a1ecb3 Compare April 17, 2023 08:41

pradithya commented Apr 17, 2023

View reviewed changes

pkg/webhook/vault_secret_manager.go Outdated Show resolved Hide resolved

hamersaw reviewed Apr 17, 2023

View reviewed changes

Pradithya Aria and others added 6 commits April 18, 2023 10:14

Implement ability to specify additional annotations when using Vault …

fcb8c7e

…secret manager Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

Infer GOOS and GOARCH from environment (flyteorg#552)

9e92736

Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

fix makefile to read variables from environment and overrides (flyteo…

dbf1402

…rg#554) Signed-off-by: Jeev B <jeevb@users.noreply.github.com> Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

Rename to annotation

87c1702

Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

pradithya force-pushed the vault_extra_annotations branch from d86aca6 to 87c1702 Compare April 18, 2023 02:14

Merge branch 'master' of github.com:flyteorg/flytepropeller into vaul…

17cb02a

…t_extra_annotations

pradithya changed the title ~~Implement ability to specify additional annotations when using Vault Secret Manager~~ Implement ability to specify additional/override annotations when using Vault Secret Manager Apr 18, 2023

pradithya self-assigned this Apr 18, 2023

hamersaw previously approved these changes Apr 18, 2023

View reviewed changes

pkg/webhook/vault_secret_manager.go Outdated Show resolved Hide resolved

pradithya dismissed hamersaw’s stale review via 56e2a38 April 19, 2023 02:04

Inline merging annotations

dcf3113

Signed-off-by: Pradithya Aria <pradithya.pura@go-jek.com>

pradithya force-pushed the vault_extra_annotations branch from 56e2a38 to dcf3113 Compare April 19, 2023 02:09

hamersaw approved these changes Apr 19, 2023

View reviewed changes

hamersaw merged commit 972c0d6 into flyteorg:master Apr 19, 2023

pradithya mentioned this pull request Apr 20, 2023

Update vault secret manager documentation flyteorg/flytesnacks#986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ability to specify additional/override annotations when using Vault Secret Manager #556

Implement ability to specify additional/override annotations when using Vault Secret Manager #556

pradithya commented Apr 17, 2023 •

edited

Loading

codecov bot commented Apr 17, 2023 •

edited

Loading

kumare3 commented Apr 17, 2023

hamersaw Apr 17, 2023 •

edited

Loading

pradithya Apr 18, 2023

hamersaw Apr 17, 2023

pradithya Apr 18, 2023

hamersaw left a comment

pradithya commented Apr 18, 2023

hamersaw left a comment

bimtauer commented May 10, 2023

kumare3 commented May 10, 2023

Implement ability to specify additional/override annotations when using Vault Secret Manager #556

Implement ability to specify additional/override annotations when using Vault Secret Manager #556

Conversation

pradithya commented Apr 17, 2023 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Apr 17, 2023 • edited Loading

Codecov Report

kumare3 commented Apr 17, 2023

hamersaw Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

pradithya Apr 18, 2023

Choose a reason for hiding this comment

hamersaw Apr 17, 2023

Choose a reason for hiding this comment

pradithya Apr 18, 2023

Choose a reason for hiding this comment

hamersaw left a comment

Choose a reason for hiding this comment

pradithya commented Apr 18, 2023

hamersaw left a comment

Choose a reason for hiding this comment

bimtauer commented May 10, 2023

kumare3 commented May 10, 2023

pradithya commented Apr 17, 2023 •

edited

Loading

codecov bot commented Apr 17, 2023 •

edited

Loading

hamersaw Apr 17, 2023 •

edited

Loading