Skip to content

Commit 789bb75

Browse files
denikdenik
authored
direct: Limit server_side_default to regular fields (#4038)
## Why Otherwise it is also applied to key-value pairs which is not intended. This is related and fixes bug introduced in #4014 ## Tests Regression acceptance test that shows how old behaviour resulted in ignoring extra permissions in remote state on direct.
1 parent e82bea2 commit 789bb75

File tree

10 files changed

+326
-1
lines changed

10 files changed

+326
-1
lines changed

‎acceptance/bundle/resources/permissions/jobs/added_remotely/databricks.yml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
bundle:
2+
name: test-bundle
3+
4+
resources:
5+
jobs:
6+
job_with_permissions:
7+
name: job permissions added remotely
8+
tasks:
9+
- task_key: main
10+
notebook_task:
11+
notebook_path: /Workspace/Users/user@example.com/notebook
12+
source: WORKSPACE
13+
permissions:
14+
- level: CAN_VIEW
15+
user_name: viewer@example.com

‎acceptance/bundle/resources/permissions/jobs/added_remotely/out.plan.direct.json‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
{
2+
"plan": {
3+
"resources.jobs.job_with_permissions": {
4+
"action": "skip",
5+
"remote_state": {
6+
"created_time": [UNIX_TIME_MILLIS],
7+
"creator_user_name": "[USERNAME]",
8+
"job_id": [JOB_ID],
9+
"run_as_user_name": "[USERNAME]",
10+
"settings": {
11+
"deployment": {
12+
"kind": "BUNDLE",
13+
"metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"
14+
},
15+
"edit_mode": "UI_LOCKED",
16+
"email_notifications": {},
17+
"format": "MULTI_TASK",
18+
"max_concurrent_runs": 1,
19+
"name": "job permissions added remotely",
20+
"queue": {
21+
"enabled": true
22+
},
23+
"tasks": [
24+
{
25+
"notebook_task": {
26+
"notebook_path": "/Workspace/Users/user@example.com/notebook",
27+
"source": "WORKSPACE"
28+
},
29+
"task_key": "main"
30+
}
31+
],
32+
"timeout_seconds": 0,
33+
"webhook_notifications": {}
34+
}
35+
},
36+
"changes": {
37+
"remote": {
38+
"email_notifications": {
39+
"action": "skip",
40+
"reason": "server_side_default"
41+
},
42+
"timeout_seconds": {
43+
"action": "skip",
44+
"reason": "server_side_default"
45+
},
46+
"webhook_notifications": {
47+
"action": "skip",
48+
"reason": "server_side_default"
49+
}
50+
}
51+
}
52+
},
53+
"resources.jobs.job_with_permissions.permissions": {
54+
"depends_on": [
55+
{
56+
"node": "resources.jobs.job_with_permissions",
57+
"label": "${resources.jobs.job_with_permissions.id}"
58+
}
59+
],
60+
"action": "update",
61+
"new_state": {
62+
"value": {
63+
"object_id": "/jobs/[JOB_ID]",
64+
"permissions": [
65+
{
66+
"permission_level": "CAN_VIEW",
67+
"user_name": "viewer@example.com"
68+
},
69+
{
70+
"permission_level": "IS_OWNER",
71+
"user_name": "[USERNAME]"
72+
}
73+
]
74+
}
75+
},
76+
"remote_state": {
77+
"object_id": "/jobs/[JOB_ID]",
78+
"permissions": [
79+
{
80+
"permission_level": "IS_OWNER",
81+
"user_name": "[USERNAME]"
82+
},
83+
{
84+
"permission_level": "CAN_VIEW",
85+
"user_name": "viewer@example.com"
86+
},
87+
{
88+
"group_name": "admin-team",
89+
"permission_level": "CAN_MANAGE"
90+
}
91+
]
92+
},
93+
"changes": {
94+
"remote": {
95+
"permissions[group_name='admin-team']": {
96+
"action": "update"
97+
}
98+
}
99+
}
100+
}
101+
}
102+
}

‎acceptance/bundle/resources/permissions/jobs/added_remotely/out.plan.terraform.json‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"plan": {
3+
"resources.jobs.job_with_permissions": {
4+
"action": "skip"
5+
},
6+
"resources.jobs.job_with_permissions.permissions": {
7+
"action": "update"
8+
}
9+
}
10+
}

‎acceptance/bundle/resources/permissions/jobs/added_remotely/out.test.toml‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎acceptance/bundle/resources/permissions/jobs/added_remotely/output.txt‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
2+
>>> [CLI] bundle plan
3+
create jobs.job_with_permissions
4+
create jobs.job_with_permissions.permissions
5+
6+
Plan: 2 to add, 0 to change, 0 to delete, 0 unchanged
7+
8+
>>> [CLI] bundle deploy
9+
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
10+
Deploying resources...
11+
Updating deployment state...
12+
Deployment complete!
13+
14+
>>> [CLI] bundle plan
15+
Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
16+
17+
>>> [CLI] permissions get jobs [JOB_ID]
18+
{
19+
"access_control_list": [
20+
{
21+
"all_permissions": [
22+
{
23+
"inherited":false,
24+
"permission_level":"CAN_VIEW"
25+
}
26+
],
27+
"display_name":"viewer@example.com",
28+
"user_name":"viewer@example.com"
29+
},
30+
{
31+
"all_permissions": [
32+
{
33+
"inherited":false,
34+
"permission_level":"IS_OWNER"
35+
}
36+
],
37+
"display_name":"[USERNAME]",
38+
"user_name":"[USERNAME]"
39+
},
40+
{
41+
"all_permissions": [
42+
{
43+
"inherited":true,
44+
"inherited_from_object": [
45+
"/jobs/"
46+
],
47+
"permission_level":"CAN_MANAGE"
48+
}
49+
],
50+
"group_name":"admins"
51+
}
52+
],
53+
"object_id":"/jobs/[JOB_ID]",
54+
"object_type":"job"
55+
}
56+
57+
=== Add permissions out of band
58+
>>> [CLI] permissions set jobs [JOB_ID] --json @remote_add.json
59+
{
60+
"access_control_list": [
61+
{
62+
"all_permissions": [
63+
{
64+
"inherited":false,
65+
"permission_level":"IS_OWNER"
66+
}
67+
],
68+
"display_name":"[USERNAME]",
69+
"user_name":"[USERNAME]"
70+
},
71+
{
72+
"all_permissions": [
73+
{
74+
"inherited":false,
75+
"permission_level":"CAN_VIEW"
76+
}
77+
],
78+
"display_name":"viewer@example.com",
79+
"user_name":"viewer@example.com"
80+
},
81+
{
82+
"all_permissions": [
83+
{
84+
"inherited":false,
85+
"permission_level":"CAN_MANAGE"
86+
}
87+
],
88+
"group_name":"admin-team"
89+
},
90+
{
91+
"all_permissions": [
92+
{
93+
"inherited":true,
94+
"inherited_from_object": [
95+
"/jobs/"
96+
],
97+
"permission_level":"CAN_MANAGE"
98+
}
99+
],
100+
"group_name":"admins"
101+
}
102+
],
103+
"object_id":"/jobs/[JOB_ID]",
104+
"object_type":"job"
105+
}
106+
107+
>>> [CLI] bundle plan
108+
update jobs.job_with_permissions.permissions
109+
110+
Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged
111+
112+
>>> [CLI] bundle plan -o json
113+
114+
>>> [CLI] bundle deploy
115+
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
116+
Deploying resources...
117+
Updating deployment state...
118+
Deployment complete!
119+
120+
>>> [CLI] bundle plan
121+
Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
122+
123+
>>> [CLI] permissions get jobs [JOB_ID]
124+
{
125+
"access_control_list": [
126+
{
127+
"all_permissions": [
128+
{
129+
"inherited":false,
130+
"permission_level":"CAN_VIEW"
131+
}
132+
],
133+
"display_name":"viewer@example.com",
134+
"user_name":"viewer@example.com"
135+
},
136+
{
137+
"all_permissions": [
138+
{
139+
"inherited":false,
140+
"permission_level":"IS_OWNER"
141+
}
142+
],
143+
"display_name":"[USERNAME]",
144+
"user_name":"[USERNAME]"
145+
},
146+
{
147+
"all_permissions": [
148+
{
149+
"inherited":true,
150+
"inherited_from_object": [
151+
"/jobs/"
152+
],
153+
"permission_level":"CAN_MANAGE"
154+
}
155+
],
156+
"group_name":"admins"
157+
}
158+
],
159+
"object_id":"/jobs/[JOB_ID]",
160+
"object_type":"job"
161+
}

‎acceptance/bundle/resources/permissions/jobs/added_remotely/remote_add.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"access_control_list": [
3+
{"permission_level": "IS_OWNER", "user_name": "tester@databricks.com"},
4+
{"permission_level": "CAN_VIEW", "user_name": "viewer@example.com"},
5+
{"permission_level": "CAN_MANAGE", "group_name": "admin-team"}
6+
]
7+
}

‎acceptance/bundle/resources/permissions/jobs/added_remotely/script‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
trace $CLI bundle plan
2+
trace $CLI bundle deploy
3+
trace $CLI bundle plan
4+
5+
job_id="$(read_id.py jobs job_with_permissions)"
6+
echo "$job_id:JOB_ID" >> ACC_REPLS
7+
8+
trace $CLI permissions get jobs "$job_id"
9+
10+
title "Add permissions out of band"
11+
trace $CLI permissions set jobs "$job_id" --json @remote_add.json
12+
13+
trace $CLI bundle plan
14+
trace $CLI bundle plan -o json > out.plan.$DATABRICKS_BUNDLE_ENGINE.json
15+
trace $CLI bundle deploy
16+
trace $CLI bundle plan
17+
18+
trace $CLI permissions get jobs "$job_id"

‎acceptance/bundle/resources/permissions/jobs/added_remotely/test.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
RecordRequests = false

‎bundle/direct/bundle_plan.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,10 +260,12 @@ func interpretOldStateVsRemoteState(ctx context.Context, adapter *dresources.Ada
260260
m := make(map[string]deployplan.Trigger)
261261

262262
for _, ch := range diff {
263-
if ch.Old == nil {
263+
if ch.Old == nil && ch.Path.IsStringKey() {
264264
// The field was not set by us, but comes from the remote state.
265265
// This could either be server-side default or a policy.
266266
// In any case, this is not a change we should react to.
267+
// Note, we only consider StringKeys here, because indexes and key-value pairs refer to slices and we want to react to new element in slices.
268+
// Note, IsStringKey is also too broad - it currently covers struct fields and map keys, we don't want to include map keys here.
267269
m[ch.Path.String()] = deployplan.Trigger{
268270
Action: deployplan.ActionTypeSkipString,
269271
Reason: "server_side_default",

‎libs/structs/structpath/path.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ func (p *PathNode) KeyValue() (key, value string, ok bool) {
7373
return "", "", false
7474
}
7575

76+
func (p *PathNode) IsStringKey() bool {
77+
return p != nil && p.index == tagStringKey
78+
}
79+
7680
// StringKey returns either Field() or MapKey() if either is available
7781
func (p *PathNode) StringKey() (string, bool) {
7882
if p == nil {

0 commit comments

Comments
 (0)