Bootstrapping PSA Roles for Healthcare #7376

trodge · 2023-03-01T01:26:56Z

fixes hashicorp/terraform-provider-google#13721

I can add bootstrap calls for the tests in hashicorp/terraform-provider-google#12908 if desired.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

modular-magician · 2023-03-01T01:43:11Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 80 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 4 files changed, 89 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

modular-magician · 2023-03-01T02:27:18Z

Tests analytics

Total tests: 2480
Passed tests 2223
Skipped tests: 255
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccSqlDatabaseInstance_Timezone

modular-magician · 2023-03-01T03:19:49Z

Tests passed during RECORDING mode:
TestAccSqlDatabaseInstance_Timezone[Debug log]

Tests failed during RECORDING mode:
TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

roaks3

Sorry for the fly-by comments, but I saw this pop up and had a couple of thoughts I wanted to share. This will help us with a lot of issues we see in our nightly tests, so thanks for implementing it!

roaks3 · 2023-03-01T16:04:00Z

mmv1/products/healthcare/terraform.yaml

@@ -59,6 +59,24 @@ overrides: !ruby/object:Overrides::ResourceOverrides
          fhir_store_name: "example-fhir-store"
          pubsub_topic: "fhir-notifications"
          bq_dataset_name: "bq_example_dataset"
+        test_vars_overrides:
+          psaRoles: 'BootstrapPSARoles(t,


I think we will end up needing this in quite a few places, and I'm a little concerned that this format will trip up other contributors. Is there a way would could simplify?

Two ideas that could shorten the call: 1) we probably shouldn't allow this for any other project, so we could omit projectID, and 2) while the *Options pattern is nice, I don't see us use it very often, and the more verbose call is less ideal in yaml

Thanks for the review!

I've tried to make the calls as terse as possible to fit in the yaml.

roaks3 · 2023-03-01T16:05:06Z

mmv1/third_party/terraform/utils/bootstrap_utils_test.go

+	}
+
+	// Map each role to whether it was already in the policy
+	rolesFound := make(map[string]bool, len(opts.Roles))


Did you consider some of the iam utility functions we have available, like mergeBindings, and compareBindings to detect changes? While probably slower technically, those functions have unit tests that might make them a bit safer. This bootstrap function will be modifying our root project policy, so any unexpected behavior could have wider impact.

I did not realize we had those. I've refactored to make use of them.

modular-magician · 2023-03-01T17:23:38Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 100 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 5 files changed, 109 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

modular-magician · 2023-03-01T17:24:58Z

Tests analytics

Total tests: 0
Passed tests 0
Skipped tests: 0
Failed tests: 0

Errors occurred during REPLAYING mode. Please fix them to complete your PR
View the build log

zli82016 · 2023-03-01T17:41:05Z

Hello, @trodge , you may need to rebase the main branch to make the PR pass the checks.

modular-magician · 2023-03-01T18:18:38Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 77 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 5 files changed, 79 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

modular-magician · 2023-03-01T18:37:38Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 94 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 5 files changed, 95 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

modular-magician · 2023-03-01T18:38:57Z

Tests analytics

Total tests: 0
Passed tests 0
Skipped tests: 0
Failed tests: 0

Errors occurred during REPLAYING mode. Please fix them to complete your PR
View the build log

modular-magician · 2023-03-01T19:06:07Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 94 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 5 files changed, 95 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

modular-magician · 2023-03-01T19:53:14Z

Tests analytics

Total tests: 2482
Passed tests 2223
Skipped tests: 255
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComposerEnvironment_withEncryptionConfigComposer1|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccDataSourceGoogleServiceAccountJwt

modular-magician · 2023-03-01T21:12:44Z

Tests passed during RECORDING mode:
TestAccDataSourceGoogleServiceAccountJwt[Debug log]

Tests failed during RECORDING mode:
TestAccComposerEnvironment_withEncryptionConfigComposer1[Error message] [Debug log]
TestAccComposerEnvironment_withEncryptionConfigComposer2[Error message] [Debug log]
TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

roaks3 · 2023-03-01T22:35:07Z

mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb

@@ -960,6 +962,21 @@ func TestAccComposerEnvironment_fixPyPiPackages(t *testing.T) {
 	})
 }

+// This bootstraps the IAM roles needed for the service agents when using encryption.
+func grantEncrypterDecrypterRoleToServiceAgents(t *testing.T) {


It might make sense to move this piece of work out into a separate PR for 2 reasons (but defer to you if it makes sense to keep it):

In case we experience any sort of issues with project permissions when these bootstrap functions are called in the nightly tests, it would make for a smaller rollback and smaller impact.

Until we remove all of the places in our examples/tests where these bindings are created, I think they will still be subject to being occasionally deleted (perhaps these were already removed though).

modular-magician · 2023-03-01T23:55:26Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 77 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 4 files changed, 78 insertions(+), 38 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 19 deletions(-))

roaks3

All of my comments have been resolved

modular-magician · 2023-03-02T00:40:20Z

Tests analytics

Total tests: 2483
Passed tests 2226
Skipped tests: 255
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample

modular-magician · 2023-03-02T01:54:06Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample[Debug log]

All tests passed
View the build log or the debug log for each test

* Add a new bootstrap function for assigning roles to service accounts. * Call the bootstrap function * Call the bootstrap function * Use an options struct, remove iam members * Bootstrap roles in the dicom store test * Grant roles in composer environment tests * Refactor bootstrap function * Delete unused options struct * Fix comments * Improve the interface * Remove extra newline * Fix variable names * Move composer environment test changes to separate PR

trodge marked this pull request as ready for review March 1, 2023 04:42

trodge requested a review from a team as a code owner March 1, 2023 04:42

trodge requested review from zli82016 and removed request for a team March 1, 2023 04:42

roaks3 reviewed Mar 1, 2023

View reviewed changes

trodge added 11 commits March 1, 2023 18:21

Add a new bootstrap function for assigning roles to service accounts.

b61427f

Call the bootstrap function

8570164

Call the bootstrap function

d808930

Use an options struct, remove iam members

6fb30bc

Bootstrap roles in the dicom store test

d8c8be4

Grant roles in composer environment tests

533730a

Refactor bootstrap function

af7b616

Delete unused options struct

5443d95

Fix comments

909467e

Improve the interface

07e3cc1

Remove extra newline

26a27a0

trodge force-pushed the healthcare-fhir-vs-dicom branch from e7e10c3 to 26a27a0 Compare March 1, 2023 18:22

Fix variable names

7557c29

roaks3 reviewed Mar 1, 2023

View reviewed changes

Move composer environment test changes to separate PR

6306b83

roaks3 approved these changes Mar 2, 2023

View reviewed changes

trodge merged commit d22c9bd into GoogleCloudPlatform:main Mar 2, 2023

trodge deleted the healthcare-fhir-vs-dicom branch March 6, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrapping PSA Roles for Healthcare #7376

Bootstrapping PSA Roles for Healthcare #7376

trodge commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

roaks3 left a comment

roaks3 Mar 1, 2023

trodge Mar 1, 2023

roaks3 Mar 1, 2023

trodge Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

zli82016 commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

modular-magician commented Mar 1, 2023

roaks3 Mar 1, 2023

trodge Mar 1, 2023

modular-magician commented Mar 1, 2023

roaks3 left a comment

modular-magician commented Mar 2, 2023

modular-magician commented Mar 2, 2023

Bootstrapping PSA Roles for Healthcare #7376

Bootstrapping PSA Roles for Healthcare #7376

Conversation

trodge commented Mar 1, 2023

modular-magician commented Mar 1, 2023

Diff report

modular-magician commented Mar 1, 2023

Tests analytics

Action taken

modular-magician commented Mar 1, 2023

roaks3 left a comment

Choose a reason for hiding this comment

roaks3 Mar 1, 2023

Choose a reason for hiding this comment

trodge Mar 1, 2023

Choose a reason for hiding this comment

roaks3 Mar 1, 2023

Choose a reason for hiding this comment

trodge Mar 1, 2023

Choose a reason for hiding this comment

modular-magician commented Mar 1, 2023

Diff report

modular-magician commented Mar 1, 2023

Tests analytics

zli82016 commented Mar 1, 2023

modular-magician commented Mar 1, 2023

Diff report

modular-magician commented Mar 1, 2023

Diff report

modular-magician commented Mar 1, 2023

Tests analytics

modular-magician commented Mar 1, 2023

Diff report

modular-magician commented Mar 1, 2023

Tests analytics

Action taken

modular-magician commented Mar 1, 2023

roaks3 Mar 1, 2023

Choose a reason for hiding this comment

trodge Mar 1, 2023

Choose a reason for hiding this comment

modular-magician commented Mar 1, 2023

Diff report

roaks3 left a comment

Choose a reason for hiding this comment

modular-magician commented Mar 2, 2023

Tests analytics

Action taken

modular-magician commented Mar 2, 2023