From dc2cf3bc42f24c180dcaec9605e0b80eda8989d8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 9 Dec 2024 16:26:41 +0100 Subject: [PATCH] Process top-level permissions for resources using dynamic config (#1964) ## Changes This PR ensures that when new resources are added they are handled by top-level permissions mutator, either by supporting or not supporting the resource type. ## Tests Added unit tests --- bundle/permissions/mutator.go | 126 ++++++++++++++--------------- bundle/permissions/mutator_test.go | 28 ++++++- bundle/permissions/utils.go | 2 +- 3 files changed, 87 insertions(+), 69 deletions(-) diff --git a/bundle/permissions/mutator.go b/bundle/permissions/mutator.go index bc1392d93..c4b970857 100644 --- a/bundle/permissions/mutator.go +++ b/bundle/permissions/mutator.go @@ -7,13 +7,18 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" ) const CAN_MANAGE = "CAN_MANAGE" const CAN_VIEW = "CAN_VIEW" const CAN_RUN = "CAN_RUN" +var unsupportedResources = []string{"clusters", "volumes", "schemas", "quality_monitors", "registered_models"} + var allowedLevels = []string{CAN_MANAGE, CAN_VIEW, CAN_RUN} var levelsMap = map[string](map[string]string){ "jobs": { @@ -26,11 +31,11 @@ var levelsMap = map[string](map[string]string){ CAN_VIEW: "CAN_VIEW", CAN_RUN: "CAN_RUN", }, - "mlflow_experiments": { + "experiments": { CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, - "mlflow_models": { + "models": { CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, @@ -57,11 +62,58 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di return diag.FromErr(err) } - applyForJobs(ctx, b) - applyForPipelines(ctx, b) - applyForMlModels(ctx, b) - applyForMlExperiments(ctx, b) - applyForModelServiceEndpoints(ctx, b) + patterns := make(map[string]dyn.Pattern, 0) + for key := range levelsMap { + patterns[key] = dyn.NewPattern( + dyn.Key("resources"), + dyn.Key(key), + dyn.AnyKey(), + ) + } + + err = b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + for key, pattern := range patterns { + v, err = dyn.MapByPattern(v, pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + var permissions []resources.Permission + pv, err := dyn.Get(v, "permissions") + + // If the permissions field is not found, we set to an empty array + if err != nil { + pv = dyn.V([]dyn.Value{}) + } + + err = convert.ToTyped(&permissions, pv) + if err != nil { + return dyn.InvalidValue, fmt.Errorf("failed to convert permissions: %w", err) + } + + permissions = append(permissions, convertPermissions( + ctx, + b.Config.Permissions, + permissions, + key, + levelsMap[key], + )...) + + pv, err = convert.FromTyped(permissions, dyn.NilValue) + if err != nil { + return dyn.InvalidValue, fmt.Errorf("failed to convert permissions: %w", err) + } + + return dyn.Set(v, "permissions", pv) + }) + + if err != nil { + return dyn.InvalidValue, err + } + } + + return v, nil + }) + + if err != nil { + return diag.FromErr(err) + } return nil } @@ -76,66 +128,6 @@ func validate(b *bundle.Bundle) error { return nil } -func applyForJobs(ctx context.Context, b *bundle.Bundle) { - for key, job := range b.Config.Resources.Jobs { - job.Permissions = append(job.Permissions, convert( - ctx, - b.Config.Permissions, - job.Permissions, - key, - levelsMap["jobs"], - )...) - } -} - -func applyForPipelines(ctx context.Context, b *bundle.Bundle) { - for key, pipeline := range b.Config.Resources.Pipelines { - pipeline.Permissions = append(pipeline.Permissions, convert( - ctx, - b.Config.Permissions, - pipeline.Permissions, - key, - levelsMap["pipelines"], - )...) - } -} - -func applyForMlExperiments(ctx context.Context, b *bundle.Bundle) { - for key, experiment := range b.Config.Resources.Experiments { - experiment.Permissions = append(experiment.Permissions, convert( - ctx, - b.Config.Permissions, - experiment.Permissions, - key, - levelsMap["mlflow_experiments"], - )...) - } -} - -func applyForMlModels(ctx context.Context, b *bundle.Bundle) { - for key, model := range b.Config.Resources.Models { - model.Permissions = append(model.Permissions, convert( - ctx, - b.Config.Permissions, - model.Permissions, - key, - levelsMap["mlflow_models"], - )...) - } -} - -func applyForModelServiceEndpoints(ctx context.Context, b *bundle.Bundle) { - for key, model := range b.Config.Resources.ModelServingEndpoints { - model.Permissions = append(model.Permissions, convert( - ctx, - b.Config.Permissions, - model.Permissions, - key, - levelsMap["model_serving_endpoints"], - )...) - } -} - func (m *bundlePermissions) Name() string { return "ApplyBundlePermissions" } diff --git a/bundle/permissions/mutator_test.go b/bundle/permissions/mutator_test.go index 1a177d902..78703e90f 100644 --- a/bundle/permissions/mutator_test.go +++ b/bundle/permissions/mutator_test.go @@ -2,12 +2,15 @@ package permissions import ( "context" + "fmt" + "slices" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -51,6 +54,10 @@ func TestApplyBundlePermissions(t *testing.T) { "endpoint_1": {}, "endpoint_2": {}, }, + Dashboards: map[string]*resources.Dashboard{ + "dashboard_1": {}, + "dashboard_2": {}, + }, }, }, } @@ -103,6 +110,10 @@ func TestApplyBundlePermissions(t *testing.T) { require.Contains(t, b.Config.Resources.ModelServingEndpoints["endpoint_2"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "TestUser"}) require.Contains(t, b.Config.Resources.ModelServingEndpoints["endpoint_2"].Permissions, resources.Permission{Level: "CAN_VIEW", GroupName: "TestGroup"}) require.Contains(t, b.Config.Resources.ModelServingEndpoints["endpoint_2"].Permissions, resources.Permission{Level: "CAN_QUERY", ServicePrincipalName: "TestServicePrincipal"}) + + require.Len(t, b.Config.Resources.Dashboards["dashboard_1"].Permissions, 2) + require.Contains(t, b.Config.Resources.Dashboards["dashboard_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "TestUser"}) + require.Contains(t, b.Config.Resources.Dashboards["dashboard_1"].Permissions, resources.Permission{Level: "CAN_READ", GroupName: "TestGroup"}) } func TestWarningOnOverlapPermission(t *testing.T) { @@ -146,5 +157,20 @@ func TestWarningOnOverlapPermission(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "CAN_VIEW", UserName: "TestUser2"}) require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "TestUser"}) require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "CAN_VIEW", GroupName: "TestGroup"}) - +} + +func TestAllResourcesExplicitlyDefinedForPermissionsSupport(t *testing.T) { + r := config.Resources{} + + for _, resource := range unsupportedResources { + _, ok := levelsMap[resource] + assert.False(t, ok, fmt.Sprintf("Resource %s is defined in both levelsMap and unsupportedResources", resource)) + } + + for _, resource := range r.AllResources() { + _, ok := levelsMap[resource.Description.PluralName] + if !slices.Contains(unsupportedResources, resource.Description.PluralName) && !ok { + assert.Fail(t, fmt.Sprintf("Resource %s is not explicitly defined in levelsMap or unsupportedResources", resource.Description.PluralName)) + } + } } diff --git a/bundle/permissions/utils.go b/bundle/permissions/utils.go index 9072cd252..cf16ea9b2 100644 --- a/bundle/permissions/utils.go +++ b/bundle/permissions/utils.go @@ -7,7 +7,7 @@ import ( "github.com/databricks/cli/libs/diag" ) -func convert( +func convertPermissions( ctx context.Context, bundlePermissions []resources.Permission, resourcePermissions []resources.Permission,