remove allowed_security_groups_count variable

[vdeyro]

No description provided.

[max-rocket-internet]

Unfortunately this won't always work. That's why it's implemented with allowed_security_groups_count in this module and many others. Moving the count to a local doesn't always work either.

Reference: hashicorp/terraform#12570

[vdeyro]

@max-rocket-internet thanks for pointing that out.

maybe we can just add ingress in aws_security_group

resource "aws_security_group" "this" {
  name_prefix = "${var.name}-"
  vpc_id      = "${var.vpc_id}"

  tags = "${var.tags}"

  ingress {
    from_port       = "${local.port}"
    to_port         = "${local.port}"
    protocol        = "tcp"
    security_groups = "${var.allowed_security_groups}"
  }
}

and remove

resource "aws_security_group_rule" "default_ingress" {
  count = "${var.allowed_security_groups_count}"

  type                     = "ingress"
  from_port                = "${aws_rds_cluster.this.port}"
  to_port                  = "${aws_rds_cluster.this.port}"
  protocol                 = "tcp"
  source_security_group_id = "${element(var.allowed_security_groups, count.index)}"
  security_group_id        = "${aws_security_group.this.id}"
}

[max-rocket-internet]

Test it and if it works in the same way, sure.

Note: aws_security_group.this.ingress.security_groups is a list:

resource "aws_security_group" "this" {
  name_prefix = "${var.name}-"

  ingress {
    from_port       = "${local.port}"
    to_port         = "${local.port}"
    protocol        = "tcp"
    security_groups = ["${var.allowed_security_groups}"]
  }
}

[max-rocket-internet]

I think is is a list: security_groups = ["${var.allowed_security_groups}"]

[vdeyro]

if we make it like that. it will become a list within a list since allowed_security_groups is already a list. won't it? already tested it earlier and it worked as i expected.

[max-rocket-internet]

if we make it like that. it will become a list within a list

Nope. It's just how TF works. e.g. https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/master/main.tf#L44

[vdeyro]

oh thanks. didn't know that. but it's still working the way i have designed it. should i change it for consistency?

[antonbabenko]

I think we need to keep this as it is now because there are situations when the security groups created and used for an RDS contain not just a single ingress rule but also other rules.

Using mix of inline ingress {...} block and separate aws_security_group_rule will lead to constant overrides where aws_security_group_rule is preferred for extensibility.

[vdeyro]

We can add cidr block and even self (introduction of new variables) on the aws_security_group as far as db is concerned it will only need to allow one port from different sources, and that is from SGs, CIDR blocks and itself.

as it is not recommended to always edit the module, for a more custom made security group we can add a different variable for a list of seperately created security groups that we would attach to the db cluster. which i guess is for a different pull request.

[antonbabenko]

With the changes proposed it is not possible to allow access from CIDR blocks (ipv4, ipv6), self, prefix lists. It is also not possible to mix this with other rules (created using aws_security_group_rule resources), which user will create outside of the module and attach to the security group created because it will be overridden by inline rules.

[vdeyro]

i didn't mean to mix with aws_security_group_rule. what i meant is to add other aws_security_group for a more custom security group. if you are going to add other aws_security_group_rule to the db security group it will most likely only be for the same port as the local.port won't it? so why not anticipate it by putting it inline?

*haven't inserted the cidr_blocks,ipv6_cidr_blocks and self in the code yet

[antonbabenko]

Ok, try to make it work and see what happens. The scenario is to get this module to create a RDS resources and:

1. Allow another security group to connect to it. Create this SG outside of the module.
2. Allow connection from another cidr block (ipv4 and ipv6) to connect to it.

If you can make this to work without using aws_security_group_rule it is great, but I don't think it is going to be an easy task (if possible).

[vdeyro]

@antonbabenko i added additional variables for CIDR blocks and self. Will this be enough for this merge?

sorry, i didn't notice that my changes on the use of externally created db_subnet_group was committed as well. i was thinking of doing this. but i only got to finish the db subnet group part today

Add conditional creation of optional resources (security group, db subnet group can be provided externally).
#2

[vdeyro]

Will redo the commits. thanks for the inputs.

[github-actions]

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.