diff --git a/README.md b/README.md index 665019bb..9b7a15fc 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ determining that location is as follows: | 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 | | essential\_contacts | A mapping of users or groups to be assigned as Essential Contacts to the project, specifying a notification category | `map(list(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\_network\_role | Whether or not to grant networkUser role on the host project/subnets | `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/docs/upgrading_to_project_factory_v13.0.md b/docs/upgrading_to_project_factory_v13.0.md new file mode 100644 index 00000000..06260395 --- /dev/null +++ b/docs/upgrading_to_project_factory_v13.0.md @@ -0,0 +1,36 @@ +# Upgrading to Project Factory v13.0 + +The v13.0 release of Project Factory is a backwards incompatible release. + +## Migration Instructions + +### `grant_services_network_role` renamed to `grant_network_role` + +Variable `grant_services_network_role` is renamed to `grant_network_role` to provide the ability to not manage networkUser role through project factory module v13.0 + +```diff + module "project-factory" { + source = "terraform-google-modules/project-factory/google" +- version = "~> 12.0" ++ version = "~> 13.0" + + name = "pf-test-1" + random_project_id = "true" + org_id = "1234567890" + usage_bucket_name = "pf-test-1-usage-report-bucket" + usage_bucket_prefix = "pf/test/1/integration" + billing_account = "ABCDEF-ABCDEF-ABCDEF" +- grant_services_network_role = "..." ++ grant_network_role = "..." + } +``` + +Service accounts principles on which networkUser can be managed through `grant_network_role` variable. +- Project default service account +- [Google APIs service agent](https://cloud.google.com/compute/docs/access/service-accounts#google_apis_service_agent) +- group_email +- dataflow, dataproc, composer, container, and vpcaccess API [agent accounts](https://github.com/terraform-google-modules/terraform-google-project-factory/blob/616ede9456cc8f86ef7995192af3473d17ee7946/modules/shared_vpc_access/main.tf#L24-L30). + +Additional roles that are managed through `grant_network_role` variable. +- roles/container.hostServiceAgentUser on "serviceAccount:service-{PROJECT-NUMBER}@container-engine-robot.iam.gserviceaccount.com +- roles/composer.sharedVpcAgent on "service-{PROJECT-NUMBER}@cloudcomposer-accounts.iam.gserviceaccount.com" diff --git a/examples/shared_vpc/main.tf b/examples/shared_vpc/main.tf index 0060e4f6..02955131 100644 --- a/examples/shared_vpc/main.tf +++ b/examples/shared_vpc/main.tf @@ -140,7 +140,7 @@ module "service-project-b" { /****************************************** Third Service Project Creation - To test the grant_services_network_role + To test the grant_network_role *****************************************/ module "service-project-c" { source = "../../modules/svpc_service_project" @@ -152,12 +152,15 @@ module "service-project-c" { folder_id = var.folder_id billing_account = var.billing_account - shared_vpc = module.host-project.project_id + shared_vpc = module.host-project.project_id + shared_vpc_subnets = module.vpc.subnets_self_links activate_apis = [ "compute.googleapis.com", "container.googleapis.com", "dataproc.googleapis.com", + "composer.googleapis.com", + "dataflow.googleapis.com" ] activate_api_identities = [{ @@ -169,7 +172,7 @@ module "service-project-c" { }] disable_services_on_destroy = false - grant_services_network_role = false + grant_network_role = false } /****************************************** diff --git a/main.tf b/main.tf index a7c04a15..9411d108 100644 --- a/main.tf +++ b/main.tf @@ -39,6 +39,7 @@ module "project-factory" { shared_vpc = var.svpc_host_project_id enable_shared_vpc_service_project = var.svpc_host_project_id != "" enable_shared_vpc_host_project = var.enable_shared_vpc_host_project + grant_network_role = var.grant_network_role billing_account = var.billing_account folder_id = var.folder_id create_project_sa = var.create_project_sa @@ -79,7 +80,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 + grant_network_role = var.grant_network_role } /****************************************** diff --git a/modules/core_project_factory/main.tf b/modules/core_project_factory/main.tf index 095592af..30882590 100644 --- a/modules/core_project_factory/main.tf +++ b/modules/core_project_factory/main.tf @@ -179,7 +179,7 @@ resource "google_service_account_iam_member" "service_account_grant_to_group" { compute.networkUser role granted to G Suite group, APIs Service account, and Project Service Account *****************************************************************************************************************/ resource "google_project_iam_member" "controlling_group_vpc_membership" { - count = var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) == 0 ? local.shared_vpc_users_length : 0 + count = var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) == 0 ? local.shared_vpc_users_length : 0 project = var.shared_vpc role = "roles/compute.networkUser" @@ -195,7 +195,7 @@ resource "google_project_iam_member" "controlling_group_vpc_membership" { *************************************************************************************/ resource "google_compute_subnetwork_iam_member" "service_account_role_to_vpc_subnets" { provider = google-beta - count = var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 && var.create_project_sa ? length(var.shared_vpc_subnets) : 0 + count = var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 && var.create_project_sa ? length(var.shared_vpc_subnets) : 0 subnetwork = element( split("/", var.shared_vpc_subnets[count.index]), @@ -219,7 +219,7 @@ resource "google_compute_subnetwork_iam_member" "service_account_role_to_vpc_sub resource "google_compute_subnetwork_iam_member" "group_role_to_vpc_subnets" { provider = google-beta - count = var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 && var.manage_group ? length(var.shared_vpc_subnets) : 0 + count = var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 && var.manage_group ? length(var.shared_vpc_subnets) : 0 subnetwork = element( split("/", var.shared_vpc_subnets[count.index]), index( @@ -242,7 +242,7 @@ resource "google_compute_subnetwork_iam_member" "group_role_to_vpc_subnets" { resource "google_compute_subnetwork_iam_member" "apis_service_account_role_to_vpc_subnets" { provider = google-beta - count = var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 ? length(var.shared_vpc_subnets) : 0 + count = var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 ? length(var.shared_vpc_subnets) : 0 subnetwork = element( split("/", var.shared_vpc_subnets[count.index]), index( diff --git a/modules/core_project_factory/variables.tf b/modules/core_project_factory/variables.tf index 5f7b0e2b..381d3cda 100644 --- a/modules/core_project_factory/variables.tf +++ b/modules/core_project_factory/variables.tf @@ -233,3 +233,9 @@ variable "default_network_tier" { type = string default = "" } + +variable "grant_network_role" { + description = "Whether or not to grant networkUser role on the host project/subnets" + type = bool + default = true +} diff --git a/modules/shared_vpc_access/README.md b/modules/shared_vpc_access/README.md index 7fd89dde..d68a989b 100644 --- a/modules/shared_vpc_access/README.md +++ b/modules/shared_vpc_access/README.md @@ -29,7 +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\_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 index 47473f22..a22135aa 100644 --- a/modules/shared_vpc_access/main.tf +++ b/modules/shared_vpc_access/main.tf @@ -48,7 +48,7 @@ locals { *****************************************/ resource "google_compute_subnetwork_iam_member" "service_shared_vpc_subnet_users" { provider = google-beta - count = var.grant_services_network_role ? length(local.subnetwork_api) : 0 + count = var.grant_network_role ? length(local.subnetwork_api) : 0 subnetwork = element( split("/", split(",", local.subnetwork_api[count.index])[1]), index( @@ -71,7 +71,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 && var.grant_services_network_role ? toset(local.active_apis) : [] + for_each = (length(var.shared_vpc_subnets) == 0) && var.enable_shared_vpc_service_project && var.grant_network_role ? toset(local.active_apis) : [] project = var.host_project_id role = "roles/compute.networkUser" member = format("serviceAccount:%s", local.apis[each.value]) @@ -82,7 +82,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 && var.grant_services_network_role ? 1 : 0 + count = local.composer_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_network_role ? 1 : 0 project = var.host_project_id role = "roles/composer.sharedVpcAgent" member = format("serviceAccount:%s", local.apis["composer.googleapis.com"]) @@ -93,7 +93,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 && var.grant_services_network_role ? 1 : 0 + count = local.gke_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_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 index e2e274c4..d798f632 100644 --- a/modules/shared_vpc_access/variables.tf +++ b/modules/shared_vpc_access/variables.tf @@ -59,7 +59,7 @@ variable "grant_services_security_admin_role" { default = false } -variable "grant_services_network_role" { +variable "grant_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 index 80b86947..88db4cfa 100644 --- a/modules/svpc_service_project/README.md +++ b/modules/svpc_service_project/README.md @@ -50,7 +50,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\_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 index aca02503..c00ad9d9 100755 --- a/modules/svpc_service_project/main.tf +++ b/modules/svpc_service_project/main.tf @@ -38,6 +38,7 @@ module "project-factory" { project_id = var.project_id shared_vpc = var.shared_vpc enable_shared_vpc_service_project = true + grant_network_role = var.grant_network_role billing_account = var.billing_account folder_id = var.folder_id create_project_sa = var.create_project_sa @@ -73,7 +74,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 + grant_network_role = var.grant_network_role } /****************************************** diff --git a/modules/svpc_service_project/variables.tf b/modules/svpc_service_project/variables.tf index b8884eb6..ada6e4f5 100755 --- a/modules/svpc_service_project/variables.tf +++ b/modules/svpc_service_project/variables.tf @@ -216,7 +216,7 @@ variable "grant_services_security_admin_role" { default = false } -variable "grant_services_network_role" { +variable "grant_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/integration/dynamic_shared_vpc/controls/svpc.rb b/test/integration/dynamic_shared_vpc/controls/svpc.rb index 09ad2dce..fa0cd28c 100644 --- a/test/integration/dynamic_shared_vpc/controls/svpc.rb +++ b/test/integration/dynamic_shared_vpc/controls/svpc.rb @@ -79,7 +79,7 @@ ) 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 + it "service project c with explicit subnets and grant_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" @@ -96,7 +96,7 @@ ) 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 + it "service project c with explicit subnets and grant_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", @@ -110,7 +110,7 @@ ) 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 + it "service project c with explicit subnets and grant_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", @@ -148,6 +148,15 @@ end end + describe "roles/compute.networkUser" do + it "service project with explicit subnets includes project default service account in the roles/compute.networkUser IAM binding" do + expect(bindings).to include( + members: including("serviceAccount:project-service-account@#{service_project_ids[0]}.iam.gserviceaccount.com"), + role: "roles/compute.networkUser", + ) + end + end + describe "roles/compute.networkUser" do it "service project with explicit subnets includes the dataflow service account in the roles/compute.networkUser IAM binding" do expect(bindings).to include( @@ -157,6 +166,42 @@ ) end end + + it "service project c with explicit subnets and grant_network_role flag set to false does not include project default service account in the roles/compute.networkUser IAM binding" do + expect(bindings).not_to include( + members: including( + "serviceAccount:project-service-account@#{service_project_ids[2]}.iam.gserviceaccount.com" + ), + role: "roles/compute.networkUser", + ) + end + + it "service project c with explicit subnets and grant_network_role flag set to false does not include the GCP Compute agent service account in the roles/compute.networkUser IAM binding" do + expect(bindings).not_to include( + members: including( + "serviceAccount:#{service_project_c_number}@cloudservices.gserviceaccount.com" + ), + role: "roles/compute.networkUser", + ) + end + + it "service project c with explicit subnets and grant_network_role flag set to false does not include the GCP Dataflow agent service account in the roles/compute.networkUser IAM binding" do + expect(bindings).not_to include( + members: including( + "serviceAccount:service-#{service_project_c_number}@dataflow-service-producer-prod.iam.gserviceaccount.com" + ), + role: "roles/compute.networkUser", + ) + end + + it "service project c with explicit subnets and grant_network_role flag set to false does not include the GCP Dataproc agent 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_02} --region #{shared_vpc_subnet_region_02} --project #{shared_vpc} --format=json") do diff --git a/variables.tf b/variables.tf index 242f23c6..8e420e12 100644 --- a/variables.tf +++ b/variables.tf @@ -275,8 +275,8 @@ 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" +variable "grant_network_role" { + description = "Whether or not to grant networkUser role on the host project/subnets" type = bool default = true }