From 487da1035de21159b4c43a7eddbf8124d96db68b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Tue, 14 Nov 2023 10:36:05 +0000 Subject: [PATCH 1/5] Add serialization tag to tests --- tests/examples/conftest.py | 8 +++++++- tests/pytest.ini | 2 ++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 tests/pytest.ini diff --git a/tests/examples/conftest.py b/tests/examples/conftest.py index 345df27f02..014815c5d1 100644 --- a/tests/examples/conftest.py +++ b/tests/examples/conftest.py @@ -18,6 +18,7 @@ from pathlib import Path import marko +import pytest FABRIC_ROOT = Path(__file__).parents[2] @@ -78,7 +79,12 @@ def pytest_generate_tests(metafunc, test_group='example', if index > 1: name += f' {index}' ids.append(f'{path}:{last_header}:{index}') - examples.append(Example(name, code, path, files[last_header])) + # if test is marked with 'serial' in tftest line then add them to this xdist group + # this, together with `--dist loadgroup` will ensure that those tests will be run one after another + # even if multiple workers are used + # see: https://pytest-xdist.readthedocs.io/en/latest/distribution.html + marks = [pytest.mark.xdist_group("serial")] if 'serial' in tftest_tag else [] + examples.append(pytest.param(Example(name, code, path, files[last_header]), marks=marks)) elif isinstance(child, marko.block.Heading): last_header = child.children[0].children index = 0 diff --git a/tests/pytest.ini b/tests/pytest.ini new file mode 100644 index 0000000000..b1da50fa48 --- /dev/null +++ b/tests/pytest.ini @@ -0,0 +1,2 @@ +[pytest] +addopts = --tb=short --dist loadgroup \ No newline at end of file From bba93ddbf0e7861e775e5e1037d0663f16d6a241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Tue, 14 Nov 2023 14:21:19 +0000 Subject: [PATCH 2/5] Standarize organization id --- tests/examples_e2e/setup_module/e2e_tests.tfvars.tftpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/examples_e2e/setup_module/e2e_tests.tfvars.tftpl b/tests/examples_e2e/setup_module/e2e_tests.tfvars.tftpl index 31a34d3df5..d23c39a47b 100644 --- a/tests/examples_e2e/setup_module/e2e_tests.tfvars.tftpl +++ b/tests/examples_e2e/setup_module/e2e_tests.tfvars.tftpl @@ -18,7 +18,7 @@ kms_key = { id = "${kms_key_id}" } group_email = "${group_email}" -organization_id = "${organization_id}" +organization_id = "organizations/${organization_id}" folder_id = "folders/${folder_id}" project_id = "${project_id}" region = "${region}" From 7f27161723454c2f624813ec0685516095d3878a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Tue, 14 Nov 2023 14:48:10 +0000 Subject: [PATCH 3/5] E2E tests for organiation module * added tag `serial` to mark tests to be run serially * added end-to-end tests for organization, not adding to custom contratints as the name has to be unique * fixed granting custom roles created in the same module call --- modules/organization/README.md | 43 ++++++------ modules/organization/iam.tf | 3 + .../modules/organization/examples/basic.yaml | 67 +++++++------------ .../organization/examples/logging.yaml | 2 +- .../organization/examples/network-tags.yaml | 6 +- .../modules/organization/examples/roles.yaml | 2 +- tests/modules/organization/examples/tags.yaml | 10 +-- 7 files changed, 55 insertions(+), 78 deletions(-) diff --git a/modules/organization/README.md b/modules/organization/README.md index b760f03626..b9a73949ef 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -35,16 +35,16 @@ To manage organization policies, the `orgpolicy.googleapis.com` service should b ```hcl module "org" { source = "./fabric/modules/organization" - organization_id = "organizations/1234567890" + organization_id = var.organization_id group_iam = { - "cloud-owners@example.org" = ["roles/owner", "roles/projectCreator"] + (var.group_email) = ["roles/owner"] } iam = { - "roles/resourcemanager.projectCreator" = ["group:cloud-admins@example.org"] + "roles/resourcemanager.projectCreator" = ["group:${var.group_email}"] } iam_bindings_additive = { am1-storage-admin = { - member = "user:am1@example.org" + member = "group:${var.group_email}" role = "roles/storage.admin" } } @@ -57,9 +57,6 @@ module "org" { } } org_policies = { - "custom.gkeEnableAutoUpgrade" = { - rules = [{ enforce = true }] - } "compute.disableGuestAttributesAccess" = { rules = [{ enforce = true }] } @@ -118,7 +115,7 @@ module "org" { } } } -# tftest modules=1 resources=15 inventory=basic.yaml +# tftest modules=1 resources=13 inventory=basic.yaml e2e serial ``` ## IAM @@ -262,7 +259,7 @@ module "org" { policy = module.firewall-policy.id } } -# tftest modules=2 resources=2 +# tftest modules=2 resources=2 e2e serial ``` ## Log Sinks @@ -273,6 +270,7 @@ The following example shows how to define organization-level log sinks: module "gcs" { source = "./fabric/modules/gcs" project_id = var.project_id + prefix = var.prefix name = "gcs_sink" force_destroy = true } @@ -292,7 +290,7 @@ module "pubsub" { module "bucket" { source = "./fabric/modules/logging-bucket" parent_type = "project" - parent = "my-project" + parent = var.project_id id = "bucket" } @@ -330,7 +328,7 @@ module "org" { no-gce-instances = "resource.type=gce_instance" } } -# tftest modules=5 resources=13 inventory=logging.yaml +# tftest modules=5 resources=13 inventory=logging.yaml e2e serial ``` ## Data Access Logs @@ -344,7 +342,7 @@ module "org" { logging_data_access = { allServices = { # logs for principals listed here will be excluded - ADMIN_READ = ["group:organization-admins@example.org"] + ADMIN_READ = ["group:${var.group_email}"] } "storage.googleapis.com" = { DATA_READ = [] @@ -352,7 +350,7 @@ module "org" { } } } -# tftest modules=1 resources=2 inventory=logging-data-access.yaml +# tftest modules=1 resources=2 inventory=logging-data-access.yaml e2e serial ``` ## Custom Roles @@ -369,10 +367,10 @@ module "org" { ] } iam = { - (module.org.custom_role_id.myRole) = ["user:me@example.com"] + (module.org.custom_role_id.myRole) = ["group:${var.group_email}"] } } -# tftest modules=1 resources=2 inventory=roles.yaml +# tftest modules=1 resources=2 inventory=roles.yaml e2e serial ``` ## Tags @@ -387,14 +385,14 @@ module "org" { environment = { description = "Environment specification." iam = { - "roles/resourcemanager.tagAdmin" = ["group:admins@example.com"] + "roles/resourcemanager.tagAdmin" = ["group:${var.group_email}"] } values = { dev = {} prod = { description = "Environment: production." iam = { - "roles/resourcemanager.tagViewer" = ["user:user1@example.com"] + "roles/resourcemanager.tagViewer" = ["group:${var.group_email}"] } } } @@ -402,10 +400,9 @@ module "org" { } tag_bindings = { env-prod = module.org.tag_values["environment/prod"].id - foo = "tagValues/12345678" } } -# tftest modules=1 resources=7 inventory=tags.yaml +# tftest modules=1 resources=6 inventory=tags.yaml e2e serial ``` You can also define network tags, through a dedicated variable *network_tags*: @@ -417,23 +414,23 @@ module "org" { network_tags = { net-environment = { description = "This is a network tag." - network = "my_project/my_vpc" + network = "${var.project_id}/${var.vpc.name}" iam = { - "roles/resourcemanager.tagAdmin" = ["group:admins@example.com"] + "roles/resourcemanager.tagAdmin" = ["group:${var.group_email}"] } values = { dev = null prod = { description = "Environment: production." iam = { - "roles/resourcemanager.tagUser" = ["user:user1@example.com"] + "roles/resourcemanager.tagUser" = ["group:${var.group_email}"] } } } } } } -# tftest modules=1 resources=5 inventory=network-tags.yaml +# tftest modules=1 resources=5 inventory=network-tags.yaml e2e serial ``` diff --git a/modules/organization/iam.tf b/modules/organization/iam.tf index 81a8d2b0ed..d0f158004d 100644 --- a/modules/organization/iam.tf +++ b/modules/organization/iam.tf @@ -46,6 +46,7 @@ resource "google_organization_iam_binding" "authoritative" { org_id = local.organization_id_numeric role = each.key members = each.value + depends_on = [google_organization_iam_custom_role.roles] } resource "google_organization_iam_binding" "bindings" { @@ -61,6 +62,7 @@ resource "google_organization_iam_binding" "bindings" { description = each.value.condition.description } } + depends_on = [google_organization_iam_custom_role.roles] } resource "google_organization_iam_member" "bindings" { @@ -76,4 +78,5 @@ resource "google_organization_iam_member" "bindings" { description = each.value.condition.description } } + depends_on = [google_organization_iam_custom_role.roles] } diff --git a/tests/modules/organization/examples/basic.yaml b/tests/modules/organization/examples/basic.yaml index d1e0a1189a..1a3992392e 100644 --- a/tests/modules/organization/examples/basic.yaml +++ b/tests/modules/organization/examples/basic.yaml @@ -14,8 +14,8 @@ values: module.org.google_org_policy_policy.default["compute.disableGuestAttributesAccess"]: - name: organizations/1234567890/policies/compute.disableGuestAttributesAccess - parent: organizations/1234567890 + name: organizations/1122334455/policies/compute.disableGuestAttributesAccess + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -27,8 +27,8 @@ values: values: [] timeouts: null module.org.google_org_policy_policy.default["compute.skipDefaultNetworkCreation"]: - name: organizations/1234567890/policies/compute.skipDefaultNetworkCreation - parent: organizations/1234567890 + name: organizations/1122334455/policies/compute.skipDefaultNetworkCreation + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -40,8 +40,8 @@ values: values: [] timeouts: null module.org.google_org_policy_policy.default["compute.trustedImageProjects"]: - name: organizations/1234567890/policies/compute.trustedImageProjects - parent: organizations/1234567890 + name: organizations/1122334455/policies/compute.trustedImageProjects + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -56,8 +56,8 @@ values: denied_values: null timeouts: null module.org.google_org_policy_policy.default["compute.vmExternalIpAccess"]: - name: organizations/1234567890/policies/compute.vmExternalIpAccess - parent: organizations/1234567890 + name: organizations/1122334455/policies/compute.vmExternalIpAccess + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -68,22 +68,9 @@ values: enforce: null values: [] timeouts: null - module.org.google_org_policy_policy.default["custom.gkeEnableAutoUpgrade"]: - name: organizations/1234567890/policies/custom.gkeEnableAutoUpgrade - parent: organizations/1234567890 - spec: - - inherit_from_parent: null - reset: null - rules: - - allow_all: null - condition: [] - deny_all: null - enforce: 'TRUE' - values: [] - timeouts: null module.org.google_org_policy_policy.default["iam.allowedPolicyMemberDomains"]: - name: organizations/1234567890/policies/iam.allowedPolicyMemberDomains - parent: organizations/1234567890 + name: organizations/1122334455/policies/iam.allowedPolicyMemberDomains + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -114,8 +101,8 @@ values: denied_values: null timeouts: null module.org.google_org_policy_policy.default["iam.disableServiceAccountKeyCreation"]: - name: organizations/1234567890/policies/iam.disableServiceAccountKeyCreation - parent: organizations/1234567890 + name: organizations/1122334455/policies/iam.disableServiceAccountKeyCreation + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -127,8 +114,8 @@ values: values: [] timeouts: null module.org.google_org_policy_policy.default["iam.disableServiceAccountKeyUpload"]: - name: organizations/1234567890/policies/iam.disableServiceAccountKeyUpload - parent: organizations/1234567890 + name: organizations/1122334455/policies/iam.disableServiceAccountKeyUpload + parent: organizations/1122334455 spec: - inherit_from_parent: null reset: null @@ -151,29 +138,23 @@ values: module.org.google_organization_iam_binding.authoritative["roles/owner"]: condition: [] members: - - group:cloud-owners@example.org - org_id: '1234567890' + - group:organization-admins@example.org + org_id: '1122334455' role: roles/owner - module.org.google_organization_iam_binding.authoritative["roles/projectCreator"]: - condition: [] - members: - - group:cloud-owners@example.org - org_id: '1234567890' - role: roles/projectCreator module.org.google_organization_iam_binding.authoritative["roles/resourcemanager.projectCreator"]: condition: [] members: - - group:cloud-admins@example.org - org_id: '1234567890' + - group:organization-admins@example.org + org_id: '1122334455' role: roles/resourcemanager.projectCreator module.org.google_organization_iam_member.bindings["am1-storage-admin"]: condition: [] - member: user:am1@example.org - org_id: '1234567890' + member: group:organization-admins@example.org + org_id: '1122334455' role: roles/storage.admin module.org.google_tags_tag_key.default["allowexternal"]: description: Allow external identities. - parent: organizations/1234567890 + parent: organizations/1122334455 purpose: null purpose_data: null short_name: allowexternal @@ -188,12 +169,12 @@ values: timeouts: null counts: - google_org_policy_policy: 8 - google_organization_iam_binding: 3 + google_org_policy_policy: 7 + google_organization_iam_binding: 2 google_organization_iam_member: 1 google_tags_tag_key: 1 google_tags_tag_value: 2 modules: 1 - resources: 15 + resources: 13 outputs: {} diff --git a/tests/modules/organization/examples/logging.yaml b/tests/modules/organization/examples/logging.yaml index d42ac424b6..46d1582707 100644 --- a/tests/modules/organization/examples/logging.yaml +++ b/tests/modules/organization/examples/logging.yaml @@ -47,7 +47,7 @@ values: name: notice org_id: '1122334455' module.org.google_logging_organization_sink.sink["warnings"]: - destination: storage.googleapis.com/gcs_sink + destination: storage.googleapis.com/test-gcs_sink disabled: false exclusions: [] filter: severity=WARNING diff --git a/tests/modules/organization/examples/network-tags.yaml b/tests/modules/organization/examples/network-tags.yaml index d9c2872402..976dccbdcf 100644 --- a/tests/modules/organization/examples/network-tags.yaml +++ b/tests/modules/organization/examples/network-tags.yaml @@ -18,13 +18,13 @@ values: parent: organizations/1122334455 purpose: GCE_FIREWALL purpose_data: - network: my_project/my_vpc + network: project-id/vpc_name short_name: net-environment timeouts: null module.org.google_tags_tag_key_iam_binding.default["net-environment:roles/resourcemanager.tagAdmin"]: condition: [] members: - - group:admins@example.com + - group:organization-admins@example.org role: roles/resourcemanager.tagAdmin module.org.google_tags_tag_value.default["net-environment/dev"]: description: Managed by the Terraform organization module. @@ -37,7 +37,7 @@ values: module.org.google_tags_tag_value_iam_binding.default["net-environment/prod:roles/resourcemanager.tagUser"]: condition: [] members: - - user:user1@example.com + - group:organization-admins@example.org role: roles/resourcemanager.tagUser counts: diff --git a/tests/modules/organization/examples/roles.yaml b/tests/modules/organization/examples/roles.yaml index ae5c9cd8bc..a6ca7851bc 100644 --- a/tests/modules/organization/examples/roles.yaml +++ b/tests/modules/organization/examples/roles.yaml @@ -16,7 +16,7 @@ values: module.org.google_organization_iam_binding.authoritative["organizations/1122334455/roles/myRole"]: condition: [] members: - - user:me@example.com + - group:organization-admins@example.org org_id: '1122334455' role: organizations/1122334455/roles/myRole module.org.google_organization_iam_custom_role.roles["myRole"]: diff --git a/tests/modules/organization/examples/tags.yaml b/tests/modules/organization/examples/tags.yaml index 390aea5a70..bed4b46188 100644 --- a/tests/modules/organization/examples/tags.yaml +++ b/tests/modules/organization/examples/tags.yaml @@ -16,10 +16,6 @@ values: module.org.google_tags_tag_binding.binding["env-prod"]: parent: //cloudresourcemanager.googleapis.com/organizations/1122334455 timeouts: null - module.org.google_tags_tag_binding.binding["foo"]: - parent: //cloudresourcemanager.googleapis.com/organizations/1122334455 - tag_value: tagValues/12345678 - timeouts: null module.org.google_tags_tag_key.default["environment"]: description: Environment specification. parent: organizations/1122334455 @@ -30,7 +26,7 @@ values: module.org.google_tags_tag_key_iam_binding.default["environment:roles/resourcemanager.tagAdmin"]: condition: [] members: - - group:admins@example.com + - group:organization-admins@example.org role: roles/resourcemanager.tagAdmin module.org.google_tags_tag_value.default["environment/dev"]: description: Managed by the Terraform organization module. @@ -43,11 +39,11 @@ values: module.org.google_tags_tag_value_iam_binding.default["environment/prod:roles/resourcemanager.tagViewer"]: condition: [] members: - - user:user1@example.com + - group:organization-admins@example.org role: roles/resourcemanager.tagViewer counts: - google_tags_tag_binding: 2 + google_tags_tag_binding: 1 google_tags_tag_key: 1 google_tags_tag_key_iam_binding: 1 google_tags_tag_value: 2 From 1e6a680a1c5466adba29f828478be0c6c8135213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Tue, 14 Nov 2023 14:55:00 +0000 Subject: [PATCH 4/5] terraform fmt --- modules/organization/iam.tf | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/organization/iam.tf b/modules/organization/iam.tf index d0f158004d..5b3c5e1d45 100644 --- a/modules/organization/iam.tf +++ b/modules/organization/iam.tf @@ -42,10 +42,10 @@ resource "google_organization_iam_custom_role" "roles" { } resource "google_organization_iam_binding" "authoritative" { - for_each = local.iam - org_id = local.organization_id_numeric - role = each.key - members = each.value + for_each = local.iam + org_id = local.organization_id_numeric + role = each.key + members = each.value depends_on = [google_organization_iam_custom_role.roles] } From c833637f5194dda1074fbbf18e993af1a669fcf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Tue, 14 Nov 2023 16:05:20 +0000 Subject: [PATCH 5/5] Remove explicit dependency on the custom roles --- modules/organization/README.md | 2 +- modules/organization/iam.tf | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/organization/README.md b/modules/organization/README.md index b9a73949ef..737e202be6 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -355,7 +355,7 @@ module "org" { ## Custom Roles -Custom roles can be defined via the `custom_roles` variable, and referenced via the `custom_role_id` output: +Custom roles can be defined via the `custom_roles` variable, and referenced via the `custom_role_id` output (this also provides explicit dependency on the custom role): ```hcl module "org" { diff --git a/modules/organization/iam.tf b/modules/organization/iam.tf index 5b3c5e1d45..16a0fa00f9 100644 --- a/modules/organization/iam.tf +++ b/modules/organization/iam.tf @@ -42,11 +42,11 @@ resource "google_organization_iam_custom_role" "roles" { } resource "google_organization_iam_binding" "authoritative" { - for_each = local.iam - org_id = local.organization_id_numeric - role = each.key - members = each.value - depends_on = [google_organization_iam_custom_role.roles] + for_each = local.iam + org_id = local.organization_id_numeric + role = each.key + members = each.value + # ensuring that custom role exists is left to the caller, by leveraging custom_role_id output } resource "google_organization_iam_binding" "bindings" { @@ -62,7 +62,7 @@ resource "google_organization_iam_binding" "bindings" { description = each.value.condition.description } } - depends_on = [google_organization_iam_custom_role.roles] + # ensuring that custom role exists is left to the caller, by leveraging custom_role_id output } resource "google_organization_iam_member" "bindings" { @@ -78,5 +78,5 @@ resource "google_organization_iam_member" "bindings" { description = each.value.condition.description } } - depends_on = [google_organization_iam_custom_role.roles] + # ensuring that custom role exists is left to the caller, by leveraging custom_role_id output }