From c414ca550593f4fb68c586ce2f73fa9347f8e28b Mon Sep 17 00:00:00 2001
From: Lorenzo Caggioni <lcaggioni@google.com>
Date: Mon, 6 Jul 2020 14:30:25 +0200
Subject: [PATCH] Fixes based on PR comments:  - fix typos  - use for_each  -
 fix code layout

---
 modules/organization/main.tf      | 24 +++++++++++++-----------
 modules/organization/variables.tf |  2 +-
 modules/project/main.tf           | 12 ++++++------
 modules/project/variables.tf      | 14 ++++++++++++--
 4 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/modules/organization/main.tf b/modules/organization/main.tf
index 37f43c78..83b29b8e 100644
--- a/modules/organization/main.tf
+++ b/modules/organization/main.tf
@@ -28,16 +28,14 @@ locals {
 
   standard_perimeters = {
     for key, value in var.vpc_sc_perimeters :
-    key => value
-    if value.type == "PERIMETER_TYPE_REGULAR"
+    key => value if value.type == "PERIMETER_TYPE_REGULAR"
   }
 
   perimeter_create = var.access_policy_name != null || var.access_policy_title != null ? true : false
 
   bridge_perimeters = {
     for key, value in var.vpc_sc_perimeters :
-    key => value
-    if value.type == "PERIMETER_TYPE_BRIDGE"
+    key => value if value.type == "PERIMETER_TYPE_BRIDGE"
   }
 
   access_policy_name = (
@@ -49,7 +47,7 @@ locals {
 
 resource "google_access_context_manager_access_policy" "default" {
   count  = var.access_policy_name == null ? 1 : 0
-  parent = format("organizations/%s", var.org_id)
+  parent = "organizations/${var.org_id}"
   title  = var.access_policy_title
 }
 
@@ -64,9 +62,11 @@ resource "google_access_context_manager_service_perimeter" "standard" {
     restricted_services = each.value.restricted_services
   }
   
-  lifecycle {
-    ignore_changes = [status[0].resources]
-  }  
+  # Uncomment if used alongside `google_access_context_manager_service_perimeter_resource`, 
+  # so they don't fight over which resources should be in the policy.
+  # lifecycle {
+  #   ignore_changes = [status[0].resources]
+  # }  
 }
 
 resource "google_access_context_manager_service_perimeter" "bridge" {
@@ -80,9 +80,11 @@ resource "google_access_context_manager_service_perimeter" "bridge" {
     restricted_services = each.value.restricted_services
   }
 
-  lifecycle {
-    ignore_changes = [status[0].resources]
-  }
+  # Uncomment if used alongside `google_access_context_manager_service_perimeter_resource`, 
+  # so they don't fight over which resources should be in the policy.
+  # lifecycle {
+  #   ignore_changes = [status[0].resources]
+  # }  
 
   depends_on = [
     google_access_context_manager_service_perimeter.standard,
diff --git a/modules/organization/variables.tf b/modules/organization/variables.tf
index dee59b46..a9031384 100644
--- a/modules/organization/variables.tf
+++ b/modules/organization/variables.tf
@@ -99,7 +99,7 @@ variable "vpc_sc_perimeters" {
 }
 
 variable "vpc_sc_perimeters_projects" {
-  description = "Perimeter - Project Number mapping in `projects/project_number` format.."
+  description = "Perimeter - Project Number mapping in `projects/project_number` format."
   type        = map(list(string))
   default     = {}
 }
diff --git a/modules/project/main.tf b/modules/project/main.tf
index f1c74413..7f441bc6 100644
--- a/modules/project/main.tf
+++ b/modules/project/main.tf
@@ -203,15 +203,15 @@ resource "google_project_organization_policy" "list" {
 }
 
 resource "google_access_context_manager_service_perimeter_resource" "standard" {
-  count          = var.vpc_sc_perimeter != "" ? 1 : 0
-  perimeter_name = var.vpc_sc_perimeter
-  resource       = format("projects/%s", google_project.project.number)
+  for_each       = toset([var.vpc_sc_perimeter])
+  perimeter_name = each.key
+  resource       = "projects/${google_project.project.number}"
 }
 
 resource "google_access_context_manager_service_perimeter_resource" "bridges" {
-  count = length(var.vpc_sc_perimeter_bridges)
-  perimeter_name = var.vpc_sc_perimeter_bridges[count.index]
-  resource       = format("projects/%s", google_project.project.number)
+  for_each       = toset(var.vpc_sc_perimeter_bridges)
+  perimeter_name = each.key
+  resource       = "projects/${google_project.project.number}"
   depends_on = [
     google_access_context_manager_service_perimeter_resource.standard,
   ]  
diff --git a/modules/project/variables.tf b/modules/project/variables.tf
index f6cb8b59..769d3ff0 100644
--- a/modules/project/variables.tf
+++ b/modules/project/variables.tf
@@ -126,13 +126,23 @@ variable "services" {
 }
 
 variable "vpc_sc_perimeter" {
-  description = "Name of the VPC-SC perimeter the project belong to. Must be of the form accessPolicies/{policy_id}/servicePerimeters/{short_name}"
+  description = <<EOF
+    Name of the VPC-SC perimeter the project belongs to. Must be of the form accessPolicies/{policy_id}/servicePerimeters/{short_name}.
+    If this resource is used alongside a `google_access_context_manager_service_perimeter` resource, 
+    the service perimeter resource must have a lifecycle block with ignore_changes = [status[0].resources] 
+    so they don't fight over which resources should be in the policy.
+  EOF
   type        = string
   default     = null
 }
 
 variable "vpc_sc_perimeter_bridges" {
-  description = "List of VPC-SC perimeter bridges the project belong to. Must be of the form accessPolicies/{policy_id}/servicePerimeters/{short_name}"
+  description = <<EOF
+    List of VPC-SC perimeter bridges the project belongs to. Must be of the form accessPolicies/{policy_id}/servicePerimeters/{short_name}.
+    If this resource is used alongside a `google_access_context_manager_service_perimeter` resource, 
+    the service perimeter resource must have a lifecycle block with ignore_changes = [status[0].resources] 
+    so they don't fight over which resources should be in the policy.
+  EOF
   type        = list(string)
   default     = []
 }