Consider security group rules with source security groups when hashing

[jszwedko]

Previously they would conflict you had multiple security group rules
with the same ingress or egress ports but different source security
groups because only the CIDR blocks were considered (which are empty
when using source security groups).

This address #2294

Couple of discussion notes:

• I think that security group rules can only have one cidr block or one source pair. We could potentially consider removing the sorting and just pulling off the first element of each list.
• Not sure if the panic() I'm doing in the sort helper struct I'm doing is idiomatic for the terraform repo

[jszwedko]

This will also cause terraform to generate different hashes now for existing security group rules that have security groups as sources (which might be considered a breaking change?), but I'm not sure how to avoid it. However, I think breaking the existing hashes is preferable to the current broken behavior which disallows multiple rules for the same ports with different sources.

[catsby]

Hey @jszwedko – thanks for the patch here!

Not sure if the panic() I'm doing in the sort helper struct I'm doing is idiomatic for the terraform repo

Probably not a good idea... we can change this

This will also cause terraform to generate different hashes now for existing security group rules that have security groups as sources (which might be considered a breaking change?)

This is true, unfortunately. It causes existing rules to not be found, and Terraform wasn't to re-create them. This will error with a duplicate warning 😦

I think though that we can fix this by migrating the state in a schema migration. I discussed this internally and we think It Should Work™, so I'm trying it out here and will report back when I'm done.

Thanks again!

[catsby]

I think this needs to be removed; it's causing TF to always want to re-make the rule because we don't create (or store) the UserID when we create the rule. The perm we send does not have this populated, which I think is OK since the documentation say it's for EC2 Classic only anyway.

I commented it out and this branch started to behave as expected.

[jszwedko]

👍 I see that now too, I must have missed it before.

[jszwedko]

@catsby thanks for taking a look! I didn't realize that there were schema migrations, but that would be a perfect fit here.

[catsby]

Hey @jszwedko – I've opened #2376 to supersede this one. I messed up the git history in my rebase though so it looks like you did all the work, thanks! 😄

Please head over there and provide any feedback you may have.

[ghost]

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.