From b21f623788263b6720355251eb822b949f7b9b7a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 15:15:51 +0530 Subject: [PATCH 01/16] Add validation mutator for volume `arifact_path` --- bundle/config/validate/validate.go | 1 + .../config/validate/validate_artifact_path.go | 101 ++++++++++++++++ .../validate/validate_artifact_path_test.go | 109 ++++++++++++++++++ bundle/libraries/filer_volume.go | 26 +++-- bundle/libraries/filer_volume_test.go | 16 +-- 5 files changed, 235 insertions(+), 18 deletions(-) create mode 100644 bundle/config/validate/validate_artifact_path.go create mode 100644 bundle/config/validate/validate_artifact_path_test.go diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index 131566fc90..45661bd802 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -36,6 +36,7 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics JobTaskClusterSpec(), ValidateFolderPermissions(), SingleNodeCluster(), + ValidateArtifactPath(), )) } diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go new file mode 100644 index 0000000000..c8b65983fc --- /dev/null +++ b/bundle/config/validate/validate_artifact_path.go @@ -0,0 +1,101 @@ +package validate + +import ( + "context" + "errors" + "fmt" + "slices" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/libraries" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/service/catalog" +) + +type validateArtifactPath struct{} + +func ValidateArtifactPath() bundle.ReadOnlyMutator { + return &validateArtifactPath{} +} + +func (v *validateArtifactPath) Name() string { + return "validate:artifact_paths" +} + +func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + // We only validate UC Volumes paths right now. + // TODO? + if !libraries.IsVolumesPath(rb.Config().Workspace.ArtifactPath) { + return nil + } + + catalogName, schemaName, volumeName, err := libraries.ExtractVolumeFromPath(rb.Config().Workspace.ArtifactPath) + if err != nil { + return diag.FromErr(err) + } + volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + w := rb.WorkspaceClient() + p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) + + wrapErrorMsg := func(s string) diag.Diagnostics { + return diag.Diagnostics{ + { + Summary: s, + Severity: diag.Error, + Locations: rb.Config().GetLocations("workspace.artifact_path"), + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }, + } + } + if errors.Is(err, apierr.ErrPermissionDenied) { + return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) + } + if errors.Is(err, apierr.ErrNotFound) { + path, locations, ok := libraries.FindVolumeInBundle(rb.Config(), catalogName, schemaName, volumeName) + if !ok { + return wrapErrorMsg(fmt.Sprintf("volume %s does not exist", volumeFullName)) + } + + // If the volume is defined in the bundle, provide a more helpful error diagnostic, + // with more details and location information. + return diag.Diagnostics{{ + Summary: fmt.Sprintf("volume %s does not exist", volumeFullName), + Severity: diag.Error, + Detail: `You are using a volume in your artifact_path that is managed by +this bundle but which has not been deployed yet. Please first deploy +the volume using 'bundle deploy' and then switch over to using it in +the artifact_path.`, + Locations: slices.Concat(rb.Config().GetLocations("workspace.artifact_path"), locations), + Paths: append([]dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, path), + }} + + } + if err != nil { + return wrapErrorMsg(fmt.Sprintf("could not fetch grants for volume %s: %s", volumeFullName, err)) + } + + allPrivileges := []catalog.Privilege{} + for _, assignments := range p.PrivilegeAssignments { + for _, privilege := range assignments.Privileges { + allPrivileges = append(allPrivileges, privilege.Privilege) + } + } + + // UC Volumes have the following privileges: [READ_VOLUME, WRITE_VOLUME, MANAGE, ALL_PRIVILEGES, APPLY TAG] + // The user needs to have either WRITE_VOLUME or ALL_PRIVILEGES to write to the volume. + canWrite := slices.Contains(allPrivileges, catalog.PrivilegeWriteVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) + if !canWrite { + return wrapErrorMsg(fmt.Sprintf("user does not have WRITE_VOLUME grant on volume %s", volumeFullName)) + } + + // READ_VOLUME is implied since the user was able to fetch the associated grants with the volume. + // We still add this explicit check out of caution incase the API behavior changes in the future. + canRead := slices.Contains(allPrivileges, catalog.PrivilegeReadVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) + if !canRead { + return wrapErrorMsg(fmt.Sprintf("user does not have READ_VOLUME grant on volume %s", volumeFullName)) + } + + return nil +} diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go new file mode 100644 index 0000000000..c91da3aae7 --- /dev/null +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -0,0 +1,109 @@ +package validate_test + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestVali + +func TestValidateArtifactPath(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/catalogN/schemaN/volumeN/abc", + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "file", Line: 1, Column: 1}}) + assertDiags := func(t *testing.T, diags diag.Diagnostics, expected string) { + assert.Len(t, diags, 1) + assert.Equal(t, diag.Diagnostics{{ + Severity: diag.Error, + Summary: expected, + Locations: []dyn.Location{{File: "file", Line: 1, Column: 1}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}, diags) + } + + wrapPrivileges := func(privileges ...catalog.Privilege) *catalog.EffectivePermissionsList { + perms := &catalog.EffectivePermissionsList{} + for _, p := range privileges { + perms.PrivilegeAssignments = append(perms.PrivilegeAssignments, catalog.EffectivePrivilegeAssignment{ + Privileges: []catalog.EffectivePrivilege{{Privilege: p}}, + }) + } + return perms + } + + rb := bundle.ReadOnly(b) + ctx := context.Background() + + tcases := []struct { + err error + permissions *catalog.EffectivePermissionsList + expectedSummary string + }{ + { + err: &apierr.APIError{ + StatusCode: 403, + Message: "User does not have USE SCHEMA on Schema 'catalogN.schemaN'", + }, + expectedSummary: "cannot access volume catalogN.schemaN.volumeN: User does not have USE SCHEMA on Schema 'catalogN.schemaN'", + }, + { + err: &apierr.APIError{ + StatusCode: 404, + }, + expectedSummary: "volume catalogN.schemaN.volumeN does not exist", + }, + { + err: &apierr.APIError{ + StatusCode: 500, + Message: "Internal Server Error", + }, + expectedSummary: "could not fetch grants for volume catalogN.schemaN.volumeN: Internal Server Error", + }, + { + permissions: wrapPrivileges(catalog.PrivilegeAllPrivileges), + }, + { + permissions: wrapPrivileges(catalog.PrivilegeApplyTag, catalog.PrivilegeManage), + expectedSummary: "user does not have WRITE_VOLUME grant on volume catalogN.schemaN.volumeN", + }, + { + permissions: wrapPrivileges(catalog.PrivilegeWriteVolume), + expectedSummary: "user does not have READ_VOLUME grant on volume catalogN.schemaN.volumeN", + }, + { + permissions: wrapPrivileges(catalog.PrivilegeWriteVolume, catalog.PrivilegeReadVolume), + }, + } + + for _, tc := range tcases { + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockGrantsAPI() + api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(tc.permissions, tc.err) + b.SetWorkpaceClient(m.WorkspaceClient) + + diags := bundle.ApplyReadOnly(ctx, rb, validate.ValidateArtifactPath()) + if tc.expectedSummary != "" { + assertDiags(t, diags, tc.expectedSummary) + } else { + assert.Len(t, diags, 0) + } + } +} diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index aecf68db13..94ff74a4c1 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" @@ -15,7 +16,7 @@ import ( "github.com/databricks/databricks-sdk-go/apierr" ) -func extractVolumeFromPath(artifactPath string) (string, string, string, error) { +func ExtractVolumeFromPath(artifactPath string) (string, string, string, error) { if !IsVolumesPath(artifactPath) { return "", "", "", fmt.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) } @@ -55,7 +56,7 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, artifactPath := b.Config.Workspace.ArtifactPath w := b.WorkspaceClient() - catalogName, schemaName, volumeName, err := extractVolumeFromPath(artifactPath) + catalogName, schemaName, volumeName, err := ExtractVolumeFromPath(artifactPath) if err != nil { return nil, "", diag.Diagnostics{ { @@ -68,8 +69,8 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, } // Check if the UC volume exists in the workspace. - volumePath := fmt.Sprintf("/Volumes/%s/%s/%s", catalogName, schemaName, volumeName) - err = w.Files.GetDirectoryMetadataByDirectoryPath(ctx, volumePath) + volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + _, err = w.Volumes.ReadByName(ctx, volumeName) // If the volume exists already, directly return the filer for the path to // upload the artifacts to. @@ -81,19 +82,24 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, baseErr := diag.Diagnostic{ Severity: diag.Error, - Summary: fmt.Sprintf("unable to determine if volume at %s exists: %s", volumePath, err), + Summary: fmt.Sprintf("unable to determine if volume at %s exists: %s", volumeFullName, err), Locations: b.Config.GetLocations("workspace.artifact_path"), Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, } + if errors.Is(err, apierr.ErrPermissionDenied) { + // If the API returned a 403, the user does not have permission to access the volume. + // Modify the error message to provide more context. + baseErr.Summary = fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err) + } if errors.Is(err, apierr.ErrNotFound) { // Since the API returned a 404, the volume does not exist. // Modify the error message to provide more context. - baseErr.Summary = fmt.Sprintf("volume %s does not exist: %s", volumePath, err) + baseErr.Summary = fmt.Sprintf("volume %s does not exist", volumeFullName) // If the volume is defined in the bundle, provide a more helpful error diagnostic, // with more details and location information. - path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) + path, locations, ok := FindVolumeInBundle(b.Config, catalogName, schemaName, volumeName) if !ok { return nil, "", diag.Diagnostics{baseErr} } @@ -108,8 +114,8 @@ the artifact_path.` return nil, "", diag.Diagnostics{baseErr} } -func findVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { - volumes := b.Config.Resources.Volumes +func FindVolumeInBundle(r config.Root, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { + volumes := r.Resources.Volumes for k, v := range volumes { if v.CatalogName != catalogName || v.Name != volumeName { continue @@ -126,7 +132,7 @@ func findVolumeInBundle(b *bundle.Bundle, catalogName, schemaName, volumeName st continue } pathString := fmt.Sprintf("resources.volumes.%s", k) - return dyn.MustPathFromString(pathString), b.Config.GetLocations(pathString), true + return dyn.MustPathFromString(pathString), r.GetLocations(pathString), true } return nil, nil, false } diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 7b2f5c5ba9..9855ee3cbe 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -48,7 +48,7 @@ func TestFindVolumeInBundle(t *testing.T) { }) // volume is in DAB. - path, locations, ok := findVolumeInBundle(b, "main", "my_schema", "my_volume") + path, locations, ok := FindVolumeInBundle(b.Config, "main", "my_schema", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ File: "volume.yml", @@ -58,26 +58,26 @@ func TestFindVolumeInBundle(t *testing.T) { assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) // wrong volume name - _, _, ok = findVolumeInBundle(b, "main", "my_schema", "doesnotexist") + _, _, ok = FindVolumeInBundle(b.Config, "main", "my_schema", "doesnotexist") assert.False(t, ok) // wrong schema name - _, _, ok = findVolumeInBundle(b, "main", "doesnotexist", "my_volume") + _, _, ok = FindVolumeInBundle(b.Config, "main", "doesnotexist", "my_volume") assert.False(t, ok) // wrong catalog name - _, _, ok = findVolumeInBundle(b, "doesnotexist", "my_schema", "my_volume") + _, _, ok = FindVolumeInBundle(b.Config, "doesnotexist", "my_schema", "my_volume") assert.False(t, ok) // schema name is interpolated but does not have the right prefix. In this case // we should not match the volume. b.Config.Resources.Volumes["foo"].SchemaName = "${foo.bar.baz}" - _, _, ok = findVolumeInBundle(b, "main", "my_schema", "my_volume") + _, _, ok = FindVolumeInBundle(b.Config, "main", "my_schema", "my_volume") assert.False(t, ok) // schema name is interpolated. b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema.name}" - path, locations, ok = findVolumeInBundle(b, "main", "valuedoesnotmatter", "my_volume") + path, locations, ok = FindVolumeInBundle(b.Config, "main", "valuedoesnotmatter", "my_volume") assert.True(t, ok) assert.Equal(t, []dyn.Location{{ File: "volume.yml", @@ -264,14 +264,14 @@ func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { } func TestExtractVolumeFromPath(t *testing.T) { - catalogName, schemaName, volumeName, err := extractVolumeFromPath("/Volumes/main/my_schema/my_volume") + catalogName, schemaName, volumeName, err := ExtractVolumeFromPath("/Volumes/main/my_schema/my_volume") require.NoError(t, err) assert.Equal(t, "main", catalogName) assert.Equal(t, "my_schema", schemaName) assert.Equal(t, "my_volume", volumeName) for _, p := range invalidVolumePaths() { - _, _, _, err := extractVolumeFromPath(p) + _, _, _, err := ExtractVolumeFromPath(p) assert.EqualError(t, err, fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) } } From 8568d92b3d07c3964eb3551307c6dd890b652030 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 16:53:22 +0530 Subject: [PATCH 02/16] Add fast validate method --- bundle/config/validate/fast_validate.go | 51 +++++++++++++ bundle/config/validate/validate.go | 10 +-- .../config/validate/validate_artifact_path.go | 1 - .../validate/validate_artifact_path_test.go | 52 ++++++++++++- bundle/libraries/filer_volume.go | 74 +------------------ cmd/bundle/deploy.go | 2 + 6 files changed, 112 insertions(+), 78 deletions(-) create mode 100644 bundle/config/validate/fast_validate.go diff --git a/bundle/config/validate/fast_validate.go b/bundle/config/validate/fast_validate.go new file mode 100644 index 0000000000..e5cc32b0cf --- /dev/null +++ b/bundle/config/validate/fast_validate.go @@ -0,0 +1,51 @@ +package validate + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +// FastValidate runs a set of fast validation checks. This is a subset of the full +// suite of validation mutators that satisfy ANY ONE of the following criteria: +// +// 1. No file i/o or network requests are made in the mutator. +// 2. Only returns errors which are blocking for a bundle deployment. +// +// The full suite of validation mutators is available in the [Validate] mutator. +type fastValidateReadonly struct{} + +func FastValidateReadonly() bundle.ReadOnlyMutator { + return &fastValidateReadonly{} +} + +func (f *fastValidateReadonly) Name() string { + return "fast_validate(readonly)" +} + +func (f *fastValidateReadonly) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { + return bundle.ApplyReadOnly(ctx, rb, bundle.Parallel( + // Fast mutators with only in-memory checks + JobClusterKeyDefined(), + JobTaskClusterSpec(), + SingleNodeCluster(), + + // Blocking mutators. Deployments will fail if these checks fail. + ValidateArtifactPath(), + )) +} + +type fastValidate struct{} + +func FastValidate() bundle.Mutator { + return &fastValidate{} +} + +func (f *fastValidate) Name() string { + return "fast_validate" +} + +func (f *fastValidate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + return bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), FastValidateReadonly()) +} diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index 45661bd802..8fdd704ab8 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -30,13 +30,13 @@ func (l location) Path() dyn.Path { // Apply implements bundle.Mutator. func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), bundle.Parallel( - JobClusterKeyDefined(), + FastValidateReadonly(), + + // Slow mutators that require network or file i/o. These are only + // run in the `bundle validate` command. FilesToSync(), - ValidateSyncPatterns(), - JobTaskClusterSpec(), ValidateFolderPermissions(), - SingleNodeCluster(), - ValidateArtifactPath(), + ValidateSyncPatterns(), )) } diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index c8b65983fc..632bce1cee 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -26,7 +26,6 @@ func (v *validateArtifactPath) Name() string { func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { // We only validate UC Volumes paths right now. - // TODO? if !libraries.IsVolumesPath(rb.Config().Workspace.ArtifactPath) { return nil } diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index c91da3aae7..26e58bb9e2 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" @@ -17,7 +18,56 @@ import ( "github.com/stretchr/testify/mock" ) -func TestVali +func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/catalogN/schemaN/volumeN/abc", + }, + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "catalogN", + Name: "volumeN", + VolumeType: "MANAGED", + SchemaName: "schemaN", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "file", Line: 1, Column: 1}}) + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{{File: "file", Line: 2, Column: 2}}) + + ctx := context.Background() + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockGrantsAPI() + api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(nil, &apierr.APIError{ + StatusCode: 404, + }) + b.SetWorkpaceClient(m.WorkspaceClient) + + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), validate.ValidateArtifactPath()) + assert.Equal(t, diag.Diagnostics{{ + Severity: diag.Error, + Summary: "volume catalogN.schemaN.volumeN does not exist", + Locations: []dyn.Location{ + {File: "file", Line: 1, Column: 1}, + {File: "file", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("workspace.artifact_path"), + dyn.MustPathFromString("resources.volumes.foo"), + }, + Detail: `You are using a volume in your artifact_path that is managed by +this bundle but which has not been deployed yet. Please first deploy +the volume using 'bundle deploy' and then switch over to using it in +the artifact_path.`, + }}, diags) +} func TestValidateArtifactPath(t *testing.T) { b := &bundle.Bundle{ diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 94ff74a4c1..b1ba4b769b 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -2,7 +2,6 @@ package libraries import ( "context" - "errors" "fmt" "path" "strings" @@ -13,7 +12,6 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/filer" - "github.com/databricks/databricks-sdk-go/apierr" ) func ExtractVolumeFromPath(artifactPath string) (string, string, string, error) { @@ -41,77 +39,11 @@ func ExtractVolumeFromPath(artifactPath string) (string, string, string, error) return catalogName, schemaName, volumeName, nil } -// This function returns a filer for ".internal" folder inside the directory configured -// at `workspace.artifact_path`. -// This function also checks if the UC volume exists in the workspace and then: -// 1. If the UC volume exists in the workspace: -// Returns a filer for the UC volume. -// 2. If the UC volume does not exist in the workspace but is (with high confidence) defined in -// the bundle configuration: -// Returns an error and a warning that instructs the user to deploy the -// UC volume before using it in the artifact path. -// 3. If the UC volume does not exist in the workspace and is not defined in the bundle configuration: -// Returns an error. func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { - artifactPath := b.Config.Workspace.ArtifactPath w := b.WorkspaceClient() - - catalogName, schemaName, volumeName, err := ExtractVolumeFromPath(artifactPath) - if err != nil { - return nil, "", diag.Diagnostics{ - { - Severity: diag.Error, - Summary: err.Error(), - Locations: b.Config.GetLocations("workspace.artifact_path"), - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }, - } - } - - // Check if the UC volume exists in the workspace. - volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) - _, err = w.Volumes.ReadByName(ctx, volumeName) - - // If the volume exists already, directly return the filer for the path to - // upload the artifacts to. - if err == nil { - uploadPath := path.Join(artifactPath, InternalDirName) - f, err := filer.NewFilesClient(w, uploadPath) - return f, uploadPath, diag.FromErr(err) - } - - baseErr := diag.Diagnostic{ - Severity: diag.Error, - Summary: fmt.Sprintf("unable to determine if volume at %s exists: %s", volumeFullName, err), - Locations: b.Config.GetLocations("workspace.artifact_path"), - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - } - - if errors.Is(err, apierr.ErrPermissionDenied) { - // If the API returned a 403, the user does not have permission to access the volume. - // Modify the error message to provide more context. - baseErr.Summary = fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err) - } - if errors.Is(err, apierr.ErrNotFound) { - // Since the API returned a 404, the volume does not exist. - // Modify the error message to provide more context. - baseErr.Summary = fmt.Sprintf("volume %s does not exist", volumeFullName) - - // If the volume is defined in the bundle, provide a more helpful error diagnostic, - // with more details and location information. - path, locations, ok := FindVolumeInBundle(b.Config, catalogName, schemaName, volumeName) - if !ok { - return nil, "", diag.Diagnostics{baseErr} - } - baseErr.Detail = `You are using a volume in your artifact_path that is managed by -this bundle but which has not been deployed yet. Please first deploy -the volume using 'bundle deploy' and then switch over to using it in -the artifact_path.` - baseErr.Paths = append(baseErr.Paths, path) - baseErr.Locations = append(baseErr.Locations, locations...) - } - - return nil, "", diag.Diagnostics{baseErr} + uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) + f, err := filer.NewFilesClient(w, uploadPath) + return f, uploadPath, diag.FromErr(err) } func FindVolumeInBundle(r config.Root, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index a25e02f6c1..560b07e396 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/render" "github.com/databricks/cli/cmd/bundle/utils" @@ -71,6 +72,7 @@ func newDeployCommand() *cobra.Command { diags = diags.Extend( bundle.Apply(ctx, b, bundle.Seq( phases.Initialize(), + validate.FastValidate(), phases.Build(), phases.Deploy(outputHandler), )), From 2a590b83bc9da1c784c7bf989385bc2a50986744 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 16:58:36 +0530 Subject: [PATCH 03/16] moved ExtractVolumeFromPath --- .../config/validate/validate_artifact_path.go | 43 +++++++++++--- .../validate/validate_artifact_path_test.go | 58 +++++++++++++++++-- bundle/libraries/filer_volume.go | 26 --------- bundle/libraries/filer_volume_test.go | 49 ---------------- 4 files changed, 89 insertions(+), 87 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index 632bce1cee..f6b33a83c2 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "slices" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/libraries" @@ -24,20 +25,37 @@ func (v *validateArtifactPath) Name() string { return "validate:artifact_paths" } +func extractVolumeFromPath(artifactPath string) (string, string, string, error) { + if !libraries.IsVolumesPath(artifactPath) { + return "", "", "", fmt.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) + } + + parts := strings.Split(artifactPath, "/") + volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", artifactPath) + + // Incorrect format. + if len(parts) < 5 { + return "", "", "", volumeFormatErr + } + + catalogName := parts[2] + schemaName := parts[3] + volumeName := parts[4] + + // Incorrect format. + if catalogName == "" || schemaName == "" || volumeName == "" { + return "", "", "", volumeFormatErr + } + + return catalogName, schemaName, volumeName, nil +} + func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { // We only validate UC Volumes paths right now. if !libraries.IsVolumesPath(rb.Config().Workspace.ArtifactPath) { return nil } - catalogName, schemaName, volumeName, err := libraries.ExtractVolumeFromPath(rb.Config().Workspace.ArtifactPath) - if err != nil { - return diag.FromErr(err) - } - volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) - w := rb.WorkspaceClient() - p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) - wrapErrorMsg := func(s string) diag.Diagnostics { return diag.Diagnostics{ { @@ -48,6 +66,15 @@ func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBund }, } } + + catalogName, schemaName, volumeName, err := extractVolumeFromPath(rb.Config().Workspace.ArtifactPath) + if err != nil { + return wrapErrorMsg(err.Error()) + } + volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) + w := rb.WorkspaceClient() + p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) + if errors.Is(err, apierr.ErrPermissionDenied) { return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) } diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index 26e58bb9e2..76e3475138 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -1,13 +1,13 @@ -package validate_test +package validate import ( "context" + "fmt" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" @@ -16,6 +16,7 @@ import ( "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { @@ -50,7 +51,7 @@ func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { }) b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), validate.ValidateArtifactPath()) + diags := bundle.ApplyReadOnly(ctx, bundle.ReadOnly(b), ValidateArtifactPath()) assert.Equal(t, diag.Diagnostics{{ Severity: diag.Error, Summary: "volume catalogN.schemaN.volumeN does not exist", @@ -149,7 +150,7 @@ func TestValidateArtifactPath(t *testing.T) { api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(tc.permissions, tc.err) b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.ApplyReadOnly(ctx, rb, validate.ValidateArtifactPath()) + diags := bundle.ApplyReadOnly(ctx, rb, ValidateArtifactPath()) if tc.expectedSummary != "" { assertDiags(t, diags, tc.expectedSummary) } else { @@ -157,3 +158,52 @@ func TestValidateArtifactPath(t *testing.T) { } } } + +func invalidVolumePaths() []string { + return []string{ + "/Volumes/", + "/Volumes/main", + "/Volumes/main/", + "/Volumes/main//", + "/Volumes/main//my_schema", + "/Volumes/main/my_schema", + "/Volumes/main/my_schema/", + "/Volumes/main/my_schema//", + "/Volumes//my_schema/my_volume", + } +} + +func TestExtractVolumeFromPath(t *testing.T) { + catalogName, schemaName, volumeName, err := extractVolumeFromPath("/Volumes/main/my_schema/my_volume") + require.NoError(t, err) + assert.Equal(t, "main", catalogName) + assert.Equal(t, "my_schema", schemaName) + assert.Equal(t, "my_volume", volumeName) + + for _, p := range invalidVolumePaths() { + _, _, _, err := extractVolumeFromPath(p) + assert.EqualError(t, err, fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) + } +} + +func TestValidateArtifactPathWithInvalidPaths(t *testing.T) { + for _, p := range invalidVolumePaths() { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: p, + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + + diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), ValidateArtifactPath()) + require.Equal(t, diags, diag.Diagnostics{{ + Severity: diag.Error, + Summary: fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p), + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}) + } +} diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index b1ba4b769b..e356d9b82c 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "path" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" @@ -14,31 +13,6 @@ import ( "github.com/databricks/cli/libs/filer" ) -func ExtractVolumeFromPath(artifactPath string) (string, string, string, error) { - if !IsVolumesPath(artifactPath) { - return "", "", "", fmt.Errorf("expected artifact_path to start with /Volumes/, got %s", artifactPath) - } - - parts := strings.Split(artifactPath, "/") - volumeFormatErr := fmt.Errorf("expected UC volume path to be in the format /Volumes////..., got %s", artifactPath) - - // Incorrect format. - if len(parts) < 5 { - return "", "", "", volumeFormatErr - } - - catalogName := parts[2] - schemaName := parts[3] - volumeName := parts[4] - - // Incorrect format. - if catalogName == "" || schemaName == "" || volumeName == "" { - return "", "", "", volumeFormatErr - } - - return catalogName, schemaName, volumeName, nil -} - func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { w := b.WorkspaceClient() uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 9855ee3cbe..c788ac9195 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -185,42 +185,6 @@ the artifact_path.`, }, diags) } -func invalidVolumePaths() []string { - return []string{ - "/Volumes/", - "/Volumes/main", - "/Volumes/main/", - "/Volumes/main//", - "/Volumes/main//my_schema", - "/Volumes/main/my_schema", - "/Volumes/main/my_schema/", - "/Volumes/main/my_schema//", - "/Volumes//my_schema/my_volume", - } -} - -func TestFilerForVolumeWithInvalidVolumePaths(t *testing.T) { - for _, p := range invalidVolumePaths() { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: p, - }, - }, - } - - bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) - - _, _, diags := GetFilerForLibraries(context.Background(), b) - require.Equal(t, diags, diag.Diagnostics{{ - Severity: diag.Error, - Summary: fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p), - Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }}) - } -} - func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ @@ -262,16 +226,3 @@ func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { assert.IsType(t, &filer.FilesClient{}, client) } } - -func TestExtractVolumeFromPath(t *testing.T) { - catalogName, schemaName, volumeName, err := ExtractVolumeFromPath("/Volumes/main/my_schema/my_volume") - require.NoError(t, err) - assert.Equal(t, "main", catalogName) - assert.Equal(t, "my_schema", schemaName) - assert.Equal(t, "my_volume", volumeName) - - for _, p := range invalidVolumePaths() { - _, _, _, err := ExtractVolumeFromPath(p) - assert.EqualError(t, err, fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p)) - } -} From 56cd37b1aa900ad92e6f6030fd8d7b4d233d5fab Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 17:00:51 +0530 Subject: [PATCH 04/16] moved FindVolumeInBundle --- .../config/validate/validate_artifact_path.go | 27 +++++++- .../validate/validate_artifact_path_test.go | 65 +++++++++++++++++++ bundle/libraries/filer_volume.go | 27 -------- bundle/libraries/filer_volume_test.go | 65 ------------------- 4 files changed, 91 insertions(+), 93 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index f6b33a83c2..ab6a733a34 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -8,9 +8,11 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/service/catalog" ) @@ -50,6 +52,29 @@ func extractVolumeFromPath(artifactPath string) (string, string, string, error) return catalogName, schemaName, volumeName, nil } +func findVolumeInBundle(r config.Root, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { + volumes := r.Resources.Volumes + for k, v := range volumes { + if v.CatalogName != catalogName || v.Name != volumeName { + continue + } + // UC schemas can be defined in the bundle itself, and thus might be interpolated + // at runtime via the ${resources.schemas.} syntax. Thus we match the volume + // definition if the schema name is the same as the one in the bundle, or if the + // schema name is interpolated. + // We only have to check for ${resources.schemas...} references because any + // other valid reference (like ${var.foo}) would have been interpolated by this point. + p, ok := dynvar.PureReferenceToPath(v.SchemaName) + isSchemaDefinedInBundle := ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) + if v.SchemaName != schemaName && !isSchemaDefinedInBundle { + continue + } + pathString := fmt.Sprintf("resources.volumes.%s", k) + return dyn.MustPathFromString(pathString), r.GetLocations(pathString), true + } + return nil, nil, false +} + func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBundle) diag.Diagnostics { // We only validate UC Volumes paths right now. if !libraries.IsVolumesPath(rb.Config().Workspace.ArtifactPath) { @@ -79,7 +104,7 @@ func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBund return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) } if errors.Is(err, apierr.ErrNotFound) { - path, locations, ok := libraries.FindVolumeInBundle(rb.Config(), catalogName, schemaName, volumeName) + path, locations, ok := findVolumeInBundle(rb.Config(), catalogName, schemaName, volumeName) if !ok { return wrapErrorMsg(fmt.Sprintf("volume %s does not exist", volumeFullName)) } diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index 76e3475138..a7a0331569 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -207,3 +207,68 @@ func TestValidateArtifactPathWithInvalidPaths(t *testing.T) { }}) } } + +func TestFindVolumeInBundle(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "foo": { + CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ + CatalogName: "main", + Name: "my_volume", + SchemaName: "my_schema", + }, + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ + { + File: "volume.yml", + Line: 1, + Column: 2, + }, + }) + + // volume is in DAB. + path, locations, ok := findVolumeInBundle(b.Config, "main", "my_schema", "my_volume") + assert.True(t, ok) + assert.Equal(t, []dyn.Location{{ + File: "volume.yml", + Line: 1, + Column: 2, + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) + + // wrong volume name + _, _, ok = findVolumeInBundle(b.Config, "main", "my_schema", "doesnotexist") + assert.False(t, ok) + + // wrong schema name + _, _, ok = findVolumeInBundle(b.Config, "main", "doesnotexist", "my_volume") + assert.False(t, ok) + + // wrong catalog name + _, _, ok = findVolumeInBundle(b.Config, "doesnotexist", "my_schema", "my_volume") + assert.False(t, ok) + + // schema name is interpolated but does not have the right prefix. In this case + // we should not match the volume. + b.Config.Resources.Volumes["foo"].SchemaName = "${foo.bar.baz}" + _, _, ok = findVolumeInBundle(b.Config, "main", "my_schema", "my_volume") + assert.False(t, ok) + + // schema name is interpolated. + b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema.name}" + path, locations, ok = findVolumeInBundle(b.Config, "main", "valuedoesnotmatter", "my_volume") + assert.True(t, ok) + assert.Equal(t, []dyn.Location{{ + File: "volume.yml", + Line: 1, + Column: 2, + }}, locations) + assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) +} diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index e356d9b82c..4ef574628c 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -2,14 +2,10 @@ package libraries import ( "context" - "fmt" "path" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/filer" ) @@ -19,26 +15,3 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, f, err := filer.NewFilesClient(w, uploadPath) return f, uploadPath, diag.FromErr(err) } - -func FindVolumeInBundle(r config.Root, catalogName, schemaName, volumeName string) (dyn.Path, []dyn.Location, bool) { - volumes := r.Resources.Volumes - for k, v := range volumes { - if v.CatalogName != catalogName || v.Name != volumeName { - continue - } - // UC schemas can be defined in the bundle itself, and thus might be interpolated - // at runtime via the ${resources.schemas.} syntax. Thus we match the volume - // definition if the schema name is the same as the one in the bundle, or if the - // schema name is interpolated. - // We only have to check for ${resources.schemas...} references because any - // other valid reference (like ${var.foo}) would have been interpolated by this point. - p, ok := dynvar.PureReferenceToPath(v.SchemaName) - isSchemaDefinedInBundle := ok && p.HasPrefix(dyn.Path{dyn.Key("resources"), dyn.Key("schemas")}) - if v.SchemaName != schemaName && !isSchemaDefinedInBundle { - continue - } - pathString := fmt.Sprintf("resources.volumes.%s", k) - return dyn.MustPathFromString(pathString), r.GetLocations(pathString), true - } - return nil, nil, false -} diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index c788ac9195..ee3523737f 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -22,71 +22,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestFindVolumeInBundle(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - SchemaName: "my_schema", - }, - }, - }, - }, - }, - } - - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{ - { - File: "volume.yml", - Line: 1, - Column: 2, - }, - }) - - // volume is in DAB. - path, locations, ok := FindVolumeInBundle(b.Config, "main", "my_schema", "my_volume") - assert.True(t, ok) - assert.Equal(t, []dyn.Location{{ - File: "volume.yml", - Line: 1, - Column: 2, - }}, locations) - assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) - - // wrong volume name - _, _, ok = FindVolumeInBundle(b.Config, "main", "my_schema", "doesnotexist") - assert.False(t, ok) - - // wrong schema name - _, _, ok = FindVolumeInBundle(b.Config, "main", "doesnotexist", "my_volume") - assert.False(t, ok) - - // wrong catalog name - _, _, ok = FindVolumeInBundle(b.Config, "doesnotexist", "my_schema", "my_volume") - assert.False(t, ok) - - // schema name is interpolated but does not have the right prefix. In this case - // we should not match the volume. - b.Config.Resources.Volumes["foo"].SchemaName = "${foo.bar.baz}" - _, _, ok = FindVolumeInBundle(b.Config, "main", "my_schema", "my_volume") - assert.False(t, ok) - - // schema name is interpolated. - b.Config.Resources.Volumes["foo"].SchemaName = "${resources.schemas.my_schema.name}" - path, locations, ok = FindVolumeInBundle(b.Config, "main", "valuedoesnotmatter", "my_volume") - assert.True(t, ok) - assert.Equal(t, []dyn.Location{{ - File: "volume.yml", - Line: 1, - Column: 2, - }}, locations) - assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) -} - func TestFilerForVolumeForErrorFromAPI(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ From 923bc96e276ff4d933b9e2ac65cba09adea2e4a0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 17:02:00 +0530 Subject: [PATCH 05/16] remove unused arg infilerForVolume(context.Background(), b) --- bundle/libraries/filer.go | 2 +- bundle/libraries/filer_volume.go | 3 +-- bundle/libraries/filer_volume_test.go | 8 ++++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/bundle/libraries/filer.go b/bundle/libraries/filer.go index 4448ed325b..e09c75e0eb 100644 --- a/bundle/libraries/filer.go +++ b/bundle/libraries/filer.go @@ -24,7 +24,7 @@ func GetFilerForLibraries(ctx context.Context, b *bundle.Bundle) (filer.Filer, s switch { case IsVolumesPath(artifactPath): - return filerForVolume(ctx, b) + return filerForVolume(b) default: return filerForWorkspace(b) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 4ef574628c..176f475c6b 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -1,7 +1,6 @@ package libraries import ( - "context" "path" "github.com/databricks/cli/bundle" @@ -9,7 +8,7 @@ import ( "github.com/databricks/cli/libs/filer" ) -func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { +func filerForVolume(b *bundle.Bundle) (filer.Filer, string, diag.Diagnostics) { w := b.WorkspaceClient() uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName) f, err := filer.NewFilesClient(w, uploadPath) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index ee3523737f..75d4595aa8 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -38,7 +38,7 @@ func TestFilerForVolumeForErrorFromAPI(t *testing.T) { m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) b.SetWorkpaceClient(m.WorkspaceClient) - _, _, diags := filerForVolume(context.Background(), b) + _, _, diags := filerForVolume(b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, @@ -65,7 +65,7 @@ func TestFilerForVolumeWithVolumeNotFound(t *testing.T) { m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(apierr.NotFound("some error message")) b.SetWorkpaceClient(m.WorkspaceClient) - _, _, diags := filerForVolume(context.Background(), b) + _, _, diags := filerForVolume(b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, @@ -129,7 +129,7 @@ func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { }, } - _, _, diags := filerForVolume(context.Background(), b) + _, _, diags := filerForVolume(b) require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") } @@ -155,7 +155,7 @@ func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) b.SetWorkpaceClient(m.WorkspaceClient) - client, uploadPath, diags := filerForVolume(context.Background(), b) + client, uploadPath, diags := filerForVolume(b) require.NoError(t, diags.Error()) assert.Equal(t, path.Join(p, ".internal"), uploadPath) assert.IsType(t, &filer.FilesClient{}, client) From 5a91081ca83d4e688fee18ac682b9ba585dfb34b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 17:32:20 +0530 Subject: [PATCH 06/16] fix tests --- bundle/libraries/filer_volume_test.go | 141 ++------------------------ bundle/libraries/upload_test.go | 7 -- 2 files changed, 9 insertions(+), 139 deletions(-) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 75d4595aa8..31c4362ed1 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -1,163 +1,40 @@ package libraries import ( - "context" - "fmt" "path" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/bundle/internal/bundletest" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" - "github.com/databricks/databricks-sdk-go/apierr" - sdkconfig "github.com/databricks/databricks-sdk-go/config" - "github.com/databricks/databricks-sdk-go/experimental/mocks" - "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) -func TestFilerForVolumeForErrorFromAPI(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, - }, - } - - bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) - - _, _, diags := filerForVolume(b) - assert.Equal(t, diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "unable to determine if volume at /Volumes/main/my_schema/my_volume exists: error from API", - Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }, - }, diags) -} - -func TestFilerForVolumeWithVolumeNotFound(t *testing.T) { +func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/doesnotexist", + ArtifactPath: "/Volume/main/my_schema/my_volume", }, }, } - bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(apierr.NotFound("some error message")) - b.SetWorkpaceClient(m.WorkspaceClient) - _, _, diags := filerForVolume(b) - assert.Equal(t, diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "volume /Volumes/main/my_schema/doesnotexist does not exist: some error message", - Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }, - }, diags) -} - -func TestFilerForVolumeNotFoundAndInBundle(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volumes/main/my_schema/my_volume", - }, - Resources: config.Resources{ - Volumes: map[string]*resources.Volume{ - "foo": { - CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ - CatalogName: "main", - Name: "my_volume", - VolumeType: "MANAGED", - SchemaName: "my_schema", - }, - }, - }, - }, - }, - } - - bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) - bundletest.SetLocation(b, "resources.volumes.foo", []dyn.Location{{File: "volume.yml", Line: 1, Column: 2}}) - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(apierr.NotFound("error from API")) - b.SetWorkpaceClient(m.WorkspaceClient) - - _, _, diags := GetFilerForLibraries(context.Background(), b) - assert.Equal(t, diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "volume /Volumes/main/my_schema/my_volume does not exist: error from API", - Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}, {File: "volume.yml", Line: 1, Column: 2}}, - Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, - Detail: `You are using a volume in your artifact_path that is managed by -this bundle but which has not been deployed yet. Please first deploy -the volume using 'bundle deploy' and then switch over to using it in -the artifact_path.`, - }, - }, diags) + require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") } -func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { +func TestFilerForVolume(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - ArtifactPath: "/Volume/main/my_schema/my_volume", + ArtifactPath: "/Volumes/main/my_schema/my_volume/abc", }, }, } - _, _, diags := filerForVolume(b) - require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") -} - -func TestFilerForVolumeWithValidVolumePaths(t *testing.T) { - validPaths := []string{ - "/Volumes/main/my_schema/my_volume", - "/Volumes/main/my_schema/my_volume/", - "/Volumes/main/my_schema/my_volume/a/b/c", - "/Volumes/main/my_schema/my_volume/a/a/a", - } - - for _, p := range validPaths { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: p, - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) - b.SetWorkpaceClient(m.WorkspaceClient) - - client, uploadPath, diags := filerForVolume(b) - require.NoError(t, diags.Error()) - assert.Equal(t, path.Join(p, ".internal"), uploadPath) - assert.IsType(t, &filer.FilesClient{}, client) - } + client, uploadPath, diags := filerForVolume(b) + require.NoError(t, diags.Error()) + assert.Equal(t, path.Join("/Volumes/main/my_schema/my_volume/abc/.internal"), uploadPath) + assert.IsType(t, &filer.FilesClient{}, client) } diff --git a/bundle/libraries/upload_test.go b/bundle/libraries/upload_test.go index 493785bf50..44b194c56a 100644 --- a/bundle/libraries/upload_test.go +++ b/bundle/libraries/upload_test.go @@ -11,8 +11,6 @@ import ( mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/filer" - sdkconfig "github.com/databricks/databricks-sdk-go/config" - "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/mock" @@ -183,11 +181,6 @@ func TestArtifactUploadForVolumes(t *testing.T) { filer.CreateParentDirectories, ).Return(nil) - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/foo/bar/artifacts").Return(nil) - b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.Apply(context.Background(), b, bundle.Seq(ExpandGlobReferences(), UploadWithClient(mockFiler))) require.NoError(t, diags.Error()) From 2d05aed9e00b6d08ecca72bf26c8adb4758468da Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 17:37:36 +0530 Subject: [PATCH 07/16] fix test --- bundle/libraries/filer_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bundle/libraries/filer_test.go b/bundle/libraries/filer_test.go index 88ba152fc1..c18da9726c 100644 --- a/bundle/libraries/filer_test.go +++ b/bundle/libraries/filer_test.go @@ -7,10 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/filer" - sdkconfig "github.com/databricks/databricks-sdk-go/config" - "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -39,11 +36,6 @@ func TestGetFilerForLibrariesValidUcVolume(t *testing.T) { }, } - m := mocks.NewMockWorkspaceClient(t) - m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(nil) - b.SetWorkpaceClient(m.WorkspaceClient) - client, uploadPath, diags := GetFilerForLibraries(context.Background(), b) require.NoError(t, diags.Error()) assert.Equal(t, "/Volumes/main/my_schema/my_volume/.internal", uploadPath) From d2cf054f13c2464d902738b930ca01f903614bce Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 17:38:24 +0530 Subject: [PATCH 08/16] fix tesT --- bundle/libraries/filer_volume_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 31c4362ed1..39bdc4135b 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -11,19 +11,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestFilerForVolumeWithInvalidPrefix(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - ArtifactPath: "/Volume/main/my_schema/my_volume", - }, - }, - } - - _, _, diags := filerForVolume(b) - require.EqualError(t, diags.Error(), "expected artifact_path to start with /Volumes/, got /Volume/main/my_schema/my_volume") -} - func TestFilerForVolume(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ From 4da773984a9e178e3f784848bae4d7c1843d1619 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 19:50:47 +0530 Subject: [PATCH 09/16] add debug log and fix tests --- bundle/config/validate/validate_artifact_path.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index ab6a733a34..ab6c94277a 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -13,6 +13,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" + "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/service/catalog" ) @@ -138,6 +139,7 @@ the artifact_path.`, // The user needs to have either WRITE_VOLUME or ALL_PRIVILEGES to write to the volume. canWrite := slices.Contains(allPrivileges, catalog.PrivilegeWriteVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) if !canWrite { + log.Infof(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) return wrapErrorMsg(fmt.Sprintf("user does not have WRITE_VOLUME grant on volume %s", volumeFullName)) } @@ -145,6 +147,7 @@ the artifact_path.`, // We still add this explicit check out of caution incase the API behavior changes in the future. canRead := slices.Contains(allPrivileges, catalog.PrivilegeReadVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) if !canRead { + log.Infof(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) return wrapErrorMsg(fmt.Sprintf("user does not have READ_VOLUME grant on volume %s", volumeFullName)) } From 1a5f35f120f33b5ce819d12313fa15187817b4e9 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 19:51:39 +0530 Subject: [PATCH 10/16] fix more test --- integration/bundle/artifacts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/bundle/artifacts_test.go b/integration/bundle/artifacts_test.go index 1b71a1c3d1..6655ef6582 100644 --- a/integration/bundle/artifacts_test.go +++ b/integration/bundle/artifacts_test.go @@ -257,7 +257,7 @@ func TestUploadArtifactFileToVolumeThatDoesNotExist(t *testing.T) { stdout, stderr, err := testcli.RequireErrorRun(t, ctx, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/doesnotexist does not exist: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume main.%s.doesnotexist does not exist at workspace.artifact_path in databricks.yml:6:18 @@ -293,7 +293,7 @@ func TestUploadArtifactToVolumeNotYetDeployed(t *testing.T) { stdout, stderr, err := testcli.RequireErrorRun(t, ctx, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/my_volume does not exist: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume main.%s.my_volume does not exist at workspace.artifact_path resources.volumes.foo in databricks.yml:6:18 From b6ab3cf4ba00c4b13f6a3f87b9acae3646b14ea8 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Dec 2024 20:20:24 +0530 Subject: [PATCH 11/16] debug logs --- bundle/config/validate/validate_artifact_path.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index ab6c94277a..97f4166b4d 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -139,7 +139,7 @@ the artifact_path.`, // The user needs to have either WRITE_VOLUME or ALL_PRIVILEGES to write to the volume. canWrite := slices.Contains(allPrivileges, catalog.PrivilegeWriteVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) if !canWrite { - log.Infof(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) + log.Debugf(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) return wrapErrorMsg(fmt.Sprintf("user does not have WRITE_VOLUME grant on volume %s", volumeFullName)) } @@ -147,7 +147,7 @@ the artifact_path.`, // We still add this explicit check out of caution incase the API behavior changes in the future. canRead := slices.Contains(allPrivileges, catalog.PrivilegeReadVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) if !canRead { - log.Infof(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) + log.Debugf(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) return wrapErrorMsg(fmt.Sprintf("user does not have READ_VOLUME grant on volume %s", volumeFullName)) } From 089ea4fa97911453630c6c756bcb01016542c3b3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 31 Dec 2024 15:27:20 +0530 Subject: [PATCH 12/16] stop using grants API --- .../config/validate/validate_artifact_path.go | 30 +------------- .../validate/validate_artifact_path_test.go | 41 +++---------------- 2 files changed, 8 insertions(+), 63 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index 97f4166b4d..0478f47992 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -13,9 +13,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" - "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/apierr" - "github.com/databricks/databricks-sdk-go/service/catalog" ) type validateArtifactPath struct{} @@ -99,7 +97,7 @@ func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBund } volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) w := rb.WorkspaceClient() - p, err := w.Grants.GetEffectiveBySecurableTypeAndFullName(ctx, catalog.SecurableTypeVolume, volumeFullName) + _, err = w.Volumes.GetByName(ctx, volumeFullName) if errors.Is(err, apierr.ErrPermissionDenied) { return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) @@ -125,31 +123,7 @@ the artifact_path.`, } if err != nil { - return wrapErrorMsg(fmt.Sprintf("could not fetch grants for volume %s: %s", volumeFullName, err)) + return wrapErrorMsg(fmt.Sprintf("cannot read volume %s: %s", volumeFullName, err)) } - - allPrivileges := []catalog.Privilege{} - for _, assignments := range p.PrivilegeAssignments { - for _, privilege := range assignments.Privileges { - allPrivileges = append(allPrivileges, privilege.Privilege) - } - } - - // UC Volumes have the following privileges: [READ_VOLUME, WRITE_VOLUME, MANAGE, ALL_PRIVILEGES, APPLY TAG] - // The user needs to have either WRITE_VOLUME or ALL_PRIVILEGES to write to the volume. - canWrite := slices.Contains(allPrivileges, catalog.PrivilegeWriteVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) - if !canWrite { - log.Debugf(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) - return wrapErrorMsg(fmt.Sprintf("user does not have WRITE_VOLUME grant on volume %s", volumeFullName)) - } - - // READ_VOLUME is implied since the user was able to fetch the associated grants with the volume. - // We still add this explicit check out of caution incase the API behavior changes in the future. - canRead := slices.Contains(allPrivileges, catalog.PrivilegeReadVolume) || slices.Contains(allPrivileges, catalog.PrivilegeAllPrivileges) - if !canRead { - log.Debugf(ctx, "Current privileges on Volume at artifact_path: %v", allPrivileges) - return wrapErrorMsg(fmt.Sprintf("user does not have READ_VOLUME grant on volume %s", volumeFullName)) - } - return nil } diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index a7a0331569..e248ed2ddc 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -45,8 +45,8 @@ func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { ctx := context.Background() m := mocks.NewMockWorkspaceClient(t) - api := m.GetMockGrantsAPI() - api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(nil, &apierr.APIError{ + api := m.GetMockVolumesAPI() + api.EXPECT().GetByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, &apierr.APIError{ StatusCode: 404, }) b.SetWorkpaceClient(m.WorkspaceClient) @@ -90,22 +90,11 @@ func TestValidateArtifactPath(t *testing.T) { }}, diags) } - wrapPrivileges := func(privileges ...catalog.Privilege) *catalog.EffectivePermissionsList { - perms := &catalog.EffectivePermissionsList{} - for _, p := range privileges { - perms.PrivilegeAssignments = append(perms.PrivilegeAssignments, catalog.EffectivePrivilegeAssignment{ - Privileges: []catalog.EffectivePrivilege{{Privilege: p}}, - }) - } - return perms - } - rb := bundle.ReadOnly(b) ctx := context.Background() tcases := []struct { err error - permissions *catalog.EffectivePermissionsList expectedSummary string }{ { @@ -126,36 +115,18 @@ func TestValidateArtifactPath(t *testing.T) { StatusCode: 500, Message: "Internal Server Error", }, - expectedSummary: "could not fetch grants for volume catalogN.schemaN.volumeN: Internal Server Error", - }, - { - permissions: wrapPrivileges(catalog.PrivilegeAllPrivileges), - }, - { - permissions: wrapPrivileges(catalog.PrivilegeApplyTag, catalog.PrivilegeManage), - expectedSummary: "user does not have WRITE_VOLUME grant on volume catalogN.schemaN.volumeN", - }, - { - permissions: wrapPrivileges(catalog.PrivilegeWriteVolume), - expectedSummary: "user does not have READ_VOLUME grant on volume catalogN.schemaN.volumeN", - }, - { - permissions: wrapPrivileges(catalog.PrivilegeWriteVolume, catalog.PrivilegeReadVolume), + expectedSummary: "cannot read volume catalogN.schemaN.volumeN: Internal Server Error", }, } for _, tc := range tcases { m := mocks.NewMockWorkspaceClient(t) - api := m.GetMockGrantsAPI() - api.EXPECT().GetEffectiveBySecurableTypeAndFullName(mock.Anything, catalog.SecurableTypeVolume, "catalogN.schemaN.volumeN").Return(tc.permissions, tc.err) + api := m.GetMockVolumesAPI() + api.EXPECT().GetByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, tc.err) b.SetWorkpaceClient(m.WorkspaceClient) diags := bundle.ApplyReadOnly(ctx, rb, ValidateArtifactPath()) - if tc.expectedSummary != "" { - assertDiags(t, diags, tc.expectedSummary) - } else { - assert.Len(t, diags, 0) - } + assertDiags(t, diags, tc.expectedSummary) } } From 2b5cd6a2a7b33125a7123e631b97f4d369febd36 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 31 Dec 2024 15:37:09 +0530 Subject: [PATCH 13/16] - --- bundle/config/validate/fast_validate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/validate/fast_validate.go b/bundle/config/validate/fast_validate.go index e5cc32b0cf..47d83036d5 100644 --- a/bundle/config/validate/fast_validate.go +++ b/bundle/config/validate/fast_validate.go @@ -7,11 +7,11 @@ import ( "github.com/databricks/cli/libs/diag" ) -// FastValidate runs a set of fast validation checks. This is a subset of the full +// FastValidate runs a subset of fast validation checks. This is a subset of the full // suite of validation mutators that satisfy ANY ONE of the following criteria: // // 1. No file i/o or network requests are made in the mutator. -// 2. Only returns errors which are blocking for a bundle deployment. +// 2. The validation is blocking for bundle deployments. // // The full suite of validation mutators is available in the [Validate] mutator. type fastValidateReadonly struct{} From a69dc7f0024ff72f65fa2e5113f3dc71219d29ca Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 31 Dec 2024 15:48:40 +0530 Subject: [PATCH 14/16] get -> read --- bundle/config/validate/validate_artifact_path.go | 2 +- bundle/config/validate/validate_artifact_path_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path.go b/bundle/config/validate/validate_artifact_path.go index 0478f47992..5bab99ccfc 100644 --- a/bundle/config/validate/validate_artifact_path.go +++ b/bundle/config/validate/validate_artifact_path.go @@ -97,7 +97,7 @@ func (v *validateArtifactPath) Apply(ctx context.Context, rb bundle.ReadOnlyBund } volumeFullName := fmt.Sprintf("%s.%s.%s", catalogName, schemaName, volumeName) w := rb.WorkspaceClient() - _, err = w.Volumes.GetByName(ctx, volumeFullName) + _, err = w.Volumes.ReadByName(ctx, volumeFullName) if errors.Is(err, apierr.ErrPermissionDenied) { return wrapErrorMsg(fmt.Sprintf("cannot access volume %s: %s", volumeFullName, err)) diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index e248ed2ddc..4cff19a91f 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -46,7 +46,7 @@ func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { ctx := context.Background() m := mocks.NewMockWorkspaceClient(t) api := m.GetMockVolumesAPI() - api.EXPECT().GetByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, &apierr.APIError{ + api.EXPECT().ReadByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, &apierr.APIError{ StatusCode: 404, }) b.SetWorkpaceClient(m.WorkspaceClient) @@ -122,7 +122,7 @@ func TestValidateArtifactPath(t *testing.T) { for _, tc := range tcases { m := mocks.NewMockWorkspaceClient(t) api := m.GetMockVolumesAPI() - api.EXPECT().GetByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, tc.err) + api.EXPECT().ReadByName(mock.Anything, "catalogN.schemaN.volumeN").Return(nil, tc.err) b.SetWorkpaceClient(m.WorkspaceClient) diags := bundle.ApplyReadOnly(ctx, rb, ValidateArtifactPath()) From 728b1257d8dd33828fd42f66f3f5475071454038 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 2 Jan 2025 14:40:58 +0530 Subject: [PATCH 15/16] address comments --- bundle/config/validate/validate_artifact_path_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index 4cff19a91f..f6e911218b 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -31,7 +31,6 @@ func TestValidateArtifactPathWithVolumeInBundle(t *testing.T) { CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{ CatalogName: "catalogN", Name: "volumeN", - VolumeType: "MANAGED", SchemaName: "schemaN", }, }, From 8feb04a05ea92510250511d42e89ddc86220ec43 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 2 Jan 2025 16:54:23 +0530 Subject: [PATCH 16/16] lint --- bundle/config/validate/validate_artifact_path_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/validate/validate_artifact_path_test.go b/bundle/config/validate/validate_artifact_path_test.go index f6e911218b..8fb5c9618e 100644 --- a/bundle/config/validate/validate_artifact_path_test.go +++ b/bundle/config/validate/validate_artifact_path_test.go @@ -169,12 +169,12 @@ func TestValidateArtifactPathWithInvalidPaths(t *testing.T) { bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), ValidateArtifactPath()) - require.Equal(t, diags, diag.Diagnostics{{ + require.Equal(t, diag.Diagnostics{{ Severity: diag.Error, Summary: fmt.Sprintf("expected UC volume path to be in the format /Volumes////..., got %s", p), Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, - }}) + }}, diags) } }