diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 44b53681dd..c1bb256e87 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -72,7 +72,7 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { // this could be surprising since most users (and tools) expect triggers // to be paused in development. // (Note that there still is an exceptional case where users set the trigger - // status to UNPAUSED at the level of an individual object, whic hwas + // status to UNPAUSED at the level of an individual object, which was // historically allowed.) if p.TriggerPauseStatus == config.Unpaused { diags = diags.Append(diag.Diagnostic{ @@ -133,12 +133,7 @@ func findNonUserPath(b *bundle.Bundle) string { return "" } -func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics { - if b.Config.Bundle.Git.Inferred { - env := b.Config.Bundle.Target - log.Warnf(ctx, "target with 'mode: production' should specify an explicit 'targets.%s.git' configuration", env) - } - +func validateProductionMode(b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics { r := b.Config.Resources for i := range r.Pipelines { if r.Pipelines[i].Development { @@ -175,7 +170,7 @@ func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Di return diags case config.Production: isPrincipal := iamutil.IsServicePrincipal(b.Config.Workspace.CurrentUser.User) - return validateProductionMode(ctx, b, isPrincipal) + return validateProductionMode(b, isPrincipal) case "": // No action default: diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 4135d5fdf2..2e4586e0ac 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -313,7 +313,7 @@ func TestProcessTargetModeDefault(t *testing.T) { func TestProcessTargetModeProduction(t *testing.T) { b := mockBundle(config.Production) - diags := validateProductionMode(context.Background(), b, false) + diags := validateProductionMode(b, false) require.ErrorContains(t, diags.Error(), "run_as") b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" @@ -321,7 +321,7 @@ func TestProcessTargetModeProduction(t *testing.T) { b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files" b.Config.Workspace.ResourcePath = "/Shared/.bundle/x/y/resources" - diags = validateProductionMode(context.Background(), b, false) + diags = validateProductionMode(b, false) require.ErrorContains(t, diags.Error(), "production") permissions := []resources.Permission{ @@ -342,7 +342,7 @@ func TestProcessTargetModeProduction(t *testing.T) { b.Config.Resources.ModelServingEndpoints["servingendpoint1"].Permissions = permissions b.Config.Resources.Clusters["cluster1"].Permissions = permissions - diags = validateProductionMode(context.Background(), b, false) + diags = validateProductionMode(b, false) require.NoError(t, diags.Error()) assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) @@ -358,11 +358,11 @@ func TestProcessTargetModeProductionOkForPrincipal(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) + diags := validateProductionMode(b, false) require.Error(t, diags.Error()) // ... but we're much less strict when a principal is used - diags = validateProductionMode(context.Background(), b, true) + diags = validateProductionMode(b, true) require.NoError(t, diags.Error()) } diff --git a/bundle/permissions/permission_diagnostics.go b/bundle/permissions/permission_diagnostics.go index d2c24fa012..3c76f35052 100644 --- a/bundle/permissions/permission_diagnostics.go +++ b/bundle/permissions/permission_diagnostics.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/iamutil" "github.com/databricks/cli/libs/set" ) @@ -33,9 +34,25 @@ func (m *permissionDiagnostics) Apply(ctx context.Context, b *bundle.Bundle) dia return nil } + me := b.Config.Workspace.CurrentUser.User + identityType := "user_name" + if iamutil.IsServicePrincipal(me) { + identityType = "service_principal_name" + } + return diag.Diagnostics{{ - Severity: diag.Warning, - Summary: fmt.Sprintf("permissions section should include %s or one of their groups with CAN_MANAGE permissions", b.Config.Workspace.CurrentUser.UserName), + Severity: diag.Recommendation, + Summary: fmt.Sprintf("permissions section should explicitly include the current deployment identity '%s' or one of its groups\n"+ + "If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy.\n\n"+ + "Consider using a adding a top-level permissions section such as the following:\n\n"+ + " permissions:\n"+ + " - %s: %s\n"+ + " level: CAN_MANAGE\n\n"+ + "See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration.", + b.Config.Workspace.CurrentUser.UserName, + identityType, + b.Config.Workspace.CurrentUser.UserName, + ), Locations: []dyn.Location{b.Config.GetLocation("permissions")}, ID: diag.PermissionNotIncluded, }} @@ -46,7 +63,7 @@ func (m *permissionDiagnostics) Apply(ctx context.Context, b *bundle.Bundle) dia // target workspace folder. // // Returns: -// - isManager: true if the current user is can manage the bundle resources. +// - canManageBundle: true if the current user or one of their groups can manage the bundle resources. // - assistance: advice on who to contact as to manage this project func analyzeBundlePermissions(b *bundle.Bundle) (bool, string) { canManageBundle := false diff --git a/bundle/permissions/permission_diagnostics_test.go b/bundle/permissions/permission_diagnostics_test.go index 7b0afefa05..38a49c2105 100644 --- a/bundle/permissions/permission_diagnostics_test.go +++ b/bundle/permissions/permission_diagnostics_test.go @@ -22,14 +22,31 @@ func TestPermissionDiagnosticsApplySuccess(t *testing.T) { require.NoError(t, diags.Error()) } +func TestPermissionDiagnosticsEmpty(t *testing.T) { + b := mockBundle(nil) + + diags := permissions.PermissionDiagnostics().Apply(context.Background(), b) + require.NoError(t, diags.Error()) +} + func TestPermissionDiagnosticsApplyFail(t *testing.T) { b := mockBundle([]resources.Permission{ {Level: "CAN_VIEW", UserName: "testuser@databricks.com"}, }) diags := permissions.PermissionDiagnostics().Apply(context.Background(), b) - require.Equal(t, diags[0].Severity, diag.Warning) - require.Contains(t, diags[0].Summary, "permissions section should include testuser@databricks.com or one of their groups with CAN_MANAGE permissions") + require.Equal(t, diags[0].Severity, diag.Recommendation) + + expectedMsg := "permissions section should explicitly include the current deployment identity " + + "'testuser@databricks.com' or one of its groups\n" + + "If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy.\n\n" + + "Consider using a adding a top-level permissions section such as the following:\n\n" + + " permissions:\n" + + " - user_name: testuser@databricks.com\n" + + " level: CAN_MANAGE\n\n" + + "See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration." + + require.Contains(t, diags[0].Summary, expectedMsg) } func mockBundle(permissions []resources.Permission) *bundle.Bundle {