From 2dad625b847c2dfcd3116d6426297dd47c65323e Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Thu, 22 Aug 2024 22:09:18 +0200 Subject: [PATCH 1/5] Encourage the use of root_path in production to ensure single deployment --- bundle/config/mutator/cleanup_targets.go | 29 +++++++++++++++++++ bundle/config/mutator/process_target_mode.go | 21 ++++++++++++-- .../mutator/process_target_mode_test.go | 22 ++++++++++++-- bundle/config/mutator/select_target.go | 4 --- cmd/bundle/summary.go | 3 +- cmd/bundle/validate.go | 2 ++ 6 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 bundle/config/mutator/cleanup_targets.go diff --git a/bundle/config/mutator/cleanup_targets.go b/bundle/config/mutator/cleanup_targets.go new file mode 100644 index 0000000000..870b5be90c --- /dev/null +++ b/bundle/config/mutator/cleanup_targets.go @@ -0,0 +1,29 @@ +package mutator + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +type cleanupTargets struct { + name string +} + +// CleanupTargets cleans up configuration properties before the configuration +// is reported by the 'bundle summary' command. +func CleanupTargets() bundle.Mutator { + return &cleanupTargets{} +} + +func (m *cleanupTargets) Name() string { + return fmt.Sprintf("Cleanup(%s)", m.name) +} + +func (m *cleanupTargets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + b.Config.Targets = nil + b.Config.Environments = nil + return nil +} diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 92ed286899..8bcff0ca38 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -130,8 +130,17 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs } } - if !isPrincipalUsed && !isRunAsSet(r) { - return diag.Errorf("'run_as' must be set for all jobs when using 'mode: production'") + // We need to verify that there is only a single deployment of the current target. + // The best way to enforce this is to explicitly set root_path. + if !isExplicitRootSet(b) { + if isRunAsSet(r) || isPrincipalUsed { + // Just setting run_as is not enough to guarantee a single deployment, + // and neither is setting a principal. + // We only show a warning for these cases since we didn't historically + // report an error for them. + return diag.Warningf("target with 'mode: production' should specify explicit 'workspace.root_path' to make sure only one copy is deployed") + } + return diag.Errorf("target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") } return nil } @@ -148,6 +157,14 @@ func isRunAsSet(r config.Resources) bool { return true } +func isExplicitRootSet(b *bundle.Bundle) bool { + targetConfig := b.Config.Targets[b.Config.Bundle.Target] + if targetConfig.Workspace == nil { + return false + } + return targetConfig.Workspace.RootPath != "" +} + func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { switch b.Config.Bundle.Mode { case config.Development: diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 1c8671b4c5..804371c01d 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -32,6 +32,9 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Branch: "main", }, }, + Targets: map[string]*config.Target{ + "": {}, + }, Workspace: config.Workspace{ CurrentUser: &config.User{ ShortName: "lennart", @@ -277,14 +280,14 @@ func TestProcessTargetModeProduction(t *testing.T) { b := mockBundle(config.Production) diags := validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "run_as") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts" b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files" diags = validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "production") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") permissions := []resources.Permission{ { @@ -326,6 +329,21 @@ func TestProcessTargetModeProductionOkForPrincipal(t *testing.T) { require.NoError(t, diags.Error()) } +func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) { + b := mockBundle(config.Production) + + // Our target has all kinds of problems when not using service principals ... + diags := validateProductionMode(context.Background(), b, false) + require.Error(t, diags.Error()) + + // ... but we're okay if we specify a root path + b.Config.Targets[""].Workspace = &config.Workspace{ + RootPath: "some-root-path", + } + diags = validateProductionMode(context.Background(), b, false) + require.NoError(t, diags.Error()) +} + // Make sure that we have test coverage for all resource types func TestAllResourcesMocked(t *testing.T) { b := mockBundle(config.Development) diff --git a/bundle/config/mutator/select_target.go b/bundle/config/mutator/select_target.go index 178686b6ed..2333c97dbc 100644 --- a/bundle/config/mutator/select_target.go +++ b/bundle/config/mutator/select_target.go @@ -49,9 +49,5 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti // TODO: remove when Environments section is not supported anymore. b.Config.Bundle.Environment = b.Config.Bundle.Target - // Clear targets after loading. - b.Config.Targets = nil - b.Config.Environments = nil - return nil } diff --git a/cmd/bundle/summary.go b/cmd/bundle/summary.go index 5a64b46c0a..b3493a81ff 100644 --- a/cmd/bundle/summary.go +++ b/cmd/bundle/summary.go @@ -8,6 +8,7 @@ import ( "path/filepath" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/terraform" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" @@ -60,7 +61,7 @@ func newSummaryCommand() *cobra.Command { } } - diags = bundle.Apply(ctx, b, terraform.Load()) + diags = bundle.Apply(ctx, b, bundle.Seq(terraform.Load(), mutator.CleanupTargets())) if err := diags.Error(); err != nil { return err } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 496d5d2b50..88b16922a3 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/render" @@ -65,6 +66,7 @@ func newValidateCommand() *cobra.Command { return nil case flags.OutputJSON: + bundle.Apply(ctx, b, mutator.CleanupTargets()) return renderJsonOutput(cmd, b, diags) default: return fmt.Errorf("unknown output type %s", root.OutputType(cmd)) From 595beed75095e975c3f1f8340483fd75b1071328 Mon Sep 17 00:00:00 2001 From: "Lennart Kats (databricks)" Date: Thu, 29 Aug 2024 16:49:05 +0200 Subject: [PATCH 2/5] Update bundle/config/mutator/process_target_mode.go Co-authored-by: Pieter Noordhuis --- bundle/config/mutator/process_target_mode.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 8bcff0ca38..bd7ca6f266 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -158,7 +158,10 @@ func isRunAsSet(r config.Resources) bool { } func isExplicitRootSet(b *bundle.Bundle) bool { - targetConfig := b.Config.Targets[b.Config.Bundle.Target] + targetConfig, ok := b.Config.Targets[b.Config.Bundle.Target] + if !ok || targetConfig == nil { + return false + } if targetConfig.Workspace == nil { return false } From 821239e45d969dccb14e64891e7abc70a44f937b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 2 Sep 2024 14:00:28 +0200 Subject: [PATCH 3/5] Add nil check --- bundle/config/mutator/process_target_mode.go | 3 +++ bundle/config/mutator/process_target_mode_test.go | 13 +++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 8bcff0ca38..4602fc5d15 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -158,6 +158,9 @@ func isRunAsSet(r config.Resources) bool { } func isExplicitRootSet(b *bundle.Bundle) bool { + if b.Config.Targets == nil { + return false + } targetConfig := b.Config.Targets[b.Config.Bundle.Target] if targetConfig.Workspace == nil { return false diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 804371c01d..a4730b102e 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -32,10 +32,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Branch: "main", }, }, - Targets: map[string]*config.Target{ - "": {}, - }, - Workspace: config.Workspace{ + Workspace: config.Workspace{ CurrentUser: &config.User{ ShortName: "lennart", User: &iam.User{ @@ -337,8 +334,12 @@ func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) { require.Error(t, diags.Error()) // ... but we're okay if we specify a root path - b.Config.Targets[""].Workspace = &config.Workspace{ - RootPath: "some-root-path", + b.Config.Targets = map[string]*config.Target{ + "": { + Workspace: &config.Workspace{ + RootPath: "some-root-path", + }, + }, } diags = validateProductionMode(context.Background(), b, false) require.NoError(t, diags.Error()) From dc63c7cd63bef65f22fc7711f466ddf5908488e9 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 9 Sep 2024 09:20:17 +0200 Subject: [PATCH 4/5] Extend message --- bundle/config/mutator/process_target_mode.go | 9 +++++++-- bundle/config/mutator/process_target_mode_test.go | 6 +++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 4602fc5d15..2c2439a81f 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -2,6 +2,7 @@ package mutator import ( "context" + "fmt" "strings" "github.com/databricks/cli/bundle" @@ -132,15 +133,19 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs // We need to verify that there is only a single deployment of the current target. // The best way to enforce this is to explicitly set root_path. + advice := fmt.Sprintf( + "set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/%s/.bundle/${bundle.name}/${bundle.target}", + b.Config.Workspace.CurrentUser.UserName, + ) if !isExplicitRootSet(b) { if isRunAsSet(r) || isPrincipalUsed { // Just setting run_as is not enough to guarantee a single deployment, // and neither is setting a principal. // We only show a warning for these cases since we didn't historically // report an error for them. - return diag.Warningf("target with 'mode: production' should specify explicit 'workspace.root_path' to make sure only one copy is deployed") + return diag.Warningf("target with 'mode: production' should " + advice) } - return diag.Errorf("target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") + return diag.Errorf("target with 'mode: production' must " + advice) } return nil } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index a4730b102e..3f85214d28 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -32,7 +32,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Branch: "main", }, }, - Workspace: config.Workspace{ + Workspace: config.Workspace{ CurrentUser: &config.User{ ShortName: "lennart", User: &iam.User{ @@ -277,14 +277,14 @@ func TestProcessTargetModeProduction(t *testing.T) { b := mockBundle(config.Production) diags := validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}") b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts" b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files" diags = validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}") permissions := []resources.Permission{ { From 6ea53065cf9a5e95a7d80c897eb59d812a632259 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 12 Oct 2024 11:54:05 +0200 Subject: [PATCH 5/5] Merge CleanupTarget back into SelectTarget --- bundle/config/bundle.go | 3 ++ bundle/config/mutator/cleanup_targets.go | 29 ------------------- bundle/config/mutator/process_target_mode.go | 4 +-- .../mutator/process_target_mode_test.go | 8 ++--- bundle/config/mutator/select_target.go | 9 +++++- bundle/config/root.go | 4 +-- cmd/bundle/summary.go | 3 +- cmd/bundle/validate.go | 2 -- 8 files changed, 19 insertions(+), 43 deletions(-) delete mode 100644 bundle/config/mutator/cleanup_targets.go diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index f533c4d184..4bbbb66fe2 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -18,6 +18,9 @@ type Bundle struct { // Target is set by the mutator that selects the target. Target string `json:"target,omitempty" bundle:"readonly"` + // TargetConfig stores a snapshot of the target configuration when it was selected by SelectTarget. + TargetConfig *Target `json:"target_config,omitempty" bundle:"internal"` + // DEPRECATED. Left for backward compatibility with Target Environment string `json:"environment,omitempty" bundle:"readonly"` diff --git a/bundle/config/mutator/cleanup_targets.go b/bundle/config/mutator/cleanup_targets.go deleted file mode 100644 index 870b5be90c..0000000000 --- a/bundle/config/mutator/cleanup_targets.go +++ /dev/null @@ -1,29 +0,0 @@ -package mutator - -import ( - "context" - "fmt" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/diag" -) - -type cleanupTargets struct { - name string -} - -// CleanupTargets cleans up configuration properties before the configuration -// is reported by the 'bundle summary' command. -func CleanupTargets() bundle.Mutator { - return &cleanupTargets{} -} - -func (m *cleanupTargets) Name() string { - return fmt.Sprintf("Cleanup(%s)", m.name) -} - -func (m *cleanupTargets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - b.Config.Targets = nil - b.Config.Environments = nil - return nil -} diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index fc56da6ecf..e2727d5b25 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -179,10 +179,10 @@ func isRunAsSet(r config.Resources) bool { } func isExplicitRootSet(b *bundle.Bundle) bool { - if b.Config.Targets == nil { + if b.Config.Bundle.TargetConfig == nil { return false } - targetConfig := b.Config.Targets[b.Config.Bundle.Target] + targetConfig := b.Config.Bundle.TargetConfig if targetConfig.Workspace == nil { return false } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 73f52dc28d..cb64fb4675 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -354,11 +354,9 @@ func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) { require.Error(t, diags.Error()) // ... but we're okay if we specify a root path - b.Config.Targets = map[string]*config.Target{ - "": { - Workspace: &config.Workspace{ - RootPath: "some-root-path", - }, + b.Config.Bundle.TargetConfig = &config.Target{ + Workspace: &config.Workspace{ + RootPath: "some-root-path", }, } diags = validateProductionMode(context.Background(), b, false) diff --git a/bundle/config/mutator/select_target.go b/bundle/config/mutator/select_target.go index 2333c97dbc..3e0ff8b1e3 100644 --- a/bundle/config/mutator/select_target.go +++ b/bundle/config/mutator/select_target.go @@ -15,6 +15,7 @@ type selectTarget struct { } // SelectTarget merges the specified target into the root configuration. +// After merging, it removes the 'Targets' section from the configuration. func SelectTarget(name string) bundle.Mutator { return &selectTarget{ name: name, @@ -31,7 +32,7 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti } // Get specified target - _, ok := b.Config.Targets[m.name] + targetConfig, ok := b.Config.Targets[m.name] if !ok { return diag.Errorf("%s: no such target. Available targets: %s", m.name, strings.Join(maps.Keys(b.Config.Targets), ", ")) } @@ -43,11 +44,17 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti } // Store specified target in configuration for reference. + b.Config.Bundle.TargetConfig = targetConfig b.Config.Bundle.Target = m.name // We do this for backward compatibility. // TODO: remove when Environments section is not supported anymore. b.Config.Bundle.Environment = b.Config.Bundle.Target + // Cleanup the original targets and environments sections since they + // show up in the JSON output of the 'summary' and 'validate' commands. + b.Config.Targets = nil + b.Config.Environments = nil + return nil } diff --git a/bundle/config/root.go b/bundle/config/root.go index 4b1467456d..5a809f329c 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -47,8 +47,8 @@ type Root struct { // Targets can be used to differentiate settings and resources between // bundle deployment targets (e.g. development, staging, production). - // If not specified, the code below initializes this field with a - // single default-initialized target called "default". + // Note that this field is set to 'nil' by the SelectTarget mutator; + // use Bundle.TargetConfig to access the selected target configuration. Targets map[string]*Target `json:"targets,omitempty"` // DEPRECATED. Left for backward compatibility with Targets diff --git a/cmd/bundle/summary.go b/cmd/bundle/summary.go index b3493a81ff..5a64b46c0a 100644 --- a/cmd/bundle/summary.go +++ b/cmd/bundle/summary.go @@ -8,7 +8,6 @@ import ( "path/filepath" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/terraform" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" @@ -61,7 +60,7 @@ func newSummaryCommand() *cobra.Command { } } - diags = bundle.Apply(ctx, b, bundle.Seq(terraform.Load(), mutator.CleanupTargets())) + diags = bundle.Apply(ctx, b, terraform.Load()) if err := diags.Error(); err != nil { return err } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 88b16922a3..496d5d2b50 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/render" @@ -66,7 +65,6 @@ func newValidateCommand() *cobra.Command { return nil case flags.OutputJSON: - bundle.Apply(ctx, b, mutator.CleanupTargets()) return renderJsonOutput(cmd, b, diags) default: return fmt.Errorf("unknown output type %s", root.OutputType(cmd))