Show actionable errors for collaborative deployment scenarios #1386

lennartkats-db · 2024-04-22T07:42:10Z

Changes

This adds diagnostics for collaborative (production) deployment scenarios, including:

Bob deploys a bundle that is normally deployed by Alice, but this fails because Bob can't write to /Users/Alice/.bundle.
Charlie deploys a bundle that is normally deployed by Alice, but this fails because he can't create a new pipeline where Alice would be the owner.
Alice deploys a bundle where she didn't list herself as one of the CAN_MANAGE users in permissions. That can work, but is probably a mistake.

Tests

Unit tests, manual testing.

codecov-commenter · 2024-06-02T12:55:44Z

Codecov Report

Attention: Patch coverage is 75.56818% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 53.75%. Comparing base (e22dd8a) to head (0b9feab).
Report is 125 commits behind head on main.

Files	Patch %	Lines
bundle/permissions/permission_diagnostics.go	87.60%	11 Missing and 4 partials ⚠️
libs/filer/workspace_files_client.go	0.00%	14 Missing ⚠️
bundle/config/mutator/run_as.go	81.81%	6 Missing ⚠️
bundle/deploy/terraform/apply.go	0.00%	4 Missing ⚠️
libs/filer/filer.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1386      +/-   ##
==========================================
+ Coverage   52.25%   53.75%   +1.50%     
==========================================
  Files         317      352      +35     
  Lines       18004    20410    +2406     
==========================================
+ Hits         9408    10972    +1564     
- Misses       7903     8636     +733     
- Partials      693      802     +109

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bundle/deploy/lock/acquire.go

bundle/config/mutator/run_as.go

bundle/config/resources.go

bundle/deploy/files/upload.go

libs/diag/diagnostic.go

libs/diag/id.go

libs/filer/filer.go

libs/filer/workspace_files_client.go

lennartkats-db · 2024-07-11T13:37:04Z

@pietern could you take another look?

bundle/config/resources.go

bundle/permissions/permission_diagnostics.go

pietern · 2024-07-15T07:21:10Z

bundle/permissions/permission_diagnostics.go

+
+	assistance := "For assistance, contact the owners of this project."
+	if len(managersSlice) > 0 {
+		assistance = fmt.Sprintf("For assistance, users or groups with appropriate permissions may include: %s.", strings.Join(managersSlice, ", "))


pietern · 2024-07-15T07:22:56Z

bundle/permissions/permission_diagnostics.go

+}
+
+func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics {
+	log.Errorf(ctx, "Failed to update %v", path)


Ping. If this is not the case, please include a comment stating why this log statement shouldn't be removed.

bundle/tests/run_as_test.go

libs/filer/workspace_files_client.go

pietern · 2024-07-15T07:41:46Z

@shreyas-goenka Can you take a pass as this as well?

shreyas-goenka · 2024-07-22T09:54:15Z

Missed the ping, taking a look.

shreyas-goenka

Does this work end to end? Can Bob collaborate on a DAB deployed by Alice if he has CAN_MANAGE?

I recall there being some validation on the Terraform provider side that would fail even if Bob has CAN_MANAGE permissions set.

shreyas-goenka · 2024-07-22T12:27:23Z

libs/filer/workspace_files_client.go

@@ -166,11 +166,21 @@ func (w *WorkspaceFilesClient) Write(ctx context.Context, name string, reader io
 		// Create parent directory.
 		err = w.workspaceClient.Workspace.MkdirsByPath(ctx, path.Dir(absPath))
 		if err != nil {
+			if errors.As(err, &aerr) && aerr.StatusCode == http.StatusForbidden {


We can move it out of the err != nil closure. errors.As automatically asserts that error is not nil.

Pleas note the errors.As() is a special case: the larger if err != nil also reports an error for non-Forbidden errors.

I meant we could reduce a level of indentation, like:

if errors.As(err, &aerr) && aerr.StatusCode == http.StatusForbidden { return PermissionError{absPath} } if err != nil { return fmt.Errorf("unable to mkdir to write file %s: %w", absPath, err) }

libs/filer/workspace_files_client.go

bundle/permissions/terraform_errors.go

bundle/permissions/permission_diagnostics.go

bundle/permissions/terraform_errors.go

bundle/permissions/mutator.go

lennartkats-db · 2024-07-31T08:07:25Z

@shreyas-goenka

Does this work end to end? Can Bob collaborate on a DAB deployed by Alice if he has CAN_MANAGE?

I recall there being some validation on the Terraform provider side that would fail even if Bob has CAN_MANAGE permissions set.

Yes, though that scenario requires the use of an explicit "OWNER: Alice" permission. Which is supported via #1387.

Based on reviewer feedback we want to minimize assumptions about API behavior

## Changes This updates the templates to include a `permissions` section. Having a permissions section is a best practice, is helpful to understand the notion of permissions, and helps diagnose permission errors (#1386). This is a cherry-pick from #1387. This change was verified to work both in dev and prod. Existing unit tests validate the validity of the templates in these modes.

pietern

What's the plan for the diagnostic IDs after this PR?

Right now they're included in the error string but not in the rendered list of diags.

libs/filer/workspace_files_client.go

bundle/deploy/files/upload.go

bundle/deploy/lock/acquire.go

bundle/permissions/permission_diagnostics.go

pietern · 2024-09-03T09:09:28Z

bundle/permissions/permission_diagnostics.go

+//
+// 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 {


This is a helper unrelated to the mutator. Please move it to a dedicated file.

pietern · 2024-09-03T09:12:26Z

bundle/permissions/permission_diagnostics.go

+// 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)


Unclear if there was an update; we don't have access to the error or the intent here.

Doesn't "possible permission" error cover what we know? The intent is just logging so maintainers can determine what's going on.

pietern · 2024-09-03T09:16:54Z

bundle/permissions/permission_diagnostics.go

+	// 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 as %s.\n"+


The error above is more apt here: unable to deploy to %s as %s..

We cannot assert if the intent is to update deployment permissions here.

bundle/phases/initialize.go

pietern · 2024-09-03T09:28:33Z

We discussed this a while back; I'd like to get integration test coverage for the permission errors on the filer. It's implemented only for the workspace files client now, not for the others (for ex the files API).

lennartkats-db · 2024-09-09T07:25:24Z

@pietern

We discussed this a while back; I'd like to get integration test coverage for the permission errors on the filer. It's implemented only for the workspace files client now, not for the others (for ex the files API).

Didn't we conclude that we would just not change the logic of the filer? I reverted that change.

lennartkats-db · 2024-09-09T07:42:17Z

@pietern Updated the PR, processing all the new comments. I'd really like to get this merged.

This was working a bit better before 26c3b42

bundle/permissions/permission_diagnostics.go

bundle/permissions/permission_diagnostics_test.go

pietern · 2024-09-13T10:02:10Z

bundle/permissions/permission_report.go

+	user := b.Config.Workspace.CurrentUser.DisplayName
+	if user == "" {
+		user = b.Config.Workspace.CurrentUser.UserName
+	}


Because of the other PR today, I now look at this and see it will show the display name for regular users.

It will affect the error message shown. I think for regular users we better show the email address.

I changed it to just show the email address in errors.

pietern · 2024-09-23T09:56:12Z

bundle/permissions/permission_diagnostics.go

+	if otherManagers.Size() > 0 {
+		assistance = fmt.Sprintf(
+			"For assistance, users or groups with appropriate permissions may include: %s.",
+			strings.Join(otherManagers.Values(), ", "),


This is not sorted and is the cause of the test assertion failure in the latest test run.

Diagnostics for collaborative deployment scenarios

2cea265

lennartkats-db mentioned this pull request Apr 22, 2024

Support IS_OWNER as a top-level permission #1387

Closed

lennartkats-db added 4 commits April 25, 2024 09:33

Merge remote-tracking branch 'databricks/main' into cp-better-errors

7cdfc8d

Merge remote-tracking branch 'databricks/main' into cp-better-errors

fc07725

Add missing file

4aa38b8

Add Lakehouse monitoring

c46ecde

lennartkats-db force-pushed the cp-better-errors branch from dc33b28 to c46ecde Compare June 2, 2024 11:22

Cleanup

0b9feab

lennartkats-db changed the title ~~[WIP] Show better errors for collaborative deployment scenarios~~ Show better errors for collaborative deployment scenarios Jun 3, 2024

lennartkats-db changed the title ~~Show better errors for collaborative deployment scenarios~~ Show actionable errors for collaborative deployment scenarios Jun 3, 2024

lennartkats-db requested review from andrewnester and pietern June 3, 2024 07:03

lennartkats-db commented Jun 21, 2024

View reviewed changes

bundle/deploy/lock/acquire.go Outdated Show resolved Hide resolved

pietern reviewed Jun 24, 2024

View reviewed changes

lennartkats-db force-pushed the cp-better-errors branch 2 times, most recently from 8d26485 to 16b80ea Compare July 6, 2024 19:37

Cleanup

7fff4bc

lennartkats-db force-pushed the cp-better-errors branch from 16b80ea to 7fff4bc Compare July 6, 2024 19:40

Refactor

c222ba3

lennartkats-db requested a review from pietern July 6, 2024 19:48

lennartkats-db force-pushed the cp-better-errors branch from 737f02f to 3c20dec Compare July 6, 2024 20:02

Merge remote-tracking branch 'databricks/main' into cp-better-errors

b384b36

lennartkats-db force-pushed the cp-better-errors branch from 3c20dec to b384b36 Compare July 8, 2024 06:51

pietern reviewed Jul 15, 2024

View reviewed changes

pietern requested a review from shreyas-goenka July 15, 2024 07:41

shreyas-goenka reviewed Jul 22, 2024

View reviewed changes

lennartkats-db added 3 commits July 31, 2024 10:14

Cleanup

9981f07

Merge remote-tracking branch 'databricks/main' into cp-better-errors

5ed6fc4

Restore original 404 handling logic

26c3b42

Based on reviewer feedback we want to minimize assumptions about API behavior

lennartkats-db force-pushed the cp-better-errors branch from 48057a5 to 26c3b42 Compare August 22, 2024 18:53

lennartkats-db added 2 commits August 22, 2024 21:34

Harden code with deeper tests

f93d394

Fix linter error

a237dfa

lennartkats-db requested review from shreyas-goenka and pietern August 22, 2024 19:41

lennartkats-db mentioned this pull request Aug 22, 2024

Include a permissions section in all templates #1713

Merged

Fix copy/paste error

49598cc

pietern reviewed Sep 3, 2024

View reviewed changes

lennartkats-db added 5 commits September 9, 2024 09:27

Update comment

8abae34

Fix path

b11917e

Cleanup

4c06a34

Extract function to separate module

e533dd0

Add comment

3b33d8c

lennartkats-db requested a review from pietern September 9, 2024 07:42

Rework error now that we can no longer detect permission errors

bfe36fc

This was working a bit better before 26c3b42

pietern reviewed Sep 13, 2024

View reviewed changes

Process PR feedback

bde209c

lennartkats-db force-pushed the cp-better-errors branch from fbf3c9d to bde209c Compare September 22, 2024 14:15

Show email addresses for regular users

a39457e

lennartkats-db requested a review from pietern September 23, 2024 06:41

Merge remote-tracking branch 'databricks/main' into cp-better-errors

c850cfb

pietern reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show actionable errors for collaborative deployment scenarios #1386

Show actionable errors for collaborative deployment scenarios #1386

lennartkats-db commented Apr 22, 2024

codecov-commenter commented Jun 2, 2024 •

edited

Loading

lennartkats-db commented Jul 11, 2024

pietern Jul 15, 2024

pietern Jul 15, 2024

pietern commented Jul 15, 2024

shreyas-goenka commented Jul 22, 2024

shreyas-goenka left a comment

shreyas-goenka Jul 22, 2024

lennartkats-db Jul 31, 2024

shreyas-goenka Jul 31, 2024

lennartkats-db commented Jul 31, 2024

pietern left a comment

pietern Sep 3, 2024

lennartkats-db Sep 9, 2024

pietern Sep 3, 2024

lennartkats-db Sep 9, 2024

pietern Sep 3, 2024

lennartkats-db Sep 21, 2024

pietern commented Sep 3, 2024

lennartkats-db commented Sep 9, 2024

lennartkats-db commented Sep 9, 2024

pietern Sep 13, 2024

lennartkats-db Sep 23, 2024

pietern Sep 23, 2024

Show actionable errors for collaborative deployment scenarios #1386

Are you sure you want to change the base?

Show actionable errors for collaborative deployment scenarios #1386

Conversation

lennartkats-db commented Apr 22, 2024

Changes

Tests

codecov-commenter commented Jun 2, 2024 • edited Loading

Codecov Report

lennartkats-db commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern commented Jul 15, 2024

shreyas-goenka commented Jul 22, 2024

shreyas-goenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennartkats-db commented Jul 31, 2024

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern commented Sep 3, 2024

lennartkats-db commented Sep 9, 2024

lennartkats-db commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 2, 2024 •

edited

Loading