Added allowed_cidr_blocks variable to allow traffic from CIDR blocks

[iuriaranda]

No description provided.

[simonrondelez]

not sure if this should be part of the RDS module

[duboisph]

PR in itself is ok. But maybe like Simon says this shouldn't be part of the RDS module at all and we should just output the SG ID so we can attach rules to it via our "standard" way of centralising them in the securitygroups module.

Of course that would be a breaking change for other projects, so if we go that way we need to be sure it's versioned.

If I'm not mistaken that's the same behavior as with the Bastion module.

[iuriaranda]

Well I sort of agree, but in any case that wasn't possible before this PR, because the ingress rule for var.security_groups was embedded inside the security group definition. After this PR is merged you can do it both ways, because both var.security_groups and var.allowed_cidr_blocks are optional. So for complex setups you can attach rules outside this module like you said, but for simpler setups you can simply pass the cidr blocks or the security groups you want to give access into the module as variables.

[duboisph]

Ah, true, you don't have to provide them. I propose you add the output for the SG then and document the possibility for such a setup.

[duboisph]

Actually, I overlooked this in my first review. Are you sure this works, did you test it? Tried to do the same a couple of TF versions ago and it was not possible in a count. That means you have to add an extra parameter to set the count fixed.

[iuriaranda]

yep it's working here: https://github.com/skyscrapers/skyscrapers-terraform/blob/master/stacks/test/static/concourse_k8s_rds.tf#L24

[duboisph]

Ok, cool, than my positive review is still valid ;)

[ringods]

Simple cases work, but not yet everything. Here is some more info what is supported and what not:

hashicorp/terraform#11482 (comment)