Better error message when reaching security-group maximum capacity

Change-Id: Iebb230f33b75a81e8d58796e4911b9b8ce92f8d0
Closes-Bug: #1540101

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.")

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 "
+            raise NSGroupMemberNotFound(member_id=target_id,
-                        "from NSGroup %(nid)s.") % {'tid': target_id,
+                                        nsgroup_id=nsgroup_id)
-                                                    'nid': nsgroup_id}
-            raise NSGroupMemberNotFound(err_msg=err_msg)
 
 
 def read_nsgroup(nsgroup_id):

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(
+        try:
-            nsgroup_id, firewall.LOGICAL_PORT, lport_id)
+            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(

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(
+                try:
-                    context, lport['id'], [], sgids)
+                    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 "

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),
+        calls = [mock.call(NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY),
-                 mock.call(NSG_IDS[2], mock.ANY, 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),
+        calls = [mock.call(NSG_IDS[0], firewall.LOGICAL_PORT, mock.ANY),
-                 mock.call(NSG_IDS[1], mock.ANY, mock.ANY),
+                 mock.call(NSG_IDS[1], firewall.LOGICAL_PORT, mock.ANY),
-                 mock.call(NSG_IDS[2], mock.ANY, 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]: