feat: added the grant_services_network_role flag to control network IAM (#618)

kaariger · bharathkkb · web-flow · commit f116dd5456dd · 2021-09-23T09:22:12.000-05:00
* feat: added the grant_services_network_role flag to control network IAM

* Update modules/shared_vpc_access/variables.tf

Co-authored-by: Bharath KKB &lt;bharathkrishnakb@gmail.com&gt;

* updated description for grant_services_network_role flag

Co-authored-by: kaariger &lt;kaariger@users.noreply.github.com&gt;
Co-authored-by: Bharath KKB &lt;bharathkrishnakb@gmail.com&gt;
diff --git a/README.md b/README.md
@@ -133,6 +133,7 @@ determining that location is as follows:
 | domain | The domain name (optional). | `string` | `""` | no |
 | enable\_shared\_vpc\_host\_project | If this project is a shared VPC host project. If true, you must *not* set svpc\_host\_project\_id variable. Default is false. | `bool` | `false` | no |
 | folder\_id | The ID of a folder to host this project | `string` | `""` | no |
+| grant\_services\_network\_role | Whether or not to grant service agents the network roles on the host project | `bool` | `true` | no |
 | grant\_services\_security\_admin\_role | Whether or not to grant Kubernetes Engine Service Agent the Security Admin role on the host project so it can manage firewall rules | `bool` | `false` | no |
 | group\_name | A group to control the project by being assigned group\_role (defaults to project editor) | `string` | `""` | no |
 | group\_role | The role to give the controlling group (group\_name) over the project (defaults to project editor) | `string` | `"roles/editor"` | no |
diff --git a/examples/shared_vpc/README.md b/examples/shared_vpc/README.md
@@ -28,6 +28,7 @@ It then attaches two new service projects to the host project.
 | network\_self\_link | The URI of the VPC being created |
 | service\_project | The service project info |
 | service\_project\_b | The second service project |
+| service\_project\_c | The third service project |
 | subnets | The shared VPC subets |
 | vpc | The network info |
 
diff --git a/examples/shared_vpc/main.tf b/examples/shared_vpc/main.tf
@@ -156,6 +156,40 @@ module "service-project-b" {
   disable_services_on_destroy = false
 }
 
+/******************************************
+  Third Service Project Creation
+  To test the grant_services_network_role
+ *****************************************/
+module "service-project-c" {
+  source = "../../modules/svpc_service_project"
+
+  name              = "c-${var.service_project_name}"
+  random_project_id = false
+
+  org_id          = var.organization_id
+  folder_id       = var.folder_id
+  billing_account = var.billing_account
+
+  shared_vpc = module.host-project.project_id
+
+  activate_apis = [
+    "compute.googleapis.com",
+    "container.googleapis.com",
+    "dataproc.googleapis.com",
+  ]
+
+  activate_api_identities = [{
+    api = "healthcare.googleapis.com"
+    roles = [
+      "roles/healthcare.serviceAgent",
+      "roles/bigquery.jobUser",
+    ]
+  }]
+
+  disable_services_on_destroy = false
+  grant_services_network_role = false
+}
+
 /******************************************
   Example dependency on service-project
  *****************************************/
diff --git a/examples/shared_vpc/outputs.tf b/examples/shared_vpc/outputs.tf
@@ -34,6 +34,11 @@ output "service_project_b" {
   description = "The second service project"
 }
 
+output "service_project_c" {
+  value       = module.service-project-c
+  description = "The third service project"
+}
+
 output "vpc" {
   value       = module.vpc
   description = "The network info"
diff --git a/modules/shared_vpc_access/README.md b/modules/shared_vpc_access/README.md
@@ -29,6 +29,7 @@ module "shared_vpc_access" {
 |------|-------------|------|---------|:--------:|
 | active\_apis | The list of active apis on the service project. If api is not active this module will not try to activate it | `list(string)` | `[]` | no |
 | enable\_shared\_vpc\_service\_project | Flag set if SVPC enabled | `bool` | n/a | yes |
+| grant\_services\_network\_role | Whether or not to grant service agents the network roles on the host project | `bool` | `true` | no |
 | grant\_services\_security\_admin\_role | Whether or not to grant Kubernetes Engine Service Agent the Security Admin role on the host project so it can manage firewall rules | `bool` | `false` | no |
 | host\_project\_id | The ID of the host project which hosts the shared VPC | `string` | n/a | yes |
 | lookup\_project\_numbers | Whether to look up the project numbers from data sources. If false, `service_project_number` will be used instead. | `bool` | `true` | no |
diff --git a/modules/shared_vpc_access/main.tf b/modules/shared_vpc_access/main.tf
@@ -45,7 +45,7 @@ locals {
  *****************************************/
 resource "google_compute_subnetwork_iam_member" "service_shared_vpc_subnet_users" {
   provider = google-beta
-  count    = length(local.subnetwork_api)
+  count    = var.grant_services_network_role ? length(local.subnetwork_api) : 0
   subnetwork = element(
     split("/", local.subnetwork_api[count.index][1]),
     index(
@@ -68,7 +68,7 @@ resource "google_compute_subnetwork_iam_member" "service_shared_vpc_subnet_users
  if "dataflow.googleapis.com" compute.networkUser role granted to dataflow service account for Dataflow on shared VPC Project if no subnets defined
  *****************************************/
 resource "google_project_iam_member" "service_shared_vpc_user" {
-  for_each = (length(var.shared_vpc_subnets) == 0) && var.enable_shared_vpc_service_project ? local.active_apis : []
+  for_each = (length(var.shared_vpc_subnets) == 0) && var.enable_shared_vpc_service_project && var.grant_services_network_role ? local.active_apis : []
   project  = var.host_project_id
   role     = "roles/compute.networkUser"
   member   = format("serviceAccount:%s", local.apis[each.value])
@@ -79,7 +79,7 @@ resource "google_project_iam_member" "service_shared_vpc_user" {
   See: https://cloud.google.com/composer/docs/how-to/managing/configuring-shared-vpc
  *****************************************/
 resource "google_project_iam_member" "composer_host_agent" {
-  count   = local.composer_shared_vpc_enabled && var.enable_shared_vpc_service_project ? 1 : 0
+  count   = local.composer_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_services_network_role ? 1 : 0
   project = var.host_project_id
   role    = "roles/composer.sharedVpcAgent"
   member  = format("serviceAccount:%s", local.apis["composer.googleapis.com"])
@@ -90,7 +90,7 @@ resource "google_project_iam_member" "composer_host_agent" {
   See: https://cloud.google.com/kubernetes-engine/docs/how-to/cluster-shared-vpc
  *****************************************/
 resource "google_project_iam_member" "gke_host_agent" {
-  count   = local.gke_shared_vpc_enabled && var.enable_shared_vpc_service_project ? 1 : 0
+  count   = local.gke_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_services_network_role ? 1 : 0
   project = var.host_project_id
   role    = "roles/container.hostServiceAgentUser"
   member  = format("serviceAccount:%s", local.apis["container.googleapis.com"])
diff --git a/modules/shared_vpc_access/variables.tf b/modules/shared_vpc_access/variables.tf
@@ -58,3 +58,9 @@ variable "grant_services_security_admin_role" {
   type        = bool
   default     = false
 }
+
+variable "grant_services_network_role" {
+  description = "Whether or not to grant service agents the network roles on the host project"
+  type        = bool
+  default     = true
+}
diff --git a/modules/svpc_service_project/README.md b/modules/svpc_service_project/README.md
@@ -49,6 +49,7 @@ module "service-project" {
 | disable\_services\_on\_destroy | Whether project services will be disabled when the resources are destroyed | `bool` | `true` | no |
 | domain | The domain name (optional). | `string` | `""` | no |
 | folder\_id | The ID of a folder to host this project | `string` | `""` | no |
+| grant\_services\_network\_role | Whether or not to grant service agents the network roles on the host project | `bool` | `true` | no |
 | grant\_services\_security\_admin\_role | Whether or not to grant Kubernetes Engine Service Agent the Security Admin role on the host project so it can manage firewall rules | `bool` | `false` | no |
 | group\_name | A group to control the project by being assigned group\_role (defaults to project editor) | `string` | `""` | no |
 | group\_role | The role to give the controlling group (group\_name) over the project (defaults to project editor) | `string` | `"roles/editor"` | no |
diff --git a/modules/svpc_service_project/main.tf b/modules/svpc_service_project/main.tf
@@ -72,6 +72,7 @@ module "shared_vpc_access" {
   service_project_number             = module.project-factory.project_number
   lookup_project_numbers             = false
   grant_services_security_admin_role = var.grant_services_security_admin_role
+  grant_services_network_role        = var.grant_services_network_role
 }
 
 /******************************************
diff --git a/modules/svpc_service_project/variables.tf b/modules/svpc_service_project/variables.tf
@@ -215,3 +215,9 @@ variable "grant_services_security_admin_role" {
   type        = bool
   default     = false
 }
+
+variable "grant_services_network_role" {
+  description = "Whether or not to grant service agents the network roles on the host project"
+  type        = bool
+  default     = true
+}
diff --git a/test/fixtures/dynamic_shared_vpc/outputs.tf b/test/fixtures/dynamic_shared_vpc/outputs.tf
@@ -27,7 +27,8 @@ output "service_project_id" {
 output "service_project_ids" {
   value = [
     module.example.service_project.project_id,
-    module.example.service_project_b.project_id
+    module.example.service_project_b.project_id,
+    module.example.service_project_c.project_id
   ]
   description = "The service project IDs"
 }
@@ -42,6 +43,11 @@ output "service_project_b_number" {
   description = "The service project b number"
 }
 
+output "service_project_c_number" {
+  value       = module.example.service_project_c.project_number
+  description = "The service project c number"
+}
+
 output "service_account_email" {
   value       = module.example.service_project.service_account_email
   description = "The service account email"
diff --git a/test/integration/dynamic_shared_vpc/controls/svpc.rb b/test/integration/dynamic_shared_vpc/controls/svpc.rb
@@ -16,6 +16,7 @@
 service_project_ids         = attribute('service_project_ids')
 service_project_number      = attribute('service_project_number')
 service_project_b_number    = attribute('service_project_b_number')
+service_project_c_number    = attribute('service_project_c_number')
 service_account_email       = attribute('service_account_email')
 shared_vpc                  = attribute('shared_vpc')
 shared_vpc_subnet_name_01   = attribute('shared_vpc_subnet_name_01')
@@ -77,6 +78,15 @@
           role: "roles/compute.networkUser",
         )
       end
+
+      it "service project c with explicit subnets and grant_services_network_role flag set to false does not include the GKE service account in the roles/compute.networkUser IAM binding" do
+        expect(bindings).not_to include(
+          members: including(
+            "serviceAccount:service-#{service_project_c_number}@container-engine-robot.iam.gserviceaccount.com"
+          ),
+          role: "roles/compute.networkUser",
+        )
+      end
     end
 
     it "service project b without explicit subnets includes the GKE service account in the roles/compute.networkUser IAM binding" do
@@ -86,12 +96,26 @@
       )
     end
 
+    it "service project c without explicit subnets and grant_services_network_role flag set to false does not include the GKE service account in the roles/compute.networkUser IAM binding" do
+      expect(bindings).not_to include(
+        members: including("serviceAccount:service-#{service_project_c_number}@container-engine-robot.iam.gserviceaccount.com"),
+        role: "roles/compute.networkUser",
+      )
+    end
+
   it "service project b without explicit subnets includes the dataproc service account in the roles/compute.networkUser IAM binding" do
     expect(bindings).to include(
       members: including("serviceAccount:service-#{service_project_b_number}@dataproc-accounts.iam.gserviceaccount.com"),
       role: "roles/compute.networkUser",
     )
   end
+
+  it "service project c without explicit subnets and grant_services_network_role flag set to false does not include the dataproc service account in the roles/compute.networkUser IAM binding" do
+    expect(bindings).not_to include(
+      members: including("serviceAccount:service-#{service_project_c_number}@dataproc-accounts.iam.gserviceaccount.com"),
+      role: "roles/compute.networkUser",
+    )
+  end
 end
 
   describe command("gcloud beta compute networks subnets get-iam-policy #{shared_vpc_subnet_name_01} --region #{shared_vpc_subnet_region_01} --project #{shared_vpc} --format=json") do
diff --git a/test/integration/dynamic_shared_vpc/inspec.yml b/test/integration/dynamic_shared_vpc/inspec.yml
@@ -19,6 +19,9 @@ attributes:
   - name: service_project_b_number
     required: true
     type: string
+  - name: service_project_c_number
+    required: true
+    type: string
   - name: service_account_email
     required: true
     type: string
diff --git a/variables.tf b/variables.tf
@@ -259,6 +259,12 @@ variable "grant_services_security_admin_role" {
   default     = false
 }
 
+variable "grant_services_network_role" {
+  description = "Whether or not to grant service agents the network roles on the host project"
+  type        = bool
+  default     = true
+}
+
 variable "consumer_quotas" {
   description = "The quotas configuration you want to override for the project."
   type = list(object({

Original file line number	Diff line number	Diff line change
`@@ -72,6 +72,7 @@ module "shared_vpc_access" {`
`72`	`72`	`service_project_number = module.project-factory.project_number`
`73`	`73`	`lookup_project_numbers = false`
`74`	`74`	`grant_services_security_admin_role = var.grant_services_security_admin_role`
	`75`	`+ grant_services_network_role = var.grant_services_network_role`
`75`	`76`	`}`
`76`	`77`
`77`	`78`	`/******************************************`