provider/openstack: Add support for FWaaS routerinsertion extension #12589

fatmcgav · 2017-03-10T16:40:27Z

Add support for the Openstack FWaaS routerinsertion extension, as added to Gophercloud here: gophercloud/gophercloud#220

Fixes #12488

Todo:

Tidy up log lines and comments
Squash commits
Test?
Document

fatmcgav · 2017-03-10T16:40:56Z

@jtopjian I thought it would be easier to create a PR, and can then use the review process :)

jtopjian · 2017-03-10T20:31:41Z

@fatmcgav Sounds good. Let me know when you'd like me to take a first pass :)

Edit: Ah, per #12488, I'll take a look now.

fatmcgav · 2017-03-10T21:58:43Z

Yeh, RouterIDs is returned as part of the response. I did try it in types.go, but it didn’t work… I’ll give it another go though :)

…

On 10 Mar 2017, at 20:42, Joe Topjian ***@***.***> wrote: @jtopjian commented on this pull request. In builtin/providers/openstack/resource_openstack_fw_firewall_v1.go <#12589 (comment)>: > @@ -60,6 +68,18 @@ func resourceFWFirewallV1() *schema.Resource { } } +// Firewall is an OpenStack firewall. +type Firewall struct { + ID string `json:"id"` + Name string `json:"name"` + Description string `json:"description"` + AdminStateUp bool `json:"admin_state_up"` + Status string `json:"status"` + PolicyID string `json:"firewall_policy_id"` + TenantID string `json:"tenant_id"` + RouterIDs []string `json:"router_ids"` Ah, does the result of GET now have router_ids? I wasn't aware of that or else I would have dealt with it in the original gophercloud PR. This should be fine for now, but can you move it to types.go? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12589 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIbIhQLWePbcg0hUY0hYXz1R67IvnrZks5rkbWpgaJpZM4MZmaJ>.

jtopjian · 2017-03-11T05:57:00Z

@fatmcgav I forgot to hit the "start review" button, so I ended up cancelling it. Glad you got that one comment, though.

I was playing around with this and I think I found a cleaner way of going about it. Take a look here: jtopjian@346eb05

I think it works for Create, but haven't finished up the Update yet. There seems to be an issue in my test environment where firewalls are not being created, so I'll have to rebuild it.

Feel free to simply copy the code if you want to work with it further. I should have some time tomorrow to keep looking at this, too.

fatmcgav · 2017-03-11T06:04:58Z

Ah, Yeh, that looks cleaner... That's how I thought it should work, but couldn't find the right combination...

Will give it a test later and confirm...

jtopjian · 2017-03-11T06:11:33Z

Yeah, I tried a number of different combinations, too, until I landed on this one :)

One other thing I think I noticed is that if this extension is enabled, then Neutron will pass a router ID anyway. This is probably where the neutron firewall-create --no-routers option comes in. This probably maps to an empty array being sent in the request. The best way to do this would be to add a new argument to this Terraform resource of no_routers of type bool.

fatmcgav · 2017-03-11T14:21:08Z

@jtopjian Yeh, I spotted the same... If I don't provide a value for associated_routers, then the firewall is automatically associated to all routers...
Passing an empty array on the POST data does result in a firewall with no associated routers.

So yeh, think will need a no_routers argument aswell...

fatmcgav · 2017-03-11T15:25:23Z

Ah, so the next challenge is that 'router_ids' is stripped out by Gophercloud if it's empty :(
https://github.com/jtopjian/gophercloud/blob/82802e3903f673a84b07aa4604b68e4caa3c3ac5/openstack/networking/v2/extensions/fwaas/routerinsertion/requests.go#L43-L45

So guess either need to update Gophercloud, or overload the method in Terraform...
Thoughts?

jtopjian · 2017-03-11T16:12:04Z

Can you confirm that --no-routers is equivalent to sending an empty array? If so, I'll fix that up in Gophercloud and we'll have to resume once it's patched.

fatmcgav · 2017-03-11T16:15:48Z

Yeh, passing --no-routers in Neutron cli passes an empty array.
I'll pull a request example later for reference.

jtopjian · 2017-03-11T16:17:26Z

Sounds good. I'll also add support for the RouterID in the results, too.

fatmcgav · 2017-03-11T16:40:38Z

Cool, cheers.

…

On 11 Mar 2017 16:17, "Joe Topjian" ***@***.***> wrote: Sounds good. I'll also add support for the RouterID in the results, too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIbIvKa82PRdBZgL3R-dzuuB1IvXoZcks5rkskegaJpZM4MZmaJ> .

fatmcgav · 2017-03-11T19:25:11Z

Pending merge of gophercloud/gophercloud#292

fatmcgav · 2017-05-27T14:23:48Z

@jtopjian So this one is ready for another review... :)

Since gophercloud/gophercloud#292 was merged, I've updated to use the latest changes and added some additional acceptance tests, which all pass for me :)

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/27 15:15:33 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/openstack -v -run=TestAccFWFirewallV1_* -timeout 120m
=== RUN   TestAccFWFirewallV1_importBasic
--- PASS: TestAccFWFirewallV1_importBasic (10.11s)
=== RUN   TestAccFWFirewallV1_basic
--- PASS: TestAccFWFirewallV1_basic (14.63s)
=== RUN   TestAccFWFirewallV1_timeout
--- PASS: TestAccFWFirewallV1_timeout (7.81s)
=== RUN   TestAccFWFirewallV1_router
--- PASS: TestAccFWFirewallV1_router (21.87s)
=== RUN   TestAccFWFirewallV1_no_router
--- PASS: TestAccFWFirewallV1_no_router (9.26s)
=== RUN   TestAccFWFirewallV1_router_update
--- PASS: TestAccFWFirewallV1_router_update (38.67s)
=== RUN   TestAccFWFirewallV1_router_remove
--- PASS: TestAccFWFirewallV1_router_remove (28.55s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/openstack      130.927s

Unfortunately all the commits have been rebased, as I had to fix a merge conflict as a result of #12863 - Once happy with the changes, I'll get it all squashed down into a couple of commits...

fatmcgav · 2017-05-27T15:06:08Z

Hmm, so travis is failing the build with:

$ make vet
go vet $(go list ./... | grep -v /terraform/vendor/)
builtin/providers/openstack/resource_openstack_fw_firewall_v1.go:113: github.com/hashicorp/terraform/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/fwaas/routerinsertion.CreateOptsExt composite literal uses unkeyed fields
builtin/providers/openstack/resource_openstack_fw_firewall_v1.go:122: github.com/hashicorp/terraform/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/fwaas/routerinsertion.CreateOptsExt composite literal uses unkeyed fields
builtin/providers/openstack/resource_openstack_fw_firewall_v1.go:234: github.com/hashicorp/terraform/vendor/github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/fwaas/routerinsertion.UpdateOptsExt composite literal uses unkeyed fields
exit status 1

However when I run a make vet locally, I don't get the same failure 😕

@jtopjian / @stack72 Any ideas on how to fix the above error?

Edit: Actually, looking closer at the error, it appears the failure is in gophercloud rather than terraform... I'll take a look there... :)

fatmcgav · 2017-05-27T16:35:08Z

So I've got a 'fix' for the vet failure... Not sure if it's the 'correct' solution though... :)

jtopjian

@fatmcgav Really nice work here!

I've left a few comments, all very minor.

In addition, I had no idea the Firewall tests were the way they currently are. They don't match the other test setups at all. If they were similar to the others, you would have probably been able to write easier tests. Let's leave everything as-is and I'll try to make some time to clean them up later.

Let me know if you have any questions.

jtopjian · 2017-05-28T22:33:22Z

builtin/providers/openstack/provider_test.go

-	if OS_IMAGE_ID == "" || OS_IMAGE_NAME == "" {
-		t.Fatal("OS_IMAGE_ID and OS_IMAGE_NAME must be set for acceptance tests")
+	if OS_IMAGE_ID == "" && OS_IMAGE_NAME == "" {
+		t.Fatal("OS_IMAGE_ID or OS_IMAGE_NAME must be set for acceptance tests")


While this is probably true, this is out of scope for this PR since it's not related to the router insertion feature.

Agreed, I'll get it raised in a different PR...

jtopjian · 2017-05-28T22:35:31Z

builtin/providers/openstack/resource_openstack_fw_firewall_v1.go

-		},
+	associatedRoutersRaw := d.Get("associated_routers").(*schema.Set).List()
+	if len(associatedRoutersRaw) > 0 {
+		log.Printf("[DEBUG] Need to associate Firewall with router(s): %+v", associatedRoutersRaw)


How about: "Will attempt to associate firewall with router(s):..." ?

jtopjian · 2017-05-28T22:36:41Z

builtin/providers/openstack/resource_openstack_fw_firewall_v1.go

@@ -106,6 +146,7 @@ func resourceFWFirewallV1Create(d *schema.ResourceData, meta interface{}) error
 	}

 	_, err = stateConf.WaitForState()
+	log.Println("[DEBUG] Firewall is active.")


How about: "Firewall (ID) is active." ?

jtopjian · 2017-05-28T22:37:46Z

builtin/providers/openstack/resource_openstack_fw_firewall_v1.go

+	result := firewalls.Get(networkingClient, d.Id())
+
+	var firewall Firewall
+	err = result.ExtractInto(&firewall)


Just a heads up that this might change in the future. What you have is the current working solution -- just something to be mindful of down the road.

Ah, ok... Well atleast the tests should show us any breakage...

I guess I could collapse this into a single line, tagging the ExtractInto onto the end of the firewalls.Get... Thoughts?

jtopjian · 2017-05-28T22:39:24Z

builtin/providers/openstack/resource_openstack_fw_firewall_v1.go

@@ -57,6 +58,18 @@ func resourceFWFirewallV1() *schema.Resource {
 				ForceNew: true,
 				Computed: true,
 			},
+			"associated_routers": &schema.Schema{
+				Type:          schema.TypeSet,


Does the API return the associated routers as an array in the same order as what the user gave? If so, this can be a TypeList. But if the API takes it upon itself to re-order (for example, alphabetical), then TypeSet is the way to go.

I'm not sure tbh... I'll give it a test with TypeList and see what it does...

So it looks like the API doesn't guarantee the ordering, as I switched to using TypeList and got a failure:

--- FAIL: TestAccFWFirewallV1_router_update (33.48s) testing.go:280: Step 1 error: After applying this step, the plan was not empty: DIFF: UPDATE: openstack_fw_firewall_v1.fw_1 associated_routers.0: "5eda6df1-0271-4468-a2eb-3e1e1a24596e" => "9ee2774f-a1af-494c-955e-bf37109c6bae" associated_routers.1: "9ee2774f-a1af-494c-955e-bf37109c6bae" => "5eda6df1-0271-4468-a2eb-3e1e1a24596e"

So looks like got to stick to using TypeSet.

jtopjian · 2017-05-28T22:44:46Z

builtin/providers/openstack/types.go

 // FirewallCreateOpts represents the attributes used when creating a new firewall.
 type FirewallCreateOpts struct {
-	firewalls.CreateOpts
+	firewalls.CreateOptsBuilder


👍

I recently ran into this with a different type in another PR.

In addition to this, I wonder if this field should be explicitly labeled CreateOptsBuilder so this type of format can work? I haven't tried to modify the custom types here to support the field name. Just a thought.

Yeh, the go vet failure had me confused for a bit... So used that format here: https://github.com/hashicorp/terraform/pull/12589/files#diff-08dbfa227aa19c57ae3521114151c1c8R111

fatmcgav · 2017-05-29T08:00:39Z

builtin/providers/openstack/types.go

+		return nil, err
+	}
+
+	if base["value_specs"] != nil {


Just spotted another optimisation I can make, by extracting this out into its own function in util.go so that's it more reusable...

fatmcgav · 2017-05-29T11:27:29Z

builtin/providers/openstack/resource_openstack_fw_firewall_v1.go

@@ -57,6 +58,18 @@ func resourceFWFirewallV1() *schema.Resource {
 				ForceNew: true,
 				Computed: true,
 			},
+			"associated_routers": &schema.Schema{
+				Type:          schema.TypeSet,


So it looks like the API doesn't guarantee the ordering, as I switched to using TypeList and got a failure:

--- FAIL: TestAccFWFirewallV1_router_update (33.48s) testing.go:280: Step 1 error: After applying this step, the plan was not empty: DIFF: UPDATE: openstack_fw_firewall_v1.fw_1 associated_routers.0: "5eda6df1-0271-4468-a2eb-3e1e1a24596e" => "9ee2774f-a1af-494c-955e-bf37109c6bae" associated_routers.1: "9ee2774f-a1af-494c-955e-bf37109c6bae" => "5eda6df1-0271-4468-a2eb-3e1e1a24596e"

So looks like got to stick to using TypeSet.

fatmcgav · 2017-05-29T15:05:13Z

@jtopjian Is a1b5ce8 closer to what you were thinking as current?
I based the changes largely on the openstack_dns_zone_v2 tests...

fatmcgav · 2017-05-29T20:36:29Z

From a commit squash POV, I'm thinking the following:

vendor: Add gophercloud/routerinsertion package.
provider/openstack: Add support for associating firewall instance with router(s).
website: Add documentation for associated_routersand no_routers arguments on openstack_fw_firewall_v1 resource.
provider/openstack: Add AddValueSpecs function and refactor existing
uses.

jtopjian · 2017-05-30T03:15:01Z

Is a1b5ce8 closer to what you were thinking as current?

Yep, that looks good to me :)

From a commit squash POV, I'm thinking the following:

Sure, that sounds good. Really, as long as the vendor commit is separate, anything else is fine.

I'll done one final sweep and test with a squashed set.

gophercloud/firewall to support router insertion

`openstack_fw_firewall_v1` resources with router(s). Added `associated_routers` and `no_routers` arguments.

…rguments on `openstack_fw_firewall_v1` resource.

uses.

fatmcgav · 2017-05-30T08:06:32Z

@jtopjian Right, all squashed down into a healthier number of commits... :)

jtopjian · 2017-06-01T05:19:46Z

This looks good to me! Really nice work on this, @fatmcgav -- thank you :)

==> Checking that code complies with gofmt requirements...                               
go generate $(go list ./... | grep -v /terraform/vendor/)                                
2017/05/31 23:16:22 Generated command/internal_plugin_list.go                            
TF_ACC=1 go test ./builtin/providers/openstack -v -run=TestAccFWFirewallV1 -timeout 120m 
=== RUN   TestAccFWFirewallV1_importBasic                                                
--- PASS: TestAccFWFirewallV1_importBasic (8.84s)                                        
=== RUN   TestAccFWFirewallV1_basic                                                      
--- PASS: TestAccFWFirewallV1_basic (17.23s)                                             
=== RUN   TestAccFWFirewallV1_timeout                                                    
--- PASS: TestAccFWFirewallV1_timeout (8.19s)                                            
=== RUN   TestAccFWFirewallV1_router                                                     
--- PASS: TestAccFWFirewallV1_router (24.65s)                                            
=== RUN   TestAccFWFirewallV1_no_router                                                  
--- PASS: TestAccFWFirewallV1_no_router (9.46s)                                          
=== RUN   TestAccFWFirewallV1_router_update                                              
--- PASS: TestAccFWFirewallV1_router_update (37.96s)                                     
=== RUN   TestAccFWFirewallV1_router_remove                                              
--- PASS: TestAccFWFirewallV1_router_remove (29.20s)                                     
PASS                                                                                     
ok      github.com/hashicorp/terraform/builtin/providers/openstack      135.567s

ghost · 2020-04-11T02:35:49Z

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.

jtopjian added enhancement provider/openstack labels Mar 10, 2017

fatmcgav force-pushed the openstack_support_fwaas_routerinsertion branch from cec412c to b994923 Compare May 27, 2017 14:19

jtopjian suggested changes May 28, 2017

View reviewed changes

fatmcgav commented May 29, 2017

View reviewed changes

jtopjian approved these changes May 30, 2017

View reviewed changes

fatmcgav added 4 commits May 30, 2017 08:51

vendor: Add gophercloud/routerinsertion package and update

e3d7755

gophercloud/firewall to support router insertion

provider/openstack: Add support for associating

cc2cf6a

`openstack_fw_firewall_v1` resources with router(s). Added `associated_routers` and `no_routers` arguments.

website: Add documentation for associated_routersand no_routers a…

9011f51

…rguments on `openstack_fw_firewall_v1` resource.

provider/openstack: Add AddValueSpecs function and refactor existing

5f5c5ec

uses.

fatmcgav force-pushed the openstack_support_fwaas_routerinsertion branch from afc163f to 5f5c5ec Compare May 30, 2017 08:01

fatmcgav changed the title ~~[WIP] provider/openstack: Add support for FWaaS routerinsertion extension~~ provider/openstack: Add support for FWaaS routerinsertion extension May 30, 2017

jtopjian merged commit 1da7fda into hashicorp:master Jun 1, 2017

ghost locked and limited conversation to collaborators Apr 11, 2020

provider/openstack: Add support for FWaaS routerinsertion extension #12589

provider/openstack: Add support for FWaaS routerinsertion extension #12589

Conversation

fatmcgav commented Mar 10, 2017 • edited Loading

fatmcgav commented Mar 10, 2017

jtopjian commented Mar 10, 2017 • edited Loading

fatmcgav commented Mar 10, 2017 via email

jtopjian commented Mar 11, 2017 • edited Loading

fatmcgav commented Mar 11, 2017

jtopjian commented Mar 11, 2017

fatmcgav commented Mar 11, 2017 • edited Loading

fatmcgav commented Mar 11, 2017

jtopjian commented Mar 11, 2017

fatmcgav commented Mar 11, 2017

jtopjian commented Mar 11, 2017

fatmcgav commented Mar 11, 2017 via email

fatmcgav commented Mar 11, 2017

fatmcgav commented May 27, 2017

fatmcgav commented May 27, 2017 • edited Loading

fatmcgav commented May 27, 2017 • edited Loading

jtopjian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatmcgav commented May 29, 2017

fatmcgav commented May 29, 2017

jtopjian commented May 30, 2017

fatmcgav commented May 30, 2017

jtopjian commented Jun 1, 2017

ghost commented Apr 11, 2020

fatmcgav commented Mar 10, 2017 •

edited

Loading

jtopjian commented Mar 10, 2017 •

edited

Loading

jtopjian commented Mar 11, 2017 •

edited

Loading

fatmcgav commented Mar 11, 2017 •

edited

Loading

fatmcgav commented May 27, 2017 •

edited

Loading

fatmcgav commented May 27, 2017 •

edited

Loading