Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: fix svpc regression #438

Merged
merged 7 commits into from
Aug 13, 2020
Merged

bug: fix svpc regression #438

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0868e02
fix regression wip
bharathkkb Aug 12, 2020
2dd285a
fix lint image
bharathkkb Aug 12, 2020
0bb52bd
add comments
bharathkkb Aug 12, 2020
75fa570
address comments
bharathkkb Aug 12, 2020
1dac307
simplify flags
bharathkkb Aug 12, 2020
77bd084
fmt
bharathkkb Aug 12, 2020
c7db87d
Apply suggestions from code review
bharathkkb Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
# Make will use bash instead of sh
SHELL := /usr/bin/env bash

DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0
DOCKER_TAG_VERSION_DEVELOPER_TOOLS := 0.12.0
DOCKER_IMAGE_DEVELOPER_TOOLS := cft/developer-tools
REGISTRY_URL := gcr.io/cloud-foundation-cicd

Expand Down
2 changes: 1 addition & 1 deletion build/int.cloudbuild.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,4 @@ tags:
- 'integration'
substitutions:
_DOCKER_IMAGE_DEVELOPER_TOOLS: 'cft/developer-tools'
_DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '0'
_DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '0.12.0'
2 changes: 1 addition & 1 deletion build/lint.cloudbuild.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ tags:
- 'lint'
substitutions:
_DOCKER_IMAGE_DEVELOPER_TOOLS: 'cft/developer-tools'
_DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '0'
_DOCKER_TAG_VERSION_DEVELOPER_TOOLS: '0.12.0'
45 changes: 23 additions & 22 deletions modules/shared_vpc_access/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,17 @@ data "google_project" "service_project" {

locals {
gke_shared_vpc_enabled = contains(var.active_apis, "container.googleapis.com")
gke_s_account = local.gke_shared_vpc_enabled ? format(
gke_s_account = local.gke_shared_vpc_enabled ? [format(
"service-%s@container-engine-robot.iam.gserviceaccount.com",
data.google_project.service_project.number,
) : ""
)] : []
dataproc_shared_vpc_enabled = contains(var.active_apis, "dataproc.googleapis.com")
dataproc_s_account = local.dataproc_shared_vpc_enabled ? format(
dataproc_s_account = local.dataproc_shared_vpc_enabled ? [format(
"service-%s@dataproc-accounts.iam.gserviceaccount.com",
data.google_project.service_project.number
) : ""
active_api_s_accounts = compact([local.gke_s_account, local.dataproc_s_account])
)] : []
active_api_s_accounts = flatten([local.gke_s_account, local.dataproc_s_account])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compact was giving me the count cannot determine number of resources error. Weirdly enough this approach does not. I think it has to do something with this hashicorp/terraform#25152

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think looping on the actual accounts is actually dangerous. I think something like this would be better:

locals {
  apis = {
    "container.googleapis.com": format("service-%s@container-engine-robot.iam.gserviceaccount.com", data.google_project.service_project.number
 }
  active_apis = setintersection(keys(local.apis), var.active_apis
  subnetwork_api = setproduct(local.active_apis, var.shared_vpc_subnets)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up having to do tolist(setproduct(local.active_apis, var.shared_vpc_subnets)) as it was otherwise a set and throwing error This value does not have any indices. In this case I dont think element ordering matters as it becomes a list of elements of type [api,subnet].

subnetwork_api = length(var.shared_vpc_subnets) != 0 ? setproduct(local.active_api_s_accounts, var.shared_vpc_subnets) : []
}

/******************************************
Expand All @@ -38,21 +39,31 @@ locals {
*****************************************/
resource "google_compute_subnetwork_iam_member" "gke_shared_vpc_subnets" {
provider = google-beta
count = local.gke_shared_vpc_enabled && length(var.shared_vpc_subnets) != 0 ? length(var.shared_vpc_subnets) : 0
count = local.gke_shared_vpc_enabled && length(local.active_api_s_accounts) != 0 ? length(local.subnetwork_api) : 0
subnetwork = element(
split("/", var.shared_vpc_subnets[count.index]),
split("/", local.subnetwork_api[count.index][1]),
index(
split("/", var.shared_vpc_subnets[count.index]),
split("/", local.subnetwork_api[count.index][1]),
"subnetworks",
) + 1,
)
role = "roles/compute.networkUser"
region = element(
split("/", var.shared_vpc_subnets[count.index]),
index(split("/", var.shared_vpc_subnets[count.index]), "regions") + 1,
split("/", local.subnetwork_api[count.index][1]),
index(split("/", local.subnetwork_api[count.index][1]), "regions") + 1,
)
project = var.host_project_id
member = format("serviceAccount:%s", local.gke_s_account)
member = format("serviceAccount:%s", local.subnetwork_api[count.index][0])
}

/******************************************
compute.networkUser role granted to GKE service account for GKE and dataproc service account for dataproc on shared VPC Project if no subnets defined
*****************************************/
resource "google_project_iam_member" "gke_shared_vpc_network_user" {
count = length(var.shared_vpc_subnets) == 0 && (local.gke_shared_vpc_enabled || local.dataproc_shared_vpc_enabled) ? length(local.active_api_s_accounts) : 0
project = var.host_project_id
role = "roles/compute.networkUser"
member = format("serviceAccount:%s", local.active_api_s_accounts[count.index])
}

/******************************************
Expand All @@ -63,16 +74,6 @@ resource "google_project_iam_member" "gke_host_agent" {
count = local.gke_shared_vpc_enabled ? 1 : 0
project = var.host_project_id
role = "roles/container.hostServiceAgentUser"
member = format("serviceAccount:%s", local.gke_s_account)
member = format("serviceAccount:%s", local.gke_s_account[0])
}

/******************************************
compute.networkUser role granted to dataproc service account for dataproc on shared VPC subnets
See: https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/network#creating_a_cluster_that_uses_a_vpc_network_in_another_project
*****************************************/
resource "google_project_iam_member" "dataproc_shared_vpc_network_user" {
count = local.dataproc_shared_vpc_enabled ? 1 : 0
project = var.host_project_id
role = "roles/compute.networkUser"
member = format("serviceAccount:%s", local.dataproc_s_account)
}
5 changes: 5 additions & 0 deletions test/fixtures/dynamic_shared_vpc/outputs.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ output "service_project_number" {
description = "The service project number"
}

output "service_project_b_number" {
value = module.example.service_project_b.project_number
description = "The service project b number"
}

output "service_account_email" {
value = module.example.service_project.service_account_email
description = "The service account email"
Expand Down
34 changes: 31 additions & 3 deletions test/integration/dynamic_shared_vpc/controls/svpc.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
service_project_id = attribute('service_project_id')
service_project_ids = attribute('service_project_ids')
service_project_number = attribute('service_project_number')
service_project_b_number = attribute('service_project_b_number')
service_account_email = attribute('service_account_email')
shared_vpc = attribute('shared_vpc')
shared_vpc_subnet_name_01 = attribute('shared_vpc_subnet_name_01')
Expand Down Expand Up @@ -53,7 +54,7 @@
)
end

it "does not include the GKE service account in the roles/compute.networkUser IAM binding" do
it "service project with explicit subnets 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_number}@container-engine-robot.iam.gserviceaccount.com"
Expand All @@ -70,13 +71,20 @@
)
end

it "includes the dataproc service account in the roles/compute.networkUser IAM binding" do
it "service project b without explicit subnets includes the GKE service account in the roles/compute.networkUser IAM binding" do
expect(bindings).to include(
members: including("serviceAccount:service-#{service_project_number}@dataproc-accounts.iam.gserviceaccount.com"),
members: including("serviceAccount:service-#{service_project_b_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
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
its('exit_status') { should eq 0 }
Expand All @@ -98,6 +106,16 @@
)
end
end

describe "roles/compute.networkUser" do
it "service project with explicit subnets includes the GKE service account in the roles/compute.networkUser IAM binding" do
expect(bindings).to include(
members: including("serviceAccount:service-#{service_project_number}@container-engine-robot.iam.gserviceaccount.com"),
role: "roles/compute.networkUser",
)
end
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
Expand All @@ -120,5 +138,15 @@
)
end
end

describe "roles/compute.networkUser" do
it "service project b without explicit subnets 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_b_number}@container-engine-robot.iam.gserviceaccount.com"),
role: "roles/compute.networkUser",
)
end
end

end
end
3 changes: 3 additions & 0 deletions test/integration/dynamic_shared_vpc/inspec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ attributes:
- name: service_project_number
required: true
type: string
- name: service_project_b_number
required: true
type: string
- name: service_account_email
required: true
type: string
Expand Down
11 changes: 6 additions & 5 deletions test/setup/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ module "pfactory_project" {
source = "terraform-google-modules/project-factory/google"
version = "~> 8.0"

name = "ci-pfactory-tests"
random_project_id = true
org_id = var.org_id
folder_id = google_folder.ci_pfactory_folder.id
billing_account = var.billing_account
name = "ci-pfactory-tests"
random_project_id = true
org_id = var.org_id
folder_id = google_folder.ci_pfactory_folder.id
billing_account = var.billing_account
skip_gcloud_download = true

activate_apis = [
"admin.googleapis.com",
Expand Down