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.

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

Allow project name to be changed separately from project ID #154

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ module "project-factory" {
random_project_id = "${var.random_project_id}"
org_id = "${var.org_id}"
name = "${var.name}"
project_id = "${var.project_id}"
shared_vpc = "${var.shared_vpc}"
billing_account = "${var.billing_account}"
folder_id = "${var.folder_id}"
Expand Down
29 changes: 14 additions & 15 deletions modules/core_project_factory/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,17 @@ resource "random_id" "random_project_id_suffix" {
*****************************************/
locals {
group_id = "${var.manage_group ? format("group:%s", var.group_email) : ""}"
project_id = "${google_project.main.project_id}"
project_number = "${google_project.main.number}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why drop local.project_number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The local here is a level of indirection that is unnecessary, IMHO.

Copy link
Contributor

@morgante morgante Mar 6, 2019

Choose a reason for hiding this comment

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

It's easier to manage this in one place instead of having to update it on every resource.

base_project_id = "${var.project_id == "" ? var.name : var.project_id}"
project_org_id = "${var.folder_id != "" ? "" : var.org_id}"
project_folder_id = "${var.folder_id != "" ? var.folder_id : ""}"
temp_project_id = "${var.random_project_id ? format("%s-%s",var.name,random_id.random_project_id_suffix.hex) : var.name}"
temp_project_id = "${var.random_project_id ? format("%s-%s",local.base_project_id,random_id.random_project_id_suffix.hex) : local.base_project_id}"
s_account_fmt = "${format("serviceAccount:%s", google_service_account.default_service_account.email)}"
api_s_account = "${format("%s@cloudservices.gserviceaccount.com", local.project_number)}"
api_s_account = "${format("%s@cloudservices.gserviceaccount.com", google_project.main.number)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this continue to reference project_number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The net effect of this change is a no-op. It removes a level of indirection since local.project_number is referring to google_project.main.number. I see no purpose for continuing to use local.project_number. It makes things a bit more difficult to read so I would much prefer to remove it.

api_s_account_fmt = "${format("serviceAccount:%s", local.api_s_account)}"
gke_shared_vpc_enabled = "${var.shared_vpc != "" && contains(var.activate_apis, "container.googleapis.com") ? "true" : "false"}"
gke_s_account = "${format("service-%s@container-engine-robot.iam.gserviceaccount.com", local.project_number)}"
gke_s_account = "${format("service-%s@container-engine-robot.iam.gserviceaccount.com", google_project.main.number)}"
gke_s_account_fmt = "${local.gke_shared_vpc_enabled ? format("serviceAccount:%s", local.gke_s_account) : ""}"
project_bucket_name = "${var.bucket_name != "" ? var.bucket_name : format("%s-state", var.name)}"
project_bucket_name = "${var.bucket_name != "" ? var.bucket_name : format("%s-state", local.temp_project_id)}"
create_bucket = "${var.bucket_project != "" ? "true" : "false"}"

shared_vpc_users = "${compact(list(local.group_id, local.s_account_fmt, local.api_s_account_fmt, local.gke_s_account_fmt))}"
Expand Down Expand Up @@ -104,7 +103,7 @@ resource "google_resource_manager_lien" "lien" {
resource "google_project_service" "project_services" {
count = "${length(var.activate_apis)}"

project = "${local.project_id}"
project = "${google_project.main.project_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep local.project_id unless there's a reason we can't, similarly for all cases of this replacement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am of the opinion that the project_id local should be removed or repurposed to make the project_id generation more clear. It also adds a layer of redirection that seems unnecessary so my comments on dropping the project_number local applies as well.

service = "${element(var.activate_apis, count.index)}"

disable_on_destroy = "${var.disable_services_on_destroy}"
Expand All @@ -119,7 +118,7 @@ resource "google_compute_shared_vpc_service_project" "shared_vpc_attachment" {
count = "${var.shared_vpc != "" ? 1 : 0}"

host_project = "${var.shared_vpc}"
service_project = "${local.project_id}"
service_project = "${google_project.main.project_id}"

depends_on = ["google_project_service.project_services"]
}
Expand All @@ -138,7 +137,7 @@ data "null_data_source" "default_service_account" {
*****************************************/
resource "null_resource" "delete_default_compute_service_account" {
provisioner "local-exec" {
command = "${path.module}/scripts/delete-service-account.sh ${local.project_id} ${data.null_data_source.default_service_account.outputs["email"]} ${var.credentials_path}"
command = "${path.module}/scripts/delete-service-account.sh ${google_project.main.project_id} ${data.null_data_source.default_service_account.outputs["email"]} ${var.credentials_path}"
}

triggers {
Expand All @@ -155,15 +154,15 @@ resource "null_resource" "delete_default_compute_service_account" {
resource "google_service_account" "default_service_account" {
account_id = "project-service-account"
display_name = "${var.name} Project Service Account"
project = "${local.project_id}"
project = "${google_project.main.project_id}"
}

/**************************************************
Policy to operate instances in shared subnetwork
*************************************************/
resource "google_project_iam_member" "default_service_account_membership" {
count = "${var.sa_role != "" ? 1 : 0}"
project = "${local.project_id}"
project = "${google_project.main.project_id}"
role = "${var.sa_role}"

member = "${local.s_account_fmt}"
Expand All @@ -176,7 +175,7 @@ resource "google_project_iam_member" "gsuite_group_role" {
count = "${var.manage_group ? 1 : 0}"

member = "${local.group_id}"
project = "${local.project_id}"
project = "${google_project.main.project_id}"
role = "${var.group_role}"
}

Expand All @@ -189,7 +188,7 @@ resource "google_service_account_iam_member" "service_account_grant_to_group" {
member = "${local.group_id}"
role = "roles/iam.serviceAccountUser"

service_account_id = "projects/${local.project_id}/serviceAccounts/${
service_account_id = "projects/${google_project.main.project_id}/serviceAccounts/${
google_service_account.default_service_account.email
}"
}
Expand Down Expand Up @@ -261,9 +260,9 @@ resource "google_compute_subnetwork_iam_member" "apis_service_account_role_to_vp
resource "google_project_usage_export_bucket" "usage_report_export" {
count = "${var.usage_bucket_name != "" ? 1 : 0}"

project = "${local.project_id}"
project = "${google_project.main.project_id}"
bucket_name = "${var.usage_bucket_name}"
prefix = "${var.usage_bucket_prefix != "" ? var.usage_bucket_prefix : "usage-${local.project_id}"}"
prefix = "${var.usage_bucket_prefix != "" ? var.usage_bucket_prefix : "usage-${google_project.main.project_id}"}"

depends_on = ["google_project_service.project_services"]
}
Expand Down
8 changes: 6 additions & 2 deletions modules/core_project_factory/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@
* limitations under the License.
*/

output "project_name" {
value = "${google_project.main.name}"
}

output "project_id" {
value = "${local.project_id}"
value = "${google_project.main.project_id}"
}

output "project_number" {
value = "${local.project_number}"
value = "${google_project.main.number}"
}

output "service_account_id" {
Expand Down
7 changes: 6 additions & 1 deletion modules/core_project_factory/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ variable "manage_group" {
default = "false"
}

variable "project_id" {
description = "If provided, the project uses the given project ID. Mutually exclusive with random_project_id being true."
default = ""
}

variable "random_project_id" {
description = "Enables project random id generation"
description = "Enables project random id generation. Mutually exclusive with project_id being non-empty."
default = "false"
}

Expand Down
1 change: 1 addition & 0 deletions modules/gsuite_enabled/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ module "project-factory" {
random_project_id = "${var.random_project_id}"
org_id = "${var.org_id}"
name = "${var.name}"
project_id = "${var.project_id}"
shared_vpc = "${var.shared_vpc}"
billing_account = "${var.billing_account}"
folder_id = "${var.folder_id}"
Expand Down
4 changes: 4 additions & 0 deletions modules/gsuite_enabled/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
* limitations under the License.
*/

output "project_name" {
value = "${module.project-factory.project_name}"
}

output "project_id" {
value = "${module.project-factory.project_id}"
}
Expand Down
7 changes: 6 additions & 1 deletion modules/gsuite_enabled/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ variable "lien" {
}

variable "random_project_id" {
description = "Enables project random id generation"
description = "Enables project random id generation. Mutually exclusive with project_id being non-empty."
default = "false"
}

Expand All @@ -38,6 +38,11 @@ variable "name" {
description = "The name for the project"
}

variable "project_id" {
description = "If provided, the project uses the given project ID. Mutually exclusive with random_project_id being true."
default = ""
}

variable "shared_vpc" {
description = "The ID of the host project which hosts the shared VPC"
default = ""
Expand Down
4 changes: 4 additions & 0 deletions outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
* limitations under the License.
*/

output "project_name" {
value = "${module.project-factory.project_name}"
}

output "project_id" {
value = "${module.project-factory.project_id}"
}
Expand Down
5 changes: 3 additions & 2 deletions test/fixtures/full/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ module "vpc" {
module "project-factory" {
source = "../../../modules/gsuite_enabled"

name = "pf-ci-test-full-${random_string.suffix.result}"
random_project_id = "true"
name = "pf-ci-test-full-name-${random_string.suffix.result}"
random_project_id = "false"
project_id = "pf-ci-test-full-id-${random_string.suffix.result}"

domain = "${var.domain}"
org_id = "${var.org_id}"
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/shared/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
* limitations under the License.
*/

output "project_name" {
value = "${module.project-factory.project_name}"
}

output "project_id" {
value = "${module.project-factory.project_id}"
}
Expand Down
14 changes: 13 additions & 1 deletion test/integration/full/controls/project-factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

extra_service_account_email = attribute('extra_service_account_email')
project_name = attribute('project_name')
project_id = attribute('project_id')
sa_role = attribute('sa_role')
service_account_email = attribute('service_account_email')
Expand All @@ -26,9 +27,20 @@
control 'project-factory' do
title 'Project Factory'

describe command("gcloud projects describe #{project_id}") do
describe command("gcloud projects describe #{project_id} --format=json") do
its('exit_status') { should be 0 }
its('stderr') { should eq '' }

let(:metadata) do
if subject.exit_status == 0
JSON.parse(subject.stdout, symbolize_names: true)
else
{}
end
end

it { expect(metadata).to include(name: project_name) }
it { expect(metadata).to include(projectId: project_id) }
end

describe command("gcloud services list --project #{project_id}") do
Expand Down
4 changes: 4 additions & 0 deletions test/integration/full/inspec.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
name: full
attributes:
- name: project_name
required: true
type: string

- name: project_id
required: true
type: string
Expand Down
13 changes: 12 additions & 1 deletion test/integration/minimal/controls/minimal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,20 @@
control 'project-factory-minimal' do
title 'Project Factory minimal configuration'

describe command("gcloud projects describe #{project_id}") do
describe command("gcloud projects describe #{project_id} --format=json") do
its('exit_status') { should be 0 }
its('stderr') { should eq '' }

let(:metadata) do
if subject.exit_status == 0
JSON.parse(subject.stdout, symbolize_names: true)
else
{}
end
end

it { expect(metadata).to include(name: project_id[0...-5]) }
it { expect(metadata).to include(projectId: project_id) }
end

describe command("gcloud services list --project #{project_id}") do
Expand Down
7 changes: 6 additions & 1 deletion variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

variable "random_project_id" {
description = "Enables project random id generation"
description = "Enables project random id generation. Mutually exclusive with project_id being non-empty."
default = "false"
}

Expand All @@ -32,6 +32,11 @@ variable "name" {
description = "The name for the project"
}

variable "project_id" {
description = "If provided, the project uses the given project ID. Mutually exclusive with random_project_id being true."
default = ""
}

variable "shared_vpc" {
description = "The ID of the host project which hosts the shared VPC"
default = ""
Expand Down