From 55ade204c6eb74c522273f920939190a820cd831 Mon Sep 17 00:00:00 2001 From: Mirko Montanari Date: Mon, 30 Sep 2019 17:56:10 -0700 Subject: [PATCH] Add a parameter 'registry_project_id' The PR allows configuring the project holding the GCR registry when used in connection with 'create_service_account'=true and grant_registry_access=true. Holding the GCR is a project with other resources increases the risk of exposing sensitive data to the service account running the nodes, as the required permissions of role roles/storage.objectViewer provide access to all storage objects in the project. --- README.md | 20 ++++++++++ autogen/README.md | 3 ++ autogen/sa.tf | 2 +- autogen/variables.tf | 6 +++ examples/workload_metadata_config/main.tf | 5 ++- .../workload_metadata_config/variables.tf | 5 +-- modules/beta-private-cluster/README.md | 4 ++ modules/beta-private-cluster/sa.tf | 2 +- modules/beta-private-cluster/variables.tf | 6 +++ modules/beta-public-cluster/README.md | 4 ++ modules/beta-public-cluster/sa.tf | 2 +- modules/beta-public-cluster/variables.tf | 6 +++ modules/private-cluster/README.md | 4 ++ modules/private-cluster/sa.tf | 2 +- modules/private-cluster/variables.tf | 6 +++ sa.tf | 2 +- test/fixtures/shared/outputs.tf | 3 ++ test/fixtures/shared/variables.tf | 4 ++ .../workload_metadata_config/example.tf | 37 ++++++++++++++++++- .../controls/gcloud.rb | 18 +++++++++ .../workload_metadata_config/inspec.yml | 6 +++ test/setup/make_source.sh | 3 ++ variables.tf | 6 +++ 23 files changed, 145 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 50e2afd63b..02f020d54a 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,22 @@ Then perform the following commands on the root folder: - `terraform apply` to apply the infrastructure build - `terraform destroy` to destroy the built infrastructure +## Upgrade to v3.0.0 + +v3.0.0 is a breaking release. Refer to the +[Upgrading to v3.0 guide][upgrading-to-v3.0] for details. + +## Upgrade to v2.0.0 + +v2.0.0 is a breaking release. Refer to the +[Upgrading to v2.0 guide][upgrading-to-v2.0] for details. + +## Upgrade to v1.0.0 + +Version 1.0.0 of this module introduces a breaking change: adding the `disable-legacy-endpoints` metadata field to all node pools. This metadata is required by GKE and [determines whether the `/0.1/` and `/v1beta1/` paths are available in the nodes' metadata server](https://cloud.google.com/kubernetes-engine/docs/how-to/protecting-cluster-metadata#disable-legacy-apis). If your applications do not require access to the node's metadata server, you can leave the default value of `true` provided by the module. If your applications require access to the metadata server, be sure to read the linked documentation to see if you need to set the value for this field to `false` to allow your applications access to the above metadata server paths. + +In either case, upgrading to module version `v1.0.0` will trigger a recreation of all node pools in the cluster. + ## Inputs @@ -151,6 +167,7 @@ Then perform the following commands on the root folder: | project\_id | The project ID to host the cluster in (required) | string | n/a | yes | | region | The region to host the cluster in (required) | string | n/a | yes | | regional | Whether is a regional cluster (zonal cluster if set false. WARNING: changing this after cluster creation is destructive!) | bool | `"true"` | no | +| registry\_project\_id | Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project. | string | `""` | no | | remove\_default\_node\_pool | Remove default node pool while setting up the cluster | bool | `"false"` | no | | service\_account | The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created. | string | `""` | no | | stub\_domains | Map of stub domains and their resolvers to forward DNS queries for a certain domain to an external DNS server | map(list(string)) | `` | no | @@ -212,6 +229,9 @@ following project roles: - roles/iam.serviceAccountUser - roles/resourcemanager.projectIamAdmin (only required if `service_account` is set to `create`) +Additionally, if `service_account` is set to `create` and `grant_registry_access` is requested, the service account requires the following role on the `registry_project_id` project: +- roles/resourcemanager.projectIamAdmin + ### Enable APIs In order to operate with the Service Account you must activate the following APIs on the project where the Service Account was created: diff --git a/autogen/README.md b/autogen/README.md index 421e4a2605..dc0b63b003 100644 --- a/autogen/README.md +++ b/autogen/README.md @@ -269,6 +269,9 @@ following project roles: - roles/iam.serviceAccountUser - roles/resourcemanager.projectIamAdmin (only required if `service_account` is set to `create`) +Additionally, if `service_account` is set to `create` and `grant_registry_access` is requested, the service account requires the following role on the `registry_project_id` project: +- roles/resourcemanager.projectIamAdmin + ### Enable APIs In order to operate with the Service Account you must activate the following APIs on the project where the Service Account was created: diff --git a/autogen/sa.tf b/autogen/sa.tf index 62b31f457a..eaebeb2a22 100644 --- a/autogen/sa.tf +++ b/autogen/sa.tf @@ -64,7 +64,7 @@ resource "google_project_iam_member" "cluster_service_account-monitoring_viewer" resource "google_project_iam_member" "cluster_service_account-gcr" { count = var.create_service_account && var.grant_registry_access ? 1 : 0 - project = var.project_id + project = var.registry_project_id == "" ? var.project_id : var.registry_project_id role = "roles/storage.objectViewer" member = "serviceAccount:${google_service_account.cluster_service_account[0].email}" } diff --git a/autogen/variables.tf b/autogen/variables.tf index 0fedacb2af..17566d238f 100644 --- a/autogen/variables.tf +++ b/autogen/variables.tf @@ -269,6 +269,12 @@ variable "grant_registry_access" { default = false } +variable "registry_project_id" { + type = string + description = "Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project." + default = "" +} + variable "service_account" { type = string description = "The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created." diff --git a/examples/workload_metadata_config/main.tf b/examples/workload_metadata_config/main.tf index 11cae808d4..f9fb25da5b 100644 --- a/examples/workload_metadata_config/main.tf +++ b/examples/workload_metadata_config/main.tf @@ -40,8 +40,9 @@ module "gke" { subnetwork = var.subnetwork ip_range_pods = var.ip_range_pods ip_range_services = var.ip_range_services - create_service_account = false - service_account = var.compute_engine_service_account + create_service_account = true + grant_registry_access = true + registry_project_id = var.registry_project_id enable_private_endpoint = true enable_private_nodes = true master_ipv4_cidr_block = "172.16.0.0/28" diff --git a/examples/workload_metadata_config/variables.tf b/examples/workload_metadata_config/variables.tf index 040c78d2c4..eaa8c36e83 100644 --- a/examples/workload_metadata_config/variables.tf +++ b/examples/workload_metadata_config/variables.tf @@ -48,7 +48,6 @@ variable "ip_range_services" { description = "The secondary ip range to use for pods" } -variable "compute_engine_service_account" { - description = "Service account to associate to the nodes in the cluster" +variable "registry_project_id" { + description = "Project name for the GCR registry" } - diff --git a/modules/beta-private-cluster/README.md b/modules/beta-private-cluster/README.md index 988d48ead8..c2920e4b28 100644 --- a/modules/beta-private-cluster/README.md +++ b/modules/beta-private-cluster/README.md @@ -190,6 +190,7 @@ In either case, upgrading to module version `v1.0.0` will trigger a recreation o | project\_id | The project ID to host the cluster in (required) | string | n/a | yes | | region | The region to host the cluster in (required) | string | n/a | yes | | regional | Whether is a regional cluster (zonal cluster if set false. WARNING: changing this after cluster creation is destructive!) | bool | `"true"` | no | +| registry\_project\_id | Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project. | string | `""` | no | | remove\_default\_node\_pool | Remove default node pool while setting up the cluster | bool | `"false"` | no | | resource\_usage\_export\_dataset\_id | The dataset id for which network egress metering for this cluster will be enabled. If enabled, a daemonset will be created in the cluster to meter network egress traffic. | string | `""` | no | | sandbox\_enabled | (Beta) Enable GKE Sandbox (Do not forget to set `image_type` = `COS_CONTAINERD` and `node_version` = `1.12.7-gke.17` or later to use it). | bool | `"false"` | no | @@ -258,6 +259,9 @@ following project roles: - roles/iam.serviceAccountUser - roles/resourcemanager.projectIamAdmin (only required if `service_account` is set to `create`) +Additionally, if `service_account` is set to `create` and `grant_registry_access` is requested, the service account requires the following role on the `registry_project_id` project: +- roles/resourcemanager.projectIamAdmin + ### Enable APIs In order to operate with the Service Account you must activate the following APIs on the project where the Service Account was created: diff --git a/modules/beta-private-cluster/sa.tf b/modules/beta-private-cluster/sa.tf index 9e063fcc22..c7f34e4fbb 100644 --- a/modules/beta-private-cluster/sa.tf +++ b/modules/beta-private-cluster/sa.tf @@ -64,7 +64,7 @@ resource "google_project_iam_member" "cluster_service_account-monitoring_viewer" resource "google_project_iam_member" "cluster_service_account-gcr" { count = var.create_service_account && var.grant_registry_access ? 1 : 0 - project = var.project_id + project = var.registry_project_id == "" ? var.project_id : var.registry_project_id role = "roles/storage.objectViewer" member = "serviceAccount:${google_service_account.cluster_service_account[0].email}" } diff --git a/modules/beta-private-cluster/variables.tf b/modules/beta-private-cluster/variables.tf index 9a869a830f..2c53d06b90 100644 --- a/modules/beta-private-cluster/variables.tf +++ b/modules/beta-private-cluster/variables.tf @@ -267,6 +267,12 @@ variable "grant_registry_access" { default = false } +variable "registry_project_id" { + type = string + description = "Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project." + default = "" +} + variable "service_account" { type = string description = "The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created." diff --git a/modules/beta-public-cluster/README.md b/modules/beta-public-cluster/README.md index 7d59e927bf..f013439240 100644 --- a/modules/beta-public-cluster/README.md +++ b/modules/beta-public-cluster/README.md @@ -181,6 +181,7 @@ In either case, upgrading to module version `v1.0.0` will trigger a recreation o | project\_id | The project ID to host the cluster in (required) | string | n/a | yes | | region | The region to host the cluster in (required) | string | n/a | yes | | regional | Whether is a regional cluster (zonal cluster if set false. WARNING: changing this after cluster creation is destructive!) | bool | `"true"` | no | +| registry\_project\_id | Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project. | string | `""` | no | | remove\_default\_node\_pool | Remove default node pool while setting up the cluster | bool | `"false"` | no | | resource\_usage\_export\_dataset\_id | The dataset id for which network egress metering for this cluster will be enabled. If enabled, a daemonset will be created in the cluster to meter network egress traffic. | string | `""` | no | | sandbox\_enabled | (Beta) Enable GKE Sandbox (Do not forget to set `image_type` = `COS_CONTAINERD` and `node_version` = `1.12.7-gke.17` or later to use it). | bool | `"false"` | no | @@ -249,6 +250,9 @@ following project roles: - roles/iam.serviceAccountUser - roles/resourcemanager.projectIamAdmin (only required if `service_account` is set to `create`) +Additionally, if `service_account` is set to `create` and `grant_registry_access` is requested, the service account requires the following role on the `registry_project_id` project: +- roles/resourcemanager.projectIamAdmin + ### Enable APIs In order to operate with the Service Account you must activate the following APIs on the project where the Service Account was created: diff --git a/modules/beta-public-cluster/sa.tf b/modules/beta-public-cluster/sa.tf index 9e063fcc22..c7f34e4fbb 100644 --- a/modules/beta-public-cluster/sa.tf +++ b/modules/beta-public-cluster/sa.tf @@ -64,7 +64,7 @@ resource "google_project_iam_member" "cluster_service_account-monitoring_viewer" resource "google_project_iam_member" "cluster_service_account-gcr" { count = var.create_service_account && var.grant_registry_access ? 1 : 0 - project = var.project_id + project = var.registry_project_id == "" ? var.project_id : var.registry_project_id role = "roles/storage.objectViewer" member = "serviceAccount:${google_service_account.cluster_service_account[0].email}" } diff --git a/modules/beta-public-cluster/variables.tf b/modules/beta-public-cluster/variables.tf index 0ae2b75661..07771a27a9 100644 --- a/modules/beta-public-cluster/variables.tf +++ b/modules/beta-public-cluster/variables.tf @@ -267,6 +267,12 @@ variable "grant_registry_access" { default = false } +variable "registry_project_id" { + type = string + description = "Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project." + default = "" +} + variable "service_account" { type = string description = "The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created." diff --git a/modules/private-cluster/README.md b/modules/private-cluster/README.md index d823f640fa..f7f8fef179 100644 --- a/modules/private-cluster/README.md +++ b/modules/private-cluster/README.md @@ -176,6 +176,7 @@ In either case, upgrading to module version `v1.0.0` will trigger a recreation o | project\_id | The project ID to host the cluster in (required) | string | n/a | yes | | region | The region to host the cluster in (required) | string | n/a | yes | | regional | Whether is a regional cluster (zonal cluster if set false. WARNING: changing this after cluster creation is destructive!) | bool | `"true"` | no | +| registry\_project\_id | Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project. | string | `""` | no | | remove\_default\_node\_pool | Remove default node pool while setting up the cluster | bool | `"false"` | no | | service\_account | The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created. | string | `""` | no | | stub\_domains | Map of stub domains and their resolvers to forward DNS queries for a certain domain to an external DNS server | map(list(string)) | `` | no | @@ -237,6 +238,9 @@ following project roles: - roles/iam.serviceAccountUser - roles/resourcemanager.projectIamAdmin (only required if `service_account` is set to `create`) +Additionally, if `service_account` is set to `create` and `grant_registry_access` is requested, the service account requires the following role on the `registry_project_id` project: +- roles/resourcemanager.projectIamAdmin + ### Enable APIs In order to operate with the Service Account you must activate the following APIs on the project where the Service Account was created: diff --git a/modules/private-cluster/sa.tf b/modules/private-cluster/sa.tf index 9e063fcc22..c7f34e4fbb 100644 --- a/modules/private-cluster/sa.tf +++ b/modules/private-cluster/sa.tf @@ -64,7 +64,7 @@ resource "google_project_iam_member" "cluster_service_account-monitoring_viewer" resource "google_project_iam_member" "cluster_service_account-gcr" { count = var.create_service_account && var.grant_registry_access ? 1 : 0 - project = var.project_id + project = var.registry_project_id == "" ? var.project_id : var.registry_project_id role = "roles/storage.objectViewer" member = "serviceAccount:${google_service_account.cluster_service_account[0].email}" } diff --git a/modules/private-cluster/variables.tf b/modules/private-cluster/variables.tf index 8008e08975..00d7779e83 100644 --- a/modules/private-cluster/variables.tf +++ b/modules/private-cluster/variables.tf @@ -257,6 +257,12 @@ variable "grant_registry_access" { default = false } +variable "registry_project_id" { + type = string + description = "Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project." + default = "" +} + variable "service_account" { type = string description = "The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created." diff --git a/sa.tf b/sa.tf index 9e063fcc22..c7f34e4fbb 100644 --- a/sa.tf +++ b/sa.tf @@ -64,7 +64,7 @@ resource "google_project_iam_member" "cluster_service_account-monitoring_viewer" resource "google_project_iam_member" "cluster_service_account-gcr" { count = var.create_service_account && var.grant_registry_access ? 1 : 0 - project = var.project_id + project = var.registry_project_id == "" ? var.project_id : var.registry_project_id role = "roles/storage.objectViewer" member = "serviceAccount:${google_service_account.cluster_service_account[0].email}" } diff --git a/test/fixtures/shared/outputs.tf b/test/fixtures/shared/outputs.tf index 1c2eb5e9ba..71b4b250de 100644 --- a/test/fixtures/shared/outputs.tf +++ b/test/fixtures/shared/outputs.tf @@ -79,3 +79,6 @@ output "service_account" { value = module.example.service_account } +output "registry_project_id" { + value = var.registry_project_id +} diff --git a/test/fixtures/shared/variables.tf b/test/fixtures/shared/variables.tf index 76280e0065..5dff24dbd4 100644 --- a/test/fixtures/shared/variables.tf +++ b/test/fixtures/shared/variables.tf @@ -33,3 +33,7 @@ variable "compute_engine_service_account" { description = "The email address of the service account to associate with the GKE cluster" } +variable "registry_project_id" { + description = "Project to use for granting access to the GCR registry, if requested" +} + diff --git a/test/fixtures/workload_metadata_config/example.tf b/test/fixtures/workload_metadata_config/example.tf index 4d4a98e119..b0db3f8b8d 100644 --- a/test/fixtures/workload_metadata_config/example.tf +++ b/test/fixtures/workload_metadata_config/example.tf @@ -17,6 +17,28 @@ module "example" { source = "../../../examples/workload_metadata_config" +<<<<<<< HEAD +<<<<<<< HEAD +<<<<<<< HEAD +<<<<<<< HEAD +======= +>>>>>>> 5258f89... Removing a few conflicting files. +======= +>>>>>>> 244108e... Removing a few conflicting files. +======= +>>>>>>> 5258f89... Removing a few conflicting files. + project_id = var.project_id + cluster_name_suffix = "-${random_string.suffix.result}" + region = var.region + zones = slice(var.zones, 0, 1) + network = google_compute_network.main.name + subnetwork = google_compute_subnetwork.main.name + ip_range_pods = google_compute_subnetwork.main.secondary_ip_range[0].range_name + ip_range_services = google_compute_subnetwork.main.secondary_ip_range[1].range_name +<<<<<<< HEAD +<<<<<<< HEAD +<<<<<<< HEAD +======= project_id = var.project_id cluster_name_suffix = "-${random_string.suffix.result}" region = var.region @@ -25,5 +47,18 @@ module "example" { subnetwork = google_compute_subnetwork.main.name ip_range_pods = google_compute_subnetwork.main.secondary_ip_range[0].range_name ip_range_services = google_compute_subnetwork.main.secondary_ip_range[1].range_name - compute_engine_service_account = var.compute_engine_service_account +<<<<<<< HEAD +<<<<<<< HEAD +>>>>>>> d791335... Removed the custom test for create_service_account +======= +>>>>>>> 5258f89... Removing a few conflicting files. +======= +>>>>>>> e7f04bb... Removed the custom test for create_service_account +======= +>>>>>>> 244108e... Removing a few conflicting files. +======= +>>>>>>> d791335... Removed the custom test for create_service_account +======= +>>>>>>> 5258f89... Removing a few conflicting files. + registry_project_id = var.registry_project_id } diff --git a/test/integration/workload_metadata_config/controls/gcloud.rb b/test/integration/workload_metadata_config/controls/gcloud.rb index e62606c78c..ad642ff7c9 100644 --- a/test/integration/workload_metadata_config/controls/gcloud.rb +++ b/test/integration/workload_metadata_config/controls/gcloud.rb @@ -13,8 +13,10 @@ # limitations under the License. project_id = attribute('project_id') +registry_project_id = attribute('registry_project_id') location = attribute('location') cluster_name = attribute('cluster_name') +service_account = attribute('service_account') control "gcloud" do title "Google Compute Engine GKE configuration" @@ -55,4 +57,20 @@ end end end + + describe command("gcloud projects get-iam-policy #{registry_project_id} --format=json") do + its(:exit_status) { should eq 0 } + its(:stderr) { should eq '' } + + let!(:iam) do + if subject.exit_status == 0 + JSON.parse(subject.stdout) + else + {} + end + end + it "has expected registry roles" do + expect(iam['bindings']).to include("members" => ["serviceAccount:#{service_account}"], "role" => "roles/storage.objectViewer") + end + end end diff --git a/test/integration/workload_metadata_config/inspec.yml b/test/integration/workload_metadata_config/inspec.yml index f6f3811afa..4f2b7d40d6 100644 --- a/test/integration/workload_metadata_config/inspec.yml +++ b/test/integration/workload_metadata_config/inspec.yml @@ -9,3 +9,9 @@ attributes: - name: project_id required: true type: string + - name: service_account + required: true + type: string + - name: registry_project_id + required: false + type: string diff --git a/test/setup/make_source.sh b/test/setup/make_source.sh index b39944af41..ad3f57165a 100755 --- a/test/setup/make_source.sh +++ b/test/setup/make_source.sh @@ -19,6 +19,9 @@ echo "#!/usr/bin/env bash" > ../source.sh project_id=$(terraform output project_id) echo "export TF_VAR_project_id='$project_id'" >> ../source.sh +# We use the same project for registry project in the tests. +echo "export TF_VAR_registry_project_id='$project_id'" >> ../source.sh + sa_json=$(terraform output sa_key) # shellcheck disable=SC2086 echo "export SERVICE_ACCOUNT_JSON='$(echo $sa_json | base64 --decode)'" >> ../source.sh diff --git a/variables.tf b/variables.tf index 460bdeaeff..da9c744646 100644 --- a/variables.tf +++ b/variables.tf @@ -257,6 +257,12 @@ variable "grant_registry_access" { default = false } +variable "registry_project_id" { + type = string + description = "Project holding the Google Container Registry. If empty, we use the cluster project. If grant_registry_access is true, storage.objectViewer role is assigned on this project." + default = "" +} + variable "service_account" { type = string description = "The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created."