Fix eos_logging idempotency issue CP into 2.4 (#40933)

* Fixes eos_logging idempotence issue #31862 (#40604)

* eos_logging idempotence fix

* fixed eos_logging idempotence issue

* Fixed pylint and pep8 errors

* Added tests for eos_logging & minor fix

* Removed q statements

(cherry picked from commit b9ea6468398d23c4b9c39f0c91cd79bf0b61af5d)

* Added eos_logging fix in CHANGELOG

3 files changed, 98 insertions, 15 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6b8085c991..11e178d0b5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,9 @@ Ansible Changes By Release
   (https://github.com/ansible/ansible/pull/37964)
 * Fix Windows setup.ps1 for slow performance in large domain environments
   (https://github.com/ansible/ansible/pull/38646)
+* Fix eos_logging idempotency issue when trying to set both logging destination & facility
+  (https://github.com/ansible/ansible/pull/40604)
+
 
 
 <a id="2.4.4"></a>
diff --git a/lib/ansible/modules/network/eos/eos_logging.py b/lib/ansible/modules/network/eos/eos_logging.py
index d769e45cc3..e895e3eb78 100644
--- a/lib/ansible/modules/network/eos/eos_logging.py
+++ b/lib/ansible/modules/network/eos/eos_logging.py
@@ -113,8 +113,8 @@ commands:
 
 import re
 
-from copy import deepcopy
 
+from copy import deepcopy
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.network_common import remove_default_spec
 from ansible.module_utils.eos import get_config, load_config
@@ -149,19 +149,32 @@ def map_obj_to_commands(updates, module):
         del w['state']
 
         if state == 'absent' and w in have:
-            if dest == 'host':
-                commands.append('no logging host {}'.format(name))
-            elif dest:
-                commands.append('no logging {}'.format(dest))
-            else:
-                module.fail_json(msg='dest must be among console, monitor, buffered, host, on')
+            if dest:
+                if dest == 'host':
+                    commands.append('no logging host {}'.format(name))
+
+                elif dest in DEST_GROUP:
+                    commands.append('no logging {}'.format(dest))
+
+                else:
+                    module.fail_json(msg='dest must be among console, monitor, buffered, host, on')
 
             if facility:
                 commands.append('no logging facility {}'.format(facility))
 
         if state == 'present' and w not in have:
             if facility:
-                commands.append('logging facility {}'.format(facility))
+                present = False
+
+                # Iterate over every dictionary in the 'have' list to check if
+                # similar configuration for facility exists or not
+
+                for entry in have:
+                    if not entry['dest'] and entry['facility'] == facility:
+                        present = True
+
+                if not present:
+                    commands.append('logging facility {}'.format(facility))
 
             if dest == 'host':
                 commands.append('logging host {}'.format(name))
@@ -170,7 +183,28 @@ def map_obj_to_commands(updates, module):
                 commands.append('logging on')
 
             elif dest == 'buffered' and size:
-                commands.append('logging buffered {}'.format(size))
+
+                present = False
+
+                # Deals with the following two cases:
+                # Case 1:       logging buffered <size> <level>
+                #               logging buffered <same-size>
+                #
+                # Case 2:       Same buffered logging configuration
+                #               already exists (i.e., both size &
+                #               level are same)
+
+                for entry in have:
+                    if entry['dest'] == 'buffered' and entry['size'] == size:
+
+                        if not level or entry['level'] == level:
+                            present = True
+
+                if not present:
+                    if size and level:
+                        commands.append('logging buffered {} {}'.format(size, level))
+                    else:
+                        commands.append('logging buffered {}'.format(size))
 
             else:
                 dest_cmd = 'logging {}'.format(dest)
@@ -221,11 +255,20 @@ def parse_name(line, dest):
     return name
 
 
-def parse_level(line, dest, module):
+def parse_level(line, dest):
     level = None
 
     if dest is not 'host':
-        match = re.search(r'logging {} (\S+)'.format(dest), line, re.M)
+
+        # Line for buffer logging entry in running-config is of the form:
+        # logging buffered <size> <level>
+
+        if dest == 'buffered':
+            match = re.search(r'logging buffered (?:\d+) (\S+)', line, re.M)
+
+        else:
+            match = re.search(r'logging {} (\S+)'.format(dest), line, re.M)
+
         if match:
             if match.group(1) in LEVEL_GROUP:
                 level = match.group(1)
@@ -239,19 +282,21 @@ def map_config_to_obj(module):
     data = get_config(module, flags=['section logging'])
 
     for line in data.split('\n'):
+
         match = re.search(r'logging (\S+)', line, re.M)
 
         if match:
             if match.group(1) in DEST_GROUP:
                 dest = match.group(1)
+
             else:
-                pass
+                dest = None
 
             obj.append({'dest': dest,
                         'name': parse_name(line, dest),
                         'size': parse_size(line, dest),
                         'facility': parse_facility(line),
-                        'level': parse_level(line, dest, module)})
+                        'level': parse_level(line, dest)})
 
     return obj
 
@@ -360,8 +405,8 @@ def main():
     if warnings:
         result['warnings'] = warnings
 
-    want = map_params_to_obj(module, required_if=required_if)
     have = map_config_to_obj(module)
+    want = map_params_to_obj(module, required_if=required_if)
 
     commands = map_obj_to_commands((want, have), module)
     result['commands'] = commands
@@ -376,5 +421,6 @@ def main():
 
     module.exit_json(**result)
 
+
 if __name__ == '__main__':
     main()
diff --git a/test/integration/targets/eos_logging/tests/cli/basic.yaml b/test/integration/targets/eos_logging/tests/cli/basic.yaml
index 8a0b7fa156..2cd06cd6b9 100644
--- a/test/integration/targets/eos_logging/tests/cli/basic.yaml
+++ b/test/integration/targets/eos_logging/tests/cli/basic.yaml
@@ -67,6 +67,7 @@
       - 'result.changed == true'
       - '"logging console warnings" in result.commands'
 
+
 - name: Configure buffer size
   eos_logging:
     dest: buffered
@@ -80,13 +81,45 @@
       - 'result.changed == true'
       - '"logging buffered 480000" in result.commands'
 
+- name: Set up logging destination and facility at the same time
+  eos_logging:
+    dest: buffered
+    size: 4096
+    facility: local7
+    level: informational
+    state: present
+  become: yes
+  register: result
+
+- assert:
+    that:
+      - 'result.changed == true'
+      - '"logging buffered 4096 informational" in result.commands'
+      - '"logging facility local7" in result.commands'
+
+- name: Set up logging destination and facility at the same time again (idempotent)
+  eos_logging:
+    dest: buffered
+    size: 4096
+    facility: local7
+    level: informational
+    state: present
+  become: yes
+  register: result
+
+- assert:
+    that:
+      - 'result.changed == false'
+
 - name: remove logging as collection tearDown
   eos_logging:
     aggregate:
       - { dest: console, level: warnings, state: absent }
-      - { dest: buffered, size: 480000, state: absent }
+      - { dest: buffered, level: informational, size: 4096, state: absent }
+      - { facility: local7, state: absent }
     authorize: yes
     provider: "{{ cli }}"
+  become: yes
   register: result
 
 - assert:
@@ -94,3 +127,4 @@
       - 'result.changed == true'
       - '"no logging console" in result.commands'
       - '"no logging buffered" in result.commands'
+      - '"no logging facility local7" in result.commands'