Closed jcejohnson closed 6 years ago
Ah, good fix. This is quite a frustrating Terraform gotcha. Thank you!
Can you describe how you tested this? We can't run automated tests on external PRs for security reasons, so I'm trying to get a sense of what you checked before merging.
Last week I set all of the allowed_inbound_cidr_blocks lists to the empty list for both our consul and vault nodes' security groups and attempted to use allowed_inbound_security_group_ids instead. Vault went fine but consul gave me the dreaded value of 'count' cannot be computed error.
In my code I use the consul-cluster module with empty lists for allowed_inbound_cidr_blocks and allowed_inbound_security_group_ids.
module "consul_cluster" {
source = "git::https://github.com/hashicorp/terraform-aws-consul.git//modules/consul-cluster?ref=v0.3.10"
allowed_inbound_cidr_blocks = []
allowed_inbound_security_group_ids = []
...
I then use consul-security-group-rules to allow access to other members of the cluster as well as the vault nodes that are clients of the cluster:
module "consul_security_group_rules" {
source = "git::https://github.com/EFXCIA/terraform-aws-consul.git//modules/consul-security-group-rules?ref=bugfix/security-group-count"
security_group_id = "${module.consul_cluster.security_group_id}"
allowed_inbound_cidr_blocks = []
allowed_inbound_security_group_ids = "${concat(
list(module.consul_cluster.security_group_id),
list(module.vault_cluster.security_group_id)
var.consul_allowed_inbound_security_group_ids
)}"
allowed_inbound_security_group_count = "${2+length(var.consul_allowed_inbound_security_group_ids)}"
}
Similarly, my vault cluster uses consul-client-security-group-rules to give the consul cluster access to the consul clients on the vault nodes:
module "security_group_rules" {
source = "git::https://github.com/EFXCIA/terraform-aws-consul.git//modules/consul-client-security-group-rules?ref=bugfix/security-group-count"
security_group_id = "${module.vault_cluster.security_group_id}"
allowed_inbound_cidr_blocks = []
allowed_inbound_security_group_ids = ["${module.consul_cluster.security_group_id}"]
allowed_inbound_security_group_count = 1
}
At a high-level, it is structured thusly:
$ egrep --color '(^[a-z])|(source =)' consul.tf | grep -v '# '
module "consul_cluster" {
source = "git::https://github.com/hashicorp/terraform-aws-consul.git//modules/consul-cluster?ref=v0.3.10"
module "consul_security_group_rules" {
source = "git::https://github.com/EFXCIA/terraform-aws-consul.git//modules/consul-security-group-rules?ref=bugfix/security-group-count"
module "consul_iam_policies_servers" {
source = "git::https://github.com/hashicorp/terraform-aws-consul.git//modules/consul-iam-policies?ref=v0.3.10"
$ egrep --color '(^[a-z])|(source =)' vault.tf | grep -v '# '
module "vault_cluster" {
source = "git::https://github.com/hashicorp/terraform-aws-vault.git//modules/vault-cluster?ref=v0.10.0"
module "security_group_rules" {
source = "git::https://github.com/EFXCIA/terraform-aws-consul.git//modules/consul-client-security-group-rules?ref=bugfix/security-group-count"
module "vault_api_elb" {
source = "git::https://github.com/hashicorp/terraform-aws-vault.git//modules/vault-elb?ref=v0.10.0"
module "vault_api_all_nodes" {
source = "git::https://github.com/hashicorp/terraform-aws-vault.git//modules/vault-elb?ref=v0.10.0"
module "vault_cluster_elb" {
source = "git::https://github.com/hashicorp/terraform-aws-vault.git//modules/vault-elb?ref=v0.10.0"
So, my internal testing, is:
There may still be an issue with the consul-security-group-rules use of consul-client-security-group-rules (module "client_security_group_rules"
in terraform-aws-consul//modules/consul-security-group-rules:main.tf@163). My PR does not modify it to include the allowed_inbound_security_group_count variable I've added. My code does not trigger that module since I provide empty lists to both allowed_inbound_cidr_blocks and allowed_inbound_security_group_ids in my usecase. If you agree it should be included there I'll go ahead & add it.
Similarly, there are three places in terraform-aws-vault using consul-client-security-group-rules (v0.3.3) that I have not touched.
There may still be an issue with the consul-security-group-rules use of consul-client-security-group-rules (module "client_security_group_rules" in terraform-aws-consul//modules/consul-security-group-rules:main.tf@163). My PR does not modify it to include the allowed_inbound_security_group_count variable I've added. My code does not trigger that module since I provide empty lists to both allowed_inbound_cidr_blocks and allowed_inbound_security_group_ids in my usecase. If you agree it should be included there I'll go ahead & add it.
Yes please! In fact, please search for any usage of the consul-client-security-group-rules
and consul-security-group-rules
and update them to pass in the new allowed_inbound_security_group_count
parameter. Similarly, could you update the consul-cluster
module to expose the same parameter and pass it through?
Will do!
OK, that's done.
I also found a nasty little bug in consul-cluster's use of consul-security-group-rules: consul-cluster is not adding its own aws_security_group.lc_security_group.id to the list of allowed_inbound_security_group_ids (and consul-security-group-rules does not automatically add its security_group_id input to the allowed inbound security group ids). Unless that was intentional?
I discovered the bug when I removed my consul_security_group_rules module and let my consul_cluster module pass allowed_inbound_security_group_ids and allowed_inbound_security_group_count itself.
It looks like terraform-aws-vault//modules/vault-cluster has the same problem with its use of vault-security-group-rules. I'm testing the same fix for this locally & will PR it later.
I didn't find any reference to 'self' in consul-security-group-rules or consul-client-security-group-rules so I've added them following the pattern in terraform-aws-vault.
…owed_inbound_security_group_count
Similar to change 707c87 in terraform-aws-vault
Tested internally. Cannot execute the test suite due to corporate restrictions.
Addresses issue #84