Repository navigation
[DECO-28500] Sync a remote run_as on jobs whose config leaves it unset - #7494
petrapostelnicu-db wants to merge 3 commits into
Conversation
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 3 tests: regression=2 unreleased=0 coverage=1 failing=0
TestAccept/bundle/config-remote-sync/job_run_as/DMS=: branch ✅ | main (e3aeacc) ❌ | latest (v1.26.0) ❌main (e3aeacc): latest (v1.26.0): TestAccept/bundle/config-remote-sync/job_run_as/DMS=true: branch ✅ | main (e3aeacc) ❌ | latest (v1.26.0) ❌main (e3aeacc): latest (v1.26.0): |
❌ Integration test reportCommit: 9d7317e
48 interesting tests: 47 FAIL, 1 RECOVERED
Top 3 slowest tests (at least 2 minutes):
|
c83ff1d to
cc6cc1c
Compare
f712989 to
759b252
Compare
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review from review-factory: 3 independent reviewer models (Claude Opus 5.5, Claude Fable 5.2, Claude Sonnet 5.5) with cross-verification. Only findings that reviewers agreed on are posted.
Syncs a remote job run_as into configs lacking one; reviewers confirmed a removed-and-redeployed run_as is written back, plus a redundant whole-object replace.
Findings: 1 major, 1 minor.
| return true | ||
| } | ||
| // The config has no run_as. Skip one unchanged since the last deploy. | ||
| if structdiff.IsEqual(cd.Old, cd.Remote) { |
There was a problem hiding this comment.
major: A run_as removed from config and deployed is written back to YAML by the next sync
The structdiff.IsEqual(cd.Old, cd.Remote) check at runas.go:41 is meant to skip a remote run_as that is unchanged since the last deploy. But cd.Old is the saved state, and the direct engine saves the config-derived state (saveState in bundle/direct/apply.go), not what the server holds. The check can therefore only match while the saved state still has a run_as that the config no longer has. That is a local removal that has not been deployed yet.
The Jobs API keeps the previous run_as when jobs/reset omits it. acceptance/bundle/run_as/job_default (Cloud = true) records this as Badness ("run_as is still set even though it's not in bundle and not in reset request"), and libs/testserver/jobs.go:211-218 mirrors it.
Scenario: deploy a job with run_as: {user_name: U}, where U is not the deployer (or a job that inherits a bundle-level run_as). Delete run_as from the YAML and deploy. The saved state now has no run_as, but the remote still has U. On the next bundle config-remote-sync:
- the plan entry at
run_asisOld=nil, New=nil, Remote=U, classifiedbackend_default(bundle/direct/bundle_plan.goshouldSkipBackendDefault;bundle planreports the job unchanged); keepSkippedRunAsChangenow keeps the whole-object path;IsEqual(nil, U)is false, andisCreatorRunAsis false unless U is the job's creator.
The result is run_as: add, and --save writes back the run_as the user just removed. This repeats on every run, because nothing in state records U. Removing a bundle-level run_as (often a service principal that is not the deployer) hits every job that inherited it. Before this PR, the resources.jobs.*.run_as: alwaysSkip entry in defaults.go stopped this.
The PR description lists only ownership transfer as a known gap, and says a run_as unchanged since the last deploy is not synced. Neither the unit tests nor the acceptance tests cover the Old == Remote branch end to end. The terraform_state case reaches the creator check instead, because migrated state is built from the config and has no run_as.
Please decide whether writing the identity back is intended, since the remote job really does run as that principal. Then either filter this case, or document it as a known limitation next to the ownership-transfer gap. Either way, add an acceptance case to config-remote-sync/job_run_as: deploy with run_as, remove it from the YAML, deploy again, then run sync. The fake server already reproduces the cloud behavior.
Agreed by 3 reviewer model(s).
| // skipJobRunAs reports whether a change to the run_as object as a whole must | ||
| // not be synced. The plan diffs a run_as set on both sides key by key, so a | ||
| // change to the whole object means one side has none. | ||
| func skipJobRunAs(path string, cd *deployplan.ChangeDesc, remoteState any) bool { |
There was a problem hiding this comment.
minor: skipJobRunAs never checks cd.New, so a config that already has run_as gets a redundant whole-object replace
The comments at runas.go:28-30 and :40 assume a whole-object run_as change means the config has none, but skipJobRunAs never checks cd.New. The plan also records a whole-object change at run_as when the saved state has no run_as and the config now has one. That happens when run_as was added locally (or inherited from a new bundle-level run_as) and not yet deployed. In that case the local diff gives Old=nil, New=<config value>, Remote=<remote value>. If the remote also has a run_as that differs, the remote diff runs key by key (run_as.user_name and so on), so the parent entry stays as an Update.
Before this PR the alwaysSkip entry for resources.jobs.*.run_as in defaults.go dropped this parent entry. Now, when the remote principal is not the creator, skipJobRunAs returns false. convertChangeDesc (diff.go:94) then emits run_as: replace with the whole remote object, next to the existing per-key run_as.<key> changes.
The PR's own flow can reach this: a sync adds run_as to the YAML, nothing is deployed, the run-as is changed again in the Jobs UI, and sync runs again. The final YAML is the same, because ResolveChanges applies deeper paths first (resolve.go:226-246). The cost is a duplicated change in the command output and telemetry, and a whole-node rewrite of the run_as block.
Suggested fix: return true from skipJobRunAs when cd.New != nil, since the per-key entries carry the change. Add a unit case to TestSkipJobRunAs with New and Remote both set (no current row has both).
Agreed by 3 reviewer model(s).
config-remote-sync skipped a job's run_as entirely, so a run-as change made in the Jobs UI never reached a config without run_as. Replace the blanket skip with a rule for the whole run_as object: sync it unless the remote has none, it is unchanged since the last deploy, or it names the job's creator. Jobs updated by the terraform engine carry an explicit run_as set to the job's identity, which the creator check filters out. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
config-remote-sync is a hidden command used only by DABs in the workspace; changes to it do not get release notes. Co-authored-by: Isaac <no-reply@databricks.com>
759b252 to
9d7317e
Compare
Sankalp-Mittal
left a comment
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.
| return true | ||
| } | ||
| // The config has no run_as. Skip one unchanged since the last deploy. | ||
| if structdiff.IsEqual(cd.Old, cd.Remote) { |
There was a problem hiding this comment.
Major: After run_as is removed from YAML and deployed, the job keeps it remotely and state has none, so every sync re-adds it. Intended? Either way, please add a job_run_as acceptance case.
| // skipJobRunAs reports whether a change to the run_as object as a whole must | ||
| // not be synced. The plan diffs a run_as set on both sides key by key, so a | ||
| // change to the whole object means one side has none. | ||
| func skipJobRunAs(path string, cd *deployplan.ChangeDesc, remoteState any) bool { |
There was a problem hiding this comment.
With run_as in config but not in state and a non-creator remote, this emits run_as: replace beside the per-key changes. Can it return true when cd.New != nil?
Stacked on #7493.
Changes
resources.jobs.*.run_asskip from config-remote-sync.run_asof a job whose config has none, unless:run_as(not synced before either);Why
run_as, which is most bundles.run_ason every update (provider code), and the server keeps it across later resets. Jobs deployed before the switch to the direct engine still carry it.Tests
config-remote-sync/job_run_as: norun_as+ service principal (synced), norun_as+ creator (skipped).config-remote-sync/terraform_state: a creator-pinnedrun_asafter migration is not reported.skipJobRunAs.This pull request and its description were written by Isaac.