Bugfix/self not in security group #94
Conversation
…lt-security-group-rules
modules/vault-cluster/main.tf
Outdated
| @@ -132,8 +132,8 @@ module "security_group_rules" { | |||
|
|
|||
| security_group_id = "${aws_security_group.lc_security_group.id}" | |||
| allowed_inbound_cidr_blocks = ["${var.allowed_inbound_cidr_blocks}"] | |||
| allowed_inbound_security_group_ids = ["${var.allowed_inbound_security_group_ids}"] | |||
| allowed_inbound_security_group_count = "${var.allowed_inbound_security_group_count}" | |||
| allowed_inbound_security_group_ids = "${concat(list(aws_security_group.lc_security_group.id), var.allowed_inbound_security_group_ids)}" | |||
There was a problem hiding this comment.
I think the proper way to do this is to add an explicit rule with self in the security group rules module.
Also, I could've sworn we did that already...
There was a problem hiding this comment.
I found my source of confusion: The rule that's there now is for the cluster port (8201). My usecase is expecting the API port (8200) also. I've updated this PR to use the 'self' approach for both. If exposing the API port between nodes doesn't fit your intent (e.g. - minimal permission) then I'll cancel this PR & implement it on my side.
There was a problem hiding this comment.
Ohhhh... Hm, I think exposing that port between nodes is the right thing to do, thx!
modules/vault-cluster/main.tf
Outdated
| @@ -132,7 +132,7 @@ module "security_group_rules" { | |||
|
|
|||
| security_group_id = "${aws_security_group.lc_security_group.id}" | |||
| allowed_inbound_cidr_blocks = ["${var.allowed_inbound_cidr_blocks}"] | |||
| allowed_inbound_security_group_ids = ["${var.allowed_inbound_security_group_ids}"] | |||
| allowed_inbound_security_group_ids = "${var.allowed_inbound_security_group_ids}" | |||
There was a problem hiding this comment.
Why remove the brackets here? I think this may lead to an error when multiple IDs are passed...
There was a problem hiding this comment.
Hrm. To be honest, I've gone back and forth on similar bits in my own code. In variables.tf, allowed_inbound_security_group_ids is defined to be a list. When I removed the brackets my assumption was that, due to that definition, it should "just work". You've got me second-guessing myself but it does work in my usecase. I'll put the brackets back since it was clearly working & passing tests.
|
Great, thanks! Merging now and will let tests run. When they pass, I'll issue a new release and paste the link here. |
When the security group for the vault cluster's nodes is created, it does not include itself in the allowed inbound security group ids.