Repository: ansible/ansible
Pull request: ios_logging: fix idempotence errors
##### SUMMARY
Fixes #53226
Fixes #53227

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
ios_logging


Problem statement:
ios_logging: the module performs the configuration, even if not required (destination 'host')
##### SUMMARY
The module performs the configuration 'host' destination, even if not required.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
ios_logging

##### ANSIBLE VERSION
```
ansible 2.8.0.dev0
  config file = /home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg
  configured module search path = [u'/home/administrator/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/administrator/ansible/ansible/lib/ansible
  executable location = /home/administrator/ansible/ansible/bin/ansible
  python version = 2.7.13 (default, Sep 26 2018, 18:42:22) [GCC 6.3.0 20170516]
```

##### CONFIGURATION
```
DEFAULT_TRANSPORT(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = paramiko
PARAMIKO_HOST_KEY_AUTO_ADD(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = True
PARAMIKO_LOOK_FOR_KEYS(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = True
PERSISTENT_COMMAND_TIMEOUT(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = 60
RETRY_FILES_ENABLED(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = False

```

##### OS / ENVIRONMENT
debian 9.8
cisco ios 12

##### STEPS TO REPRODUCE
cisco config:
```
test-sw1#sh run | i logging
no logging buffered
no logging console
no logging monitor
```

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: all
  gather_facts: no
  tasks:
  - name: configure syslog servers
    ios_logging:
     dest: host
     name: "192.168.1.1"

  - name: configure syslog servers (idempotent)
    register: result
    ios_logging:
     dest: host
     name: "192.168.1.1"

  - name: test
    assert:
     that:
     - "result.changed == false"
```
##### EXPECTED RESULTS
The task "configure syslog servers" should have the status "changed",
The task "configure syslog servers (idempotent)" should have the status "ok"
The task "test" should have the status "ok".

##### ACTUAL RESULTS
```
PLAY [all] ****************************************************************************************************

TASK [configure syslog servers] *******************************************************************************
changed: [test-sw1] => {"changed": true, "commands": ["logging 192.168.1.1"]}

TASK [configure syslog servers (idempotent)] ******************************************************************
changed: [test-sw1] => {"changed": true, "commands": ["logging 192.168.1.1"]}

TASK [test] ***************************************************************************************************
fatal: [test-sw1]: FAILED! => {
    "assertion": "result.changed == false ", 
    "changed": false, 
    "evaluated_to": false, 
    "msg": "Assertion failed"
}

PLAY RECAP ****************************************************************************************************
test-sw1               : ok=2    changed=2    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   
```


ios_logging: The module does not perform configuration
##### SUMMARY
The module does not perform configuration if the logging destination is disabled in the configuration and the required logging level is 'debugging'. Relevant for 'dest types: console, monitor, buffered (if the playbook parameter 'size' is set to 4096).

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
ios_logging

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
ansible 2.8.0.dev0
  config file = /home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg
  configured module search path = [u'/home/administrator/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/administrator/ansible/ansible/lib/ansible
  executable location = /home/administrator/ansible/ansible/bin/ansible
  python version = 2.7.13 (default, Sep 26 2018, 18:42:22) [GCC 6.3.0 20170516]

```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```
DEFAULT_TRANSPORT(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = paramiko
PARAMIKO_HOST_KEY_AUTO_ADD(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = True
PARAMIKO_LOOK_FOR_KEYS(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = True
PERSISTENT_COMMAND_TIMEOUT(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = 60
RETRY_FILES_ENABLED(/home/administrator/ansible/playbooks/ios_logging_test/ansible.cfg) = False

```

##### OS / ENVIRONMENT
debian 9.8
cisco ios 12, 15

##### STEPS TO REPRODUCE
cisco config:
```
test-sw1#sh run | i logging
no logging buffered
no logging console
no logging monitor
```
<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: all
  gather_facts: no
  tasks:
  - name: configure console logging
    register: result
    ios_logging:
     dest: monitor
     level: console

  - name: test
    assert:
     that:
     - "'logging console debugging' in result.commands"
```
##### EXPECTED RESULTS
module should send command "'logging console debugging'"

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```
PLAY [all] ****************************************************************************************************

TASK [configure console logging] ******************************************************************************
ok: [test-sw1] => {"changed": false, "commands": []}

TASK [test] ***************************************************************************************************
fatal: [test-sw1]: FAILED! => {
    "assertion": "'logging console debugging' in result.commands", 
    "changed": false, 
    "evaluated_to": false, 
    "msg": "Assertion failed"
}

PLAY RECAP ****************************************************************************************************
test-sw1               : ok=1    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/network/ios/ios_logging.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/ios/ios_logging.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
@hexdump0x0200, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in `#ansible-network` on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: community_workgroups --->
Files identified in the description:
* [lib/ansible/modules/network/ios/ios_logging.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/ios/ios_logging.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
@hexdump0x0200, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in `#ansible-network` on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: community_workgroups --->

Commit message: ios_logging: fixes #53227

Patch to review:
diff --git a/lib/ansible/modules/network/ios/ios_logging.py b/lib/ansible/modules/network/ios/ios_logging.py
index 16211dcc11680d..ad19833c6a68ca 100644
--- a/lib/ansible/modules/network/ios/ios_logging.py
+++ b/lib/ansible/modules/network/ios/ios_logging.py
@@ -274,6 +274,8 @@ def map_config_to_obj(module):
     data = get_config(module, flags=['| include logging'])
 
     for line in data.split('\n'):
+        if line.startswith("no "):  # 'no logging console' or similar
+            continue  # we do not have this setting, it is disabled
         match = re.search(r'logging (\S+)', line, re.M)
         if match:
             if match.group(1) in dest_group:
@@ -291,6 +293,7 @@ def map_config_to_obj(module):
                 obj.append({
                     'dest': dest,
                     'name': match.group(1),
+                    'size': parse_size(line, dest),
                     'facility': parse_facility(line, dest),
                     'level': parse_level(line, dest)
                 })
@@ -301,6 +304,7 @@ def map_config_to_obj(module):
                     obj.append({
                         'dest': dest,
                         'name': match.group(1),
+                        'size': parse_size(line, dest),
                         'facility': parse_facility(line, dest),
                         'level': parse_level(line, dest)
                     })
diff --git a/test/units/modules/network/ios/fixtures/ios_logging_config.cfg b/test/units/modules/network/ios/fixtures/ios_logging_config.cfg
index 88ea91035230a8..8a51afa7e5c6db 100644
--- a/test/units/modules/network/ios/fixtures/ios_logging_config.cfg
+++ b/test/units/modules/network/ios/fixtures/ios_logging_config.cfg
@@ -2,6 +2,7 @@
 logging buffered 5000
 logging console informational
 logging facility local0
+no logging monitor
 logging host 1.2.3.4 transport tcp
 logging host 1.2.3.4
 logging host 2.3.4.5
diff --git a/test/units/modules/network/ios/fixtures/ios_logging_config_ios12.cfg b/test/units/modules/network/ios/fixtures/ios_logging_config_ios12.cfg
new file mode 100644
index 00000000000000..58be36dff5b6a1
--- /dev/null
+++ b/test/units/modules/network/ios/fixtures/ios_logging_config_ios12.cfg
@@ -0,0 +1,6 @@
+!
+logging buffered 5000
+logging console informational
+logging facility local0
+logging 2.3.4.5
+!
diff --git a/test/units/modules/network/ios/test_ios_logging.py b/test/units/modules/network/ios/test_ios_logging.py
index 2031302b88d6b7..503adc9426ece4 100644
--- a/test/units/modules/network/ios/test_ios_logging.py
+++ b/test/units/modules/network/ios/test_ios_logging.py
@@ -66,3 +66,77 @@ def test_ios_logging_buffer_size_changed_explicit(self):
         set_module_args(dict(dest='buffered', size=6000))
         commands = ['logging buffered 6000']
         self.execute_module(changed=True, commands=commands)
+
+    def test_ios_logging_add_host(self):
+        set_module_args(dict(dest='host', name='192.168.1.1'))
+        commands = ['logging host 192.168.1.1']
+        self.execute_module(changed=True, commands=commands)
+
+    def test_ios_logging_host_idempotent(self):
+        set_module_args(dict(dest='host', name='2.3.4.5'))
+        commands = []
+        self.execute_module(changed=False, commands=commands)
+
+    def test_ios_logging_delete_non_exist_host(self):
+        set_module_args(dict(dest='host', name='192.168.1.1', state='absent'))
+        commands = []
+        self.execute_module(changed=False, commands=commands)
+
+    def test_ios_logging_delete_host(self):
+        set_module_args(dict(dest='host', name='2.3.4.5', state='absent'))
+        commands = ['no logging host 2.3.4.5']
+        self.execute_module(changed=True, commands=commands)
+
+    def test_ios_logging_configure_disabled_monitor_destination(self):
+        set_module_args(dict(dest='monitor', level='debugging'))
+        commands = ['logging monitor debugging']
+        self.execute_module(changed=True, commands=commands)
+
+
+class TestIosLoggingModuleIOS12(TestIosModule):
+
+    module = ios_logging
+
+    def setUp(self):
+        super(TestIosLoggingModuleIOS12, self).setUp()
+
+        self.mock_get_config = patch('ansible.modules.network.ios.ios_logging.get_config')
+        self.get_config = self.mock_get_config.start()
+
+        self.mock_load_config = patch('ansible.modules.network.ios.ios_logging.load_config')
+        self.load_config = self.mock_load_config.start()
+
+        self.mock_get_capabilities = patch('ansible.modules.network.ios.ios_logging.get_capabilities')
+        self.get_capabilities = self.mock_get_capabilities.start()
+        self.get_capabilities.return_value = {'device_info': {'network_os_version': '12.1(2)T'}}
+
+    def tearDown(self):
+        super(TestIosLoggingModuleIOS12, self).tearDown()
+
+        self.mock_get_config.stop()
+        self.mock_load_config.stop()
+        self.mock_get_capabilities.stop()
+
+    def load_fixtures(self, commands=None):
+        self.get_config.return_value = load_fixture('ios_logging_config_ios12.cfg')
+        self.load_config.return_value = None
+
+    def test_ios_logging_add_host(self):
+        set_module_args(dict(dest='host', name='192.168.1.1'))
+        commands = ['logging 192.168.1.1']
+        self.execute_module(changed=True, commands=commands)
+
+    def test_ios_logging_host_idempotent(self):
+        set_module_args(dict(dest='host', name='2.3.4.5'))
+        commands = []
+        self.execute_module(changed=False, commands=commands)
+
+    def test_ios_logging_delete_non_exist_host(self):
+        set_module_args(dict(dest='host', name='192.168.1.1', state='absent'))
+        commands = []
+        self.execute_module(changed=False, commands=commands)
+
+    def test_ios_logging_delete_host(self):
+        set_module_args(dict(dest='host', name='2.3.4.5', state='absent'))
+        commands = ['no logging 2.3.4.5']
+        self.execute_module(changed=True, commands=commands)


Code review comments:
diff_hunk: @@ -274,6 +274,8 @@ def map_config_to_obj(module):
     data = get_config(module, flags=['| include logging'])
 
     for line in data.split('\n'):
+        if line.startswith("no "):  # 'no logging console' or similar
path: lib/ansible/modules/network/ios/ios_logging.py
text: @user1:
This can be replaced with modifying the regex in L279 to '^logging (\S+)', i.e., fetch lines that strictly start with 'logging'.

@author:
Done.