From a03c5e8d86f8157f4b63adec9bc40f16f321d342 Mon Sep 17 00:00:00 2001
From: Imran Nayer <29933900+imrannayer@users.noreply.github.com>
Date: Wed, 1 Mar 2023 13:20:47 -0600
Subject: [PATCH] feat: added datastream support in shared_vpc_access module
 (#788)

---
 CONTRIBUTING.md                               |  6 ++++++
 Makefile                                      | 20 +++++++++++--------
 build/int.cloudbuild.yaml                     |  3 ++-
 build/lint.cloudbuild.yaml                    |  2 +-
 modules/shared_vpc_access/README.md           |  5 ++++-
 modules/shared_vpc_access/main.tf             | 20 ++++++++++++++++---
 modules/shared_vpc_access/variables.tf        |  6 ++++++
 test/fixtures/shared_vpc_no_subnets/main.tf   |  2 ++
 .../shared_vpc_no_subnets/controls/gcloud.rb  | 11 ++++++++++
 test/setup/outputs.tf                         |  4 ++++
 test/setup/variables.tf                       |  4 ++++
 11 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 1b5fd04c..bb441e8a 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -81,6 +81,12 @@ export TF_VAR_folder_id="your_folder_id"
 export TF_VAR_billing_account="your_billing_account_id"
 export TF_VAR_gsuite_admin_email="your_gsuite_admin_email"
 export TF_VAR_gsuite_domain="your_gsuite_domain"
+export TF_VAR_policy_id="your_access_context_manager_policy_id"
+```
+
+you can find Access Context Manager policy ID by executing following command
+```bash
+gcloud access-context-manager policies list --organization="your_org_id"
 ```
 
 With these settings in place, you can prepare the test setup using Docker:
diff --git a/Makefile b/Makefile
index d6f57374..1aa49dd8 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@
 # Make will use bash instead of sh
 SHELL := /usr/bin/env bash
 
-DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 1
+DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 1.8
 DOCKER_IMAGE_DEVELOPER_TOOLS := cft/developer-tools
 REGISTRY_URL := gcr.io/cloud-foundation-cicd
 
@@ -32,6 +32,7 @@ docker_run:
 		-e TF_VAR_billing_account \
 		-e TF_VAR_gsuite_admin_email \
 		-e TF_VAR_gsuite_domain \
+		-e TF_VAR_policy_id \
 		-v "${CURDIR}":/workspace \
 		$(REGISTRY_URL)/${DOCKER_IMAGE_DEVELOPER_TOOLS}:${DOCKER_TAG_VERSION_DEVELOPER_TOOLS} \
 		/bin/bash
@@ -46,6 +47,7 @@ docker_test_prepare:
 		-e TF_VAR_billing_account \
 		-e TF_VAR_gsuite_admin_email \
 		-e TF_VAR_gsuite_domain \
+		-e TF_VAR_policy_id \
 		-v "${CURDIR}":/workspace \
 		$(REGISTRY_URL)/${DOCKER_IMAGE_DEVELOPER_TOOLS}:${DOCKER_TAG_VERSION_DEVELOPER_TOOLS} \
 		/usr/local/bin/execute_with_credentials.sh prepare_environment
@@ -58,8 +60,9 @@ docker_test_cleanup:
 		-e TF_VAR_org_id \
 		-e TF_VAR_folder_id \
 		-e TF_VAR_billing_account \
-                -e TF_VAR_gsuite_admin_email \
-                -e TF_VAR_gsuite_domain \
+		-e TF_VAR_gsuite_admin_email \
+		-e TF_VAR_gsuite_domain \
+		-e TF_VAR_policy_id \
 		-v "${CURDIR}":/workspace \
 		$(REGISTRY_URL)/${DOCKER_IMAGE_DEVELOPER_TOOLS}:${DOCKER_TAG_VERSION_DEVELOPER_TOOLS} \
 		/usr/local/bin/execute_with_credentials.sh cleanup_environment
@@ -69,11 +72,12 @@ docker_test_cleanup:
 docker_test_integration:
 	docker run --rm -it \
 		-e SERVICE_ACCOUNT_JSON \
-                -e TF_VAR_org_id \
-                -e TF_VAR_folder_id \
-                -e TF_VAR_billing_account \
-                -e TF_VAR_gsuite_admin_email \
-                -e TF_VAR_gsuite_domain \
+		-e TF_VAR_org_id \
+		-e TF_VAR_folder_id \
+		-e TF_VAR_billing_account \
+		-e TF_VAR_gsuite_admin_email \
+		-e TF_VAR_gsuite_domain \
+		-e TF_VAR_policy_id \
 		-v "${CURDIR}":/workspace \
 		$(REGISTRY_URL)/${DOCKER_IMAGE_DEVELOPER_TOOLS}:${DOCKER_TAG_VERSION_DEVELOPER_TOOLS} \
 		/usr/local/bin/test_integration.sh
diff --git a/build/int.cloudbuild.yaml b/build/int.cloudbuild.yaml
index 179c3f8b..2b82c920 100644
--- a/build/int.cloudbuild.yaml
+++ b/build/int.cloudbuild.yaml
@@ -165,7 +165,7 @@ tags:
 - 'integration'
 substitutions:
   _DOCKER_IMAGE_DEVELOPER_TOOLS: 'cft/developer-tools'
-  _DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '1'
+  _DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '1.8'
 options:
   machineType: 'N1_HIGHCPU_8'
   env:
@@ -175,3 +175,4 @@ options:
     - 'TF_VAR_gsuite_admin_email=project-factory-test-admin@test.infra.cft.tips'
     - 'TF_VAR_gsuite_domain=test.infra.cft.tips'
     - 'TF_VAR_domain=test.infra.cft.tips.'
+    - 'TF_VAR_policy_id=$_POLICY_ID'
diff --git a/build/lint.cloudbuild.yaml b/build/lint.cloudbuild.yaml
index 86fc6da2..ec096c8e 100644
--- a/build/lint.cloudbuild.yaml
+++ b/build/lint.cloudbuild.yaml
@@ -21,4 +21,4 @@ tags:
 - 'lint'
 substitutions:
   _DOCKER_IMAGE_DEVELOPER_TOOLS: 'cft/developer-tools'
-  _DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '1'
+  _DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '1.8'
diff --git a/modules/shared_vpc_access/README.md b/modules/shared_vpc_access/README.md
index 3d04fa48..dde1ee94 100644
--- a/modules/shared_vpc_access/README.md
+++ b/modules/shared_vpc_access/README.md
@@ -10,10 +10,12 @@ module "shared_vpc_access" {
   host_project_id     = var.shared_vpc
   service_project_id  = var.service_project
   active_apis         = [
-    "compute.googleapis.com",
     "container.googleapis.com",
     "dataproc.googleapis.com",
     "dataflow.googleapis.com",
+    "datastream.googleapis.com",
+    "composer.googleapis.com",
+    "vpcaccess.googleapis.com",
   ]
   shared_vpc_subnets  = [
     "projects/pf-ci-shared2/regions/us-west1/subnetworks/shared-network-subnet-01",
@@ -30,6 +32,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\_network\_role | Whether or not to grant service agents the network roles on the host project | `bool` | `true` | no |
+| grant\_services\_network\_admin\_role | Whether or not to grant Datastream Service acount the Network Admin role on the host project so it can manage firewall rules | `bool` | `false` | 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 7f1b2462..a4993ca1 100644
--- a/modules/shared_vpc_access/main.tf
+++ b/modules/shared_vpc_access/main.tf
@@ -27,10 +27,12 @@ locals {
     "dataflow.googleapis.com" : format("service-%s@dataflow-service-producer-prod.iam.gserviceaccount.com", local.service_project_number),
     "composer.googleapis.com" : format("service-%s@cloudcomposer-accounts.iam.gserviceaccount.com", local.service_project_number)
     "vpcaccess.googleapis.com" : format("service-%s@gcp-sa-vpcaccess.iam.gserviceaccount.com", local.service_project_number)
+    "datastream.googleapis.com" : format("service-%s@gcp-sa-datastream.iam.gserviceaccount.com", local.service_project_number)
   }
-  gke_shared_vpc_enabled      = contains(var.active_apis, "container.googleapis.com")
-  composer_shared_vpc_enabled = contains(var.active_apis, "composer.googleapis.com")
-  active_apis                 = [for api in keys(local.apis) : api if contains(var.active_apis, api)]
+  gke_shared_vpc_enabled        = contains(var.active_apis, "container.googleapis.com")
+  composer_shared_vpc_enabled   = contains(var.active_apis, "composer.googleapis.com")
+  datastream_shared_vpc_enabled = contains(var.active_apis, "datastream.googleapis.com")
+  active_apis                   = [for api in keys(local.apis) : api if contains(var.active_apis, api)]
   # Can't use setproduct due to https://github.com/terraform-google-modules/terraform-google-project-factory/issues/635
   subnetwork_api = length(var.shared_vpc_subnets) != 0 ? flatten([
     for i, api in local.active_apis : [for i, subnet in var.shared_vpc_subnets : "${api},${subnet}"]
@@ -113,3 +115,15 @@ resource "google_project_iam_member" "gke_security_admin" {
   role    = "roles/compute.securityAdmin"
   member  = format("serviceAccount:%s", local.apis["container.googleapis.com"])
 }
+
+/******************************************
+  roles/compute.networkAdmin role granted to Datastream's service account for datastream connectivity configuration on shared VPC host project
+  See: https://cloud.google.com/datastream/docs/create-a-private-connectivity-configuration
+  Service Account: service-[project_number]@gcp-sa-datastream.iam.gserviceaccount.com
+ *****************************************/
+resource "google_project_iam_member" "datastream_network_admin" {
+  count   = local.datastream_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_services_network_admin_role ? 1 : 0
+  project = var.host_project_id
+  role    = "roles/compute.networkAdmin"
+  member  = format("serviceAccount:%s", local.apis["datastream.googleapis.com"])
+}
diff --git a/modules/shared_vpc_access/variables.tf b/modules/shared_vpc_access/variables.tf
index d798f632..ad25a6b6 100644
--- a/modules/shared_vpc_access/variables.tf
+++ b/modules/shared_vpc_access/variables.tf
@@ -59,6 +59,12 @@ variable "grant_services_security_admin_role" {
   default     = false
 }
 
+variable "grant_services_network_admin_role" {
+  description = "Whether or not to grant Datastream Service acount the Network Admin role on the host project so it can manage firewall rules"
+  type        = bool
+  default     = false
+}
+
 variable "grant_network_role" {
   description = "Whether or not to grant service agents the network roles on the host project"
   type        = bool
diff --git a/test/fixtures/shared_vpc_no_subnets/main.tf b/test/fixtures/shared_vpc_no_subnets/main.tf
index d6d2ea1b..84360ac1 100644
--- a/test/fixtures/shared_vpc_no_subnets/main.tf
+++ b/test/fixtures/shared_vpc_no_subnets/main.tf
@@ -63,11 +63,13 @@ module "project-factory" {
   group_name                        = "pf-secondgroup-${var.random_string_for_testing}"
   shared_vpc                        = var.shared_vpc
   enable_shared_vpc_service_project = true
+  grant_services_network_admin_role = true
 
   activate_apis = [
     "compute.googleapis.com",
     "container.googleapis.com",
     "dataflow.googleapis.com",
+    "datastream.googleapis.com",
   ]
 
   disable_services_on_destroy = false
diff --git a/test/integration/shared_vpc_no_subnets/controls/gcloud.rb b/test/integration/shared_vpc_no_subnets/controls/gcloud.rb
index f561e16e..17ba482d 100644
--- a/test/integration/shared_vpc_no_subnets/controls/gcloud.rb
+++ b/test/integration/shared_vpc_no_subnets/controls/gcloud.rb
@@ -39,6 +39,17 @@
       end
     end
 
+    describe "roles/compute.networkAdmin" do
+      it "includes the Datastream service account in the roles/compute.networkAdmin IAM binding" do
+        expect(bindings).to include(
+          members: including(
+            "serviceAccount:service-#{project_number}@gcp-sa-datastream.iam.gserviceaccount.com"
+          ),
+          role: "roles/compute.networkAdmin",
+        )
+      end
+    end
+
     describe "roles/compute.networkUser" do
       it "includes the project service account in the roles/compute.networkUser IAM binding" do
         expect(bindings).to include(
diff --git a/test/setup/outputs.tf b/test/setup/outputs.tf
index 27a1631a..13e3cc5e 100644
--- a/test/setup/outputs.tf
+++ b/test/setup/outputs.tf
@@ -58,3 +58,7 @@ output "group_name" {
 output "service_account_email" {
   value = google_service_account.int_test.email
 }
+
+output "policy_id" {
+  value = var.policy_id
+}
diff --git a/test/setup/variables.tf b/test/setup/variables.tf
index 20a64769..59ddeb56 100644
--- a/test/setup/variables.tf
+++ b/test/setup/variables.tf
@@ -34,3 +34,7 @@ variable "gsuite_domain" {
   description = "Gsuite domain"
 }
 
+variable "policy_id" {
+  type        = string
+  description = "The ID of the access context manager policy the perimeter lies in"
+}