[wip] FWaaS v2.0 Rule Create by Elethiomel · Pull Request #1768 · gophercloud/gophercloud

Elethiomel · 2019-11-14T16:50:50Z

https://github.com/openstack/neutron-fwaas/blob/27906d0acff349c9e5ac7955d203fb2233c295db/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L68-L82

coveralls · 2019-11-14T17:35:17Z

Coverage increased (+0.2%) to 76.97% when pulling f2ca260 on Elethiomel:fwaasv2_rule_create into b311dd7 on gophercloud:master.

theopenlab-ci · 2019-11-14T19:18:55Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 48s
gophercloud-acceptance-test : SUCCESS in 1h 53m 27s
gophercloud-acceptance-test-ironic : SUCCESS in 40m 17s

jtopjian

@Elethiomel Thanks for continuing to look into this. Overall this looks good. I've left a few comments. In addition, I'd still like to see some acceptance tests added. It should be pretty easy to mirror the current fwaas tests and modify for fwaasv2 - I'd be happy to help if needed.

Please let me know if you have any questions.

jtopjian · 2019-11-15T02:51:40Z

openstack/networking/v2/extensions/fwaas_v2/rules/requests.go

+// CreateOpts contains all the values needed to create a new firewall rule.
+type CreateOpts struct {
+	Protocol             Protocol              `json:"protocol" required:"true"`
+	Action               string                `json:"action" required:"true"`


I think we can create a specific type for action: https://github.com/openstack/neutron-fwaas/blob/27906d0acff349c9e5ac7955d203fb2233c295db/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L80

Great idea :) I've added this in commit c677015

jtopjian · 2019-11-15T03:01:03Z

openstack/networking/v2/extensions/fwaas_v2/rules/results.go

+	DestinationPort      string `json:"destination_port,omitempty"`
+	Shared               bool   `json:"shared,omitempty"`
+	Enabled              bool   `json:"enabled,omitempty"`
+	PolicyID             string `json:"firewall_policy_id"`


FirewallPolicyID

I've made this change in commit 19de2bd

jtopjian · 2019-11-15T03:09:52Z

openstack/networking/v2/extensions/fwaas_v2/rules/results.go

+	Shared               bool   `json:"shared,omitempty"`
+	Enabled              bool   `json:"enabled,omitempty"`
+	PolicyID             string `json:"firewall_policy_id"`
+	Position             int    `json:"position"`


I'm unable to tell how this field is added to the result here: https://github.com/openstack/neutron-fwaas/blob/27906d0acff349c9e5ac7955d203fb2233c295db/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L269

Can you verify this field is really part of the JSON result?

Well spotted! I've verified that it's not. Removed in commit f1ad97e

The JSON result is also returning project_id, so I've added this in commit be00d44

…phercloud/pull/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346643142

…loud/gophercloud/pull/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346644820

…ll/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346646305

theopenlab-ci · 2019-11-15T14:21:31Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 59s
gophercloud-acceptance-test : SUCCESS in 1h 53m 33s
gophercloud-acceptance-test-ironic : SUCCESS in 23m 14s

Elethiomel · 2019-11-15T14:45:37Z

@jtopjian commit ee2fb65 introduces an acceptance test for creating a rule, but the test is currently failing with a rather odd error... Any suggestion appreciated :)

$ go test -tags fwaas_v2 . --- FAIL: TestRuleC (0.60s) fwaas_v2.go:24: Attempting to create rule TESTACC-6t7dLduz with source 192.168.1.73:73 and destination 192.168.2.73:73 fwaas_v2.go:42: Rule TESTACC-6t7dLduz successfully created convenience.go:35: Failure in fwaas_v2.go, line 45: expected "tcp" but got "tcp" FAIL FAIL github.com/gophercloud/gophercloud/acceptance/openstack/networking/v2/extensions/fwaas_v2 0.601s

theopenlab-ci · 2019-11-15T17:29:07Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 02s
gophercloud-acceptance-test : SUCCESS in 2h 45m 16s
gophercloud-acceptance-test-ironic : SUCCESS in 28m 40s

jtopjian · 2019-11-16T16:41:32Z

Nice work :)

The error is because you're comparing a string "tcp" with the Protocol type "tcp". There are two ways to resolve this:

th.AssertEquals(t, rule.Protocol, "tcp")
th.AssertEquals(t, rule.Action, "allow")

or

th.AssertEquals(t, rule.Protocol, string(rules.ProtocolTCP))
th.AssertEquals(t, rule.Action, string(rules.ActionAllow))

…s failures

Elethiomel · 2019-11-18T11:06:30Z

@jtopjian Whoops! Thanks for the help :) Fixed in f2ca260

theopenlab-ci · 2019-11-18T12:31:47Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 15s
gophercloud-acceptance-test : SUCCESS in 1h 39m 39s
gophercloud-acceptance-test-ironic : SUCCESS in 30m 24s

Elethiomel · 2019-11-19T11:10:55Z

@jtopjian What's the etiquette here in gophercloud dev land? Should I close off the changes requested or should I leave it to you?

jtopjian · 2019-11-19T15:54:25Z

What's the etiquette here in gophercloud dev land? Should I close off the changes requested or should I leave it to you?

That's all on me. Sorry for the delay.

jtopjian

LGTM - thank you!

FWaaSv2 Rule Create

2f26185

Elethiomel mentioned this pull request Nov 14, 2019

Feature request: Add support for FWaaS v2.0 #514

Closed

minor formatting changes in imports to pass ./script/format

ad42c62

jtopjian requested changes Nov 15, 2019

View reviewed changes

Elethiomel added 4 commits November 15, 2019 11:01

Add specific type for Action as per https://github.com/gophercloud/go…

c677015

…phercloud/pull/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346643142

rename PolicyID to FirewallPolicyID as per https://github.com/gopherc…

19de2bd

…loud/gophercloud/pull/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346644820

removed Position as per https://github.com/gophercloud/gophercloud/pu…

f1ad97e

…ll/1768/files/ad42c6277e24d42c4dacb5cb0ccf10181d2c8306#r346646305

Add ProjectID to Rule

be00d44

Initial acceptance test for creating rule

ee2fb65

convert Protocol to string in acceptance tests to prevent AssertEqual…

f2ca260

…s failures

jtopjian approved these changes Nov 19, 2019

View reviewed changes

jtopjian merged commit d8a57ca into gophercloud:master Nov 19, 2019

Elethiomel deleted the fwaasv2_rule_create branch December 2, 2019 17:06

Conversation

Elethiomel commented Nov 14, 2019

Uh oh!

coveralls commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Nov 14, 2019

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

jtopjian Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Elethiomel Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

jtopjian Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Elethiomel Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

jtopjian Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Elethiomel Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Elethiomel Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Nov 15, 2019

Uh oh!

Elethiomel commented Nov 15, 2019

Uh oh!

theopenlab-ci bot commented Nov 15, 2019

Uh oh!

jtopjian commented Nov 16, 2019

Uh oh!

Elethiomel commented Nov 18, 2019

Uh oh!

theopenlab-ci bot commented Nov 18, 2019

Uh oh!

Elethiomel commented Nov 19, 2019

Uh oh!

jtopjian commented Nov 19, 2019

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Nov 14, 2019 •

edited

Loading