Warn and record when state_path folder permissions exceed static permissions block.#5439
Warn and record when state_path folder permissions exceed static permissions block.#5439shreyas-goenka wants to merge 1 commit into
permissions block.#5439Conversation
60fba4e to
cc47397
Compare
e7e9e83 to
7c783aa
Compare
|
Commit: 313ab31
23 interesting tests: 15 SKIP, 7 KNOWN, 1 flaky
Top 28 slowest tests (at least 2 minutes):
|
cc47397 to
b2c7271
Compare
7c783aa to
5e587f0
Compare
b2c7271 to
c2aa303
Compare
5e587f0 to
2cd3ef3
Compare
c2aa303 to
b1d732b
Compare
2cd3ef3 to
07abec1
Compare
b1d732b to
9e2f26b
Compare
07abec1 to
26c3380
Compare
9e2f26b to
00c5b7b
Compare
26c3380 to
30172b5
Compare
00c5b7b to
addcb6a
Compare
30172b5 to
3646391
Compare
46c9c69 to
bf9f878
Compare
bf9f878 to
abe3dfb
Compare
| has_serverless_compute true | ||
| local.cache.attempt true | ||
| local.cache.miss true | ||
| permissions_section_is_set false |
There was a problem hiding this comment.
Because permissions are not set here, by definition state_path_acl_exceeds_permissions is true.
There was a problem hiding this comment.
Analysis needs to take this into account when looking at state_path_acl_exceeds_permissions.
Can we choose to not emit it when permissions are not set?
permissions block.
Approval status: pending
|
abe3dfb to
3c6a454
Compare
3c6a454 to
681e089
Compare
| has_serverless_compute true | ||
| local.cache.attempt true | ||
| local.cache.miss true | ||
| permissions_section_is_set false |
There was a problem hiding this comment.
Analysis needs to take this into account when looking at state_path_acl_exceeds_permissions.
Can we choose to not emit it when permissions are not set?
| - level: CAN_MANAGE, group_name: data-engineers | ||
|
|
||
| Add them to your bundle permissions or remove them from the folder. | ||
| See https://docs.databricks.com/dev-tools/bundles/permissions |
There was a problem hiding this comment.
It is odd to see the same warning twice here. Anything we can do about it?
There was a problem hiding this comment.
That is by design - we get two warnings, one for the root path and one for the state path (because they are different). The naming is a bit confusing because the target name is state_path as well.
| statePath := b.Config.Workspace.StatePath | ||
| for i, p := range bundlePaths { | ||
| if pathContains(p, statePath) && !libraries.IsWorkspaceSharedPath(p) { | ||
| b.Metrics.SetBoolValue(metrics.StatePathAclExceedsPermissions, len(results[i]) > 0) |
There was a problem hiding this comment.
This depends on the presence or absence of diags for a path? Seems brittle if so.
| * Set the default `data_security_mode` to `DATA_SECURITY_MODE_AUTO` in bundle templates ([#5452](https://github.com/databricks/cli/pull/5452)). | ||
| * Mark vector search index index_subtype as backend_default to prevent drift after deployment ([#5454](https://github.com/databricks/cli/pull/5454)). | ||
| * `bundle deployment migrate`: handle resources added to or removed from `databricks.yml` since the last Terraform deploy ([#5463](https://github.com/databricks/cli/pull/5463)). | ||
| * Warn during `bundle deploy` when a workspace folder used by the bundle grants broader permissions than the bundle's top-level `permissions` section declares, for example through permissions inherited from a parent folder ([#5439](https://github.com/databricks/cli/pull/5439)). |
There was a problem hiding this comment.
If I understand correctly, we already do this and this is not new. The only difference is that we record it.
There was a problem hiding this comment.
We only warn in the bundle validate command today, where we fetch the permissions on all folders. This is new because now we also warn on bundle deploy.
…he bundle's ValidateFolderPermissions already compares the live workspace ACL against the declared permissions, but it only runs during `bundle validate`. This brings the same check to `bundle deploy` without adding any API latency: ApplyWorkspaceRoot- Permissions already calls SetPermissions on each workspace path prefix (root_path and, when separate, state_path), and the response carries the resulting ACL. Reusing that response, we compare against the declared permissions. Because the Set replaces the folder's direct ACL with the declared set, any principal still showing higher access is inherited from a parent folder — the broader access that actually persists after deploy, which is the scope mismatch worth surfacing. Three telemetry signals are recorded in bool_values during deploy: - state_path_acl_exceeds_permissions: whether the folder holding the deployment state grants more access than the permissions section declares. True by definition when no permissions are declared; determined statically for /Workspace/Shared state folders (all users have read/write) and from the live SetPermissions response otherwise. - state_path_is_shared: state_path is under /Workspace/Shared. - permissions_section_is_set: the bundle declares top-level permissions. Covered by acceptance tests (no permissions / clean ACL / shared state path, and an inherited-ACL mismatch staged via a server override) instead of unit tests. Co-authored-by: Shreyas Goenka <shreyas.goenka@databricks.com>
681e089 to
e6521fb
Compare
Summary
Previously the "workspace folder has permissions not configured in bundle" warning was only emitted by
bundle validate. This emits it onbundle deployas well, and records related telemetry.No extra API latency: deploy already calls
SetPermissionson each workspace path prefix, and the response carries the folder's resulting ACL. We compare that against the declared permissions instead of issuing a separateGetPermissions. Since the Set replaces the direct ACL, any principal still showing higher access is inherited from a parent folder — broader access that persists after the deploy.Telemetry (
bool_values):state_path_acl_exceeds_permissions— the folder holding the deployment state grants more access than declared.state_path_is_shared,permissions_section_is_set