Repository navigation
Allow resource permissions to override root and target defaults - #7291
lennartkats-db wants to merge 9 commits into
Conversation
✅ Integration test reportCommit: e5e4408
Top 3 slowest tests (at least 2 minutes):
|
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 4 tests: regression=4 unreleased=0 coverage=0 failing=0
TestAccept/bundle/validate/permissions_overlap/DMS=: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/validate/permissions_overlap/DMS=true: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/validate/permissions_target/DMS=: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) TestAccept/bundle/validate/permissions_target/DMS=true: branch ✅ | main (2699068) ❌ | latest (v1.26.0) ❌main (2699068): latest (v1.26.0): same output as main (2699068) |
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
| The permissions to apply to this resource. | ||
| "markdown_description": |- | ||
| A Sequence of permissions to apply to this resource, where each item grants a permission `level` to a single `user_name`, `group_name`, or `service_principal_name`. A principal cannot be set in both a resource's `permissions` and the top-level `permissions` mapping. | ||
| A Sequence of permissions to apply to this resource, where each item grants a permission `level` to a single `user_name`, `group_name`, or `service_principal_name`. For the same principal, resource permissions take precedence over top-level or target permissions. |
There was a problem hiding this comment.
cluster_policies isn't in levelsMap, so top-level permissions never reach it and there is nothing to take precedence over; secret_scopes is, but lacks the sentence. Can you drop it here and add it there?
There was a problem hiding this comment.
I removed the precedence sentence from cluster_policies and added it to secret_scopes, then regenerated the bundle schema. cluster_policies is skipped by ApplyBundlePermissions; secret_scopes receives the defaults. The schema and permission unit tests pass.
-- Isaac
There was a problem hiding this comment.
Fixed: cluster_policies.permissions in bundle/internal/schema/annotations.yml:528-534 no longer has the precedence sentence. secret_scopes.permissions (line 1976) now has it, and the regenerated bundle/schema/jsonschema.json matches.
🤖 review-factory reply (Claude Opus 5.5)
| @@ -0,0 +1 @@ | |||
| * Allow resource permissions to override bundle and target defaults without overlap warnings for all supported resources and principal types. ([#7291](https://github.com/databricks/cli/pull/7291)) | |||
There was a problem hiding this comment.
nit: Effective permissions don't change here; only the warning goes away. Could this say that bundle validate no longer warns when a resource and the top-level or target permissions name the same principal?
There was a problem hiding this comment.
I changed the release note to say bundle validate no longer warns when resource and top-level or target permissions name the same principal. Effective permissions are unchanged. The focused permission tests pass.
-- Isaac
There was a problem hiding this comment.
Fixed: .nextchanges/bundles/7291.md now says bundle validate no longer warns when resource permissions and top-level or target permissions name the same principal.
🤖 review-factory reply (Claude Opus 5.5)
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review (Claude Fable 5.2 + Claude Opus 5.5, cross-verified). Only points the reviewers agreed on are posted.
| } | ||
| } | ||
|
|
||
| func TestDuplicatePermissionsInOneSection(t *testing.T) { |
There was a problem hiding this comment.
nit: TestDuplicatePermissionsInOneSection never has a resource and a default naming the same principal, so it never reaches the isPermissionOverlap branch. Is it left over from 7cff8d1, and can it go?
There was a problem hiding this comment.
I removed TestDuplicatePermissionsInOneSection and its unused import in e5e4408. It tested duplicates within one section, while the retained TestResourcePermissionsOverrideDefaultsWithoutWarning exercises the resource/default overlap. The focused Go package tests pass.
-- Isaac
There was a problem hiding this comment.
Fixed: e5e4408 deletes TestDuplicatePermissionsInOneSection and the now-unused iam import. The resource/default overlap is still covered by TestResourcePermissionsOverrideDefaultsWithoutWarning (apply_bundle_permissions_test.go:227).
🤖 review-factory reply (Claude Opus 5.5)
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review (Claude Opus 5.5 + Claude Fable 5.2, cross-verified). Only points the reviewers agreed on are posted.
No issues found.
Changes
Fixes #7032
Remove overlap warnings when resource permissions and bundle or target permissions name the same principal. This lets the layered configuration in the issue pass
bundle validate --strictwhile retaining the resource permission. The rule covers supported resources, users, groups, and service principals, including resolved complex variables. Effective resource permissions are unchanged.Correct the cluster policy and secret scope schema descriptions to match which resources inherit bundle or target permissions.
Why
In #7032, a group has
CAN_VIEWon a target andCAN_MANAGE_RUNon a job. Validation kept the job permission but emitted a warning, which made strict validation fail. The same resource-over-default rule applies to bundle-level permissions.Tests
./task generate-schemapassed after the documentation correction.go test ./bundle/config/mutator/resourcemutator ./bundle/internal/schema ./bundle/config -count=1passed with Go 1.27.4.main, confirming that the warning regression is detected for both DMS settings.403 module is blockedforgithub.com/BurntSushi/[email protected].