From 67c9b09efb7956620aed83560b0b6b3cb2569c3f Mon Sep 17 00:00:00 2001 From: Roey Chen Date: Sun, 31 Jan 2016 07:47:57 -0800 Subject: [PATCH] Better error message when reaching security-group maximum capacity Change-Id: Iebb230f33b75a81e8d58796e4911b9b8ce92f8d0 Closes-Bug: #1540101 --- vmware_nsx/common/exceptions.py | 5 ++ vmware_nsx/nsxlib/v3/dfw_api.py | 14 ++-- vmware_nsx/nsxlib/v3/security.py | 13 +++- vmware_nsx/plugins/nsx_v3/plugin.py | 26 ++++--- .../unit/extensions/test_securitygroup.py | 68 ++++++++++++++++--- 5 files changed, 100 insertions(+), 26 deletions(-) diff --git a/vmware_nsx/common/exceptions.py b/vmware_nsx/common/exceptions.py index 9bb4e9037b..7a42d25dd5 100644 --- a/vmware_nsx/common/exceptions.py +++ b/vmware_nsx/common/exceptions.py @@ -161,3 +161,8 @@ class NsxL2GWInUse(n_exc.InUse): class InvalidIPAddress(n_exc.InvalidInput): message = _("'%(ip_address)s' must be a /32 CIDR based IPv4 address") + + +class SecurityGroupMaximumCapacityReached(NsxPluginException): + message = _("Security Group %(sg_id)s has reached its maximum capacity, " + "no more ports can be associated with this security-group.") diff --git a/vmware_nsx/nsxlib/v3/dfw_api.py b/vmware_nsx/nsxlib/v3/dfw_api.py index 13de6f8b5a..b704c36390 100644 --- a/vmware_nsx/nsxlib/v3/dfw_api.py +++ b/vmware_nsx/nsxlib/v3/dfw_api.py @@ -71,11 +71,13 @@ IPV4_IPV6 = 'IPV4_IPV6' class NSGroupMemberNotFound(nsx_exc.NsxPluginException): - pass + message = _("Could not find NSGroup %(nsgroup_id)s member %(member_id)s " + "for removal.") class NSGroupIsFull(nsx_exc.NsxPluginException): - pass + message = _("NSGroup %(nsgroup_id)s contains has reached its maximum " + "capacity, unable to add additional members.") def get_nsservice(resource_type, **properties): @@ -132,7 +134,7 @@ def add_nsgroup_member(nsgroup_id, target_type, target_id): {'target_type': target_type, 'target_id': target_id, 'nsgroup_id': nsgroup_id}) - raise NSGroupIsFull() + raise NSGroupIsFull(nsgroup_id=nsgroup_id) def remove_nsgroup_member(nsgroup_id, target_type, target_id, verify=False): @@ -143,10 +145,8 @@ def remove_nsgroup_member(nsgroup_id, target_type, target_id, verify=False): nsgroup_id, members, REMOVE_MEMBERS) except nsx_exc.ManagerError: if verify: - err_msg = _("Failed to remove member %(tid)s " - "from NSGroup %(nid)s.") % {'tid': target_id, - 'nid': nsgroup_id} - raise NSGroupMemberNotFound(err_msg=err_msg) + raise NSGroupMemberNotFound(member_id=target_id, + nsgroup_id=nsgroup_id) def read_nsgroup(nsgroup_id): diff --git a/vmware_nsx/nsxlib/v3/security.py b/vmware_nsx/nsxlib/v3/security.py index e0b26afa35..53db09c11b 100644 --- a/vmware_nsx/nsxlib/v3/security.py +++ b/vmware_nsx/nsxlib/v3/security.py @@ -196,8 +196,17 @@ def update_lport_with_security_groups(context, lport_id, original, updated): removed = set(original) - set(updated) for sg_id in added: nsgroup_id, s = get_sg_mappings(context.session, sg_id) - firewall.add_nsgroup_member( - nsgroup_id, firewall.LOGICAL_PORT, lport_id) + try: + firewall.add_nsgroup_member( + nsgroup_id, firewall.LOGICAL_PORT, lport_id) + except firewall.NSGroupIsFull: + for sg_id in added: + nsgroup_id, s = get_sg_mappings(context.session, sg_id) + # NOTE(roeyc): If the port was not added to the nsgroup yet, + # then this request will silently fail. + firewall.remove_nsgroup_member( + nsgroup_id, firewall.LOGICAL_PORT, lport_id) + raise nsx_exc.SecurityGroupMaximumCapacityReached(sg_id=sg_id) for sg_id in removed: nsgroup_id, s = get_sg_mappings(context.session, sg_id) firewall.remove_nsgroup_member( diff --git a/vmware_nsx/plugins/nsx_v3/plugin.py b/vmware_nsx/plugins/nsx_v3/plugin.py index f1f86088d0..fe2a264386 100644 --- a/vmware_nsx/plugins/nsx_v3/plugin.py +++ b/vmware_nsx/plugins/nsx_v3/plugin.py @@ -739,9 +739,16 @@ class NsxV3Plugin(addr_pair_db.AllowedAddressPairsMixin, self._process_port_create_security_group( context, port_data, sgids) if sgids: - security.update_lport_with_security_groups( - context, lport['id'], [], sgids) - + try: + security.update_lport_with_security_groups( + context, lport['id'], [], sgids) + except nsx_exc.SecurityGroupMaximumCapacityReached: + with excutils.save_and_reraise_exception(): + LOG.debug("Couldn't associate port %s with " + "one or more security-groups, reverting " + "reverting logical-port creation (%s).", + port_data['id'], lport['id']) + self._port_client.delete(lport['id']) nsx_rpc.handle_port_metadata_access(self, context, neutron_db) return port_data @@ -893,6 +900,11 @@ class NsxV3Plugin(addr_pair_db.AllowedAddressPairsMixin, else: name = updated_port.get('name') + security.update_lport_with_security_groups( + context, lport_id, + original_port.get(ext_sg.SECURITYGROUPS, []), + updated_port.get(ext_sg.SECURITYGROUPS, [])) + self._port_client.update( lport_id, vif_uuid, name=name, attachment_type=attachment_type, @@ -903,11 +915,6 @@ class NsxV3Plugin(addr_pair_db.AllowedAddressPairsMixin, parent_name=parent_name, parent_tag=tag) - security.update_lport_with_security_groups( - context, lport_id, - original_port.get(ext_sg.SECURITYGROUPS, []), - updated_port.get(ext_sg.SECURITYGROUPS, [])) - def update_port(self, context, id, port): original_port = super(NsxV3Plugin, self).get_port(context, id) _, nsx_lport_id = nsx_db.get_nsx_switch_and_port_id( @@ -946,7 +953,8 @@ class NsxV3Plugin(addr_pair_db.AllowedAddressPairsMixin, original_port, updated_port, address_bindings, switch_profile_ids) - except nsx_exc.ManagerError: + except (nsx_exc.ManagerError, + nsx_exc.SecurityGroupMaximumCapacityReached): # In case if there is a failure on NSX-v3 backend, rollback the # previous update operation on neutron side. LOG.exception(_LE("Unable to update NSX backend, rolling back " diff --git a/vmware_nsx/tests/unit/extensions/test_securitygroup.py b/vmware_nsx/tests/unit/extensions/test_securitygroup.py index 644d43036c..cc57f4ebcc 100644 --- a/vmware_nsx/tests/unit/extensions/test_securitygroup.py +++ b/vmware_nsx/tests/unit/extensions/test_securitygroup.py @@ -14,7 +14,8 @@ # under the License. import mock -from neutron.tests.unit.extensions import test_securitygroup as ext_sg +from neutron.extensions import securitygroup as ext_sg +from neutron.tests.unit.extensions import test_securitygroup as test_ext_sg from vmware_nsx.nsxlib.v3 import dfw_api as firewall from vmware_nsx.nsxlib.v3 import security @@ -54,7 +55,7 @@ def _mock_create_and_list_nsgroups(test_method): class TestSecurityGroups(test_nsxv3.NsxV3PluginTestCaseMixin, - ext_sg.TestSecurityGroups): + test_ext_sg.TestSecurityGroups): @_mock_create_and_list_nsgroups @mock.patch.object(firewall, 'remove_nsgroup_member') @@ -67,8 +68,8 @@ class TestSecurityGroups(test_nsxv3.NsxV3PluginTestCaseMixin, # The first nsgroup is associated with the default secgroup, which is # not added to this port. - calls = [mock.call(NSG_IDS[1], mock.ANY, mock.ANY), - mock.call(NSG_IDS[2], mock.ANY, mock.ANY)] + calls = [mock.call(NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY), + mock.call(NSG_IDS[2], firewall.LOGICAL_PORT, mock.ANY)] add_member_mock.assert_has_calls(calls, any_order=True) @_mock_create_and_list_nsgroups @@ -80,9 +81,9 @@ class TestSecurityGroups(test_nsxv3.NsxV3PluginTestCaseMixin, super(TestSecurityGroups, self).test_update_port_with_multiple_security_groups() - calls = [mock.call(NSG_IDS[0], mock.ANY, mock.ANY), - mock.call(NSG_IDS[1], mock.ANY, mock.ANY), - mock.call(NSG_IDS[2], mock.ANY, mock.ANY)] + calls = [mock.call(NSG_IDS[0], firewall.LOGICAL_PORT, mock.ANY), + mock.call(NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY), + mock.call(NSG_IDS[2], firewall.LOGICAL_PORT, mock.ANY)] add_member_mock.assert_has_calls(calls, any_order=True) remove_member_mock.assert_called_with( @@ -102,6 +103,57 @@ class TestSecurityGroups(test_nsxv3.NsxV3PluginTestCaseMixin, remove_member_mock.assert_called_with( NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY) + @_mock_create_and_list_nsgroups + @mock.patch.object(firewall, 'add_nsgroup_member') + def test_create_port_with_full_security_group(self, add_member_mock): + + def _add_member_mock(nsgroup, target_type, target_id): + if nsgroup in NSG_IDS: + raise firewall.NSGroupIsFull(nsgroup_id=nsgroup) + add_member_mock.side_effect = _add_member_mock + + with self.network() as net: + with self.subnet(net): + res = self._create_port(self.fmt, net['network']['id']) + res_body = self.deserialize(self.fmt, res) + + self.assertEqual(500, res.status_int) + self.assertEqual('SecurityGroupMaximumCapacityReached', + res_body['NeutronError']['type']) + + @_mock_create_and_list_nsgroups + @mock.patch.object(firewall, 'remove_nsgroup_member') + @mock.patch.object(firewall, 'add_nsgroup_member') + def test_update_port_with_full_security_group(self, + add_member_mock, + remove_member_mock): + def _add_member_mock(nsgroup, target_type, target_id): + if nsgroup == NSG_IDS[2]: + raise firewall.NSGroupIsFull(nsgroup_id=nsgroup) + add_member_mock.side_effect = _add_member_mock + + with self.port() as port: + with self.security_group() as sg1: + with self.security_group() as sg2: + data = {'port': {ext_sg.SECURITYGROUPS: + [sg1['security_group']['id'], + sg2['security_group']['id']]}} + req = self.new_update_request( + 'ports', data, port['port']['id']) + res = req.get_response(self.api) + res_body = self.deserialize(self.fmt, res) + + self.assertEqual(500, res.status_int) + self.assertEqual('SecurityGroupMaximumCapacityReached', + res_body['NeutronError']['type']) + + # Because the update has failed we excpect that the plugin will try to + # revert any changes in the NSGroups - It is required to remove the + # lport from any NSGroups which it was added to during that call. + calls = [mock.call(NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY), + mock.call(NSG_IDS[2], firewall.LOGICAL_PORT, mock.ANY)] + remove_member_mock.assert_has_calls(calls, any_order=True) + class TestNSGroupManager(nsxlib_testcase.NsxLibTestCase): """ @@ -167,7 +219,7 @@ class TestNSGroupManager(nsxlib_testcase.NsxLibTestCase): def _add_member_mock(nsgroup, target_type, target_id): if nsgroup == NSG_IDS[2]: - raise firewall.NSGroupIsFull() + raise firewall.NSGroupIsFull(nsgroup_id=nsgroup) def _remove_member_mock(nsgroup, target_type, target_id, verify=False): if nsgroup == NSG_IDS[2]: