PaloAltoNetworks / terraform-aws-swfw-modules

Terraform Reusable Modules for Software Firewalls on AWS
https://registry.terraform.io/modules/PaloAltoNetworks/swfw-modules/aws
MIT License
8 stars 10 forks source link

fix(modules/asg): make tags_dest customizable with additional resources when needed #30

Closed srihas619 closed 3 months ago

srihas619 commented 3 months ago

Description

Fixing tags related bug while launching resources via ASG. Addresses the issue https://github.com/PaloAltoNetworks/terraform-aws-swfw-modules/issues/29

sebastianczech commented 3 months ago

/validate paths="modules/asg"

Testing job ID: 8598039382 Job result: FAILURE

srihas619 commented 3 months ago

@sebastianczech Thanks for the pointer. I would prefer to use concat in this case since it will allow more resources to be added when needed via variable.

srihas619 commented 3 months ago

/validate paths="modules/asg"

sebastianczech commented 3 months ago

/validate paths="modules/asg"

Testing job ID: 8599598637 Job result: SUCCESS

sebastianczech commented 3 months ago

/plan paths="examples/combined_design_autoscale"

Testing job ID: 8599628127 Job result: SUCCESS

sebastianczech commented 3 months ago

/sca

Testing job ID: 8599664796 Job result: SUCCESS

sebastianczech commented 3 months ago

/idempotence paths="examples/combined_design_autoscale"

Testing job ID: 8599693816 Job result: SUCCESS

sebastianczech commented 3 months ago

Now your changes work, but maybe we should keep it simpler?

For example - do not use concat or coalesce, just remove local.tags_dest and use variable directly:

  dynamic "tag_specifications" {
    for_each = toset(var.tags_dest)
    content {
      resource_type = tag_specifications.key
      tags          = local.tags_merged
    }
  }

Then variable will have default value - if somebody wants to define his/her custom list, it's dooable:

variable "tags_dest" {
  description = "List of resources that need to be tagged when launched via autoscaling group"
  type        = list(string)
  default     = ["instance", "volume", "network-interface"]
}
srihas619 commented 3 months ago

Thanks for validating changes.

Regarding your suggestion, I also gave a thought along these lines initially but then, I realized if we need to add more resources then we have to initialize this variable with defaults again and then append extra resources like ["instance", "volume", "network-interface", "<someother_resource1>","<someother_resource2>"]

But I agree, it makes the implementation simpler, I will push an update.

sebastianczech commented 3 months ago

/validate paths="modules/asg"

Testing job ID: 8600063384 Job result: SUCCESS

sebastianczech commented 3 months ago

/sca

Testing job ID: 8600095417 Job result: SUCCESS

sebastianczech commented 3 months ago

/idempotence paths="examples/combined_design_autoscale"

Testing job ID: 8600124073 Job result: SUCCESS

srihas619 commented 3 months ago

@sebastianczech Thanks for the quick approval and all the suggestions. Now, we wait for the merge and the release cut from it so we can unpin the 2.0.4 version.

sebastianczech commented 3 months ago

/validate paths="modules/asg"

Testing job ID: 8613757738 Job result: SUCCESS

sebastianczech commented 3 months ago

/sca

Testing job ID: 8613784364 Job result: SUCCESS

sebastianczech commented 3 months ago

/plan paths="examples/combined_design_autoscale"

Testing job ID: 8614325342 Job result: SUCCESS