steve/databricks-cli
steve
/
databricks-cli
mirror of https://github.com/databricks/cli.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Change warning about incomplete permissions section into a recommendation (#2043) 

## Changes

Changes the warning about an incomplete / implicit permissions section
into a recommendation, and does a minor bit of cleanup.

## Tests
New unit test.
This commit is contained in:
Lennart Kats (databricks) 2025-02-24 10:39:03 +01:00 committed by GitHub
parent 6e18d94264
commit ce7e64062b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 53 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
bundle/config/mutator/process_target_mode.go
View File

@ -73,7 +73,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{
@ -134,7 +134,7 @@ func findNonUserPath(b *bundle.Bundle) string {
return ""
}
func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics {
func validateProductionMode(b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics {
r := b.Config.Resources
for i := range r.Pipelines {
if r.Pipelines[i].Development {
@ -203,7 +203,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:

18
bundle/config/mutator/process_target_mode_test.go
View File

@ -328,16 +328,16 @@ func TestProcessTargetModeDefault(t *testing.T) {
func TestProcessTargetModeProduction(t *testing.T) {
b := mockBundle(config.Production)
diags := validateProductionMode(context.Background(), b, false)
assert.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n root_path: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
diags := validateProductionMode(b, false)
require.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n root_path: /Workspace/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"
b.Config.Workspace.ResourcePath = "/Shared/.bundle/x/y/resources"
diags = validateProductionMode(context.Background(), b, false)
assert.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n root_path: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
diags = validateProductionMode(b, false)
require.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n root_path: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
permissions := []resources.Permission{
{
@ -357,7 +357,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)
@ -375,11 +375,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())
}
@ -387,7 +387,7 @@ 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)
diags := validateProductionMode(b, false)
require.Error(t, diags.Error())
// ... but we're okay if we specify a root path
@ -396,7 +396,7 @@ func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) {
RootPath: "some-root-path",
},
}
diags = validateProductionMode(context.Background(), b, false)
diags = validateProductionMode(b, false)
require.NoError(t, diags.Error())
}

23
bundle/permissions/permission_diagnostics.go
View File

@ -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

25
bundle/permissions/permission_diagnostics_test.go
View File

@ -18,7 +18,14 @@ func TestPermissionDiagnosticsApplySuccess(t *testing.T) {
{Level: "CAN_MANAGE", UserName: "testuser@databricks.com"},
})
diags := permissions.PermissionDiagnostics().Apply(context.Background(), b)
diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
require.NoError(t, diags.Error())
}
func TestPermissionDiagnosticsEmpty(t *testing.T) {
b := mockBundle(nil)
diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
require.NoError(t, diags.Error())
}
@ -27,9 +34,19 @@ func TestPermissionDiagnosticsApplyFail(t *testing.T) {
{Level: "CAN_VIEW", UserName: "testuser@databricks.com"},
})
diags := permissions.PermissionDiagnostics().Apply(context.Background(), b)
require.Equal(t, diag.Warning, diags[0].Severity)
require.Contains(t, diags[0].Summary, "permissions section should include testuser@databricks.com or one of their groups with CAN_MANAGE permissions")
diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
require.Equal(t, diag.Recommendation, diags[0].Severity)
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 {