feat: Move to using for_each for resource blocks #326

pincher95 · 2024-08-29T07:58:57Z

Description

Move the security groups rules resource blocks to using the for_each meta-argument from the count.

Motivation and Context

It is pretty widely understood that using the count meta-argument can cause churn (ref) in some contexts, such as the context in which this module is used.

Breaking Changes

This change will break all security group rules resources based on count. It will cause churn when switching from an old count-based version of the module to a for_each-based version, because the resources are switching from being an ordered list (sg_rule[0], sg_rule[1], etc.) to a map (sg_rule["443-443-tcp"], sg_rule["22-22-tcp"], etc.).

How Has This Been Tested?

I have updated at least one of the examples/* to demonstrate and validate my change(s)
I have tested and validated these changes using one or more of the provided examples/* projects

# module.security_group.aws_security_group_rule.ingress_with_cidr_blocks["443-443-tcp"] will be created
  + resource "aws_security_group_rule" "ingress_with_cidr_blocks" {
      + cidr_blocks              = [
          + "10.10.0.0/16",
        ]
      + description              = "HTTPS"
      + from_port                = 443
      + id                       = (known after apply)
      + ipv6_cidr_blocks         = []
      + prefix_list_ids          = []
      + protocol                 = "tcp"
      + security_group_id        = (known after apply)
      + security_group_rule_id   = (known after apply)
      + self                     = false
      + source_security_group_id = (known after apply)
      + to_port                  = 443
      + type                     = "ingress"
    }

I have executed pre-commit run -a on my pull request

…orm-aws-modules

pincher95 · 2024-09-02T09:57:53Z

@antonbabenko @bryantbiggs
Can you please check this PR.

Thank you

bryantbiggs · 2024-09-05T13:41:28Z

I think if we are going to do a breaking change on this module, we should evaluate all potential changes in order to minimize the amount of disruption. For example, I would hope in the next breaking change we would drop the use of aws_security_group_rule and replace it with aws_vpc_security_group_ingress_rule/ aws_vpc_security_group_egress_rule

bryantbiggs

just an initial glance - if we are going to do this, we should re-evaluate it from a first principles approach, not just for the sake of changing from count to for_each

bryantbiggs · 2024-09-05T13:42:07Z

main.tf

-# Computed - Security group rules with "cidr_blocks" and it uses list of rules names
-resource "aws_security_group_rule" "computed_ingress_rules" {
-  count = local.create ? var.number_of_computed_ingress_rules : 0
+  for_each = local.create ? toset(var.ingress_rules) : toset([])


we cannot use toset() in this manner because it will inevitably cause conflicts down the road with computed values as keys

bryantbiggs · 2024-09-05T13:42:38Z

main.tf

@@ -64,123 +41,70 @@ resource "aws_security_group" "this_name_prefix" {
 ###################################
 # Security group rules with "cidr_blocks" and it uses list of rules names
 resource "aws_security_group_rule" "ingress_rules" {


this should be

Suggested change

resource "aws_security_group_rule" "ingress_rules" {

resource "aws_vpc_security_group_ingress_rule" "this" {

bryantbiggs · 2024-09-05T13:43:32Z

main.tf

-  protocol  = var.rules[var.computed_ingress_rules[count.index]][2]
+  from_port = var.rules[each.value][0]
+  to_port   = var.rules[each.value][1]
+  protocol  = var.rules[each.value][2]
 }

 ##########################
 # Ingress - Maps of 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" {


all of these can go away to one generic ingress rule resource and one generic egress rule resource

bryantbiggs · 2024-09-05T13:43:47Z

main.tf

@@ -189,295 +113,98 @@ resource "aws_security_group_rule" "computed_ingress_with_source_security_group_

 # Security group rules with "cidr_blocks", but without "ipv6_cidr_blocks", "source_security_group_id" and "self"
 resource "aws_security_group_rule" "ingress_with_cidr_blocks" {


bryantbiggs · 2024-09-05T13:44:00Z

main.tf

-      "rule",
-      "_",
-    )][2],
+    var.rules[lookup(each.value, "rule", "_")][2],
  )
 }

 # Security group rules with "self", but without "cidr_blocks" and "source_security_group_id"
 resource "aws_security_group_rule" "ingress_with_self" {


bryantbiggs · 2024-09-05T13:44:23Z

main.tf

@@ -525,123 +252,69 @@ resource "aws_security_group_rule" "computed_ingress_with_prefix_list_ids" {
 ##################################
 # Security group rules with "cidr_blocks" and it uses list of rules names
 resource "aws_security_group_rule" "egress_rules" {


Suggested change

resource "aws_security_group_rule" "egress_rules" {

resource "aws_vpc_security_group_egress_rule" "this" {

bryantbiggs · 2024-09-05T13:44:31Z

main.tf

@@ -525,123 +252,69 @@ resource "aws_security_group_rule" "computed_ingress_with_prefix_list_ids" {
 ##################################
 # Security group rules with "cidr_blocks" and it uses list of rules names
 resource "aws_security_group_rule" "egress_rules" {
-  count = local.create ? length(var.egress_rules) : 0
+  for_each = local.create ? toset(var.ingress_rules) : toset([])


no toset()

bryantbiggs · 2024-09-05T13:44:42Z

main.tf

@@ -650,255 +323,98 @@ resource "aws_security_group_rule" "computed_egress_with_source_security_group_i

 # Security group rules with "cidr_blocks", but without "ipv6_cidr_blocks", "source_security_group_id" and "self"
 resource "aws_security_group_rule" "egress_with_cidr_blocks" {


bryantbiggs · 2024-09-05T13:44:51Z

main.tf

-      "rule",
-      "_",
-    )][2],
+    var.rules[lookup(each.value, "rule", "_")][2],
  )
 }

 # Security group rules with "self", but without "cidr_blocks" and "source_security_group_id"
 resource "aws_security_group_rule" "egress_with_self" {


bryantbiggs · 2024-09-05T13:44:58Z

main.tf

    "protocol",
-    var.rules[lookup(var.computed_egress_with_self[count.index], "rule", "_")][2],
+    var.rules[lookup(each.value, "rule", "_")][2],
  )
 }

 # Security group rules with "egress_prefix_list_ids", but without "cidr_blocks", "self" or "source_security_group_id"
 resource "aws_security_group_rule" "egress_with_prefix_list_ids" {


github-actions · 2024-10-06T00:09:54Z

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

github-actions · 2024-10-17T00:09:10Z

This PR was automatically closed because of stale in 10 days

github-actions · 2024-11-16T02:19:52Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

pincher95 added 6 commits August 27, 2024 08:54

Initial commit

970a718

Update examples

e02398b

Update modules to use Terraform 1.x and AWS provider 5.63.x by terraf…

3225e8f

…orm-aws-modules

Update README.md

042342f

Revert "Update README.md"

35db973

Update README.md files

b69e9dc

Merge branch 'master' into master

2c46286

bryantbiggs requested changes Sep 5, 2024

View reviewed changes

github-actions bot added the stale label Oct 6, 2024

github-actions bot closed this Oct 17, 2024

github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Move to using for_each for resource blocks #326

feat: Move to using for_each for resource blocks #326

pincher95 commented Aug 29, 2024 •

edited

Loading

pincher95 commented Sep 2, 2024

bryantbiggs commented Sep 5, 2024

bryantbiggs left a comment

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

bryantbiggs Sep 5, 2024

github-actions bot commented Oct 6, 2024

github-actions bot commented Oct 17, 2024

github-actions bot commented Nov 16, 2024

	resource "aws_security_group_rule" "ingress_rules" {
	resource "aws_vpc_security_group_ingress_rule" "this" {

		@@ -189,295 +113,98 @@ resource "aws_security_group_rule" "computed_ingress_with_source_security_group_

		# Security group rules with "cidr_blocks", but without "ipv6_cidr_blocks", "source_security_group_id" and "self"
		resource "aws_security_group_rule" "ingress_with_cidr_blocks" {

	resource "aws_security_group_rule" "egress_rules" {
	resource "aws_vpc_security_group_egress_rule" "this" {

		@@ -650,255 +323,98 @@ resource "aws_security_group_rule" "computed_egress_with_source_security_group_i

		# Security group rules with "cidr_blocks", but without "ipv6_cidr_blocks", "source_security_group_id" and "self"
		resource "aws_security_group_rule" "egress_with_cidr_blocks" {

feat: Move to using for_each for resource blocks #326

feat: Move to using for_each for resource blocks #326

Conversation

pincher95 commented Aug 29, 2024 • edited Loading

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

pincher95 commented Sep 2, 2024

bryantbiggs commented Sep 5, 2024

bryantbiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2024

github-actions bot commented Oct 17, 2024

github-actions bot commented Nov 16, 2024

pincher95 commented Aug 29, 2024 •

edited

Loading