Fix secret scope permissions migration from Terraform to Direct engine #4866
Open
Fix secret scope permissions migration from Terraform to Direct engine #4866
Conversation
Collaborator
|
Commit: 2a198db
19 interesting tests: 10 SKIP, 7 KNOWN, 2 flaky
Top 20 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/fix-secrets-migration
branch
from
1f437ff to
9434908
Compare
denik
force-pushed
the
denik/fix-secrets-migration
branch
from
9434908 to
2a198db
Compare
denik
requested review from
andrewnester,
anton-107,
pietern,
shreyas-goenka and
simonfaltum
as code owners
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @shreyas-goenka, @simonfaltum Suggestions based on git history of 20 changed files (9 scored). See CODEOWNERS for path-specific ownership rules. |
During migration, Terraform's databricks_secret_acl resources are not tracked in the migration state. The direct engine manages secret scope permissions as a sub-resource (secret_scopes.*.permissions), so without a state entry, the post-migration plan shows a "create" action. Add state entries for secret_scopes.*.permissions after migration Apply to prevent phantom drift. Co-authored-by: Isaac
Add databricks_secret_acl to TerraformToGroupName mapping and handle it in parseResourcesState, similar to how databricks_permissions and databricks_grants are handled. Multiple ACL resources per scope map to a single .permissions entry with the scope name as ID. Co-authored-by: Isaac
…rcesState ParseResourcesState now creates .permissions entries for ALL secret scopes (not just those with databricks_secret_acl), so the post-Apply fixup in migrate.go is no longer needed. Co-authored-by: Isaac
Add a test config with a secret scope that has two explicit ACL permissions (READ for users, WRITE for account users). This exercises the multi-ACL migration path in parseResourcesState. Co-authored-by: Isaac
During migration, SecretScopeFixups wasn't running (it's part of PreDeployChecks which migration skips). For scopes without explicit ACLs, parseResourcesState synthesizes .permissions entries in the state, but the config didn't have them, causing a Delete→forced Update→no StateCache entry error chain. Fix: apply SecretScopeFixups(EngineDirect) before CalculatePlan so the config and state agree on .permissions entries. Also simplify the identical if/else branches in the safety net. Co-authored-by: Isaac
The 'account users' group doesn't exist on all test workspaces, causing cloud test failures. Co-authored-by: Isaac
The convertSecretAclResourceNameToKey function and its dedicated databricks_secret_acl branch in parseResourcesState are no longer needed — all secret scope permissions are now handled uniformly through the post-processing loop. Co-authored-by: Isaac
Add SecretScopeFixups(EngineDirect) before reading b.Config.Value() so the config includes .permissions entries for secret scopes. Without this, CalculatePlan sees .permissions in state but not in config and produces incorrect plan entries during YAML sync conversion. Co-authored-by: Isaac
…sState Co-authored-by: Isaac
Check silentlyUpdatedResources before logging unknown resource type warning, matching the pattern used in showplanfile.go. This prevents misleading "Unknown Terraform resource type: databricks_secret_acl" warnings during normal secret scope migrations. Co-authored-by: Isaac
Replace the single-entry map with a direct type check for databricks_secret_acl in both showplanfile.go and util.go. Co-authored-by: Isaac
Add "secret_acls" mapping for databricks_secret_acl instead of special-casing it in the unknown-type check. In parseResourcesState, secret_acls are skipped via the switch (post-processing creates .permissions entries). In populatePlan, secret ACL changes are mapped to resources.secret_scopes.<key>.permissions with GetHigherAction for merging multiple ACL changes per scope. Task: 017.md Co-authored-by: Isaac
Co-authored-by: Isaac
Secret ACL changes now appear as .permissions entries in the Terraform plan output, reflecting the new TerraformToGroupName mapping. Task: 017.md Co-authored-by: Isaac
Instead of a post-processing loop that adds .permissions for every secret scope, create the entry directly in the "secret_scopes" case. This keeps all resource types handled inside the switch. Task: 018.md
When multiple secret ACL changes for the same scope include both creates and deletes, report the net action as "update" instead of "delete". GetHigherAction picks the highest severity (delete > create), but mixed ACL changes represent a permissions update, not a deletion. Task: 019.md Co-authored-by: Isaac
The previous isCreateDeleteMix helper only handled the narrow case of separate Create and Delete actions. In practice, Terraform produces Recreate (delete+create pair) for ACLs with changed principals, mixed with Delete for removed principals. GetHigherAction(Recreate, Delete) returned Delete (severity 7>6), incorrectly reporting permissions as deleted rather than updated. Simplify the logic: for secret ACLs, any mix of different action types means permissions are being updated. Only same-action merges (e.g., all Recreate when scope is recreated) keep the original action. Task-review: /Users/denis.bilenko/work/prompts/features/fix-secrets-migration/021.SUMMARY.md Co-authored-by: Isaac
denik
force-pushed
the
denik/fix-secrets-migration
branch
from
a3675b6 to
b7abfb4
Compare
andrewnester
approved these changes
Apr 1, 2026
denik
force-pushed
the
denik/fix-secrets-migration
branch
from
6a02590 to
c8f3b0e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bundle deployment migratefor bundles with secret scopes to prevent phantom drift onsecret_scopes.*.permissionsafter migration from Terraform to Direct engine.databricks_secret_aclinParseResourcesState: multiple ACL resources per scope are mapped to a single.permissionsstate entry with the scope name as ID, similar to howdatabricks_permissionsanddatabricks_grantsare handled.Test plan