Refactor security modules and retry

A small refactor to the nsxlib:
1. change the security object code to be more similar to the other resources
2. Use retry code in base resources class only and not in resources
implementations
3. generelize the resource update code, to avoid duplications in different
classes
4. Adding some tests to verify the fix does not damage anything

Change-Id: Iac2cc1d55d3525ad21cb6399da691e212d6d4722

vmware_nsxlib/tests/unit/v3/test_qos_switching_profile.py

@@ -107,7 +107,8 @@ class NsxLibQosTestCase(nsxlib_testcase.NsxClientTestCase):
                 update.assert_called_with(
                     'switching-profiles/%s'
                     % test_constants.FAKE_QOS_PROFILE['id'],
-                    self._body(description=new_description))
+                    self._body(description=new_description),
+                    headers=None)
 
     def _enable_qos_switching_profile_shaping(
         self, direction=nsx_constants.EGRESS, new_burst_size=100):

vmware_nsxlib/tests/unit/v3/test_resources.py

@@ -331,8 +331,6 @@ class LogicalPortTestCase(BaseTestResource):
 
         pkt_classifiers, binding_repr = self._get_pktcls_bindings()
 
-        fake_port['address_bindings'] = binding_repr
-
         mocked_resource = self.get_mocked_resource()
         description = 'dummy'
         switch_profile = resources.SwitchingProfile
@@ -352,7 +350,7 @@ class LogicalPortTestCase(BaseTestResource):
                 'id': fake_port['attachment']['id']
             },
             'admin_state': 'UP',
-            'address_bindings': fake_port['address_bindings'],
+            'address_bindings': binding_repr,
             'description': description
         }
 
@@ -366,7 +364,7 @@ class LogicalPortTestCase(BaseTestResource):
         """Test creating a port returns the correct response and 200 status
 
         """
-        fake_port = test_constants.FAKE_CONTAINER_PORT.copy()
+        fake_port = copy.deepcopy(test_constants.FAKE_CONTAINER_PORT)
 
         profile_dicts = self._get_profile_dicts(fake_port)
 
@@ -451,12 +449,13 @@ class LogicalPortTestCase(BaseTestResource):
         fake_port['address_bindings'] = ['a', 'b']
         mocked_resource = self.get_mocked_resource()
 
-        def get_fake_port(*args):
+        def get_fake_port(*args, **kwargs):
-            return fake_port
+            return copy.copy(fake_port)
 
-        mocked_resource.get = get_fake_port
+        mocked_resource.client.get = get_fake_port
         mocked_resource.update(
-            fake_port['id'], fake_port['id'], address_bindings=[])
+            fake_port['id'], fake_port['attachment']['id'],
+            address_bindings=[])
 
         fake_port['address_bindings'] = []
         test_client.assert_json_call(
@@ -483,6 +482,98 @@ class LogicalPortTestCase(BaseTestResource):
         except exceptions.ManagerError as e:
             self.assertIn(nsxlib_testcase.NSX_MANAGER, e.msg)
 
+    def test_update_logical_port_no_addr_binding(self):
+        fake_port = copy.deepcopy(test_constants.FAKE_CONTAINER_PORT)
+        mocked_resource = self.get_mocked_resource()
+        new_name = 'updated_port'
+        new_desc = 'updated'
+        fake_port_ctx = fake_port['attachment']['context']
+        fake_container_host_vif_id = fake_port_ctx['container_host_vif_id']
+
+        def get_fake_port(*args, **kwargs):
+            return copy.copy(fake_port)
+
+        mocked_resource.client.get = get_fake_port
+
+        mocked_resource.update(
+            fake_port['id'],
+            fake_port['attachment']['id'],
+            name=new_name,
+            description=new_desc,
+            parent_vif_id=fake_container_host_vif_id,
+            traffic_tag=fake_port_ctx['vlan_tag'],
+            vif_type=fake_port_ctx['vif_type'],
+            app_id=fake_port_ctx['app_id'],
+            allocate_addresses=fake_port_ctx['allocate_addresses'])
+
+        fake_port['display_name'] = new_name
+        fake_port['description'] = new_desc
+        fake_port['attachment'] = {
+            'attachment_type': 'VIF',
+            'id': fake_port['attachment']['id'],
+            'context': {
+                'resource_type': 'VifAttachmentContext',
+                'allocate_addresses': 'Both',
+                'parent_vif_id': fake_container_host_vif_id,
+                'traffic_tag': fake_port_ctx['vlan_tag'],
+                'app_id': fake_port_ctx['app_id'],
+                'vif_type': 'CHILD',
+            }
+        }
+
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/logical-ports/%s' % fake_port['id'],
+            data=jsonutils.dumps(fake_port, sort_keys=True),
+            headers=self.default_headers())
+
+    def test_update_logical_port_with_addr_binding(self):
+        fake_port = copy.deepcopy(test_constants.FAKE_CONTAINER_PORT)
+        mocked_resource = self.get_mocked_resource()
+        new_name = 'updated_port'
+        new_desc = 'updated'
+        fake_port_ctx = fake_port['attachment']['context']
+        fake_container_host_vif_id = fake_port_ctx['container_host_vif_id']
+        pkt_classifiers, binding_repr = self._get_pktcls_bindings()
+
+        def get_fake_port(*args, **kwargs):
+            return copy.copy(fake_port)
+
+        mocked_resource.client.get = get_fake_port
+
+        mocked_resource.update(
+            fake_port['id'],
+            fake_port['attachment']['id'],
+            name=new_name,
+            description=new_desc,
+            parent_vif_id=fake_container_host_vif_id,
+            traffic_tag=fake_port_ctx['vlan_tag'],
+            vif_type=fake_port_ctx['vif_type'],
+            app_id=fake_port_ctx['app_id'],
+            allocate_addresses=fake_port_ctx['allocate_addresses'],
+            address_bindings=pkt_classifiers)
+
+        fake_port['display_name'] = new_name
+        fake_port['description'] = new_desc
+        fake_port['attachment'] = {
+            'attachment_type': 'VIF',
+            'id': fake_port['attachment']['id'],
+            'context': {
+                'resource_type': 'VifAttachmentContext',
+                'allocate_addresses': 'Both',
+                'parent_vif_id': fake_container_host_vif_id,
+                'traffic_tag': fake_port_ctx['vlan_tag'],
+                'app_id': fake_port_ctx['app_id'],
+                'vif_type': 'CHILD',
+            }
+        }
+        fake_port['address_bindings'] = binding_repr
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/logical-ports/%s' % fake_port['id'],
+            data=jsonutils.dumps(fake_port, sort_keys=True),
+            headers=self.default_headers())
+
 
 class LogicalRouterTestCase(BaseTestResource):
 
@@ -776,7 +867,8 @@ class LogicalRouterPortTestCase(BaseTestResource):
         uuid = fake_router_port['id']
         fake_relay_uuid = uuidutils.generate_uuid()
         lrport = self.get_mocked_resource()
-        with mock.patch.object(lrport, 'get', return_value=fake_router_port),\
+        with mock.patch.object(lrport.client, 'get',
+                               return_value=fake_router_port),\
             mock.patch("vmware_nsxlib.v3.NsxLib.get_version",
                        return_value='2.0.0'):
             lrport.update(uuid, relay_service_uuid=fake_relay_uuid)
@@ -1443,6 +1535,120 @@ class LogicalDhcpServerTestCase(BaseTestResource):
         super(LogicalDhcpServerTestCase, self).setUp(
             resources.LogicalDhcpServer)
 
+    def test_update_empty_dhcp_server(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        ip = '1.1.1.1'
+
+        with mock.patch.object(mocked_resource.client, "get", return_value={}):
+            mocked_resource.update(server_uuid, server_ip=ip)
+        body = {'ipv4_dhcp_server': {'dhcp_server_ip': ip}}
+
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s' %
+            (mocked_resource.uri_segment, server_uuid),
+            data=jsonutils.dumps(body, sort_keys=True),
+            headers=self.default_headers())
+
+    def test_update_dhcp_server_new_val(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        ip = '1.1.1.1'
+        domain_name = 'dummy'
+        existing_server = {'ipv4_dhcp_server': {'domain_name': domain_name}}
+
+        # add the server ip
+        with mock.patch.object(mocked_resource.client, "get",
+                               return_value=existing_server):
+            mocked_resource.update(server_uuid, server_ip=ip)
+
+        existing_server['ipv4_dhcp_server']['dhcp_server_ip'] = ip
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s' %
+            (mocked_resource.uri_segment, server_uuid),
+            data=jsonutils.dumps(existing_server, sort_keys=True),
+            headers=self.default_headers())
+
+    def test_update_dhcp_server_replace_val(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        ip = '1.1.1.1'
+        domain_name = 'dummy'
+        existing_server = {'ipv4_dhcp_server': {'domain_name': domain_name,
+                                                'dhcp_server_ip': ip}}
+
+        # replace the server ip
+        new_ip = '2.2.2.2'
+        with mock.patch.object(mocked_resource.client, "get",
+                               return_value=existing_server):
+            mocked_resource.update(server_uuid, server_ip=new_ip)
+
+        existing_server['ipv4_dhcp_server']['dhcp_server_ip'] = new_ip
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s' %
+            (mocked_resource.uri_segment, server_uuid),
+            data=jsonutils.dumps(existing_server, sort_keys=True),
+            headers=self.default_headers())
+
+    def test_create_binding(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        mac = 'aa:bb:cc:dd:ee:ff'
+        ip = '1.1.1.1'
+        host = 'host'
+        mocked_resource.create_binding(server_uuid, mac, ip, hostname=host)
+        body = {
+            'mac_address': mac,
+            'ip_address': ip,
+            'host_name': host,
+        }
+        test_client.assert_json_call(
+            'post', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s/static-bindings' %
+            (mocked_resource.uri_segment, server_uuid),
+            data=jsonutils.dumps(body, sort_keys=True),
+            headers=self.default_headers())
+
+    def test_get_binding(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        binding_uuid = 'binding-uuid'
+        mocked_resource.get_binding(server_uuid, binding_uuid)
+        test_client.assert_json_call(
+            'get', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s/static-bindings/%s' %
+            (mocked_resource.uri_segment, server_uuid, binding_uuid),
+            headers=self.default_headers())
+
+    def test_update_binding(self):
+        mocked_resource = self.get_mocked_resource()
+        server_uuid = 'server-uuid'
+        binding_uuid = 'binding-uuid'
+        mac = 'aa:bb:cc:dd:ee:ff'
+        new_mac = 'dd:bb:cc:dd:ee:ff'
+        ip = '1.1.1.1'
+        host = 'host'
+        body = {
+            'mac_address': mac,
+            'ip_address': ip,
+            'host_name': host,
+        }
+        with mock.patch.object(mocked_resource.client, "get",
+                               return_value=body):
+            mocked_resource.update_binding(server_uuid,
+                                           binding_uuid,
+                                           mac_address=new_mac)
+        body['mac_address'] = new_mac
+        test_client.assert_json_call(
+            'put', mocked_resource,
+            'https://1.2.3.4/api/v1/%s/%s/static-bindings/%s' %
+            (mocked_resource.uri_segment, server_uuid, binding_uuid),
+            data=jsonutils.dumps(body, sort_keys=True),
+            headers=self.default_headers())
+
 
 class DummyCachedResource(utils.NsxLibApiBase):
 

vmware_nsxlib/tests/unit/v3/test_security.py

@@ -85,7 +85,7 @@ class TestNsxLibFirewallSection(nsxlib_testcase.NsxLibTestCase):
                 'display-name', 'section-description', rules=[rule])
             resource = 'firewall/sections?operation=insert_bottom' \
                 '&action=create_with_rules'
-            create.assert_called_with(resource, expected_body)
+            create.assert_called_with(resource, expected_body, headers=None)
 
     def test_get_excludelist(self):
         with mock.patch.object(self.nsxlib.client, 'list') as clist:
@@ -153,7 +153,7 @@ class TestNsxLibIPSet(nsxlib_testcase.NsxClientTestCase):
                 self.nsxlib.ip_set.update(
                     fake_ip_set['id'], ip_addresses=new_ip_addresses)
                 resource = 'ip-sets/%s' % fake_ip_set['id']
-                update.assert_called_with(resource, data)
+                update.assert_called_with(resource, data, headers=None)
 
     def test_update_ip_set_empty_ip_addresses(self):
         fake_ip_set = test_constants.FAKE_IP_SET.copy()
@@ -170,7 +170,7 @@ class TestNsxLibIPSet(nsxlib_testcase.NsxClientTestCase):
                 self.nsxlib.ip_set.update(
                     fake_ip_set['id'], ip_addresses=new_ip_addresses)
                 resource = 'ip-sets/%s' % fake_ip_set['id']
-                update.assert_called_with(resource, data)
+                update.assert_called_with(resource, data, headers=None)
 
 
 class TestNsxLibNSGroup(nsxlib_testcase.NsxClientTestCase):
@@ -194,4 +194,4 @@ class TestNsxLibNSGroup(nsxlib_testcase.NsxClientTestCase):
                 self.nsxlib.ns_group.update('nsgroupid', tags_update=nsg_tags)
                 resource = 'ns-groups/nsgroupid'
                 data = {'tags': nsg_tags}
-                update.assert_called_with(resource, data)
+                update.assert_called_with(resource, data, headers=None)

vmware_nsxlib/v3/core_resources.py

@@ -186,39 +186,24 @@ class NsxLibLogicalSwitch(utils.NsxLibApiBase):
         return self.client.create(self.get_path(), body)
 
     def delete(self, lswitch_id):
-        # Using internal method so we can access max_attempts in the decorator
+        resource = '%s?detach=true&cascade=true' % lswitch_id
-        @utils.retry_upon_exception(
+        self._delete_with_retry(resource)
-            exceptions.StaleRevision,
-            max_attempts=self.nsxlib_config.max_attempts)
-        def _do_delete():
-            resource = '%s?detach=true&cascade=true' % lswitch_id
-            self.client.delete(self.get_path(resource))
-
-        _do_delete()
 
     def update(self, lswitch_id, name=None, admin_state=None, tags=None,
                description=None):
-        # Using internal method so we can access max_attempts in the decorator
+        body = {}
-        @utils.retry_upon_exception(
+        if name:
-            exceptions.StaleRevision,
+            body['display_name'] = name
-            max_attempts=self.nsxlib_config.max_attempts)
+        if admin_state is not None:
-        def _do_update():
+            if admin_state:
-            lswitch = self.get(lswitch_id)
+                body['admin_state'] = nsx_constants.ADMIN_STATE_UP
-            # Assign name to a local variable since 'name' is out of scope
+            else:
-            ls_name = name or lswitch.get('display_name')
+                body['admin_state'] = nsx_constants.ADMIN_STATE_DOWN
-            lswitch['display_name'] = ls_name
+        if tags is not None:
-            if admin_state is not None:
+            body['tags'] = tags
-                if admin_state:
+        if description is not None:
-                    lswitch['admin_state'] = nsx_constants.ADMIN_STATE_UP
+            body['description'] = description
-                else:
+        return self._update_with_retry(lswitch_id, body)
-                    lswitch['admin_state'] = nsx_constants.ADMIN_STATE_DOWN
-            if tags is not None:
-                lswitch['tags'] = tags
-            if description is not None:
-                lswitch['description'] = description
-            return self.client.update(self.get_path(lswitch_id), lswitch)
-
-        return _do_update()
 
 
 class SwitchingProfileTypes(object):
@@ -408,9 +393,8 @@ class NsxLibQosSwitchingProfile(NsxLibSwitchingProfile):
         return self.client.create(self.get_path(), body)
 
     def update(self, profile_id, tags, name=None, description=None):
-        # get the current configuration
-        body = self.get(profile_id)
         # update the relevant fields
+        body = {}
         body = self._update_args(body, name, description)
         if tags is not None:
             body['tags'] = tags
@@ -615,7 +599,7 @@ class NsxLibLogicalRouter(utils.NsxLibApiBase):
     def update_nat_rule(self, logical_router_id, nat_rule_id, **kwargs):
         resource = 'logical-routers/%s/nat/rules/%s' % (
             logical_router_id, nat_rule_id)
-        return self._update_resource_with_retry(resource, kwargs)
+        return self._update_resource(resource, kwargs, retry=True)
 
     def update_advertisement(self, logical_router_id, **kwargs):
         resource = ('logical-routers/%s/routing/advertisement' %
@@ -632,12 +616,12 @@ class NsxLibLogicalRouter(utils.NsxLibApiBase):
                               {'arg': arg, 'rtr': logical_router_id})
                 del kwargs[arg]
 
-        return self._update_resource_with_retry(resource, kwargs)
+        return self._update_resource(resource, kwargs, retry=True)
 
     def update_advertisement_rules(self, logical_router_id, rules):
         resource = ('logical-routers/%s/routing/advertisement/rules' %
                     logical_router_id)
-        return self._update_resource_with_retry(resource, {'rules': rules})
+        return self._update_resource(resource, {'rules': rules}, retry=True)
 
     def get_advertisement_rules(self, logical_router_id):
         resource = ('logical-routers/%s/routing/advertisement/rules' %
@@ -666,21 +650,7 @@ class NsxLibLogicalRouter(utils.NsxLibApiBase):
         return self.client.delete(self.get_path(url))
 
     def update(self, lrouter_id, *args, **kwargs):
-        # Using internal method so we can access max_attempts in the decorator
+        return self._update_with_retry(lrouter_id, kwargs)
-        @utils.retry_upon_exception(
-            exceptions.StaleRevision,
-            max_attempts=self.client.max_attempts)
-        def _do_update():
-            lrouter = self.get(lrouter_id)
-            for k in kwargs:
-                lrouter[k] = kwargs[k]
-            # If revision_id of the payload that we send is older than what
-            # NSX has, we will get a 412: Precondition Failed.
-            # In that case we need to re-fetch, patch the response and send
-            # it again with the new revision_id
-            return self.client.update(self.get_path(lrouter_id), body=lrouter)
-
-        return _do_update()
 
     def get_firewall_section_id(self, lrouter_id, router_body=None):
         """Return the id of the auto created firewall section of the router
@@ -808,8 +778,7 @@ class NsxLibMetadataProxy(utils.NsxLibApiBase):
         return 'MetadataProxy'
 
     def update(self, uuid, server_url=None, secret=None, edge_cluster_id=None):
-        # get the current configuration
+        body = {}
-        body = self.get(uuid)
         # update the relevant fields
         if server_url is not None:
             body['metadata_server_url'] = server_url

vmware_nsxlib/v3/resources.py

@@ -83,7 +83,6 @@ class LogicalPort(utils.NsxLibApiBase):
             attachment=None,
             description=None):
         tags = tags or []
-        address_bindings = address_bindings or []
         switch_profile_ids = switch_profile_ids or []
         body = {}
         if tags:
@@ -108,8 +107,7 @@ class LogicalPort(utils.NsxLibApiBase):
                     address_classifier['vlan'] = int(binding.vlan)
                 bindings.append(address_classifier)
             body['address_bindings'] = bindings
-        elif address_bindings == []:
+        elif address_bindings is not None:
-            # explicitly clear out address bindings
             body['address_bindings'] = []
 
         if switch_profile_ids:
@@ -151,14 +149,6 @@ class LogicalPort(utils.NsxLibApiBase):
         else:
             return False  # no attachment change
 
-    def _build_address_bindings(self, address_bindings):
-        addr_bindings = []
-        for binding in address_bindings:
-            addr_bindings.append(PacketAddressClassifier(
-                binding.get('ip_address'), binding.get('mac_address'),
-                binding.get('vlan')))
-        return addr_bindings
-
     def create(self, lswitch_id, vif_uuid, tags=None,
                attachment_type=nsx_constants.ATTACHMENT_VIF,
                admin_state=True, name=None, address_bindings=None,
@@ -184,15 +174,7 @@ class LogicalPort(utils.NsxLibApiBase):
         return self.client.create(self.get_path(), body=body)
 
     def delete(self, lport_id):
-        # Using internal method so we can access max_attempts in the decorator
+        self._delete_with_retry('%s?detach=true' % lport_id)
-        @utils.retry_upon_exception(
-            exceptions.StaleRevision,
-            max_attempts=self.client.max_attempts)
-        def _do_delete():
-            return self.client.url_delete(
-                self.get_path('%s?detach=true' % lport_id))
-
-        return _do_delete()
 
     def update(self, lport_id, vif_uuid,
                name=None, admin_state=None,
@@ -203,38 +185,23 @@ class LogicalPort(utils.NsxLibApiBase):
                vif_type=None, app_id=None,
                allocate_addresses=nsx_constants.ALLOCATE_ADDRESS_NONE,
                description=None):
-        # Using internal method so we can access max_attempts in the decorator
+        attachment = self._prepare_attachment(attachment_type, vif_uuid,
-        @utils.retry_upon_exception(
+                                              allocate_addresses, vif_type,
-            exceptions.StaleRevision,
+                                              parent_vif_id, traffic_tag,
-            max_attempts=self.client.max_attempts)
+                                              app_id)
-        def do_update():
+        lport = {}
-            lport = self.get(lport_id)
+        if tags_update is not None:
-            tags = lport.get('tags', [])
+            lport['tags_update'] = tags_update
-            if tags_update:
+        lport.update(self._build_body_attrs(
-                tags = utils.update_v3_tags(tags, tags_update)
+            display_name=name,
-            # Assign outer function argument to a local scope
+            admin_state=admin_state,
-            addr_bindings = address_bindings
+            address_bindings=address_bindings,
-            if addr_bindings is None:
+            switch_profile_ids=switch_profile_ids,
-                addr_bindings = self._build_address_bindings(
+            attachment=attachment,
-                    lport.get('address_bindings'))
+            description=description))
-            attachment = self._prepare_attachment(attachment_type, vif_uuid,
-                                                  allocate_addresses, vif_type,
-                                                  parent_vif_id, traffic_tag,
-                                                  app_id)
-            lport.update(self._build_body_attrs(
-                display_name=name,
-                admin_state=admin_state, tags=tags,
-                address_bindings=addr_bindings,
-                switch_profile_ids=switch_profile_ids,
-                attachment=attachment,
-                description=description))
 
-            # If revision_id of the payload that we send is older than what
+        return self._update_resource(
-            # NSX has, we will get a 412: Precondition Failed.
+            self.get_path(lport_id), lport, retry=True)
-            # In that case we need to re-fetch, patch the response and send
-            # it again with the new revision_id
-            return self.client.update(self.get_path(lport_id), body=lport)
-        return do_update()
 
     def get_by_attachment(self, attachment_type, attachment_id):
         """Return all logical port matching the attachment type and Id"""
@@ -306,50 +273,34 @@ class LogicalRouterPort(utils.NsxLibApiBase):
         return self.client.create(self.get_path(), body=body)
 
     def update(self, logical_port_id, **kwargs):
-        # Using internal method so we can access max_attempts in the decorator
+        logical_router_port = {}
-        @utils.retry_upon_exception(
+        # special treatment for updating/removing the relay service
-            exceptions.StaleRevision,
+        if 'relay_service_uuid' in kwargs:
-            max_attempts=self.client.max_attempts)
+            if kwargs['relay_service_uuid']:
-        def _do_update():
+                if (self.nsxlib and
-            logical_router_port = self.get(logical_port_id)
+                    self.nsxlib.feature_supported(
-            # special treatment for updating/removing the relay service
+                        nsx_constants.FEATURE_DHCP_RELAY)):
-            if 'relay_service_uuid' in kwargs:
+                    logical_router_port['service_bindings'] = [
-                if kwargs['relay_service_uuid']:
+                        self._get_relay_binding(
-                    if (self.nsxlib and
+                            kwargs['relay_service_uuid'])]
-                        self.nsxlib.feature_supported(
-                            nsx_constants.FEATURE_DHCP_RELAY)):
-                        logical_router_port['service_bindings'] = [
-                            self._get_relay_binding(
-                                kwargs['relay_service_uuid'])]
-                    else:
-                        LOG.error("Ignoring relay_service_uuid for router "
-                                  "port %s: This feature is not supported.",
-                                  logical_port_id)
                 else:
-                    # delete the current one
+                    LOG.error("Ignoring relay_service_uuid for router "
-                    if 'service_bindings' in logical_router_port:
+                              "port %s: This feature is not supported.",
-                        logical_router_port['service_bindings'] = []
+                              logical_port_id)
-                del kwargs['relay_service_uuid']
+            else:
+                # delete the current one
+                if 'service_bindings' in logical_router_port:
+                    logical_router_port['service_bindings'] = []
+            del kwargs['relay_service_uuid']
 
-            for k in kwargs:
+        for k in kwargs:
-                logical_router_port[k] = kwargs[k]
+            logical_router_port[k] = kwargs[k]
-            # If revision_id of the payload that we send is older than what
+
-            # NSX has, we will get a 412: Precondition Failed.
+        return self._update_resource(
-            # In that case we need to re-fetch, patch the response and send
+            self.get_path(logical_port_id), logical_router_port, retry=True)
-            # it again with the new revision_id
-            return self.client.update(self.get_path(logical_port_id),
-                                      body=logical_router_port)
-        return _do_update()
 
     def delete(self, logical_port_id):
-        # Using internal method so we can access max_attempts in the decorator
+        self._delete_with_retry(logical_port_id)
-        @utils.retry_upon_exception(
-            exceptions.StaleRevision,
-            max_attempts=self.client.max_attempts)
-        def _do_delete():
-            return self.client.url_delete(self.get_path(logical_port_id))
-
-        return _do_delete()
 
     def get_by_lswitch_id(self, logical_switch_id):
         resource = '?logical_switch_id=%s' % logical_switch_id
@@ -501,18 +452,11 @@ class LogicalDhcpServer(utils.NsxLibApiBase):
     def update(self, uuid, dhcp_profile_id=None, server_ip=None, name=None,
                dns_nameservers=None, domain_name=None, gateway_ip=False,
                options=None, tags=None):
-        # Using internal method so we can access max_attempts in the decorator
+        body = {'ipv4_dhcp_server': {}}
-        @utils.retry_upon_exception(
+        self._construct_server(body, dhcp_profile_id, server_ip, name,
-            exceptions.StaleRevision,
+                               dns_nameservers, domain_name, gateway_ip,
-            max_attempts=self.client.max_attempts)
+                               options, tags)
-        def _do_update():
+        return self._update_with_retry(uuid, body)
-            body = self.get(uuid)
-            self._construct_server(body, dhcp_profile_id, server_ip, name,
-                                   dns_nameservers, domain_name, gateway_ip,
-                                   options, tags)
-            return self.client.update(self.get_path(uuid), body=body)
-
-        return _do_update()
 
     def create_binding(self, server_uuid, mac, ip, hostname=None,
                        lease_time=None, options=None, gateway_ip=False):
@@ -534,17 +478,10 @@ class LogicalDhcpServer(utils.NsxLibApiBase):
         return self.get(url)
 
     def update_binding(self, server_uuid, binding_uuid, **kwargs):
-        # Using internal method so we can access max_attempts in the decorator
+        body = {}
-        @utils.retry_upon_exception(
+        body.update(kwargs)
-            exceptions.StaleRevision,
+        url = "%s/static-bindings/%s" % (server_uuid, binding_uuid)
-            max_attempts=self.client.max_attempts)
+        self._update_resource(self.get_path(url), body, retry=True)
-        def _do_update():
-            body = self.get_binding(server_uuid, binding_uuid)
-            body.update(kwargs)
-            url = "%s/static-bindings/%s" % (server_uuid, binding_uuid)
-            return self.client.url_put(self.get_path(url), body)
-
-        return _do_update()
 
     def delete_binding(self, server_uuid, binding_uuid):
         url = "%s/static-bindings/%s" % (server_uuid, binding_uuid)

vmware_nsxlib/v3/security.py

@@ -41,6 +41,14 @@ class NsxLibNsGroup(utils.NsxLibApiBase):
         super(NsxLibNsGroup, self).__init__(client, nsxlib_config,
                                             nsxlib=nsxlib)
 
+    @property
+    def uri_segment(self):
+        return 'ns-groups'
+
+    @property
+    def resource_type(self):
+        return 'NSGroup'
+
     def update_on_backend(self, context, security_group,
                           nsgroup_id, section_id,
                           log_sg_allowed_traffic):
@@ -130,35 +138,30 @@ class NsxLibNsGroup(utils.NsxLibApiBase):
                 body.update({'membership_criteria': membership_criteria})
             else:
                 body.update({'membership_criteria': [membership_criteria]})
-        return self.client.create('ns-groups', body)
+        return self.client.create(self.get_path(), body)
 
     def list(self):
         return self.client.list(
-            'ns-groups?populate_references=false').get('results', [])
+            '%s?populate_references=false' % self.get_path()).get(
+            'results', [])
 
     def update(self, nsgroup_id, display_name=None, description=None,
                membership_criteria=None, members=None, tags_update=None):
-        # Using internal method so we can access max_attempts in the decorator
+        nsgroup = {}
-        @utils.retry_upon_exception(
+        if display_name is not None:
-            exceptions.StaleRevision,
+            nsgroup['display_name'] = display_name
-            max_attempts=self.nsxlib_config.max_attempts)
+        if description is not None:
-        def _do_update():
+            nsgroup['description'] = description
-            nsgroup = self.read(nsgroup_id)
+        if members is not None:
-            if display_name is not None:
+            nsgroup['members'] = members
-                nsgroup['display_name'] = display_name
+        if membership_criteria is not None:
-            if description is not None:
+            nsgroup['membership_criteria'] = [membership_criteria]
-                nsgroup['description'] = description
+        if tags_update is not None:
-            if members is not None:
+            nsgroup['tags_update'] = tags_update
-                nsgroup['members'] = members
+        return self._update_resource(
-            if membership_criteria is not None:
+            self.get_path(nsgroup_id), nsgroup,
-                nsgroup['membership_criteria'] = [membership_criteria]
+            get_params='?populate_references=true',
-            if tags_update is not None:
+            retry=True)
-                nsgroup['tags'] = utils.update_v3_tags(nsgroup.get('tags', []),
-                                                       tags_update)
-            return self.client.update(
-                'ns-groups/%s' % nsgroup_id, nsgroup)
-
-        return _do_update()
 
     def get_member_expression(self, target_type, target_id):
         return {
@@ -169,7 +172,7 @@ class NsxLibNsGroup(utils.NsxLibApiBase):
             'value': target_id}
 
     def _update_with_members(self, nsgroup_id, members, action):
-        members_update = 'ns-groups/%s?action=%s' % (nsgroup_id, action)
+        members_update = '%s?action=%s' % (self.get_path(nsgroup_id), action)
         return self.client.create(members_update, members)
 
     def add_members(self, nsgroup_id, target_type, target_ids):
@@ -210,12 +213,12 @@ class NsxLibNsGroup(utils.NsxLibApiBase):
 
     def read(self, nsgroup_id):
         return self.client.get(
-            'ns-groups/%s?populate_references=true' % nsgroup_id)
+            '%s?populate_references=true' % self.get_path(nsgroup_id))
 
     def delete(self, nsgroup_id):
         try:
             return self.client.delete(
-                'ns-groups/%s?force=true' % nsgroup_id)
+                '%s?force=true' % self.get_path(nsgroup_id))
         # FIXME(roeyc): Should only except NotFound error.
         except Exception:
             LOG.debug("NSGroup %s does not exists for delete request.",
@@ -231,28 +234,24 @@ class NsxLibNsGroup(utils.NsxLibApiBase):
 
 class NsxLibFirewallSection(utils.NsxLibApiBase):
 
-    def add_member_to_fw_exclude_list(self, target_id, target_type):
+    @property
-        @utils.retry_upon_exception(
+    def uri_segment(self):
-            exceptions.StaleRevision,
+        return 'firewall/sections'
-            max_attempts=self.nsxlib_config.max_attempts)
-        def _add_member_to_fw_exclude_list():
-            resource = 'firewall/excludelist?action=add_member'
-            body = {"target_id": target_id,
-                    "target_type": target_type}
-            self.client.create(resource, body)
 
-        _add_member_to_fw_exclude_list()
+    @property
+    def resource_type(self):
+        return 'FirewallSection'
+
+    def add_member_to_fw_exclude_list(self, target_id, target_type):
+        resource = 'firewall/excludelist?action=add_member'
+        body = {"target_id": target_id,
+                "target_type": target_type}
+        self._create_with_retry(resource, body)
 
     def remove_member_from_fw_exclude_list(self, target_id, target_type):
-        @utils.retry_upon_exception(
+        resource = ('firewall/excludelist?action=remove_member&object_id='
-            exceptions.StaleRevision,
+                    + target_id)
-            max_attempts=self.nsxlib_config.max_attempts)
+        self._create_with_retry(resource)
-        def _remove_member_from_fw_exclude_list():
-            resource = ('firewall/excludelist?action=remove_member&object_id='
-                        + target_id)
-            self.client.create(resource)
-
-        _remove_member_from_fw_exclude_list()
 
     def get_excludelist(self):
         return self.client.list('firewall/excludelist')
@@ -332,92 +331,77 @@ class NsxLibFirewallSection(utils.NsxLibApiBase):
                      applied_tos, tags,
                      operation=consts.FW_INSERT_BOTTOM,
                      other_section=None):
-        @utils.retry_upon_exception(
+        resource = '%s?operation=%s' % (self.uri_segment, operation)
-            exceptions.StaleRevision,
+        body = self._build(display_name, description,
-            max_attempts=self.nsxlib_config.max_attempts)
+                           applied_tos, tags)
-        def _create_empty():
+        if other_section:
-            resource = 'firewall/sections?operation=%s' % operation
+            resource += '&id=%s' % other_section
-            body = self._build(display_name, description,
+        return self._create_with_retry(resource, body)
-                               applied_tos, tags)
-            if other_section:
-                resource += '&id=%s' % other_section
-            return self.client.create(resource, body)
-        return _create_empty()
 
     def create_with_rules(self, display_name, description, applied_tos=None,
                           tags=None, operation=consts.FW_INSERT_BOTTOM,
                           other_section=None, rules=None):
-        @utils.retry_upon_exception(
+        resource = '%s?operation=%s' % (self.uri_segment, operation)
-            exceptions.StaleRevision,
+        body = {
-            max_attempts=self.nsxlib_config.max_attempts)
+            'display_name': display_name,
-        def _create_with_rules():
+            'description': description,
-            resource = 'firewall/sections?operation=%s' % operation
+            'stateful': True,
-            body = {
+            'section_type': consts.FW_SECTION_LAYER3,
-                'display_name': display_name,
+            'applied_tos': applied_tos or [],
-                'description': description,
+            'tags': tags or []
-                'stateful': True,
+        }
-                'section_type': consts.FW_SECTION_LAYER3,
+        if rules is not None:
-                'applied_tos': applied_tos or [],
+            resource += '&action=create_with_rules'
-                'tags': tags or []
+            body['rules'] = rules
-            }
+        if other_section:
-            if rules is not None:
+            resource += '&id=%s' % other_section
-                resource += '&action=create_with_rules'
+        return self._create_with_retry(resource, body)
-                body['rules'] = rules
-            if other_section:
-                resource += '&id=%s' % other_section
-            return self.client.create(resource, body)
-        return _create_with_rules()
 
     def update(self, section_id, display_name=None, description=None,
                applied_tos=None, rules=None, tags_update=None, force=False):
-        # Using internal method so we can access max_attempts in the decorator
+        resource = self.get_path(section_id)
-        @utils.retry_upon_exception(
+        params = None
-            exceptions.StaleRevision,
+        section = {}
-            max_attempts=self.nsxlib_config.max_attempts)
+        if rules is not None:
-        def _do_update():
+            params = '?action=update_with_rules'
-            resource = 'firewall/sections/%s' % section_id
+            section['rules'] = rules
-            section = self.read(section_id)
+        if display_name is not None:
+            section['display_name'] = display_name
+        if description is not None:
+            section['description'] = description
+        if applied_tos is not None:
+            section['applied_tos'] = [self.get_nsgroup_reference(nsg_id)
+                                      for nsg_id in applied_tos]
+        if tags_update is not None:
+            section['tags_update'] = tags_update
 
-            if rules is not None:
+        headers = None
-                resource += '?action=update_with_rules'
+        if force:
-                section.update({'rules': rules})
+            # shared sections (like default section) can serve multiple
-            if display_name is not None:
+            # openstack deployments. If some operate under protected
-                section['display_name'] = display_name
+            # identities, force-overwrite is needed.
-            if description is not None:
+            # REVISIT(annak): find better solution for shared sections
-                section['description'] = description
+            headers = {'X-Allow-Overwrite': 'true'}
-            if applied_tos is not None:
-                section['applied_tos'] = [self.get_nsgroup_reference(nsg_id)
-                                          for nsg_id in applied_tos]
-            if tags_update is not None:
-                section['tags'] = utils.update_v3_tags(section.get('tags', []),
-                                                       tags_update)
-            headers = None
-            if force:
-                # shared sections (like default section) can serve multiple
-                # openstack deployments. If some operate under protected
-                # identities, force-overwrite is needed.
-                # REVISIT(annak): find better solution for shared sections
-                headers = {'X-Allow-Overwrite': 'true'}
 
-            if rules is not None:
+        if rules is not None:
-                return self.client.create(resource, section, headers=headers)
+            return self._update_resource(resource, section,
+                                         headers=headers,
+                                         create_action=True,
+                                         action_params=params,
+                                         retry=True)
 
-            elif any(p is not None for p in (display_name, description,
+        elif any(p is not None for p in (display_name, description,
-                                             applied_tos, tags_update)):
+                                         applied_tos, tags_update)):
-                return self.client.update(resource, section, headers=headers)
+            return self._update_resource(resource, section,
-
+                                         headers=headers,
-        return _do_update()
+                                         action_params=params,
-
+                                         retry=True)
-    def read(self, section_id):
-        resource = 'firewall/sections/%s' % section_id
-        return self.client.get(resource)
 
     def list(self):
-        resource = 'firewall/sections'
+        return self.client.list(self.get_path()).get('results', [])
-        return self.client.list(resource).get('results', [])
 
     def delete(self, section_id):
-        resource = 'firewall/sections/%s?cascade=true' % section_id
+        resource = '%s?cascade=true' % self.get_path(section_id)
         return self.client.delete(resource)
 
     def get_nsgroup_reference(self, nsgroup_id):
@@ -470,36 +454,21 @@ class NsxLibFirewallSection(utils.NsxLibApiBase):
         return rule_dict
 
     def add_rule(self, rule, section_id, operation=consts.FW_INSERT_BOTTOM):
-        @utils.retry_upon_exception(
+        resource = '%s/rules' % self.get_path(section_id)
-            exceptions.StaleRevision,
+        params = '?operation=%s' % operation
-            max_attempts=self.nsxlib_config.max_attempts)
+        return self._create_with_retry(resource + params, rule)
-        def _add_rule():
-            resource = 'firewall/sections/%s/rules' % section_id
-            params = '?operation=%s' % operation
-            return self.client.create(resource + params, rule)
-        return _add_rule()
 
     def add_rules(self, rules, section_id, operation=consts.FW_INSERT_BOTTOM):
-        @utils.retry_upon_exception(
+        resource = '%s/rules' % self.get_path(section_id)
-            exceptions.StaleRevision,
+        params = '?action=create_multiple&operation=%s' % operation
-            max_attempts=self.nsxlib_config.max_attempts)
+        return self._create_with_retry(resource + params, {'rules': rules})
-        def _add_rules():
-            resource = 'firewall/sections/%s/rules' % section_id
-            params = '?action=create_multiple&operation=%s' % operation
-            return self.client.create(resource + params, {'rules': rules})
-        return _add_rules()
 
     def delete_rule(self, section_id, rule_id):
-        @utils.retry_upon_exception(
+        resource = '%s/rules/%s' % (section_id, rule_id)
-            exceptions.StaleRevision,
+        return self._delete_with_retry(resource)
-            max_attempts=self.nsxlib_config.max_attempts)
-        def _delete_rule():
-            resource = 'firewall/sections/%s/rules/%s' % (section_id, rule_id)
-            return self.client.delete(resource)
-        return _delete_rule()
 
     def get_rules(self, section_id):
-        resource = 'firewall/sections/%s/rules' % section_id
+        resource = '%s/rules' % self.get_path(section_id)
         return self.client.get(resource)
 
     def get_default_rule(self, section_id):
@@ -621,50 +590,43 @@ class NsxLibFirewallSection(utils.NsxLibApiBase):
 
 class NsxLibIPSet(utils.NsxLibApiBase):
 
+    @property
+    def uri_segment(self):
+        return 'ip-sets'
+
+    @property
+    def resource_type(self):
+        return 'IPSet'
+
     def create(self, display_name, description=None, ip_addresses=None,
                tags=None):
-        resource = 'ip-sets'
         body = {
             'display_name': display_name,
             'description': description or '',
             'ip_addresses': ip_addresses or [],
             'tags': tags or []
         }
-        return self.client.create(resource, body)
+        return self.client.create(self.get_path(), body)
 
     def update(self, ip_set_id, display_name=None, description=None,
                ip_addresses=None, tags_update=None):
-        # Using internal method so we can access max_attempts in the decorator
+        ip_set = {}
-        @utils.retry_upon_exception(
+        if tags_update:
-            exceptions.StaleRevision,
+            ip_set['tags_update'] = tags_update
-            max_attempts=self.nsxlib_config.max_attempts)
+        if display_name is not None:
-        def _do_update():
+            ip_set['display_name'] = display_name
-            resource = 'ip-sets/%s' % ip_set_id
+        if description is not None:
-            ip_set = self.read(ip_set_id)
+            ip_set['description'] = description
-            tags = ip_set.get('tags', [])
+        if ip_addresses is not None:
-            if tags_update:
+            ip_set['ip_addresses'] = ip_addresses
-                tags = utils.update_v3_tags(tags, tags_update)
+        return self._update_resource(self.get_path(ip_set_id),
-            if display_name is not None:
+                                     ip_set, retry=True)
-                ip_set['display_name'] = display_name
-            if description is not None:
-                ip_set['description'] = description
-            if ip_addresses is not None:
-                ip_set['ip_addresses'] = ip_addresses
-            return self.client.update(resource, ip_set)
-
-        return _do_update()
 
     def read(self, ip_set_id):
         return self.client.get('ip-sets/%s' % ip_set_id)
 
     def delete(self, ip_set_id):
-        # Using internal method so we can access max_attempts in the decorator
+        self._delete_with_retry(ip_set_id)
-        @utils.retry_upon_exception(
-            exceptions.StaleRevision,
-            max_attempts=self.nsxlib_config.max_attempts)
-        def _do_delete():
-            return self.client.delete('ip-sets/%s' % ip_set_id)
-        return _do_delete()
 
     def get_ipset_reference(self, ip_set_id):
         return {'target_id': ip_set_id,

vmware_nsxlib/v3/utils.py

@@ -316,6 +316,10 @@ class NsxLibApiBase(object):
             self.cache.update(uuid, result)
         return result
 
+    def read(self, uuid, silent=False):
+        """The same as get"""
+        return self.get(uuid, silent=silent)
+
     def delete(self, uuid):
         if self.use_cache_for_get:
             self.cache.remove(uuid)
@@ -331,19 +335,96 @@ class NsxLibApiBase(object):
     def _update_with_retry(self, uuid, payload):
         if self.use_cache_for_get:
             self.cache.remove(uuid)
-        return self._update_resource_with_retry(self.get_path(uuid), payload)
+        return self._update_resource(self.get_path(uuid), payload, retry=True)
 
-    def _update_resource_with_retry(self, resource, payload):
+    def _internal_update_resource(self, resource, payload, headers=None,
-        # Using internal method so we can access max_attempts in the decorator
+                                  create_action=False,
-        @retry_upon_exception(nsxlib_exceptions.StaleRevision,
+                                  get_params=None,
-                              max_attempts=self.nsxlib_config.max_attempts)
+                                  action_params=None,
-        def do_update():
+                                  update_payload_cbk=None):
-            revised_payload = self.client.get(resource)
+        get_path = action_path = resource
-            for key_name in payload.keys():
+        if get_params:
+            get_path = get_path + get_params
+        if action_params:
+            action_path = action_path + action_params
+        revised_payload = self.client.get(get_path)
+        # custom resource callback for updating the payload
+        if update_payload_cbk:
+            update_payload_cbk(revised_payload, payload)
+        # special treatment for tags (merge old and new)
+        if 'tags_update' in payload.keys():
+            revised_payload['tags'] = update_v3_tags(
+                revised_payload.get('tags', []),
+                payload['tags_update'])
+            del payload['tags_update']
+        # update all the rest of the parameters
+        for key_name in payload.keys():
+            # handle 2 levels of dictionary:
+            if isinstance(payload[key_name], dict):
+                if key_name not in revised_payload:
+                    revised_payload[key_name] = payload[key_name]
+                else:
+                    # copy each key
+                    revised_payload[key_name].update(payload[key_name])
+            else:
                 revised_payload[key_name] = payload[key_name]
-            return self.client.update(resource, revised_payload)
+        if create_action:
+            return self.client.create(action_path, revised_payload,
+                                      headers=headers)
+        else:
+            return self.client.update(action_path, revised_payload,
+                                      headers=headers)
 
-        return do_update()
+    def _update_resource(self, resource, payload, headers=None,
+                         create_action=False, get_params=None,
+                         action_params=None, update_payload_cbk=None,
+                         retry=False):
+        if retry:
+            # If revision_id of the payload that we send is older than what
+            # NSX has, we will get a 412: Precondition Failed.
+            # In that case we need to re-fetch, patch the response and send
+            # it again with the new revision_id
+            @retry_upon_exception(
+                nsxlib_exceptions.StaleRevision,
+                max_attempts=self.nsxlib_config.max_attempts)
+            def do_update():
+                return self._internal_update_resource(
+                    resource, payload,
+                    headers=headers,
+                    create_action=create_action,
+                    get_params=get_params,
+                    action_params=action_params,
+                    update_payload_cbk=update_payload_cbk)
+
+            return do_update()
+        else:
+            return self._internal_update_resource(
+                resource, payload,
+                headers=headers,
+                create_action=create_action,
+                get_params=get_params,
+                action_params=action_params,
+                update_payload_cbk=update_payload_cbk)
+
+    def _delete_with_retry(self, resource):
+        # Using internal method so we can access max_attempts in the decorator
+        @retry_upon_exception(
+            nsxlib_exceptions.StaleRevision,
+            max_attempts=self.nsxlib_config.max_attempts)
+        def _do_delete():
+            self.client.delete(self.get_path(resource))
+
+        _do_delete()
+
+    def _create_with_retry(self, resource, body=None, headers=None):
+        # Using internal method so we can access max_attempts in the decorator
+        @retry_upon_exception(
+            nsxlib_exceptions.StaleRevision,
+            max_attempts=self.nsxlib_config.max_attempts)
+        def _do_create():
+            return self.client.create(resource, body, headers=headers)
+
+        return _do_create()
 
     def _get_resource_by_name_or_id(self, name_or_id, resource):
         all_results = self.client.list(resource)['results']