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

Show actionable errors for collaborative deployment scenarios #1386

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2cea265
Diagnostics for collaborative deployment scenarios
lennartkats-db Apr 19, 2024
7cdfc8d
Merge remote-tracking branch 'databricks/main' into cp-better-errors
lennartkats-db Apr 25, 2024
fc07725
Merge remote-tracking branch 'databricks/main' into cp-better-errors
lennartkats-db Jun 2, 2024
4aa38b8
Add missing file
lennartkats-db Jun 2, 2024
c46ecde
Add Lakehouse monitoring
lennartkats-db Jun 2, 2024
0b9feab
Cleanup
lennartkats-db Jun 2, 2024
7fff4bc
Cleanup
lennartkats-db Jul 6, 2024
c222ba3
Refactor
lennartkats-db Jul 6, 2024
b384b36
Merge remote-tracking branch 'databricks/main' into cp-better-errors
lennartkats-db Jul 6, 2024
3ba3c17
Address reviewer comments
lennartkats-db Jul 28, 2024
9981f07
Cleanup
lennartkats-db Jul 31, 2024
5ed6fc4
Merge remote-tracking branch 'databricks/main' into cp-better-errors
lennartkats-db Aug 22, 2024
26c3b42
Restore original 404 handling logic
lennartkats-db Aug 22, 2024
f93d394
Harden code with deeper tests
lennartkats-db Aug 22, 2024
a237dfa
Fix linter error
lennartkats-db Aug 22, 2024
49598cc
Fix copy/paste error
lennartkats-db Aug 22, 2024
8abae34
Update comment
lennartkats-db Sep 9, 2024
b11917e
Fix path
lennartkats-db Sep 9, 2024
4c06a34
Cleanup
lennartkats-db Sep 9, 2024
e533dd0
Extract function to separate module
lennartkats-db Sep 9, 2024
3b33d8c
Add comment
lennartkats-db Sep 9, 2024
bfe36fc
Rework error now that we can no longer detect permission errors
lennartkats-db Sep 12, 2024
bde209c
Process PR feedback
lennartkats-db Sep 22, 2024
a39457e
Show email addresses for regular users
lennartkats-db Sep 23, 2024
c850cfb
Merge remote-tracking branch 'databricks/main' into cp-better-errors
lennartkats-db Sep 23, 2024
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
80 changes: 33 additions & 47 deletions bundle/config/mutator/run_as.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,44 +30,30 @@ func (m *setRunAs) Name() string {
return "SetRunAs"
}

type errUnsupportedResourceTypeForRunAs struct {
resourceType string
resourceLocation dyn.Location
currentUser string
runAsUser string
func reportRunAsNotSupported(resourceType string, location dyn.Location, currentUser string, runAsUser string) diag.Diagnostics {
return diag.Diagnostics{{
Summary: fmt.Sprintf("%s do not support a setting a run_as user that is different from the owner.\n"+
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
"Current identity: %s. Run as identity: %s.\n"+
"See https://docs.databricks.com/dev-tools/bundles/run-as.html to learn more about the run_as property.", resourceType, currentUser, runAsUser),
Location: location,
}}
}

func (e errUnsupportedResourceTypeForRunAs) Error() string {
return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Please refer to the documentation at https://docs.databricks.com/dev-tools/bundles/run-as.html for more details. Location of the unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, e.resourceLocation, e.currentUser, e.runAsUser)
}

type errBothSpAndUserSpecified struct {
spName string
spLoc dyn.Location
userName string
userLoc dyn.Location
}

func (e errBothSpAndUserSpecified) Error() string {
return fmt.Sprintf("run_as section must specify exactly one identity. A service_principal_name %q is specified at %s. A user_name %q is defined at %s", e.spName, e.spLoc, e.userName, e.userLoc)
}

func validateRunAs(b *bundle.Bundle) error {
func validateRunAs(b *bundle.Bundle) diag.Diagnostics {
runAs := b.Config.RunAs

// Error if neither service_principal_name nor user_name are specified
if runAs.ServicePrincipalName == "" && runAs.UserName == "" {
return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.GetLocation("run_as"))
return diag.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.GetLocation("run_as"))
}

// Error if both service_principal_name and user_name are specified
if runAs.UserName != "" && runAs.ServicePrincipalName != "" {
return errBothSpAndUserSpecified{
spName: runAs.ServicePrincipalName,
userName: runAs.UserName,
spLoc: b.Config.GetLocation("run_as.service_principal_name"),
userLoc: b.Config.GetLocation("run_as.user_name"),
}
return diag.Diagnostics{{
Summary: "run_as section cannot specify both user_name and service_principal_name",
Location: b.Config.GetLocation("run_as"),
Severity: diag.Error,
}}
}

identity := runAs.ServicePrincipalName
Expand All @@ -82,32 +68,32 @@ func validateRunAs(b *bundle.Bundle) error {

// DLT pipelines do not support run_as in the API.
if len(b.Config.Resources.Pipelines) > 0 {
return errUnsupportedResourceTypeForRunAs{
resourceType: "pipelines",
resourceLocation: b.Config.GetLocation("resources.pipelines"),
currentUser: b.Config.Workspace.CurrentUser.UserName,
runAsUser: identity,
}
return reportRunAsNotSupported(
"pipelines",
b.Config.GetLocation("resources.pipelines"),
b.Config.Workspace.CurrentUser.UserName,
identity,
)
}

// Model serving endpoints do not support run_as in the API.
if len(b.Config.Resources.ModelServingEndpoints) > 0 {
return errUnsupportedResourceTypeForRunAs{
resourceType: "model_serving_endpoints",
resourceLocation: b.Config.GetLocation("resources.model_serving_endpoints"),
currentUser: b.Config.Workspace.CurrentUser.UserName,
runAsUser: identity,
}
return reportRunAsNotSupported(
"model_serving_endpoints",
b.Config.GetLocation("resources.model_serving_endpoints"),
b.Config.Workspace.CurrentUser.UserName,
identity,
)
}

// Monitors do not support run_as in the API.
if len(b.Config.Resources.QualityMonitors) > 0 {
return errUnsupportedResourceTypeForRunAs{
resourceType: "quality_monitors",
resourceLocation: b.Config.GetLocation("resources.quality_monitors"),
currentUser: b.Config.Workspace.CurrentUser.UserName,
runAsUser: identity,
}
return reportRunAsNotSupported(
"quality_monitors",
b.Config.GetLocation("resources.quality_monitors"),
b.Config.Workspace.CurrentUser.UserName,
identity,
)
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
Expand Down Expand Up @@ -183,7 +169,7 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics {

// Assert the run_as configuration is valid in the context of the bundle
if err := validateRunAs(b); err != nil {
return diag.FromErr(err)
return err
}
pietern marked this conversation as resolved.
Show resolved Hide resolved

setRunAsForJobs(b)
Expand Down
9 changes: 3 additions & 6 deletions bundle/config/mutator/run_as_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,8 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) {
Config: *r,
}
diags := bundle.Apply(context.Background(), b, SetRunAs())
assert.Equal(t, diags.Error().Error(), errUnsupportedResourceTypeForRunAs{
resourceType: rt,
resourceLocation: dyn.Location{},
currentUser: "alice",
runAsUser: "bob",
}.Error(), "expected run_as with a different identity than the current deployment user to not supported for resources of type: %s", rt)
assert.Contains(t, diags.Error().Error(), "do not support a setting a run_as user that is different from the owner.\n"+
"Current identity: alice. Run as identity: bob.\n"+
"See https://docs.databricks.com/dev-tools/bundles/run-as.html to learn more about the run_as property.", rt)
}
}
16 changes: 10 additions & 6 deletions bundle/config/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"context"
"encoding/json"
"fmt"

"github.com/databricks/cli/bundle/config/resources"
Expand All @@ -25,6 +26,15 @@ type UniqueResourceIdTracker struct {
ConfigPath map[string]string
}

type ConfigResource interface {
Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error)
TerraformResourceName() string
Validate() error

json.Marshaler
json.Unmarshaler
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
}

// verifies merging is safe by checking no duplicate identifiers exist
func (r *Resources) VerifySafeMerge(other *Resources) error {
rootTracker, err := r.VerifyUniqueResourceIdentifiers()
Expand Down Expand Up @@ -211,12 +221,6 @@ func (r *Resources) ConfigureConfigFilePath() {
}
}

type ConfigResource interface {
Exists(ctx context.Context, w *databricks.WorkspaceClient, id string) (bool, error)
TerraformResourceName() string
Validate() error
}

func (r *Resources) FindResourceByConfigKey(key string) (ConfigResource, error) {
found := make([]ConfigResource, 0)
for k := range r.Jobs {
Expand Down
7 changes: 7 additions & 0 deletions bundle/deploy/files/upload.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ package files

import (
"context"
"errors"
"fmt"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/cmdio"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/filer"
"github.com/databricks/cli/libs/log"
)

Expand All @@ -25,6 +28,10 @@ func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {

err = sync.RunOnce(ctx)
if err != nil {
permissionError := filer.PermissionError{}
if errors.As(err, &permissionError) {
return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath)
}
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
return diag.FromErr(err)
}

Expand Down
10 changes: 5 additions & 5 deletions bundle/deploy/lock/acquire.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/filer"
"github.com/databricks/cli/libs/locker"
Expand Down Expand Up @@ -51,12 +52,11 @@ func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics
if err != nil {
log.Errorf(ctx, "Failed to acquire deployment lock: %v", err)

notExistsError := filer.NoSuchDirectoryError{}
if errors.As(err, &notExistsError) {
// If we get a "doesn't exist" error from the API this indicates
// we either don't have permissions or the path is invalid.
return diag.Errorf("cannot write to deployment root (this can indicate a previous deploy was done with a different identity): %s", b.Config.Workspace.RootPath)
permissionError := filer.PermissionError{}
if errors.As(err, &permissionError) {
return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath)
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
}
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved

return diag.FromErr(err)
}

Expand Down
5 changes: 5 additions & 0 deletions bundle/deploy/terraform/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/cmdio"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/log"
Expand Down Expand Up @@ -31,6 +32,10 @@ func (w *apply) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {

err = tf.Apply(ctx)
if err != nil {
diagnosis := permissions.TryExtendTerraformPermissionError(ctx, b, err)
if diagnosis != nil {
return diagnosis
}
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved
return diag.Errorf("terraform apply: %v", err)
}

Expand Down
1 change: 1 addition & 0 deletions bundle/permissions/mutator.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
const CAN_MANAGE = "CAN_MANAGE"
const CAN_VIEW = "CAN_VIEW"
const CAN_RUN = "CAN_RUN"
const IS_OWNER = "IS_OWNER"
lennartkats-db marked this conversation as resolved.
Show resolved Hide resolved

var allowedLevels = []string{CAN_MANAGE, CAN_VIEW, CAN_RUN}
var levelsMap = map[string](map[string]string){
Expand Down
Loading