From 94b1a439ee5eba699de93e74e1e6fd2f7ac362b9 Mon Sep 17 00:00:00 2001 From: Julio Castillo Date: Wed, 31 Mar 2021 09:57:23 +0200 Subject: [PATCH 1/2] Fix IAM bindings for logging sinks - Move to non-authoritative bindings to allow multiple sinks to write to the same destination - Allow automatically adding logging bucket IAM bindings --- .gitignore | 1 + modules/folder/README.md | 2 +- modules/folder/main.tf | 21 +++++++++++++++------ modules/folder/outputs.tf | 2 +- modules/organization/README.md | 2 +- modules/organization/main.tf | 21 +++++++++++++++------ modules/organization/outputs.tf | 2 +- modules/project/README.md | 2 +- modules/project/main.tf | 25 +++++++++++++++++-------- modules/project/outputs.tf | 2 +- 10 files changed, 54 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index 5670b794..3fd3e820 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ credentials.json key.json terraform-ls.tf bundle.zip +.DS_Store diff --git a/modules/folder/README.md b/modules/folder/README.md index 586ad1e9..5bb7b4cd 100644 --- a/modules/folder/README.md +++ b/modules/folder/README.md @@ -186,5 +186,5 @@ module "folder2" { | folder | Folder resource. | | | id | Folder id. | | | name | Folder name. | | -| sink_writer_identities | None | | +| sink_writer_identities | Writer identities created for each sink | | diff --git a/modules/folder/main.tf b/modules/folder/main.tf index 6d1c632c..6816adf0 100644 --- a/modules/folder/main.tf +++ b/modules/folder/main.tf @@ -202,27 +202,36 @@ resource "google_logging_folder_sink" "sink" { } } -resource "google_storage_bucket_iam_binding" "gcs-sinks-binding" { +resource "google_storage_bucket_iam_member" "gcs-sinks-binding" { for_each = local.sink_bindings["gcs"] bucket = each.value.destination role = "roles/storage.objectCreator" - members = [google_logging_folder_sink.sink[each.key].writer_identity] + member = google_logging_folder_sink.sink[each.key].writer_identity } -resource "google_bigquery_dataset_iam_binding" "bq-sinks-binding" { +resource "google_bigquery_dataset_iam_member" "bq-sinks-binding" { for_each = local.sink_bindings["bigquery"] project = split("/", each.value.destination)[1] dataset_id = split("/", each.value.destination)[3] role = "roles/bigquery.dataEditor" - members = [google_logging_folder_sink.sink[each.key].writer_identity] + member = google_logging_folder_sink.sink[each.key].writer_identity } -resource "google_pubsub_topic_iam_binding" "pubsub-sinks-binding" { +resource "google_pubsub_topic_iam_member" "pubsub-sinks-binding" { for_each = local.sink_bindings["pubsub"] project = split("/", each.value.destination)[1] topic = split("/", each.value.destination)[3] role = "roles/pubsub.publisher" - members = [google_logging_folder_sink.sink[each.key].writer_identity] + member = google_logging_folder_sink.sink[each.key].writer_identity +} + +resource "google_project_iam_member" "bucket-sinks-binding" { + for_each = local.sink_bindings["logging"] + project = split("/", each.value.destination)[1] + role = "roles/logging.bucketWriter" + member = google_logging_folder_sink.sink[each.key].writer_identity + # TODO(jccb): use a condition to limit writer-identity only to this + # bucket } resource "google_logging_folder_exclusion" "logging-exclusion" { diff --git a/modules/folder/outputs.tf b/modules/folder/outputs.tf index ddcc2d3f..48edfcc7 100644 --- a/modules/folder/outputs.tf +++ b/modules/folder/outputs.tf @@ -51,7 +51,7 @@ output "firewall_policy_id" { } output "sink_writer_identities" { - description = "" + description = "Writer identities created for each sink." value = { for name, sink in google_logging_folder_sink.sink : name => sink.writer_identity } diff --git a/modules/organization/README.md b/modules/organization/README.md index f7b57f20..933ce8f4 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -163,5 +163,5 @@ module "org" { | firewall_policies | Map of firewall policy resources created in the organization. | | | firewall_policy_id | Map of firewall policy ids created in the organization. | | | organization_id | Organization id dependent on module resources. | | -| sink_writer_identities | None | | +| sink_writer_identities | Writer identities created for each sink | | diff --git a/modules/organization/main.tf b/modules/organization/main.tf index e6f7272a..9a16903c 100644 --- a/modules/organization/main.tf +++ b/modules/organization/main.tf @@ -286,27 +286,36 @@ resource "google_logging_organization_sink" "sink" { } } -resource "google_storage_bucket_iam_binding" "gcs-sinks-binding" { +resource "google_storage_bucket_iam_member" "gcs-sinks-binding" { for_each = local.sink_bindings["gcs"] bucket = each.value.destination role = "roles/storage.objectCreator" - members = [google_logging_organization_sink.sink[each.key].writer_identity] + member = google_logging_organization_sink.sink[each.key].writer_identity } -resource "google_bigquery_dataset_iam_binding" "bq-sinks-binding" { +resource "google_bigquery_dataset_iam_member" "bq-sinks-binding" { for_each = local.sink_bindings["bigquery"] project = split("/", each.value.destination)[1] dataset_id = split("/", each.value.destination)[3] role = "roles/bigquery.dataEditor" - members = [google_logging_organization_sink.sink[each.key].writer_identity] + member = google_logging_organization_sink.sink[each.key].writer_identity } -resource "google_pubsub_topic_iam_binding" "pubsub-sinks-binding" { +resource "google_pubsub_topic_iam_member" "pubsub-sinks-binding" { for_each = local.sink_bindings["pubsub"] project = split("/", each.value.destination)[1] topic = split("/", each.value.destination)[3] role = "roles/pubsub.publisher" - members = [google_logging_organization_sink.sink[each.key].writer_identity] + member = google_logging_organization_sink.sink[each.key].writer_identity +} + +resource "google_project_iam_member" "bucket-sinks-binding" { + for_each = local.sink_bindings["logging"] + project = split("/", each.value.destination)[1] + role = "roles/logging.bucketWriter" + member = google_logging_organization_sink.sink[each.key].writer_identity + # TODO(jccb): use a condition to limit writer-identity only to this + # bucket } resource "google_logging_organization_exclusion" "logging-exclusion" { diff --git a/modules/organization/outputs.tf b/modules/organization/outputs.tf index 4cbd1dff..1dd51ee8 100644 --- a/modules/organization/outputs.tf +++ b/modules/organization/outputs.tf @@ -45,7 +45,7 @@ output "firewall_policy_id" { } output "sink_writer_identities" { - description = "" + description = "Writer identities created for each sink." value = { for name, sink in google_logging_organization_sink.sink : name => sink.writer_identity } diff --git a/modules/project/README.md b/modules/project/README.md index e3691fa9..a640edf0 100644 --- a/modules/project/README.md +++ b/modules/project/README.md @@ -191,6 +191,6 @@ module "project-host" { | number | Project number. | | | project_id | Project id. | | | service_accounts | Product robot service accounts in project. | | -| sink_writer_identities | None | | +| sink_writer_identities | Writer identities created for each sink | | diff --git a/modules/project/main.tf b/modules/project/main.tf index 3a880a4d..dd9b0058 100644 --- a/modules/project/main.tf +++ b/modules/project/main.tf @@ -277,27 +277,36 @@ resource "google_logging_project_sink" "sink" { } } -resource "google_storage_bucket_iam_binding" "gcs-sinks-binding" { +resource "google_storage_bucket_iam_member" "gcs-sinks-binding" { for_each = local.sink_bindings["gcs"] bucket = each.value.destination role = "roles/storage.objectCreator" - members = [google_logging_project_sink.sink[each.key].writer_identity] + member = google_logging_project_sink.sink[each.key].writer_identity } -resource "google_bigquery_dataset_iam_binding" "bq-sinks-binding" { +resource "google_bigquery_dataset_iam_member" "bq-sinks-binding" { for_each = local.sink_bindings["bigquery"] project = split("/", each.value.destination)[1] dataset_id = split("/", each.value.destination)[3] role = "roles/bigquery.dataEditor" - members = [google_logging_project_sink.sink[each.key].writer_identity] + member = google_logging_project_sink.sink[each.key].writer_identity } -resource "google_pubsub_topic_iam_binding" "pubsub-sinks-binding" { +resource "google_pubsub_topic_iam_member" "pubsub-sinks-binding" { for_each = local.sink_bindings["pubsub"] project = split("/", each.value.destination)[1] topic = split("/", each.value.destination)[3] role = "roles/pubsub.publisher" - members = [google_logging_project_sink.sink[each.key].writer_identity] + member = google_logging_project_sink.sink[each.key].writer_identity +} + +resource "google_project_iam_member" "bucket-sinks-binding" { + for_each = local.sink_bindings["logging"] + project = split("/", each.value.destination)[1] + role = "roles/logging.bucketWriter" + member = google_logging_project_sink.sink[each.key].writer_identity + # TODO(jccb): use a condition to limit writer-identity only to this + # bucket } resource "google_logging_project_exclusion" "logging-exclusion" { @@ -318,7 +327,7 @@ resource "google_essential_contacts_contact" "contact" { } resource "google_access_context_manager_service_perimeter_resource" "service-perimeter-resource-standard" { - count = var.service_perimeter_standard != null ? 1 : 0 + count = var.service_perimeter_standard != null ? 1 : 0 # If used, remember to uncomment 'lifecycle' block in the # modules/vpc-sc/google_access_context_manager_service_perimeter resource. @@ -327,7 +336,7 @@ resource "google_access_context_manager_service_perimeter_resource" "service-per } resource "google_access_context_manager_service_perimeter_resource" "service-perimeter-resource-bridges" { - for_each = toset(var.service_perimeter_bridges != null ? var.service_perimeter_bridges : []) + for_each = toset(var.service_perimeter_bridges != null ? var.service_perimeter_bridges : []) # If used, remember to uncomment 'lifecycle' block in the # modules/vpc-sc/google_access_context_manager_service_perimeter resource. diff --git a/modules/project/outputs.tf b/modules/project/outputs.tf index 8f93a35e..4f54bc65 100644 --- a/modules/project/outputs.tf +++ b/modules/project/outputs.tf @@ -68,7 +68,7 @@ output "custom_roles" { } output "sink_writer_identities" { - description = "" + description = "Writer identities created for each sink." value = { for name, sink in google_logging_project_sink.sink : name => sink.writer_identity } From 7ca2e6039983e74490c9f7de4132810789fe3934 Mon Sep 17 00:00:00 2001 From: Julio Castillo Date: Wed, 31 Mar 2021 10:45:35 +0200 Subject: [PATCH 2/2] Fix tests --- modules/folder/README.md | 4 ++-- modules/organization/README.md | 4 ++-- modules/project/README.md | 4 ++-- tests/modules/folder/test_plan_logging.py | 20 ++++++++++--------- .../modules/organization/test_plan_logging.py | 18 +++++++++-------- tests/modules/project/test_plan_logging.py | 18 +++++++++-------- 6 files changed, 37 insertions(+), 31 deletions(-) diff --git a/modules/folder/README.md b/modules/folder/README.md index 5bb7b4cd..d08bfac0 100644 --- a/modules/folder/README.md +++ b/modules/folder/README.md @@ -114,7 +114,7 @@ module "folder-sink" { no-gce-instances = "resource.type=gce_instance" } } -# tftest:modules=5:resources=11 +# tftest:modules=5:resources=12 ``` ### Hierarchical firewall policies @@ -186,5 +186,5 @@ module "folder2" { | folder | Folder resource. | | | id | Folder id. | | | name | Folder name. | | -| sink_writer_identities | Writer identities created for each sink | | +| sink_writer_identities | Writer identities created for each sink. | | diff --git a/modules/organization/README.md b/modules/organization/README.md index 933ce8f4..acc3f18f 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -131,7 +131,7 @@ module "org" { no-gce-instances = "resource.type=gce_instance" } } -# tftest:modules=5:resources=10 +# tftest:modules=5:resources=11 ``` @@ -163,5 +163,5 @@ module "org" { | firewall_policies | Map of firewall policy resources created in the organization. | | | firewall_policy_id | Map of firewall policy ids created in the organization. | | | organization_id | Organization id dependent on module resources. | | -| sink_writer_identities | Writer identities created for each sink | | +| sink_writer_identities | Writer identities created for each sink. | | diff --git a/modules/project/README.md b/modules/project/README.md index a640edf0..2d99f977 100644 --- a/modules/project/README.md +++ b/modules/project/README.md @@ -146,7 +146,7 @@ module "project-host" { no-gce-instances = "resource.type=gce_instance" } } -# tftest:modules=5:resources=11 +# tftest:modules=5:resources=12 ``` @@ -191,6 +191,6 @@ module "project-host" { | number | Project number. | | | project_id | Project id. | | | service_accounts | Product robot service accounts in project. | | -| sink_writer_identities | Writer identities created for each sink | | +| sink_writer_identities | Writer identities created for each sink. | | diff --git a/tests/modules/folder/test_plan_logging.py b/tests/modules/folder/test_plan_logging.py index d2e9edc4..9bd7688f 100644 --- a/tests/modules/folder/test_plan_logging.py +++ b/tests/modules/folder/test_plan_logging.py @@ -62,15 +62,16 @@ def test_sinks(plan_runner): } """ _, resources = plan_runner(FIXTURES_DIR, logging_sinks=logging_sinks) - assert len(resources) == 8 + assert len(resources) == 9 resource_types = Counter([r["type"] for r in resources]) assert resource_types == { - "google_bigquery_dataset_iam_binding": 1, - "google_folder": 1, "google_logging_folder_sink": 4, - "google_pubsub_topic_iam_binding": 1, - "google_storage_bucket_iam_binding": 1, + "google_folder": 1, + "google_bigquery_dataset_iam_member": 1, + "google_project_iam_member": 1, + "google_pubsub_topic_iam_member": 1, + "google_storage_bucket_iam_member": 1, } sinks = [r for r in resources if r["type"] == "google_logging_folder_sink"] @@ -111,12 +112,13 @@ def test_sinks(plan_runner): ("warning", "severity=WARNING", "storage.googleapis.com/mybucket", True), ] - bindings = [r for r in resources if "binding" in r["type"]] + bindings = [r for r in resources if "member" in r["type"]] values = [(r["index"], r["type"], r["values"]["role"]) for r in bindings] assert sorted(values) == [ - ("info", "google_bigquery_dataset_iam_binding", "roles/bigquery.dataEditor"), - ("notice", "google_pubsub_topic_iam_binding", "roles/pubsub.publisher"), - ("warning", "google_storage_bucket_iam_binding", "roles/storage.objectCreator"), + ("debug", "google_project_iam_member", "roles/logging.bucketWriter"), + ("info", "google_bigquery_dataset_iam_member", "roles/bigquery.dataEditor"), + ("notice", "google_pubsub_topic_iam_member", "roles/pubsub.publisher"), + ("warning", "google_storage_bucket_iam_member", "roles/storage.objectCreator"), ] exclusions = [(r["index"], r["values"]["exclusions"]) for r in sinks] diff --git a/tests/modules/organization/test_plan_logging.py b/tests/modules/organization/test_plan_logging.py index f6684d12..1ad6aec4 100644 --- a/tests/modules/organization/test_plan_logging.py +++ b/tests/modules/organization/test_plan_logging.py @@ -62,14 +62,15 @@ def test_sinks(plan_runner): } """ _, resources = plan_runner(FIXTURES_DIR, logging_sinks=logging_sinks) - assert len(resources) == 7 + assert len(resources) == 8 resource_types = Counter([r["type"] for r in resources]) assert resource_types == { - "google_bigquery_dataset_iam_binding": 1, "google_logging_organization_sink": 4, - "google_pubsub_topic_iam_binding": 1, - "google_storage_bucket_iam_binding": 1, + "google_bigquery_dataset_iam_member": 1, + "google_project_iam_member": 1, + "google_pubsub_topic_iam_member": 1, + "google_storage_bucket_iam_member": 1, } sinks = [r for r in resources if r["type"] == "google_logging_organization_sink"] @@ -110,12 +111,13 @@ def test_sinks(plan_runner): ("warning", "severity=WARNING", "storage.googleapis.com/mybucket", True), ] - bindings = [r for r in resources if "binding" in r["type"]] + bindings = [r for r in resources if "member" in r["type"]] values = [(r["index"], r["type"], r["values"]["role"]) for r in bindings] assert sorted(values) == [ - ("info", "google_bigquery_dataset_iam_binding", "roles/bigquery.dataEditor"), - ("notice", "google_pubsub_topic_iam_binding", "roles/pubsub.publisher"), - ("warning", "google_storage_bucket_iam_binding", "roles/storage.objectCreator"), + ("debug", "google_project_iam_member", "roles/logging.bucketWriter"), + ("info", "google_bigquery_dataset_iam_member", "roles/bigquery.dataEditor"), + ("notice", "google_pubsub_topic_iam_member", "roles/pubsub.publisher"), + ("warning", "google_storage_bucket_iam_member", "roles/storage.objectCreator"), ] exclusions = [(r["index"], r["values"]["exclusions"]) for r in sinks] diff --git a/tests/modules/project/test_plan_logging.py b/tests/modules/project/test_plan_logging.py index 6841d194..cf5c992e 100644 --- a/tests/modules/project/test_plan_logging.py +++ b/tests/modules/project/test_plan_logging.py @@ -62,15 +62,16 @@ def test_sinks(plan_runner): } """ _, resources = plan_runner(FIXTURES_DIR, logging_sinks=logging_sinks) - assert len(resources) == 8 + assert len(resources) == 9 resource_types = Counter([r["type"] for r in resources]) assert resource_types == { - "google_bigquery_dataset_iam_binding": 1, "google_logging_project_sink": 4, + "google_bigquery_dataset_iam_member": 1, "google_project": 1, - "google_pubsub_topic_iam_binding": 1, - "google_storage_bucket_iam_binding": 1, + "google_project_iam_member": 1, + "google_pubsub_topic_iam_member": 1, + "google_storage_bucket_iam_member": 1, } sinks = [r for r in resources if r["type"] == "google_logging_project_sink"] @@ -111,12 +112,13 @@ def test_sinks(plan_runner): ("warning", "severity=WARNING", "storage.googleapis.com/mybucket", False), ] - bindings = [r for r in resources if "binding" in r["type"]] + bindings = [r for r in resources if "member" in r["type"]] values = [(r["index"], r["type"], r["values"]["role"]) for r in bindings] assert sorted(values) == [ - ("info", "google_bigquery_dataset_iam_binding", "roles/bigquery.dataEditor"), - ("notice", "google_pubsub_topic_iam_binding", "roles/pubsub.publisher"), - ("warning", "google_storage_bucket_iam_binding", "roles/storage.objectCreator"), + ("debug", "google_project_iam_member", "roles/logging.bucketWriter"), + ("info", "google_bigquery_dataset_iam_member", "roles/bigquery.dataEditor"), + ("notice", "google_pubsub_topic_iam_member", "roles/pubsub.publisher"), + ("warning", "google_storage_bucket_iam_member", "roles/storage.objectCreator"), ] exclusions = [(r["index"], r["values"]["exclusions"]) for r in sinks]