Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/openstack: Change rules type to List #7194

Merged
merged 1 commit into from
Jun 24, 2016

Conversation

scrossan
Copy link

Allows for ordering of rules that are applied to the firewall policy,
including addition and removal of rules in certain positions.

Fixes #7172.

Allows for ordering of rules that are applied to the firewall policy,
including addition and removal of rules in certain positions.
@jen20
Copy link
Contributor

jen20 commented Jun 17, 2016

Hi @scrossan! This change looks good to me in principle - does it give a reasonable looking diff for those migrating from the previous format?

@scrossan
Copy link
Author

Hi @jen20 - I believe it does. If we expand the example in #7172 to include 11 firewall rules and apply that with the current release, Terraform plans the following when it is rebuilt with this change:

$ ~/Developer/go/src/github.com/hashicorp/terraform/pkg/darwin_amd64/terraform plan
...
~ openstack_fw_policy_v1.policy
    rules.0:  "" => "5c7c8680-15be-4884-958c-9ffcb80dc2e3"
    rules.1:  "" => "630826d8-522d-4751-af9a-ab6c0bad6abc"
    rules.10: "" => "62e68220-1c92-411b-9671-c41d03b588f3"
    rules.2:  "" => "e1c55715-15d0-491a-a485-ebe17efca03a"
    rules.3:  "" => "50a41c95-0d7a-42ec-81d0-face533226cb"
    rules.4:  "" => "ec9b1702-9961-47fb-b6cf-12a271c76782"
    rules.5:  "" => "be45af29-e57b-4a7b-8f07-d2471de5888b"
    rules.6:  "" => "7da6e008-4505-47f2-93fc-dd1fd569c6de"
    rules.7:  "" => "c7928376-e54b-401f-b647-cdd04de77a4f"
    rules.8:  "" => "c58efbfb-f8e4-4468-866a-458a2f2e7057"
    rules.9:  "" => "2c195512-cd82-4112-81cb-924d98eff4cb"


Plan: 0 to add, 1 to change, 0 to destroy.

It lists the rules alphabetically rather than numerically, but now wants to order the rules as they are specified in the array of rules in the firewall policy resource. When I check the firewall policy in the OpenStack GUI I can see that the rules have actually been applied in the same order.

For our own configuration, with 144 rules that have seemingly so far have been applied in no particular order, Terraform wants to do the same as it does above: order the rules as they are ordered in the array of rules. We will need to check that applying Terraform with this change will not lead to any interruption of service by way of re-ordering the existing firewall policy, but I believe that Terraform is now behaving as we originally expected it to do so.

@jtopjian
Copy link
Contributor

We will need to check that applying Terraform with this change will not lead to any interruption of service by way of re-ordering the existing firewall policy

This is probably unavoidable, but I think as long as it's detailed in the CHANGELOG, it should be sufficient.

To confirm: the old method of ordering was alphabetical by ID and now the ordering is position in the list?

but I believe that Terraform is now behaving as we originally expected it to do so.

Indeed. I dug through the Neutron code last night and confirmed that firewall rules are applied as an array/list in order of element (Firewall rules are passed into the policy on the command-line by a space-separated string. The string is then split by whitespace and applied).

Are you able to run the fwaas acceptance tests and see if everything still passes? If so, I'm good with having this merged. If you aren't able to run them, I will be able to in the next day or so.

@scrossan
Copy link
Author

To confirm: the old method of ordering was alphabetical by ID and now the ordering is position in the list?

In the diff ordering is still alphabetical (as above), but as for how the rules are actually applied - with this change - it's now based on position in the list, whereas before it does look like it was alphabetical by ID although I'm not certain about that.

Regarding the acceptance tests, I ran the following command which completed successfully:

make testacc TEST=./builtin/providers/openstack TESTARGS='-run=TestAccFW'

Just wanted to double check that I ran the correct tests before claiming that they were passing successfully!

@jtopjian
Copy link
Contributor

@scrossan Yup - that's the right command.

@jen20 I'm good with merging this if you feel the schema change has been done correctly.

@jtopjian
Copy link
Contributor

@scrossan Thank you for your work on this! I just reviewed and tested everything and I think it's good to go!

@jtopjian jtopjian merged commit 6974345 into hashicorp:master Jun 24, 2016
terraformbot pushed a commit that referenced this pull request Jun 24, 2016
[origin/master] Merge pull request #7194 from scrossan/master
6974345
@ghost
Copy link

ghost commented Apr 24, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform does not respect ordering of firewall rules in an Openstack firewall's policy
3 participants