v0.40.0 vs v0.39.0 security group behavior

[syphernl]

With 0.40.0 the setup process of this module has become a little more complex. Prior to 0.40.0 (e.g. with 0.39.0) one could simply tell the module to allow which SG's like so:

  allowed_security_groups = [
    module.api.ecs_service_security_group_id,
    join(",", module.bastion.security_group_ids)
  ]

It would allow any of these SG's to connect to var.port. Easy.

With v0.40.0 this has changed and now requires explicit declaration of rules like this:

  security_group_rules = [
    {
      type                     = "egress"
      from_port                = 0
      to_port                  = 65535
      protocol                 = "-1"
      cidr_blocks              = ["0.0.0.0/0"]
      source_security_group_id = null
      description              = "Allow all outbound traffic"
    },
    {
      type                     = "ingress"
      from_port                = 6379
      to_port                  = 6379
      protocol                 = "tcp"
      cidr_blocks              = []
      source_security_group_id = module.api.ecs_service_security_group_id
      description              = "Allow inbound Redis traffic from ECS"
    },
    {
      type                     = "ingress"
      from_port                = 6379
      to_port                  = 6379
      protocol                 = "tcp"
      cidr_blocks              = []
      source_security_group_id = join(",", module.bastion.security_group_ids)
      description              = "Allow inbound Redis traffic from Bastion"
    },
  ]

I have tried to define them as such:

  security_groups = [
    module.api.ecs_service_security_group_id,
    join(",", module.bastion.security_group_ids)
  ]

While they do get added to the Redis instance, it does not allow to connect from them.

The README shows a nice "allow all outbound" and a "allow all from VPC" example but the default value of the security_group_rules only contains the former.

Am I using this module the wrong way or is this actually they way it is supposed to work (now)? If so, is there something we can do to improve this behavior?

[nitrocode]

Apologies for the changes. We're reviewing internally on release notes and how to better manage the upgrade process.

Notable changes

* These resources were removed since the module requires passing in the sg rules via `var.security_group_rules` * `aws_security_group.default` sg itself * `aws_security_group_rule.egress` (previously using `var.egress_cidr_blocks` for all ports) * `aws_security_group_rule.ingress_security_groups` (previously using `var.allowed_security_groups` for `var.port`) * `aws_security_group_rule.ingress_cidr_blocks` (previously using `var.allowed_cidr_blocks` for `var.port`)

It looks like this may be the culprit in your example.

    {
      type                     = "ingress"
      from_port                = 6379
      to_port                  = 6379
      protocol                 = "tcp"
      cidr_blocks              = []
      source_security_group_id = join(",", module.bastion.security_group_ids)
      description              = "Allow inbound Redis traffic from Bastion"
    },

You can only pass in a single security group (see cloudposse/terraform-aws-security-group) to source_security_group_id

Also the module used to add

  allowed_security_groups          = [module.vpc.vpc_default_security_group_id]

Try this terraform and see if it works for you

hcl

Note: Untested ```hcl locals { bastion_allowed_security_groups = [ for sg in module.bastion.security_group_ids : { type = "ingress" from_port = 6379 to_port = 6379 protocol = "tcp" cidr_blocks = [] source_security_group_id = sg description = "Allow inbound Redis traffic from Bastion" } ] } module "redis" { source = "cloudposse/elasticache-redis/aws" # ... security_group_rules = concat([ { type = "egress" from_port = 0 to_port = 0 protocol = "-1" cidr_blocks = ["0.0.0.0/0"] # previously var.egress_cidr_blocks source_security_group_id = null description = "Allow outbound traffic from existing cidr blocks" }, { type = "ingress" from_port = 6379 to_port = 6379 protocol = "tcp" cidr_blocks = [] source_security_group_id = module.api.ecs_service_security_group_id description = "Allow inbound Redis traffic from ECS" }, { type = "ingress" from_port = 6379 to_port = 6379 protocol = "tcp" cidr_blocks = [] # note: vpc 0.26.0 removed this output by mistake so use 0.25.0 or > 0.26.0 when it's released source_security_group_id = module.vpc.vpc_default_security_group_id description = "Allow inbound Redis traffic from VPC" } ], local.bastion_allowed_security_groups) } ```

[nitrocode]

cc: @syphernl

[syphernl]

@nitrocode The rules I posted in my initial message (2nd code block) works for us as our module.bastion.security_group_ids only contains one entry. It would however be a bit more complex if it did contain more than one.

With your example however I could technically remove the specific rules for ECS and Bastion, since they both exist within the VPC. But that would also allow anything in the VPC to connect to Redis, which might not be desired.

The reason why I opened this issue is the fact that the usage of the module has been made a bit more complicated switching to this new approach. Before I could just tell it to allow security group X and Y (ECS + Bastion), now I have to explicitly add the rules for each of them in there.

I fully understand the reasoning behind standardization and while it does add flexibility, it also takes some away we had in versions prior to 0.40.0.

[nitrocode]

Apologies for the trouble @syphernl.

I wrote up some release notes here on 0.40.0 on the breaking changes and the upgrade procedure. Let us know if we should include additional details or if it doesn't work for you. If you have additional suggestions, please feel free to comment here.

We'll try to provide breaking change release notes on future upgrades across our modules.

[Nuru]

@syphernl We are taking feedback like yours into account and have marked v0.40.0 pre-release. We recommend using 0.39.0 for now and waiting for a later release which will provide better backward compatibility and clearer migration instructions.