Detect run_as drift in job resource #2626

dvkuznietsov · 2023-08-29T19:37:46Z

Changes

run_as field in jobs is asymmetric: it's JobSettings.RunAs in create/update requests and RunAsUserName in read requests. As a result, when the job drifts, terraform can't detect the drift. To fix that, adjust the Read function in terraform to fill in the RunAs struct based on the RunAsUsername field.

Tests

Tested locally E2E

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

codecov-commenter · 2023-08-29T19:43:00Z

Codecov Report

Merging #2626 (b4ad4b0) into master (cedc1b4) will decrease coverage by 0.02%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #2626      +/-   ##
==========================================
- Coverage   86.77%   86.76%   -0.02%     
==========================================
  Files         146      146              
  Lines       12655    12663       +8     
==========================================
+ Hits        10982    10987       +5     
- Misses       1108     1110       +2     
- Partials      565      566       +1

Files Changed	Coverage
jobs/resource_job.go	`62.50%`

tanmay-db · 2023-08-30T13:03:07Z

Hi, can you mention how you tested this E2E locally?

tanmay-db · 2023-08-30T13:05:43Z

jobs/resource_job.go

+			job.Settings.RunAs = &JobRunAs{
+				UserName: job.RunAsUserName,
+			}
+		} else {
+			job.Settings.RunAs = &JobRunAs{
+				UserName: job.RunAsUserName,
+			}


Both the statements in if else is same?

sorry, didn't push the latest commit

dvkuznietsov · 2023-08-30T13:11:38Z

@tanmay-db

Hi, can you mention how you tested this E2E locally?

I've built the provider from sources, configured terraform to use the binary and ran terraform apply.
then I logged into the workspace and verified the job was created with correct run as
then I changed the run as of the job
lstly, I ran terraform plan. Terraform detected the drift

tanmay-db · 2023-08-30T13:15:53Z

I have triggered an integration test run. Please wait until it's finished before this can be merged.

tanmay-db · 2023-08-30T13:20:45Z

jobs/resource_job.go

+			}
+		} else {
+			job.Settings.RunAs = &JobRunAs{
+				ServicePrincipalName: job.RunAsUserName,


@dvkuznietsov Two questions:

Is it certain that if a job is not userEmail then it is service principal?

Is checking for @ for email enough?

Hi @nkvuong, do you know if there is some existing utility for checking valid email in terraform?

Is it certain that if a job is not userEmail then it is service principal?
Is checking for @ for email enough?

as of now, run_as_user_name is either an email or a UUID. String parsing is not ideal and that's why we moved on to run_as field in write path, but that's what we work with for now

and, to answer your question, I beleive that's enough

@dvkuznietsov is it too much to ask the Jobs API to use run_as for reads and writes rather than force clients to do this mapping themselves? Thinking RESTfully, when a user sets a field on a resource, they would expect to get that field as they set it when getting that resource back. However, here, Jobs may be breaking that pattern. WDYT?

@mgyucht the fields are not symmetric: run_as is an optional field that we have recently added and run_as_user_name is an old field that is almost always filled in.

The intention for this change is if users don't provide run_as, we don't need to catch drifts in run_as_user_name, but if they do provide it, we do need to detect drifts in run_as_user_name

Can you include run_as in the get response though? I guess what I don't understand is why it is a write-only field.

we can't. It would be a breaking change for customers that pipe GET responses to create/reset calls

Forgive me for asking, but why would that be a breaking change? Does the Jobs API not tolerate both of these parameters being set at the same time in a create/reset call?

mgyucht · 2023-08-31T15:30:30Z

jobs/resource_job_test.go

+						RunAs: &JobRunAs{
+							UserName: "user@mail.com",
+						},


I thought RunAs is not included in get requests?

Just to confirm @dvkuznietsov what was the conclusion on these comments? I am guessing these were discussed offline?

dvkuznietsov · 2023-09-05T12:35:26Z

@tanmay-db could you re-run integration tests please?

tanmay-db

LGTM, unrelated integration tests are failing and shouldn't be a blocker for this.

* Detect run_as drift in job resource * fix SP UUID drift * suppress_diff for run_as

alexott · 2023-10-05T07:23:15Z

Opened #2626 to fix it on specific installations

dvkuznietsov added 2 commits August 29, 2023 21:32

Detect run_as drift in job resource

b4ad4b0

fix SP UUID drift

694ee16

dvkuznietsov requested review from a team as code owners August 29, 2023 19:37

dvkuznietsov requested review from tanmay-db and removed request for a team August 29, 2023 19:37

tanmay-db reviewed Aug 30, 2023

View reviewed changes

mgyucht reviewed Aug 31, 2023

View reviewed changes

dvkuznietsov added 2 commits September 5, 2023 14:14

suppress_diff for run_as

0f173ba

Merge remote-tracking branch 'origin/master' into detect-run-as-drift

fe60039

dvkuznietsov requested review from tanmay-db and mgyucht September 5, 2023 12:35

tanmay-db approved these changes Sep 6, 2023

View reviewed changes

tanmay-db added this pull request to the merge queue Sep 6, 2023

Merged via the queue into databricks:master with commit 55b47c7 Sep 6, 2023

tanmay-db mentioned this pull request Sep 7, 2023

Release v1.25.0 #2659

Merged

5 tasks

nkvuong pushed a commit that referenced this pull request Oct 3, 2023

Detect run_as drift in job resource (#2626)

48932c1

* Detect run_as drift in job resource * fix SP UUID drift * suppress_diff for run_as

pietern mentioned this pull request Nov 21, 2023

Remove diff suppression for a job's run_as block #2943

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect run_as drift in job resource #2626

Detect run_as drift in job resource #2626

dvkuznietsov commented Aug 29, 2023

codecov-commenter commented Aug 29, 2023

tanmay-db commented Aug 30, 2023

tanmay-db Aug 30, 2023

dvkuznietsov Aug 30, 2023

dvkuznietsov commented Aug 30, 2023

tanmay-db commented Aug 30, 2023

tanmay-db Aug 30, 2023

dvkuznietsov Aug 30, 2023

dvkuznietsov Aug 30, 2023

mgyucht Aug 30, 2023

dvkuznietsov Aug 31, 2023 •

edited

Loading

mgyucht Aug 31, 2023

dvkuznietsov Aug 31, 2023

mgyucht Aug 31, 2023

mgyucht Aug 31, 2023

tanmay-db Sep 6, 2023

dvkuznietsov commented Sep 5, 2023

tanmay-db left a comment •

edited

Loading

alexott commented Oct 5, 2023

Detect run_as drift in job resource #2626

Detect run_as drift in job resource #2626

Conversation

dvkuznietsov commented Aug 29, 2023

Changes

Tests

codecov-commenter commented Aug 29, 2023

Codecov Report

tanmay-db commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvkuznietsov commented Aug 30, 2023

tanmay-db commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvkuznietsov Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvkuznietsov commented Sep 5, 2023

tanmay-db left a comment • edited Loading

Choose a reason for hiding this comment

alexott commented Oct 5, 2023

dvkuznietsov Aug 31, 2023 •

edited

Loading

tanmay-db left a comment •

edited

Loading