From 26c3b421e84542e1fc59d292eb1f936528276010 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Thu, 22 Aug 2024 20:52:26 +0200 Subject: [PATCH] Restore original 404 handling logic Based on reviewer feedback we want to minimize assumptions about API behavior --- bundle/deploy/files/upload.go | 2 +- bundle/deploy/lock/acquire.go | 10 +++++++++- bundle/permissions/permission_diagnostics.go | 14 +++++++++----- bundle/permissions/permission_diagnostics_test.go | 8 ++++---- bundle/tests/run_as_test.go | 1 - libs/filer/workspace_files_client.go | 13 +++---------- 6 files changed, 26 insertions(+), 22 deletions(-) diff --git a/bundle/deploy/files/upload.go b/bundle/deploy/files/upload.go index d8c4a607d..0f531299e 100644 --- a/bundle/deploy/files/upload.go +++ b/bundle/deploy/files/upload.go @@ -29,7 +29,7 @@ func (m *upload) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { b.Files, err = sync.RunOnce(ctx) if err != nil { if errors.Is(err, fs.ErrPermission) { - return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath) + return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) } return diag.FromErr(err) } diff --git a/bundle/deploy/lock/acquire.go b/bundle/deploy/lock/acquire.go index f010d66df..ee47f7744 100644 --- a/bundle/deploy/lock/acquire.go +++ b/bundle/deploy/lock/acquire.go @@ -8,6 +8,7 @@ import ( "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" "github.com/databricks/cli/libs/log" ) @@ -53,7 +54,14 @@ func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) if errors.Is(err, fs.ErrPermission) { - return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath) + return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) + } + + notExistsError := filer.NoSuchDirectoryError{} + if errors.As(err, ¬ExistsError) { + // 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) } return diag.FromErr(err) diff --git a/bundle/permissions/permission_diagnostics.go b/bundle/permissions/permission_diagnostics.go index 37eb0f8d5..5b19dd026 100644 --- a/bundle/permissions/permission_diagnostics.go +++ b/bundle/permissions/permission_diagnostics.go @@ -109,20 +109,24 @@ func isGroupOfCurrentUser(b *bundle.Bundle, groupName string) bool { return false } -func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics { - log.Errorf(ctx, "Failed to update, encountated permission denied error: %v", path) +// ReportPossiblePermissionDenied generates a diagnostic message when a permission denied error is encountered. +// +// Note that since the workspace API doesn't always distinguish between permission denied and path errors, +// we must treat this as a "possible permission error". See acquire.go for more about this. +func ReportPossiblePermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics { + log.Errorf(ctx, "Failed to update, encountered possible permission error: %v", path) user := b.Config.Workspace.CurrentUser.DisplayName canManageBundle, assistance := analyzeBundlePermissions(b) if !canManageBundle { return diag.Diagnostics{{ - Summary: fmt.Sprintf("deployment permission denied for %s.\n"+ + Summary: fmt.Sprintf("unable to deploy to %s as %s.\n"+ "Please make sure the current user or one of their groups is listed under the permissions of this bundle.\n"+ "%s\n"+ "They may need to redeploy the bundle to apply the new permissions.\n"+ "Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.", - user, assistance), + path, user, assistance), Severity: diag.Error, ID: diag.PathPermissionDenied, }} @@ -132,7 +136,7 @@ func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string) // But we're still seeing permission errors. So someone else will need // to redeploy the bundle with the right set of permissions. return diag.Diagnostics{{ - Summary: fmt.Sprintf("access denied while updating deployment permissions for %s.\n"+ + Summary: fmt.Sprintf("access denied while updating deployment permissions as %s.\n"+ "%s\n"+ "They can redeploy the project to apply the latest set of permissions.\n"+ "Please refer to https://docs.databricks.com/dev-tools/bundles/permissions.html for more on managing permissions.", diff --git a/bundle/permissions/permission_diagnostics_test.go b/bundle/permissions/permission_diagnostics_test.go index dca9c7762..8660e9574 100644 --- a/bundle/permissions/permission_diagnostics_test.go +++ b/bundle/permissions/permission_diagnostics_test.go @@ -46,7 +46,7 @@ func TestPermissionDiagnosticsPermissionDeniedWithPermission(t *testing.T) { {Level: "CAN_MANAGE", GroupName: "testgroup"}, }) - diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath") + diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath") require.ErrorContains(t, diags.Error(), string(diag.CannotChangePathPermissions)) } @@ -55,14 +55,14 @@ func TestPermissionDiagnosticsPermissionDeniedWithoutPermission(t *testing.T) { {Level: "CAN_VIEW", UserName: "testuser@databricks.com"}, }) - diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath") + diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath") require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied)) } func TestPermissionDiagnosticsPermissionDeniedNilPermission(t *testing.T) { b := mockBundle(nil) - diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath") + diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath") require.ErrorContains(t, diags.Error(), string(diag.PathPermissionDenied)) } @@ -72,7 +72,7 @@ func TestPermissionDiagnosticsFindOtherOwners(t *testing.T) { {Level: "CAN_MANAGE", UserName: "alice@databricks.com"}, }) - diags := permissions.ReportPermissionDenied(context.Background(), b, "testpath") + diags := permissions.ReportPossiblePermissionDenied(context.Background(), b, "testpath") require.ErrorContains(t, diags.Error(), "include: alice@databricks.com") } diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 24a48e43b..920577146 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -3,7 +3,6 @@ package config_tests import ( "context" "fmt" - "path/filepath" "testing" "github.com/databricks/cli/bundle" diff --git a/libs/filer/workspace_files_client.go b/libs/filer/workspace_files_client.go index 5710f354a..1f76be832 100644 --- a/libs/filer/workspace_files_client.go +++ b/libs/filer/workspace_files_client.go @@ -185,14 +185,7 @@ func (w *workspaceFilesClient) Write(ctx context.Context, name string, reader io } // Retry without CreateParentDirectories mode flag. - err = w.Write(ctx, name, bytes.NewReader(body), sliceWithout(mode, CreateParentDirectories)...) - if errors.Is(err, fs.ErrNotExist) { - // If we still get a 404 error when the dir exists, - // the problem is a permission error. - return PermissionError{absPath} - } - return err - + return w.Write(ctx, name, bytes.NewReader(body), sliceWithout(mode, CreateParentDirectories)...) } // This API returns 409 if the file already exists, when the object type is file @@ -309,8 +302,8 @@ func (w *workspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.D return nil, err } - // This API returns a 404 if the specified path does not exist, - // or if we don't have access to write to the path. + // NOTE: This API returns a 404 if the specified path does not exist, + // but can also do so if we don't have access to write to the path. if aerr.StatusCode == http.StatusNotFound { return nil, NoSuchDirectoryError{path.Dir(absPath)} }