Last minute fixes (module is almost ready)

[antonbabenko]

I am almost done with this module. Remaining:

• create rules groups (copy them from tf_aws_sg) + Add TCP/UDP:8302(Serf WAN) to sg_consul terraform-community-modules/tf_aws_sg#56
• update README to describe how to update_groups
• review it all
• tag, publish to registry, deprecate old repo

I need a second look at all of this, so could you guys take a look and tell me if you see something completely bad/wrong/missing in this module?

/cc @kamilboratynski @brandoconnor @solarce @hakamadare @joestump @tfhartmann ...

[brandonjbjelland]

Quite a different structure than the previous repo now that the registry imposes some constraints. Overall, it looks good to me but here are a few critiques:

• a few of the outputs are the same as their input variables. Where this is the case, I don't think an output is needed (e.g. this_security_group_vpc_id, this_security_group_description). Outputs (in my opinion) should really consist of strings generated by terraform apply that will be useful to other resource declarations or to the operators.
• I tend to avoid filling in descriptions as done here. There are a handful of AWS resources (SG used to be one of them) where if the description is changed from TF the resource will be destroyed and recreated. In light of that and the inconsistency of that policy across various resources, I choose to eliminate the overhead of having to think about that and look it up for each resource and instead always use a tag to create descriptions. That could have been fixed but I would still guess there to be gotchas here and there around description changes.
• the map of rules is an interesting and good way to handle the data. I wish we were able to use a map of maps and not rely on the order of the array but I haven't tried to come up with something like this yet. Probably possible; likely wouldn't be so elegant.

[antonbabenko]

Thanks for the feedback.

I like to treat Terraform resource modules as dumb and logicless (almost) as possible, but versioned. It means all names, outputs, variables, descriptions, types of variables are the same as in Terraform documentation. This way consumers of these modules don't have to think about minor things and think more about how to combine these modules together to create infrastructure modules, which we don't have here (yet).

A security group has default description, which users of this module can override. It won't cause recreation of the security group.

I tend to use the standard types of outputs (as returned by Terraform) and play with maps or custom structures in higher level (eg, infrastructure modules).

[github-actions]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.