Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds variable for members policy #75

Merged
merged 6 commits into from
Aug 11, 2021
Merged

Adds variable for members policy #75

merged 6 commits into from
Aug 11, 2021

Conversation

amandakarina
Copy link
Collaborator

@amandakarina amandakarina commented Aug 2, 2021

Fixes #47

  • Tests pass
  • Appropriate changes to README are included in PR

@erlanderlo erlanderlo requested a review from prashantkul August 3, 2021 01:59
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amandakarina

modules/data_warehouse_taxonomy/main.tf Show resolved Hide resolved
@@ -101,34 +126,37 @@ resource "google_data_catalog_policy_tag" "ssn_child_policy_tag" {
}

resource "google_data_catalog_policy_tag_iam_member" "private_sa_name" {
count = length(local.private_accounts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use for_each

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and throughout

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bharathkkb
If I use for_each here, I got the following error:

Error: Invalid for_each argument
Step #2 - "converge-bigquery-sensitive-data":        
Step #2 - "converge-bigquery-sensitive-data":          on ../../../modules/data_warehouse_taxonomy/main.tf line 150, in resource "google_data_catalog_policy_tag_iam_member" "confidential_sa_ssn":
Step #2 - "converge-bigquery-sensitive-data":         150:   for_each   = toset(local.confidential_accounts)
Step #2 - "converge-bigquery-sensitive-data":        
Step #2 - "converge-bigquery-sensitive-data":        The "for_each" value depends on resource attributes that cannot be determined
Step #2 - "converge-bigquery-sensitive-data":        until apply, so Terraform cannot predict how many instances will be created.
Step #2 - "converge-bigquery-sensitive-data":        To work around this, use the -target argument to first apply only the
Step #2 - "converge-bigquery-sensitive-data":        resources that the for_each depends on.

Because I'm creating an array that depends of a decision to create the service account or use an array.

 private_accounts      = length(var.confidential_access_members) == 0 ? ["serviceAccount:${module.service_accounts.emails["terraform-private-sa"]}"] : var.private_access_members
 confidential_accounts = length(var.private_access_members) == 0 ? ["serviceAccount:${module.service_accounts.emails["terraform-confidential-sa"]}"] : var.confidential_access_members

At this point, the terraform can only knows the size of the array, but can reach the value that for_each access.

Do you have any idea of how we can solve this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amandakarina Could we use something like private_accounts = length(var.private_access_members) == 0 ? {"terraform-private-sa"="serviceAccount:${module.service_accounts.emails["terraform-private-sa"]}"} : {for m in var.private_access_members: m=>m} to get known keys with unknown values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bharathkkb sounds pretty good!

modules/data_warehouse_taxonomy/main.tf Outdated Show resolved Hide resolved
"roles/bigquery.dataViewer",
"roles/datacatalog.viewer",
]
create_confidential_sa = length(var.confidential_access_members) == 0 ? ["terraform-confidential-sa"] : []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erlanderlo wondering about this #47 (comment). Did we reach a conclusion?

Copy link
Member

@erlanderlo erlanderlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look in line with what we discussed. will be good after address other comments

@amandakarina amandakarina requested a review from bharathkkb August 6, 2021 17:04
@bharathkkb bharathkkb merged commit 1a7d394 into GoogleCloudPlatform:main Aug 11, 2021
@amandakarina amandakarina deleted the feat/adds-variable-for-sensitive-data-service-accounts branch September 20, 2021 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private and Confidential service accounts refactoring
3 participants