Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change warning about incomplete permissions section into a recommendation #2043

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions bundle/config/mutator/process_target_mode.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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:
Expand Down
10 changes: 5 additions & 5 deletions bundle/config/mutator/process_target_mode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,15 +313,15 @@ 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"
b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts"
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{
Expand All @@ -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)
Expand All @@ -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())
}

Expand Down
23 changes: 20 additions & 3 deletions bundle/permissions/permission_diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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,
}}
Expand All @@ -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
Expand Down
21 changes: 19 additions & 2 deletions bundle/permissions/permission_diagnostics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]"},
})

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 [email protected] 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 " +
"'[email protected]' 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: [email protected]\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 {
Expand Down
Loading