search

dcos-terraform / terraform-aws-vpc-peering

Apache License 2.0
1 stars 3 forks source link

Intermodule change should be avoided. #3

Open fatz opened 5 years ago

fatz commented 5 years ago

https://github.com/dcos-terraform/terraform-aws-vpc-peering/blob/af2e48cb01ec73356c390f0e1326af608bdb7c2c/main.tf#L108-L128

ok now I kinda understand the changes in security group module.

However I do not like the idea to make changes on resources another module is holding. As all local subnets and supernets are known before creation I'd rather suggest having each security group module doing this on their own and not here. Which means there is no need for introducing a breaking change to security groups

bernadinm commented 5 years ago

NACL was considered here but currently by default it allows all traffic already: https://docs.aws.amazon.com/vpc/latest/userguide/vpc-network-acls.html#default-network-acl

This is the same for all security groups as well: https://docs.aws.amazon.com/vpc/latest/userguide/VPC_SecurityGroups.html#DefaultSecurityGroup

Since this is the case that we're already configuring rules at the security group level, that would mean that if one terraform module wants to scale out, the other module would need to run to make sure that its rules are applied too for the hybrid cloud environment and this will cause problems with circular dependency for resources that potentially can come in and out of existence more often and the use of the data resource problem that was mentioned in https://github.com/terraform-providers/terraform-provider-aws/issues/7014

If we instead use a module where every module can look for i.e "internal" security group where any module can append or remove asynchronously without issue

fatz commented 5 years ago

lets have a chat about this in todays standup.

I still disagree in general. As the user has to decide on remote subnets anyways. We should not even try to have defaults for those subnets as it will lead to conflicts when users are not aware about this fact.

With that in mind there is already a static input about subnets on the top level module. which means we simply can have a variable containing all subnets. This list can be filtered and simply added by each module locally.