Allow the addition of IP-based ingress rules #44
Conversation
main.tf
Outdated
| resource "aws_security_group_rule" "default_ingress" { | ||
| count = "${var.allowed_security_groups_count}" | ||
| count = "${local.allowed_security_groups_count}" |
There was a problem hiding this comment.
This won't work. In many situations it will give the famous value of 'count' cannot be computed error. That's why it's coded in this annoying way with the xxx_count variable, specifically to avoid this error.
Maybe TF 0.12 has addressed this but I'm not sure.
There was a problem hiding this comment.
I'll claim ignorance here: using a local, and declaring the local, above, allows you to actually count the elements of a list, which produces a number.
In which situation will you encounter value of 'count' cannot be computed?
Thanks!
There was a problem hiding this comment.
The local thing doesn't change the problem, just moves it. It happens sometimes when a resource attribute being counted and that resource isn't created yet.
e.g.
allowed_security_groups = ["sg-12345678", "${aws_security_group.xxxx.id}"]Where aws_security_group.xxxx.id is not created yet. So first run will fail. You can work around it by running terraform apply -target=aws_security_group.xxxx.id and then terraform apply after.
Statement is here: hashicorp/terraform#10857 (comment)
And a longer issue is here: hashicorp/terraform#12570
You can see the _count thing being used in other modules, e.g. https://github.com/terraform-aws-modules/terraform-aws-alb
There was a problem hiding this comment.
Ok, I've readded that variable and another for cidr blocks.
There was a problem hiding this comment.
@antonbabenko is this annoying count workaround resolved in 0.12 now?
There was a problem hiding this comment.
Yes, it looks like (at least to some degree if I understand it correctly, for example, this bug in ACM module).
I am updating modules to be as close as possible with existing features and after that, I am planning to update the code and do some breaking changes like this. I am looking forward to that myself :)
There was a problem hiding this comment.
@antonbabenko looking forward too, because I can't access RDS from outside if I won't have additional cidr blocks in here to add my office IP :)
There was a problem hiding this comment.
because I can't access RDS from outside if I won't have additional cidr blocks
You know you can just define an SG outside of the module with the required access and then pass it to the module as allowed_security_groups? That's the intended design.
There was a problem hiding this comment.
@max-rocket-internet we tried that, if you will defines your own sg it will be EC2 sg, which is not RDS sg. Because RDS sg is ment to add a sg from EC2, which is basically ties VPC to RDS sg. And my intention is avoid VPC, and connect to RDS via DNS which AWS provides with "publicly_accessible" parameter :)
In this case if you want RDS DNS to work, then source field in RDS sg should be CIDR, and not another sg ID like it's done now. I'll just repeat in here - like if you provide your own SG to this module, it will create RDS SG and will link to your provided SG. So this is not what I need at all :) I needs RDS SG to contain CIDR in source field. Because otherwise you'll only be able to connect through vpc, for which you need your own ip, and it's more complex setup than just a DNS from RDS. I've currently forked this project and applied your PR. This is the only way to make it work.
@antonbabenko please пожалуйста can this be merged, I can't provision database without this PR. 5$ donation been sent to you :)
There was a problem hiding this comment.
Thanks, Roman for the donation! Good surprise :) Will review and merge now.
README.md
Outdated
| @@ -31,7 +31,9 @@ module "db" { | |||
|
|
|||
| replica_count = 1 | |||
| allowed_security_groups = ["sg-12345678"] | |||
| allowed_security_groups_count = 1 | |||
| alllowed_security_groups_count = 1 | |||
There was a problem hiding this comment.
It should allowed_security_groups_count
|
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. |
As a user of this module, I would like to be able to whitelist IP address ranges that are allowed to reach my RDS cluster.
I can add ingress rules and reference the output as the target security group, but I can also submit this pull request.
Also, there's no need for the allowed_security_groups_count variable. Just set a local variable and refer to that for counts.
It's worthy to note that I have already used my fork of this module and it does what I want.