Fixed CoPP module states implementation bug (#381)

* Fixed CoPP module states implementation bug

* Add fragment

* Address review comments

* Address review comment

changelogs/fragments/381-fix-copp-states.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - sonic_copp - Fix CoPP states implementation bug (https://github.com/ansible-collections/dellemc.enterprise_sonic/pull/381).

plugins/module_utils/network/sonic/config/copp/copp.py

@@ -14,6 +14,7 @@
 from __future__ import absolute_import, division, print_function
 __metaclass__ = type
 
+from copy import deepcopy
 from ansible_collections.ansible.netcommon.plugins.module_utils.network.common.cfg.base import (
     ConfigBase,
 )
@@ -23,7 +24,6 @@
 from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.utils import (
     get_diff,
     get_replaced_config,
-    send_requests,
     remove_empties,
     update_states,
 )
@@ -114,7 +114,7 @@ def get_copp_facts(self):
         facts, _warnings = Facts(self._module).get_facts(self.gather_subset, self.gather_network_resources)
         copp_facts = facts['ansible_network_resources'].get('copp')
         if not copp_facts:
-            return []
+            return {}
         return copp_facts
 
     def execute_module(self):
@@ -150,6 +150,7 @@ def execute_module(self):
             result.pop('after', None)
             new_config = get_new_config(commands, existing_copp_facts,
                                         TEST_KEYS_generate_config)
+            self.sort_lists_in_config(new_config)
             result['after(generated)'] = new_config
 
         if self._module._diff:
@@ -169,7 +170,7 @@ def set_config(self, existing_copp_facts):
         :returns: the commands necessary to migrate the current configuration
                   to the desired configuration
         """
-        want = self._module.params['config']
+        want = remove_empties(self._module.params['config'])
         have = existing_copp_facts
         resp = self.set_state(want, have)
         return to_list(resp)
@@ -205,28 +206,36 @@ def _state_replaced(self, want, have, diff):
         :returns: the commands necessary to migrate the current configuration
                   to the desired configuration
         """
+        self.validate_want_for_replaced_overridden(want, 'Replaced')
+        commands = []
+        mod_commands = []
+        requests = []
         replaced_config = get_replaced_config(want, have, TEST_KEYS)
 
         if replaced_config:
-            is_delete_all = True
-            requests = self.get_delete_copp_requests(replaced_config, None, is_delete_all)
-            send_requests(self._module, requests)
+            self.sort_lists_in_config(replaced_config)
+            self.sort_lists_in_config(have)
+            is_delete_all = replaced_config == have
 
-            commands = want
-        else:
-            commands = diff
+            # trap_action cannot be deleted
+            copp_groups = replaced_config.get('copp_groups')
+            for group in copp_groups:
+                if 'trap_action' in group and group['trap_action']:
+                    group.pop('trap_action')
 
-        requests = []
+            del_requests = self.get_delete_copp_requests(replaced_config, have, is_delete_all)
+            requests.extend(del_requests)
+            commands.extend(update_states(replaced_config, 'deleted'))
+            mod_commands = want
+        else:
+            mod_commands = diff
 
-        if commands:
-            requests = self.get_modify_copp_groups_request(commands)
+        if mod_commands:
+            mod_request = self.get_modify_copp_groups_request(mod_commands)
 
-            if len(requests) > 0:
-                commands = update_states(commands, "replaced")
-            else:
-                commands = []
-        else:
-            commands = []
+            if mod_request:
+                requests.append(mod_request)
+                commands.extend(update_states(mod_commands, 'replaced'))
 
         return commands, requests
 
@@ -239,27 +248,28 @@ def _state_overridden(self, want, have):
         :returns: the commands necessary to migrate the current configuration
                   to the desired configuration
         """
+        self.validate_want_for_replaced_overridden(want, 'Overridden')
+        commands = []
+        requests = []
         self.sort_lists_in_config(want)
         self.sort_lists_in_config(have)
+        new_have = deepcopy(have)
+        new_have = self.filter_copp_groups(new_have)
 
-        have = self.filter_copp_groups(have)
-        if have and have != want:
+        if new_have and new_have != want:
             is_delete_all = True
-            requests = self.get_delete_copp_requests(have, None, is_delete_all)
-            send_requests(self._module, requests)
-            have = []
-
-        commands = []
-        requests = []
+            del_requests = self.get_delete_copp_requests(new_have, None, is_delete_all)
+            requests.extend(del_requests)
+            commands.extend(update_states(new_have, 'deleted'))
+            new_have = []
 
-        if not have and want:
-            commands = want
-            requests = self.get_modify_copp_groups_request(commands)
+        if not new_have and want:
+            mod_commands = want
+            mod_request = self.get_modify_copp_groups_request(mod_commands)
 
-            if len(requests) > 0:
-                commands = update_states(commands, "overridden")
-            else:
-                commands = []
+            if mod_request:
+                requests.append(mod_request)
+                commands.extend(update_states(mod_commands, 'overridden'))
 
         return commands, requests
 
@@ -286,14 +296,14 @@ def _state_deleted(self, want, have):
                   of the provided objects
         """
         is_delete_all = False
-        # if want is none, then delete ALL
-        want = remove_empties(want)
+
         if not want:
-            commands = have
+            commands = deepcopy(have)
             is_delete_all = True
+            commands = self.filter_copp_groups(commands)
         else:
-            commands = want
-        commands = self.filter_copp_groups(commands)
+            commands = deepcopy(want)
+
         requests = self.get_delete_copp_requests(commands, have, is_delete_all)
         if commands and len(requests) > 0:
             commands = update_states(commands, "deleted")
@@ -354,6 +364,7 @@ def get_delete_copp_requests(self, commands, have, is_delete_all):
         else:
             copp_groups = commands.get('copp_groups', None)
             if copp_groups:
+                copp_list = []
                 for group in copp_groups:
                     copp_name = group.get('copp_name', None)
                     trap_priority = group.get('trap_priority', None)
@@ -374,20 +385,31 @@ def get_delete_copp_requests(self, commands, have, is_delete_all):
                                 cfg_cbs = cfg_group.get('cbs', None)
 
                                 if copp_name == cfg_copp_name:
+                                    copp_dict = {}
                                     if trap_priority and trap_priority == cfg_trap_priority:
                                         requests.append(self.get_delete_copp_groups_attr_request(copp_name, 'trap-priority'))
+                                        copp_dict.update({'copp_name': copp_name, 'trap_priority': trap_priority})
                                     if trap_action and trap_action == cfg_trap_action:
-                                        err_msg = "Deletion of trap-action attribute is not supported."
-                                        self._module.fail_json(msg=err_msg, code=405)
-                                        requests.append(self.get_delete_copp_groups_attr_request(copp_name, 'trap-action'))
+                                        self._module.fail_json(msg='Deletion of trap-action attribute is not supported.')
                                     if queue and queue == cfg_queue:
                                         requests.append(self.get_delete_copp_groups_attr_request(copp_name, 'queue'))
+                                        copp_dict.update({'copp_name': copp_name, 'queue': queue})
                                     if cir and cir == cfg_cir:
                                         requests.append(self.get_delete_copp_groups_attr_request(copp_name, 'cir'))
+                                        copp_dict.update({'copp_name': copp_name, 'cir': cir})
                                     if cbs and cbs == cfg_cbs:
                                         requests.append(self.get_delete_copp_groups_attr_request(copp_name, 'cbs'))
+                                        copp_dict.update({'copp_name': copp_name, 'cbs': cbs})
                                     if not trap_priority and not trap_action and not queue and not cir and not cbs:
                                         requests.append(self.get_delete_single_copp_group_request(copp_name))
+                                        copp_dict['copp_name'] = copp_name
+                                    if copp_dict:
+                                        copp_list.append(copp_dict)
+                                    break
+                if copp_list:
+                    commands['copp_groups'] = copp_list
+                else:
+                    commands = {}
 
         return requests
 
@@ -425,3 +447,12 @@ def get_copp_groups_key(self, copp_groups_key):
     def sort_lists_in_config(self, config):
         if 'copp_groups' in config and config['copp_groups'] is not None:
             config['copp_groups'].sort(key=self.get_copp_groups_key)
+
+    def validate_want_for_replaced_overridden(self, want, state):
+        if want:
+            copp_groups = want.get('copp_groups', None)
+            if copp_groups:
+                for group in copp_groups:
+                    copp_name = group.get('copp_name', None)
+                    if copp_name in reserved_copp_names:
+                        self._module.fail_json(msg=state + ' not supported for reserved CoPP classifier. Use merged and/or deleted state(s).')

plugins/module_utils/network/sonic/facts/copp/copp.py

@@ -18,6 +18,9 @@
 from ansible_collections.ansible.netcommon.plugins.module_utils.network.common import (
     utils,
 )
+from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.utils import (
+    remove_empties
+)
 from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.argspec.copp.copp import CoppArgs
 from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.sonic import (
     to_request,
@@ -56,26 +59,14 @@ def populate_facts(self, connection, ansible_facts, data=None):
         if not data:
             copp_cfg = self.get_copp_config(self._module)
             data = self.update_copp_groups(copp_cfg)
-        objs = self.render_config(self.generated_spec, data)
+        objs = data
         facts = {}
         if objs:
             params = utils.validate_config(self.argument_spec, {'config': objs})
-            facts['copp'] = params['config']
+            facts['copp'] = remove_empties(params['config'])
         ansible_facts['ansible_network_resources'].update(facts)
         return ansible_facts
 
-    def render_config(self, spec, conf):
-        """
-        Render config as dictionary structure and delete keys
-          from spec for null values
-
-        :param spec: The facts tree, generated from the argspec
-        :param conf: The configuration
-        :rtype: dictionary
-        :returns: The generated config
-        """
-        return conf
-
     def update_copp_groups(self, data):
         config_dict = {}
         all_copp_groups = []

plugins/modules/sonic_copp.py

@@ -29,12 +29,6 @@
 from __future__ import absolute_import, division, print_function
 __metaclass__ = type
 
-ANSIBLE_METADATA = {
-    'metadata_version': '1.1',
-    'status': ['preview'],
-    'supported_by': 'community'
-}
-
 DOCUMENTATION = """
 ---
 module: sonic_copp
@@ -255,26 +249,26 @@
 """
 RETURN = """
 before:
-  description: The configuration prior to the model invocation.
+  description: The configuration prior to the module invocation.
   returned: always
   type: list
   sample: >
     The configuration returned will always be in the same format
-    of the parameters above.
+    as the parameters above.
 after:
-  description: The resulting configuration model invocation.
+  description: The resulting configuration module invocation.
   returned: when changed
   type: list
   sample: >
     The configuration returned will always be in the same format
-    of the parameters above.
+    as the parameters above.
 after(generated):
-  description: The generated configuration model invocation.
+  description: The generated configuration module invocation.
   returned: when C(check_mode)
   type: list
   sample: >
     The configuration returned will always be in the same format
-     of the parameters above.
+    as the parameters above.
 commands:
   description: The set of commands pushed to the remote device.
   returned: always

tests/unit/modules/network/sonic/fixtures/sonic_copp.yaml

@@ -136,6 +136,9 @@ overridden_01:
                    cir: '45'
                    cbs: '45'
   expected_config_requests:
+    - path: "/data/openconfig-copp-ext:copp/copp-groups/copp-group=copp-1"
+      method: "delete"
+      data:
     - path: "/data/openconfig-copp-ext:copp/copp-groups"
       method: "patch"
       data:

tests/unit/modules/network/sonic/test_sonic_copp.py

@@ -31,9 +31,6 @@ def setUpClass(cls):
         cls.mock_get_interface_naming_mode = patch(
             "ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.utils.get_device_interface_naming_mode"
         )
-        cls.mock_send_requests = patch(
-            "ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.config.copp.copp.send_requests"
-        )
         cls.fixture_data = cls.load_fixtures('sonic_copp.yaml')
 
     def setUp(self):
@@ -44,8 +41,6 @@ def setUp(self):
         self.config_edit_config.side_effect = self.config_side_effect
         self.get_interface_naming_mode = self.mock_get_interface_naming_mode.start()
         self.get_interface_naming_mode.return_value = 'native'
-        self.send_requests = self.mock_send_requests.start()
-        self.send_requests.return_value = None
         self.utils_edit_config = self.mock_utils_edit_config.start()
         self.utils_edit_config.side_effect = self.facts_side_effect
 
@@ -54,7 +49,6 @@ def tearDown(self):
         self.mock_facts_edit_config.stop()
         self.mock_config_edit_config.stop()
         self.mock_get_interface_naming_mode.stop()
-        self.mock_send_requests.stop()
         self.mock_utils_edit_config.stop()
 
     def test_sonic_copp_merged_01(self):