Add project variable #28
Conversation
…ue var.project if it's populated.
…e-consul into dev-consul-set-project
There was a problem hiding this comment.
Thanks for this Pull Request! This looks like a nice addition. Other than the one comment I left, ihave just one additional request: Can you rename var.project to var.project_id to make it explicit that it's the project ID we want and not the project name or something else?
modules/consul-cluster/variables.tf
Outdated
| description = "All zones you want to deploy to within the specified region" | ||
| type = "list" | ||
| } | ||
|
|
There was a problem hiding this comment.
Why add this var? It doesn't appear to be used anywhere?
|
I've changed I forgot I left |
|
Looks good. The only thing I now realize we're missing is an update to https://github.com/hashicorp/terraform-google-consul/blob/master/main.tf to include the new |
|
That should now be done |
…e-consul into dev-consul-set-project
Major Change:Similar to the change I made in hashicorp/terraform-google-vault#25, I've just added a new variable called |
There was a problem hiding this comment.
Thanks for this updated PR. The ability to customize the network project seems like a nice addition. It turns out this PR is blocking hashicorp/terraform-google-vault#25, which is in turn blocking another PR, so I'm going to manually make these updates and merge this.
modules/consul-cluster/main.tf
Outdated
| subnetwork = "${var.subnetwork_name != "" ? var.subnetwork_name : ""}" | ||
| network = "${var.subnetwork_name != "" ? "" : var.network_name}" | ||
| subnetwork = "${var.subnetwork_name != "" ? var.subnetwork_name : ""}" | ||
| subnetwork_project = "${var.network_project_id}" |
There was a problem hiding this comment.
Requiring a value for network_project_id might be surprising to users who have all resources in a single network. Therefore, let's update this line as follows:
subnetwork = "${var.subnetwork_name != "" ? var.subnetwork_name : ""}"
subnetwork_project = "${var.network_project_id != "" ? var.network_project_id : var.gcp_project_id}"Can you update the default value and description of var.network_project_id accordingly? Specifically, var.network_project_id should now default to "" and the description should mention that, if var.network_project_id is empty, var.gcp_project_id will be used for the network.
| @@ -93,7 +96,8 @@ resource "google_compute_instance_template" "consul_server_public" { | |||
| # Create the Instance Template that will be used to populate the Managed Instance Group. | |||
| # NOTE: This Compute Instance Template is only created if var.assign_public_ip_addresses is false. | |||
| resource "google_compute_instance_template" "consul_server_private" { | |||
| count = "${1 - var.assign_public_ip_addresses}" | |||
| project = "${var.gcp_project_id}" | |||
| count = "${1 - var.assign_public_ip_addresses}" | |||
There was a problem hiding this comment.
It's a nit, but by convention, we always put the count property as the very first one in any resource and separate it from others by a blank line. So please update to:
resource "google_compute_instance_template" "consul_server_private" {
count = "${1 - var.assign_public_ip_addresses}"
project = "${var.gcp_project_id}"
...
}
modules/consul-cluster/main.tf
Outdated
| subnetwork = "${var.subnetwork_name != "" ? var.subnetwork_name : ""}" | ||
| network = "${var.subnetwork_name != "" ? "" : var.network_name}" | ||
| subnetwork = "${var.subnetwork_name != "" ? var.subnetwork_name : ""}" | ||
| subnetwork_project = "${var.network_project_id}" |
modules/consul-cluster/main.tf
Outdated
| @@ -153,6 +158,8 @@ resource "google_compute_instance_template" "consul_server_private" { | |||
| # - This Firewall Rule may be redundant depnding on the settings of your VPC Network, but if your Network is locked down, | |||
| # this Rule will open up the appropriate ports. | |||
| resource "google_compute_firewall" "allow_intracluster_consul" { | |||
| project = "${var.network_project_id}" | |||
There was a problem hiding this comment.
See above comment. Should be:
project = "${var.network_project_id != "" ? var.network_project_id : var.gcp_project_id}"
modules/consul-cluster/main.tf
Outdated
| @@ -189,6 +196,8 @@ resource "google_compute_firewall" "allow_intracluster_consul" { | |||
| # - Note that public access to your Consul Cluster will only be permitted if var.assign_public_ip_addresses is true. | |||
| # - This Firewall Rule is only created if at least one source tag or source CIDR block is specified. | |||
| resource "google_compute_firewall" "allow_inbound_http_api" { | |||
| project = "${var.network_project_id}" | |||
| @@ -213,7 +222,8 @@ resource "google_compute_firewall" "allow_inbound_http_api" { | |||
| # - Note that public access to your Consul Cluster will only be permitted if var.assign_public_ip_addresses is true. | |||
| # - This Firewall Rule is only created if at least one source tag or source CIDR block is specified. | |||
| resource "google_compute_firewall" "allow_inbound_dns" { | |||
| count = "${length(var.allowed_inbound_cidr_blocks_dns) + length(var.allowed_inbound_tags_dns) > 0 ? 1 : 0}" | |||
| project = "${var.network_project_id}" | |||
| count = "${length(var.allowed_inbound_cidr_blocks_dns) + length(var.allowed_inbound_tags_dns) > 0 ? 1 : 0}" | |||
There was a problem hiding this comment.
See above comments. Should be:
resource "google_compute_firewall" "allow_inbound_dns" {
count = "${length(var.allowed_inbound_cidr_blocks_dns) + length(var.allowed_inbound_tags_dns) > 0 ? 1 : 0}"
project = "${var.network_project_id != "" ? var.network_project_id : var.gcp_project_id}"
...
}
modules/consul-cluster/variables.tf
Outdated
|
|
||
| variable "network_project_id" { | ||
| description = "The name of the GCP Project where the network is located. Useful when using networks shared between projects" | ||
| } |
There was a problem hiding this comment.
Per above comments, update description to:
variable "network_project_id" {
description = "The name of the GCP Project where the network is located. Useful when using networks shared between projects. If empty, var.gcp_project_id will be used."
default = ""
}
variables.tf
Outdated
| description = "The name of the GCP Project where all resources will be launched." | ||
| } | ||
|
|
||
| variable "network_project_id" { | ||
| description = "The name of the GCP Project where the network is located. Useful for using networks shared between projects." | ||
| } |
There was a problem hiding this comment.
Per above, update to:
variable "network_project_id" {
description = "The name of the GCP Project where the network is located. Useful when using networks shared between projects. If empty, var.gcp_project_id will be used."
default = ""
}
|
Merging now so I can validate with tests. |
|
Validated in #30. Once that's merged, I'll issue a new release! |
What this MR does
This MR allows specifying of which project
consul-clusterwill deploy to, by adding a project variable for each compute instance as${var.gcp_project_id}, as well as a project variable for the network and firewall rules,${var.network_project_id}.Why this MR is needed
The current method uses the project defined in the 'provider' block. This is good for single-project deployments, but when using
consul-clusterwith other terraform files which need deploying into other projects, there will be no project specified inprovider. Each terraform object will have it's project specified instread. This MR will make sure thatconsul-clusterhas the same ability.