Safer security group settings

[alexjurkiewicz]

what

• Specify a name_prefix, rather than name
• Ignore changes in the security group's name
• Set the security group as create_before_destroy

why

• SG name: The default security name is extremely generic and additionally conflicts with itself if the DB is rebuilt
• SG ignore_changes: Security group rebuilds are generally disruptive and should be avoided
• SG create_before_destroy: Minimise disruption when we do have to rebuild

references

• Fixes Use name_prefix and create_before_destroy on security groups #86

[Gowiem]

@alexjurkiewicz typically we don't use name_prefix AFAIK. This is because the label module (which is module.this.id in this case) allows for enough customization to create a unique enough name for any given scenario. If you need to rebuild a specific RDS cluster you can use the attributes input variable to accomplish creating a new one and then you can delete the old. Does that make sense?

[alexjurkiewicz]

Thanks for the feedback. Just making sure I'm clear: you are saying it's not a bug if the module fails while re-creating resources if the context is unchanged?

[Gowiem]

@alexjurkiewicz hm, putting it that way makes me re-think this. I might be being overly biased here -- I'll bring it up with the contributor team and get others thoughts.

[jamengual]

I use this module a lot to create and destroy cluster and clones.
recreating has always the problem of conflicts depending if the destroy plan happen to be in the order that was supposed to be, although this happened mostly in the early days I do not see it very often but as like @Gowiem said using the label module is very easy to customize a name enough that you will not have a conflict.

could @alexjurkiewicz please expand on the cases you have seen this? and maybe paste and output? I'm interested in reproducing it.

Thanks.

[alexjurkiewicz]

I created this module with the following context: namespace = "myapp" stage = "prd" name = "auth" It created a security group called "myapp-prd-auth". I thought that name was a little generic, so I changed name => name_prefix. I could have also added a hardcoded suffix of "-rds" but decided not to. I then added create_before_destroy because IMO it is better default behaviour for security groups.

…

On Fri, 4 Dec 2020 at 15:40, PePe Amengual ***@***.***> wrote: I use this module a lot to create and destroy cluster and clones. recreating has always the problem of conflicts depending if the destroy plan happen to be in the order that was supposed to be, although this happened mostly in the early days I do not see it very often but as like @Gowiem <https://github.com/Gowiem> said using the label module is very easy to customize a name enough that you will not have a conflict. could @alexjurkiewicz <https://github.com/alexjurkiewicz> please expand on the cases you have seen this? and maybe paste and output? I'm interested in reproducing it. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4U5KV6RXNVCJKUSG3PLLSTBR3JANCNFSM4UKCFE4Q> .

[jamengual]

I think the https://github.com/cloudposse/terraform-null-label gives enough flexibility to solve this problem, if you did not like the name then you could have added an attribute or something to change it to make it look like you wanted.

[Nuru]

@alexjurkiewicz The idea of the naming scheme is that you specify enough information so that you can recognize the component being referenced from the generated id. In your example, I might choose the name "authdb", and then the SG name " "myapp-prd-authdb" makes sense to me. The point is that the security group is named the same as the cluster so that you can easily associate the two.

It is true that deleting security groups (or even worse, VPCs) can take a long time or fail completely. That is more of a consequence of what is in them than how they are named. If there is a genuine issue of things not being created or deleted in the correct order, we should fix that using depends_on, not paper over it.

That said, I'm not opposed to using name_prefix and create_before_destroy, but since it is not backward compatible and will cause everyone using the module to change their SGs using the old destroy before create behavior, I would not want to make it the default behavior. Unfortunately I do not see a way to add this feature in a way that will not disturb existing clusters, so it would have to be solving a serious problem for me to support it.

[nitrocode]

Why not support both name and name_prefix arguments using a variable switch?

 name        = var.use_name ? module.this.id : null
  name_prefix = var.use_name ? null :  module.this.id

[alexjurkiewicz]

Thanks for the feedback all.

That said, I'm not opposed to using name_prefix and create_before_destroy, but since it is not backward compatible and will cause everyone using the module to change their SGs using the old destroy before create behavior, I would not want to make it the default behavior. Unfortunately I do not see a way to add this feature in a way that will not disturb existing clusters, so it would have to be solving a serious problem for me to support it.

You are right, I think we can't break backwards compatibility with this change. That's why I added a lifecycle directive to ignore changes in the security group name. For people with this module already deployed, their security group won't change. If they ever rebuild from scratch, they'll get the new name style.

[Nuru]

You are right, I think we can't break backwards compatibility with this change. That's why I added a lifecycle directive to ignore changes in the security group name. For people with this module already deployed, their security group won't change. If they ever rebuild from scratch, they'll get the new name style.

The problem is that the introduction of the lifecycle directive itself will cause the current resource to be destroyed and recreated.

[alexjurkiewicz]

Dang. I never realised that. I looked into alternate solutions here but couldn't find any. Thanks for your feedback.