From f78902aee85d1ab10f1acf3b9727d188c349cdb9 Mon Sep 17 00:00:00 2001 From: Julio Castillo Date: Fri, 31 Dec 2021 11:36:14 +0000 Subject: [PATCH] Update hierarchical firewall resource This replaces all the `google_compute_organization_security_*` resources with the newer `google_compute_firewall_*` resources. --- modules/folder/README.md | 22 ++++---- modules/folder/firewall-policy.tf | 51 ++++++++----------- modules/folder/outputs.tf | 10 +--- modules/folder/variables.tf | 4 +- modules/organization/README.md | 14 +++-- modules/organization/firewall-policy.tf | 50 ++++++++---------- modules/organization/outputs.tf | 14 ++--- modules/organization/variables.tf | 4 +- tests/modules/folder/fixture/main.tf | 2 +- tests/modules/folder/fixture/variables.tf | 2 +- .../folder/test_plan_firewall_policy.py | 20 ++++---- tests/modules/organization/fixture/main.tf | 2 +- .../modules/organization/fixture/variables.tf | 2 +- .../organization/test_plan_firewall.py | 14 ++--- 14 files changed, 87 insertions(+), 124 deletions(-) diff --git a/modules/folder/README.md b/modules/folder/README.md index 10a1e990..593b875c 100644 --- a/modules/folder/README.md +++ b/modules/folder/README.md @@ -192,22 +192,20 @@ module "folder1" { firewall_policies = { iap-policy = { allow-iap-ssh = { - description = "Always allow ssh from IAP" - direction = "INGRESS" - action = "allow" - priority = 100 - ranges = ["35.235.240.0/20"] - ports = { - tcp = ["22"] - } + description = "Always allow ssh from IAP" + direction = "INGRESS" + action = "allow" + priority = 100 + ranges = ["35.235.240.0/20"] + ports = { tcp = ["22"] } target_service_accounts = null target_resources = null logging = false } } } - firewall_policy_attachments = { - iap-policy = module.folder1.firewall_policy_id["iap-policy"] + firewall_policy_association = { + iap-policy = "iap-policy" } } @@ -215,7 +213,7 @@ module "folder2" { source = "./modules/folder" parent = var.organization_id name = "hf2" - firewall_policy_attachments = { + firewall_policy_association = { iap-policy = module.folder1.firewall_policy_id["iap-policy"] } } @@ -232,7 +230,7 @@ module "folder2" { |---|---|:---:|:---:|:---:| | contacts | List of essential contacts for this resource. Must be in the form EMAIL -> [NOTIFICATION_TYPES]. Valid notification types are ALL, SUSPENSION, SECURITY, TECHNICAL, BILLING, LEGAL, PRODUCT_UPDATES | map(list(string)) | | {} | | firewall_policies | Hierarchical firewall policies created in this folder. | map(map(object({…}))) | | {} | -| firewall_policy_attachments | List of hierarchical firewall policy IDs to attached to this folder. | map(string) | | {} | +| firewall_policy_association | The hierarchical firewall policy to associate to this folder. Must be either a key in the `firewall_policies` map or the id of a policy defined somewhere else. | map(string) | | {} | | firewall_policy_factory | Configuration for the firewall policy factory. | object({…}) | | null | | folder_create | Create folder. When set to false, uses id to reference an existing folder. | bool | | true | | group_iam | Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable. | map(list(string)) | | {} | diff --git a/modules/folder/firewall-policy.tf b/modules/folder/firewall-policy.tf index abda1187..312d9adb 100644 --- a/modules/folder/firewall-policy.tf +++ b/modules/folder/firewall-policy.tf @@ -52,17 +52,15 @@ locals { } } -resource "google_compute_organization_security_policy" "policy" { - provider = google-beta - for_each = local.firewall_policies - display_name = each.key - parent = local.folder.id +resource "google_compute_firewall_policy" "policy" { + for_each = local.firewall_policies + short_name = each.key + parent = local.folder.id } -resource "google_compute_organization_security_policy_rule" "rule" { - provider = google-beta +resource "google_compute_firewall_policy_rule" "rule" { for_each = local.firewall_rules - policy_id = google_compute_organization_security_policy.policy[each.value.policy].id + firewall_policy = google_compute_firewall_policy.policy[each.value.policy].id action = each.value.action direction = each.value.direction priority = try(each.value.priority, null) @@ -70,33 +68,26 @@ resource "google_compute_organization_security_policy_rule" "rule" { target_service_accounts = try(each.value.target_service_accounts, null) enable_logging = try(each.value.logging, null) # preview = each.value.preview + description = each.value.description match { - description = each.value.description - config { - src_ip_ranges = each.value.direction == "INGRESS" ? each.value.ranges : null - dest_ip_ranges = each.value.direction == "EGRESS" ? each.value.ranges : null - dynamic "layer4_config" { - for_each = each.value.ports - iterator = port - content { - ip_protocol = port.key - ports = port.value - } + src_ip_ranges = each.value.direction == "INGRESS" ? each.value.ranges : null + dest_ip_ranges = each.value.direction == "EGRESS" ? each.value.ranges : null + dynamic "layer4_configs" { + for_each = each.value.ports + iterator = port + content { + ip_protocol = port.key + ports = port.value } } } - # TODO: remove once provider issues is fixed - # https://github.com/hashicorp/terraform-provider-google/issues/7790 - lifecycle { - ignore_changes = [match.0.description] - } } -resource "google_compute_organization_security_policy_association" "attachment" { - provider = google-beta - for_each = var.firewall_policy_attachments - name = "${local.folder.id}-${each.key}" - attachment_id = local.folder.id - policy_id = each.value + +resource "google_compute_firewall_policy_association" "association" { + for_each = var.firewall_policy_association + name = replace(local.folder.id, "/", "-") + attachment_target = local.folder.id + firewall_policy = try(google_compute_firewall_policy.policy[each.value].id, each.value) } diff --git a/modules/folder/outputs.tf b/modules/folder/outputs.tf index 8e1a9f0b..da368210 100644 --- a/modules/folder/outputs.tf +++ b/modules/folder/outputs.tf @@ -15,18 +15,12 @@ */ output "firewall_policies" { description = "Map of firewall policy resources created in this folder." - value = { - for name, _ in var.firewall_policies : - name => google_compute_organization_security_policy.policy[name] - } + value = { for k, v in google_compute_firewall_policy.policy : k => v } } output "firewall_policy_id" { description = "Map of firewall policy ids created in this folder." - value = { - for name, _ in local.firewall_policies : - name => google_compute_organization_security_policy.policy[name].id - } + value = { for k, v in google_compute_firewall_policy.policy : k => v.id } } output "folder" { diff --git a/modules/folder/variables.tf b/modules/folder/variables.tf index 0abcc48e..4bf5a83c 100644 --- a/modules/folder/variables.tf +++ b/modules/folder/variables.tf @@ -36,8 +36,8 @@ variable "firewall_policies" { default = {} } -variable "firewall_policy_attachments" { - description = "List of hierarchical firewall policy IDs to attached to this folder." +variable "firewall_policy_association" { + description = "The hierarchical firewall policy to associate to this folder. Must be either a key in the `firewall_policies` map or the id of a policy defined somewhere else." type = map(string) default = {} } diff --git a/modules/organization/README.md b/modules/organization/README.md index d0956d47..45a298a0 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -52,7 +52,7 @@ Hirerarchical firewall policies can be managed in two ways: - via the `firewall_policies` variable, to directly define policies and rules in Terraform - via the `firewall_policy_factory` variable, to leverage external YaML files via a simple "factory" embedded in the module ([see here](../../factories) for more context on factories) -Once you have policies (either created via the module or externally), you can attach them using the `firewall_policy_attachments` variable. +Once you have policies (either created via the module or externally), you can associate them using the `firewall_policy_association` variable. ### Directly defined firewall policies @@ -77,8 +77,8 @@ module "org" { } } } - firewall_policy_attachments = { - iap_policy = module.org.firewall_policy_id["iap-policy"] + firewall_policy_association = { + iap_policy = "iap-policy" } } # tftest:modules=1:resources=3 @@ -250,7 +250,7 @@ module "org" { | contacts | List of essential contacts for this resource. Must be in the form EMAIL -> [NOTIFICATION_TYPES]. Valid notification types are ALL, SUSPENSION, SECURITY, TECHNICAL, BILLING, LEGAL, PRODUCT_UPDATES | map(list(string)) | | {} | | custom_roles | Map of role name => list of permissions to create in this project. | map(list(string)) | | {} | | firewall_policies | Hierarchical firewall policy rules created in the organization. | map(map(object({…}))) | | {} | -| firewall_policy_attachments | List of hierarchical firewall policy IDs attached to the organization. | map(string) | | {} | +| firewall_policy_association | The hierarchical firewall policy to associate to this folder. Must be either a key in the `firewall_policies` map or the id of a policy defined somewhere else. | map(string) | | {} | | firewall_policy_factory | Configuration for the firewall policy factory. | object({…}) | | null | | group_iam | Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable. | map(list(string)) | | {} | | iam | IAM bindings, in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | @@ -270,11 +270,9 @@ module "org" { |---|---|:---:| | custom_role_id | Map of custom role IDs created in the organization. | | | custom_roles | Map of custom roles resources created in the organization. | | -| firewall_policies | Map of firewall policy resources created in the organization. | | -| firewall_policy_id | Map of firewall policy ids created in the organization. | | +| firewall_policies | Map of firewall policy resources created in this folder. | | +| firewall_policy_id | Map of firewall policy ids created in this folder. | | | organization_id | Organization id dependent on module resources. | | | sink_writer_identities | Writer identities created for each sink. | | - - diff --git a/modules/organization/firewall-policy.tf b/modules/organization/firewall-policy.tf index 4d111023..2c304601 100644 --- a/modules/organization/firewall-policy.tf +++ b/modules/organization/firewall-policy.tf @@ -52,11 +52,10 @@ locals { } } -resource "google_compute_organization_security_policy" "policy" { - provider = google-beta - for_each = local.firewall_policies - display_name = each.key - parent = var.organization_id +resource "google_compute_firewall_policy" "policy" { + for_each = local.firewall_policies + short_name = each.key + parent = var.organization_id depends_on = [ google_organization_iam_audit_config.config, google_organization_iam_binding.authoritative, @@ -66,10 +65,9 @@ resource "google_compute_organization_security_policy" "policy" { ] } -resource "google_compute_organization_security_policy_rule" "rule" { - provider = google-beta +resource "google_compute_firewall_policy_rule" "rule" { for_each = local.firewall_rules - policy_id = google_compute_organization_security_policy.policy[each.value.policy].id + firewall_policy = google_compute_firewall_policy.policy[each.value.policy].id action = each.value.action direction = each.value.direction priority = try(each.value.priority, null) @@ -77,33 +75,25 @@ resource "google_compute_organization_security_policy_rule" "rule" { target_service_accounts = try(each.value.target_service_accounts, null) enable_logging = try(each.value.logging, null) # preview = each.value.preview + description = each.value.description match { - description = each.value.description - config { - src_ip_ranges = each.value.direction == "INGRESS" ? each.value.ranges : null - dest_ip_ranges = each.value.direction == "EGRESS" ? each.value.ranges : null - dynamic "layer4_config" { - for_each = each.value.ports - iterator = port - content { - ip_protocol = port.key - ports = port.value - } + src_ip_ranges = each.value.direction == "INGRESS" ? each.value.ranges : null + dest_ip_ranges = each.value.direction == "EGRESS" ? each.value.ranges : null + dynamic "layer4_configs" { + for_each = each.value.ports + iterator = port + content { + ip_protocol = port.key + ports = port.value } } } - # TODO: remove once provider issues is fixed - # https://github.com/hashicorp/terraform-provider-google/issues/7790 - lifecycle { - ignore_changes = [match.0.description] - } } -resource "google_compute_organization_security_policy_association" "attachment" { - provider = google-beta - for_each = var.firewall_policy_attachments - name = "${var.organization_id}-${each.key}" - attachment_id = var.organization_id - policy_id = each.value +resource "google_compute_firewall_policy_association" "association" { + for_each = var.firewall_policy_association + name = replace(var.organization_id, "/", "-") + attachment_target = var.organization_id + firewall_policy = try(google_compute_firewall_policy.policy[each.value].id, each.value) } diff --git a/modules/organization/outputs.tf b/modules/organization/outputs.tf index 3435242f..4c67183c 100644 --- a/modules/organization/outputs.tf +++ b/modules/organization/outputs.tf @@ -34,19 +34,13 @@ output "custom_roles" { } output "firewall_policies" { - description = "Map of firewall policy resources created in the organization." - value = { - for name, _ in var.firewall_policies : - name => google_compute_organization_security_policy.policy[name] - } + description = "Map of firewall policy resources created in this folder." + value = { for k, v in google_compute_firewall_policy.policy : k => v } } output "firewall_policy_id" { - description = "Map of firewall policy ids created in the organization." - value = { - for name, _ in local.firewall_policies : - name => google_compute_organization_security_policy.policy[name].id - } + description = "Map of firewall policy ids created in this folder." + value = { for k, v in google_compute_firewall_policy.policy : k => v.id } } output "organization_id" { diff --git a/modules/organization/variables.tf b/modules/organization/variables.tf index 2ca79a6d..374b7270 100644 --- a/modules/organization/variables.tf +++ b/modules/organization/variables.tf @@ -43,8 +43,8 @@ variable "firewall_policies" { default = {} } -variable "firewall_policy_attachments" { - description = "List of hierarchical firewall policy IDs attached to the organization." +variable "firewall_policy_association" { + description = "The hierarchical firewall policy to associate to this folder. Must be either a key in the `firewall_policies` map or the id of a policy defined somewhere else." type = map(string) default = {} } diff --git a/tests/modules/folder/fixture/main.tf b/tests/modules/folder/fixture/main.tf index 2d6e8ff9..d9bd34ba 100644 --- a/tests/modules/folder/fixture/main.tf +++ b/tests/modules/folder/fixture/main.tf @@ -22,7 +22,7 @@ module "test" { policy_boolean = var.policy_boolean policy_list = var.policy_list firewall_policies = var.firewall_policies - firewall_policy_attachments = var.firewall_policy_attachments + firewall_policy_association = var.firewall_policy_association logging_sinks = var.logging_sinks logging_exclusions = var.logging_exclusions } diff --git a/tests/modules/folder/fixture/variables.tf b/tests/modules/folder/fixture/variables.tf index 9d994bc0..824560d9 100644 --- a/tests/modules/folder/fixture/variables.tf +++ b/tests/modules/folder/fixture/variables.tf @@ -49,7 +49,7 @@ variable "firewall_policies" { default = {} } -variable "firewall_policy_attachments" { +variable "firewall_policy_association" { type = map(string) default = {} } diff --git a/tests/modules/folder/test_plan_firewall_policy.py b/tests/modules/folder/test_plan_firewall_policy.py index 9754d437..9a7a31b5 100644 --- a/tests/modules/folder/test_plan_firewall_policy.py +++ b/tests/modules/folder/test_plan_firewall_policy.py @@ -54,17 +54,17 @@ def test_firweall_policy(plan_runner): } } """ - attachment = '{ iap_policy = "policy1" }' + association = '{policy1="policy1"}' _, resources = plan_runner(FIXTURES_DIR, firewall_policies=policy, - firewall_policy_attachments=attachment) + firewall_policy_association=association) assert len(resources) == 5 - + policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_firewall_policy'] assert len(policies) == 1 rules = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy_rule'] + if r['type'] == 'google_compute_firewall_policy_rule'] assert len(rules) == 2 rule_values = [] @@ -74,22 +74,20 @@ def test_firweall_policy(plan_runner): action = rule['values']['action'] direction = rule['values']['direction'] priority = rule['values']['priority'] - config = rule['values']['match'] - assert len(config) == 1 - config = config[0]['config'] - rule_values.append((name, index, action, direction, priority, config)) + match = rule['values']['match'] + rule_values.append((name, index, action, direction, priority, match)) assert sorted(rule_values) == sorted([ ('rule', 'policy1-allow-ingress', 'allow', 'INGRESS', 100,[ { 'dest_ip_ranges': None, - 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['22']}], + 'layer4_configs': [{'ip_protocol': 'tcp', 'ports': ['22']}], 'src_ip_ranges': ['10.0.0.0/8'] }]), ('rule', 'policy1-deny-egress', 'deny', 'EGRESS', 200, [ { 'dest_ip_ranges': ['192.168.0.0/24'], - 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['443']}], + 'layer4_configs': [{'ip_protocol': 'tcp', 'ports': ['443']}], 'src_ip_ranges': None }]) ]) diff --git a/tests/modules/organization/fixture/main.tf b/tests/modules/organization/fixture/main.tf index 6c948c38..e875cae2 100644 --- a/tests/modules/organization/fixture/main.tf +++ b/tests/modules/organization/fixture/main.tf @@ -26,7 +26,7 @@ module "test" { policy_boolean = var.policy_boolean policy_list = var.policy_list firewall_policies = var.firewall_policies - firewall_policy_attachments = var.firewall_policy_attachments + firewall_policy_association = var.firewall_policy_association firewall_policy_factory = var.firewall_policy_factory logging_sinks = var.logging_sinks logging_exclusions = var.logging_exclusions diff --git a/tests/modules/organization/fixture/variables.tf b/tests/modules/organization/fixture/variables.tf index 8e04290c..bd3d2ce3 100644 --- a/tests/modules/organization/fixture/variables.tf +++ b/tests/modules/organization/fixture/variables.tf @@ -74,7 +74,7 @@ variable "firewall_policies" { default = {} } -variable "firewall_policy_attachments" { +variable "firewall_policy_association" { type = map(string) default = {} } diff --git a/tests/modules/organization/test_plan_firewall.py b/tests/modules/organization/test_plan_firewall.py index 8998bf7b..1aa8c1cb 100644 --- a/tests/modules/organization/test_plan_firewall.py +++ b/tests/modules/organization/test_plan_firewall.py @@ -80,9 +80,9 @@ def test_custom(plan_runner): _, resources = plan_runner(FIXTURES_DIR, firewall_policies=_POLICIES) assert len(resources) == 5 policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_firewall_policy'] rules = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy_rule'] + if r['type'] == 'google_compute_firewall_policy_rule'] assert set(r['index'] for r in policies) == set([ 'policy1', 'policy2' ]) @@ -96,9 +96,9 @@ def test_factory(plan_runner): _, resources = plan_runner(FIXTURES_DIR, firewall_policy_factory=_FACTORY) assert len(resources) == 3 policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_firewall_policy'] rules = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy_rule'] + if r['type'] == 'google_compute_firewall_policy_rule'] assert set(r['index'] for r in policies) == set([ 'factory-1' ]) @@ -113,7 +113,7 @@ def test_factory_name(plan_runner): _, resources = plan_runner(FIXTURES_DIR, firewall_policy_factory=factory) assert len(resources) == 3 policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_firewall_policy'] assert set(r['index'] for r in policies) == set([ 'factory' ]) @@ -125,9 +125,9 @@ def test_combined(plan_runner): firewall_policy_factory=_FACTORY) assert len(resources) == 8 policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_firewall_policy'] rules = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy_rule'] + if r['type'] == 'google_compute_firewall_policy_rule'] assert set(r['index'] for r in policies) == set([ 'factory-1', 'policy1', 'policy2' ])