terraform-aws-modules/terraform-aws-security-group

Referencing Security Group IDs not yet created as ingress/egress sources

HUSSTECH opened this issue · 8 comments

Hi, ran into an issue yesterday when refactoring my tf code, specifically switching to use more security groups as explicit sources for ingress/egress rules. i.e. like point to point networking. To do this I use the ingress_with_source_security_group_id param like this:

  ingress_with_source_security_group_id = [
    {
      from_port = 5432
      to_port = 5432
      protocol = "tcp"
      description = "DB port ingress from ec2-1-sg"
      source_security_group_id = "${module.ec2-1-sg.this_security_group_id}"
    },
    {
      from_port = 5432
      to_port = 5432
      protocol = "tcp"
      description = "DB port ingress from ec2-2-sg"
      source_security_group_id = "${module.ec2-2-sg.this_security_group_id}"
    }
  ]

This fails to plan, because within the module main.tf file the line count = "${length(var.ingress_with_source_security_group_id)}" is trying to count the length of a list that has a computed value somewhere within it...i.e. the source_security_group_id key of the list elements. After a lot of searching, this is a known issue hashicorp/terraform#12570 (comment).

As an experiment, I tired to just pass the value to count directly as a hard coded number of the total number of rules being defined, in the example above that was 2. That planned and applied successfully 🎉.

So I made the following mods to the module to try to include this in a backwards compatible way. The idea being that if you will be using a computed value in the list of rules, to specify the total number. It is a redundant parameter, but I think it's a small price to pay to allow you to be able to reference security groups that are not yet created, from the same single run of terraform apply.

diff --git a/main.tf b/main.tf
index acfb9a6..5efa91e 100644
--- a/main.tf
+++ b/main.tf
@@ -33,7 +33,7 @@ resource "aws_security_group_rule" "ingress_rules" {
 ##########################
 # Security group rules with "source_security_group_id", but without "cidr_blocks" and "self"
 resource "aws_security_group_rule" "ingress_with_source_security_group_id" {
-  count = "${length(var.ingress_with_source_security_group_id)}"
+  count = "${var.ingress_with_source_security_group_id_count > 0 ? var.ingress_with_source_security_group_id_count : length(var.ingress_with_source_security_group_id)}"
 
   security_group_id = "${aws_security_group.this.id}"
   type              = "ingress"
@@ -121,7 +121,7 @@ resource "aws_security_group_rule" "egress_rules" {
 #########################
 # Security group rules with "source_security_group_id", but without "cidr_blocks" and "self"
 resource "aws_security_group_rule" "egress_with_source_security_group_id" {
-  count = "${length(var.egress_with_source_security_group_id)}"
+  count = "${var.egress_with_source_security_group_id_count > 0 ? var.egress_with_source_security_group_id_count : length(var.egress_with_source_security_group_id)}"
 
   security_group_id = "${aws_security_group.this.id}"
   type              = "egress"
diff --git a/variables.tf b/variables.tf
index f82755c..43ac20d 100644
--- a/variables.tf
+++ b/variables.tf
@@ -47,6 +47,11 @@ variable "ingress_with_source_security_group_id" {
   default     = []
 }
 
+variable "ingress_with_source_security_group_id_count" {
+  description = "Number of ingress rules to create where 'source_security_group_id' is used"
+  default     = 0
+}
+
 variable "ingress_cidr_blocks" {
   description = "List of IPv4 CIDR ranges to use on all ingress rules"
   default     = []
@@ -90,6 +95,11 @@ variable "egress_with_source_security_group_id" {
   default     = []
 }
 
+variable "egress_with_source_security_group_id_count" {
+  description = "Number of egress rules to create where 'source_security_group_id' is used"
+  default     = 0
+}
+
 variable "egress_cidr_blocks" {
   description = "List of IPv4 CIDR ranges to use on all egress rules"
   default     = ["0.0.0.0/0"]

However due to another known issue (hashicorp/hil#50), this fails because the interpreter does not skip the evaluation of the false path of the conditional. So the original error of not allowing computed count values still appears. This issue being resolved should make my code above work, but I don't think it will be backwards compatible. Unless they backport the change into all versions of terraform.

I tired to think through all sorts of syntax hacks to try to get the list to evaluate to a number and ignore the computed sg id's, but no luck. Just wanted to post my findings here in case someone else runs into the issue, or if anyone has any ideas. It could be worth adding a note in the docs to warn about this. If so, am happy to make the PR.

EDIT:

  • better naming of the module in my example code

Thanks for making an effort to make this module better.

I also tried this before and, as you can see, no luck:

I tried to think through all sorts of syntax hacks to try to get the list to evaluate to a number and ignore the computed sg id's, but no luck.

I think we should try to not make this module even more complicated (it is already pretty hard to debug it and test edge cases), but rather mention in README that this is a known limitation of Terraform and users should only pass calculated values. If you could make a PR it would be great!

Thanks @HUSSTECH for your PR!

+1

This is now fixed in v2.0.0. See this section in README.

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.