provider/openstack: Security Group Rule fixes

[jtopjian]

This commit fixes an issue with security group rules where the rules
were not being correctly computed due to a typo in the rule map.

Once rules were successfully computed, the rules then needed to be
converted into a Set so they can be correctly ordered.

Fixes #3788
Fixes #3816

[jtopjian]

@Fodoj @jhoblitt Let me know if this fixes your issues.

[jtopjian]

@phinze This patch is doing a similar TypeList to TypeSet conversion as #3651. Does this need a migration or will the data type conversion be done implicitly again?

[phinze]

Is this change exercised in any of the tests?

[jtopjian]

Ah - it's not. Thanks!

[jtopjian]

Well, that escalated quickly!

[phinze]

@jtopjian this looks to be the same scenario so I think we're okay to skip the state migration. Code LGTM with one inline question. 👌

[Fodoj]

There seems to be some issue with (I didn't test with last commit though): terraform tries to delete SG from the instance, deletes it (it's no associated any more) but fails and shows "Faield to remove". But the weird part is that it happens during terraform apply, meaning that it actually should not even try to remove SG from the instance.

[Fodoj]

Ignore last comment, it seems to be no related to this PR.

[Fodoj]

More info:

1. AKAIK security group names are not unique in Openstack => using names for SGs list of instance is wrong then
2. It one passes ID to security_groups attribute of an instance instead of name then weird bugs that the one I described above will happen:

Here it has name in state file, id passed in and then it explodes.

2015/11/10 16:52:04 [DEBUG] terraform-provider-openstack: 2015/11/10 16:52:04 [DEBUG] oldSGSet: &{0x575320 map[47827970:security_group_1] {{0 0} 1}}
2015/11/10 16:52:04 [DEBUG] terraform-provider-openstack: 2015/11/10 16:52:04 [DEBUG] newSGSet: &{0x575320 map[1589016600:84c4b52f-e023-40d6-9edf-7dbd73b91d7c] {{0 0} 1}}

[jtopjian]

Yeah, the places where to specify a security group name and ID are very inconsistent in OpenStack.

The Nova instances expect security group names. Neutron Ports expect IDs. The Nova security group rules expect an ID, but will return a name and not the ID.

Unfortunately there's not a lot that can be done here besides documentation. I tried doing some workarounds to enable security group IDs in the security_groups attribute of instances in #2009, but ultimately wasn't successful :(

[jtopjian]

@Fodoj With the wonkiness of security groups aside, does this patch work for you?

@phinze If you have a minute, could you do a quick code review? Adding the missing test you caught required modification to a few other areas (again, thanks for catching it :)

[Fodoj]

Yes, after using names instead of IDs everything started working as expected

Best regards,
Kirill Shirinkin

On Fri, Nov 13, 2015 at 6:02 AM, Joe Topjian notifications@github.com
wrote:

@Fodoj With the wonkiness of security groups aside, does this patch work for you?

@phinze If you have a minute, could you do a quick code review? Adding the missing test you caught required modification to a few other areas (again, thanks for catching it :)

Reply to this email directly or view it on GitHub:
#3857 (comment)

[phinze]

@jtopjian This is looking good! Merge at will. 👍

[jhoblitt]

@jtopjian Sorry for the slow response... It has been one of "those weeks". I will test and report back later this morning but please don't hold up merging on my account.

[jtopjian]

@phinze @Fodoj Thank you both!

@jhoblitt No problem at all. I'll merge this later today. If you don't get a chance to test before then, please let me know if you run into any issues afterward.

[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.