Networking v2: Port Fixes #510

jtopjian · 2017-08-31T05:55:14Z

Port SecurityGroups and AllowedAddressPairs have a tertiary type of behavior.

During creation, if security_groups is omitted from the request body, then Neutron will apply the default security group. If an empty array is passed, no security groups will be applied to the port. If an array of security groups is passed, those security groups will be applied to the port.

For update, if security_groups is omitted from the request body, existing security groups on the port are unchanged. If an empty array is passed, all security groups are removed. If an array of security groups is passed, then the port will have those groups applied.

allowed_address_pairs has the same behavior, but only for updates.

For #509

Amends #236

#236 has the appropriate links to the API code, but the client code is easier to understand for this one:

https://github.com/openstack/python-neutronclient/blob/bc56657633cb7b3938e8a3fedb70dd050e68805c/neutronclient/neutron/v2_0/port.py#L142-L147

https://github.com/openstack/python-neutronclient/blob/bc56657633cb7b3938e8a3fedb70dd050e68805c/neutronclient/neutron/v2_0/port.py#L215-L219

@jrperritt I can split this into 2 PRs, but 99% of this PR is tests. The actual change is quite small (though significant).

This commit fixes the way security groups are handled during port creation and updating. If no SecurityGroups are specified during Create, then `security_groups` is not passed in the request body. The default behavior for this is to have the default security group applied to the port. If a pointer to an empty string slice is passed, then the port is created with no security groups applied. If no SecurityGroups are specified during Update, then no modification is done to the port's security groups. If a pointer to an empty string slice is passed, then all security groups are removed.

This commit fixes the way Allowed Address Pairs are handled during port updating. If no AllowedAddressPairs are specified during update, then `allowed_address_pairs` is not passed in the request body. Existing address pairs on the port will remain untouched. If a pointer to an empty string slice is passed, then the address pairs are removed from the port.

coveralls · 2017-08-31T06:17:23Z

Coverage remained the same at 71.555% when pulling 20607e0 on jtopjian:networkingv2-port-fixes into 31ccc1a on gophercloud:master.

jtopjian · 2017-08-31T06:18:13Z

@jrperritt Ping for review. The unit test clutter makes this one a little awkward to review. The cleanup I did in #445 would have helped, but I'd like to make this one a priority since this is more of a bug.

jrperritt · 2017-08-31T13:55:28Z

openstack/networking/v2/ports/requests.go

+	FixedIPs            interface{}    `json:"fixed_ips,omitempty"`
+	DeviceID            string         `json:"device_id,omitempty"`
+	DeviceOwner         string         `json:"device_owner,omitempty"`
+	SecurityGroups      *[]string      `json:"security_groups,omitempty"`


👍 exactly the way to fix this

jrperritt · 2017-08-31T13:55:46Z

openstack/networking/v2/ports/requests.go

@@ -81,7 +81,7 @@ type CreateOpts struct {
 	DeviceID            string        `json:"device_id,omitempty"`
 	DeviceOwner         string        `json:"device_owner,omitempty"`
 	TenantID            string        `json:"tenant_id,omitempty"`
-	SecurityGroups      []string      `json:"security_groups,omitempty"`
+	SecurityGroups      *[]string     `json:"security_groups,omitempty"`


Why this one too?

The previous PR only modified the updating of security groups, but while researching this I found that security groups are handled the same way during create. I included it in here hoping to get all port SG/AAP quirks done and over with. If this is too much of a change for a single PR, I can submit this one in a separate one.

The PR description and commit message lay out the details. As for the code to prove this, the same functions are called both on the client and server side to trigger this behavior. You can see that port creation supports this behavior on the client-side here.

I've included unit tests to mimic the create behavior. The acceptance tests were updated as well and my test environment behaves accordingly, though I only added a "warning" instead of a failure because I have heard of some clouds that modify this behavior (to account for the odd case that someone runs the acceptance test in one of these clouds).

One more note: what might make this ultra-confusing is that the client CreatePort inherits from the UpdatePortSecGroupMixin

jrperritt · 2017-08-31T14:48:42Z

@jtopjian As a side note, if the commits for a PR are organized (like all yours seem to be), I have recently been (and plan to continue) just merging (instead of squashing and creating a new commit), with the assumption that should help with the hierarchy of "pending" PRs

jtopjian · 2017-08-31T17:14:20Z

I like to tell a story with my commits :P

Thanks for the quick review and merge!

Fix for cloud routes enabled instances will have their security groups removed when the allowed address pair is added to the instance's port. Upstream bug report is in: gophercloud/gophercloud#509 Upstream bug fix is in: gophercloud/gophercloud#510

Automatic merge from submit-queue (batch tested with PRs 51984, 51351, 51873, 51795, 51634) Bug Fix - Adding an allowed address pair wipes port security groups **What this PR does / why we need it**: Fix for cloud routes enabled instances will have their security groups removed when the allowed address pair is added to the instance's port. Upstream bug report is in: gophercloud/gophercloud#509 Upstream bug fix is in: gophercloud/gophercloud#510 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # Fixes #51755 **Special notes for your reviewer**: Just an fix in vendored code. minimal changes needed in OpenStack cloud provider **Release note**: ```release-note NONE ```

Automatic merge from submit-queue (batch tested with PRs 51984, 51351, 51873, 51795, 51634) Bug Fix - Adding an allowed address pair wipes port security groups **What this PR does / why we need it**: Fix for cloud routes enabled instances will have their security groups removed when the allowed address pair is added to the instance's port. Upstream bug report is in: gophercloud/gophercloud#509 Upstream bug fix is in: gophercloud/gophercloud#510 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # Fixes #51755 **Special notes for your reviewer**: Just an fix in vendored code. minimal changes needed in OpenStack cloud provider **Release note**: ```release-note NONE ``` Kubernetes-commit: 9a8cb435b77085fa7d518c4428a02eae316b1003

jtopjian added 4 commits August 31, 2017 05:24

Networking v2: Fix Port and Security Groups acceptance tests

3efdd8f

Networking v2: Fix Port and Allowed Address Pairs acceptance test

45bd7d5

jtopjian requested a review from jrperritt August 31, 2017 05:55

This was referenced Aug 31, 2017

Network v2: Security groups are wiped out on port update if not supplied #509

Closed

Network v2: portsbinding refactor #445

Merged

Networking v2: Fix portsbinding unit tests

20607e0

jrperritt reviewed Aug 31, 2017

View reviewed changes

jrperritt merged commit 2bf16b9 into gophercloud:master Aug 31, 2017

dims mentioned this pull request Sep 1, 2017

Bug Fix - Adding an allowed address pair wipes port security groups kubernetes/kubernetes#51795

Merged

jtopjian deleted the networkingv2-port-fixes branch April 24, 2018 03:17

jtopjian mentioned this pull request Jan 8, 2019

Networking: port resource doesn't allow to unset security groups terraform-provider-openstack/terraform-provider-openstack#575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Networking v2: Port Fixes #510

Networking v2: Port Fixes #510

Uh oh!

jtopjian commented Aug 31, 2017 •

edited

Loading

Uh oh!

coveralls commented Aug 31, 2017 •

edited

Loading

Uh oh!

jtopjian commented Aug 31, 2017

Uh oh!

jrperritt Aug 31, 2017

Uh oh!

jrperritt Aug 31, 2017

Uh oh!

jtopjian Aug 31, 2017

Uh oh!

jtopjian Aug 31, 2017

Uh oh!

jrperritt commented Aug 31, 2017

Uh oh!

jtopjian commented Aug 31, 2017

Uh oh!

Uh oh!

Networking v2: Port Fixes #510

Networking v2: Port Fixes #510

Uh oh!

Conversation

jtopjian commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Aug 31, 2017

Uh oh!

jrperritt Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

jrperritt Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

jtopjian Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

jtopjian Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

jrperritt commented Aug 31, 2017

Uh oh!

jtopjian commented Aug 31, 2017

Uh oh!

Uh oh!

jtopjian commented Aug 31, 2017 •

edited

Loading

coveralls commented Aug 31, 2017 •

edited

Loading