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

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

Closed
HUSSTECH opened this Issue Nov 5, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@HUSSTECH
Copy link
Contributor

HUSSTECH commented Nov 5, 2017

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
@antonbabenko

This comment has been minimized.

Copy link
Member

antonbabenko commented Nov 11, 2017

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!

@antonbabenko

This comment has been minimized.

Copy link
Member

antonbabenko commented Nov 13, 2017

Thanks @HUSSTECH for your PR!

@ntman4real

This comment has been minimized.

Copy link

ntman4real commented Apr 12, 2018

+1

3 similar comments
@angusfz

This comment has been minimized.

Copy link

angusfz commented Apr 13, 2018

+1

@nicklucking

This comment has been minimized.

Copy link

nicklucking commented Apr 23, 2018

+1

@teamhanded

This comment has been minimized.

Copy link

teamhanded commented Apr 26, 2018

+1

@antonbabenko

This comment has been minimized.

Copy link
Member

antonbabenko commented May 29, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.