roll up of fixes and updates for junos modules (#22543)

* removes cli functions
* adds comment and confirm to arguments
* implements zeroize argument
* fixes get_diff function in junos shared lib to return diff
* lots of minor bug fixes in junos_config
* minor syntax fixes in junos_netconf
* updates netconf integration tests

lib/ansible/module_utils/junos.py

@@ -21,7 +21,7 @@ from contextlib import contextmanager
 from xml.etree.ElementTree import Element, SubElement, tostring
 
 from ansible.module_utils.basic import env_fallback
-from ansible.module_utils.netconf import send_request
+from ansible.module_utils.netconf import send_request, children
 from ansible.module_utils.netconf import discard_changes, validate
 from ansible.module_utils.network_common import to_list
 from ansible.module_utils.connection import exec_command
@@ -91,7 +91,7 @@ def load_configuration(module, candidate=None, action='merge', rollback=None, fo
             cfg.text = '\n'.join(candidate)
         else:
             cfg = SubElement(obj, lookup[format])
-            cfg.append(candidate)
+            cfg.text = '\n'.join(candidate)
 
     return send_request(module, obj)
 
@@ -112,9 +112,11 @@ def commit_configuration(module, confirm=False, check=False, comment=None, confi
     if check:
         SubElement(obj, 'check')
     if comment:
-        children(obj, ('log', str(comment)))
+        subele = SubElement(obj, 'log')
+        subele.text = str(comment)
     if confirm_timeout:
-        children(obj, ('confirm-timeout', int(confirm_timeout)))
+        subele = SubElement(obj, 'confirm-timeout')
+        subele.text = int(confirm_timeout)
     return send_request(module, obj)
 
 def command(module, command, format='text', rpc_only=False):
@@ -138,14 +140,14 @@ def locked_config(module):
 def get_diff(module):
     reply = get_configuration(module, compare=True, format='text')
     output = reply.find('.//configuration-output')
-    if output:
+    if output is not None:
-        return output[0].text
+        return output.text
 
 def load_config(module, candidate, action='merge', commit=False, format='xml',
                 comment=None, confirm=False, confirm_timeout=None):
 
     with locked_config(module):
-        resp = load_configuration(module, candidate, action=action, format=format)
+        reply = load_configuration(module, candidate, action=action, format=format)
 
         validate(module)
         diff = get_diff(module)

lib/ansible/modules/network/junos/junos_config.py

@@ -180,31 +180,24 @@ import json
 from xml.etree import ElementTree
 
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.junos import get_config, get_diff, load_config
+from ansible.module_utils.junos import get_diff, load_config, get_configuration
 from ansible.module_utils.junos import junos_argument_spec
 from ansible.module_utils.junos import check_args as junos_check_args
-from ansible.module_utils.netcfg import NetworkConfig
+from ansible.module_utils.netconf import send_request
 from ansible.module_utils.six import string_types
 
 USE_PERSISTENT_CONNECTION = True
 DEFAULT_COMMENT = 'configured by junos_config'
 
-def check_transport(module):
-    transport = (module.params['provider'] or {}).get('transport')
-
-    if transport == 'netconf':
-        module.fail_json(msg='junos_config module is only supported over cli transport')
-
-
 def check_args(module, warnings):
     junos_check_args(module, warnings)
-    if module.params['zeroize']:
-        module.fail_json(msg='argument zeroize is deprecated and no longer '
-                'supported, use junos_command instead')
 
     if module.params['replace'] is not None:
         module.fail_json(msg='argument replace is deprecated, use update')
 
+zeroize = lambda x: send_request(x, Element('request-system-zeroize'))
+rollback = lambda x: get_diff(x)
+
 def guess_format(config):
     try:
         json.loads(config)
@@ -223,27 +216,15 @@ def guess_format(config):
 
     return 'text'
 
-def config_to_commands(config):
-    set_format = config.startswith('set') or config.startswith('delete')
-    candidate = NetworkConfig(indent=4, contents=config)
-    if not set_format:
-        candidate = [c.line for c in candidate.items]
-        commands = list()
-        # this filters out less specific lines
-        for item in candidate:
-            for index, entry in enumerate(commands):
-                if item.startswith(entry):
-                    del commands[index]
-                    break
-            commands.append(item)
-
-    else:
-        commands = str(candidate).split('\n')
-
-    return commands
-
 def filter_delete_statements(module, candidate):
-    config = get_config(module)
+    reply = get_configuration(module, format='set')
+    match = reply.find('.//configuration-set')
+    if match is None:
+        module.fail_json(msg='unable to retrieve device configuration')
+    config = str(match.text)
+
+    #if 'delete interfaces lo0' in candidate:
+    #    raise ValueError(config)
 
     modified_candidate = candidate[:]
     for index, line in enumerate(candidate):
@@ -251,39 +232,43 @@ def filter_delete_statements(module, candidate):
             newline = re.sub('^delete', 'set', line)
             if newline not in config:
                 del modified_candidate[index]
+
     return modified_candidate
 
-def load(module):
+def configure_device(module):
     candidate = module.params['lines'] or module.params['src']
     if isinstance(candidate, string_types):
         candidate = candidate.split('\n')
 
     kwargs = {
-        'confirm': module.params['confirm'] is not None,
-        'confirm_timeout': module.params['confirm'],
         'comment': module.params['comment'],
-        'commit': not module.check_mode,
+        'commit': not module.check_mode
     }
 
+    if module.params['confirm'] > 0:
+        kwargs.update({
+            'confirm': True,
+            'confirm_timeout': module.params['confirm']
+        })
+
+    config_format = None
+
     if module.params['src']:
         config_format = module.params['src_format'] or guess_format(str(candidate))
-        kwargs.update({'format': config_format, 'action': module.params['update']})
+        if config_format == 'set':
+            kwargs.update({'format': 'text', 'action': 'set'})
+        else:
+            kwargs.update({'format': config_format, 'action': module.params['update']})
 
     # this is done to filter out `delete ...` statements which map to
     # nothing in the config as that will cause an exception to be raised
-    if module.params['lines']:
+    if any((module.params['lines'], config_format == 'set')):
         candidate = filter_delete_statements(module, candidate)
+        kwargs['format'] = 'text'
+        kwargs['action'] = 'set'
 
     return load_config(module, candidate, **kwargs)
 
-def update_result(module, result, diff=None):
-    if diff == '':
-        diff = None
-    result['changed'] = diff is not None
-    if module._diff:
-        result['diff'] = {'prepared': diff}
-
-
 def main():
     """ main entry point for module execution
     """
@@ -306,35 +291,47 @@ def main():
         backup=dict(type='bool', default=False),
         rollback=dict(type='int'),
 
-        # deprecated zeroize in Ansible 2.3
         zeroize=dict(default=False, type='bool'),
     )
 
     argument_spec.update(junos_argument_spec)
 
-    mutually_exclusive = [('lines', 'src', 'rollback')]
+    mutually_exclusive = [('lines', 'src', 'rollback', 'zeroize')]
 
     module = AnsibleModule(argument_spec=argument_spec,
                            mutually_exclusive=mutually_exclusive,
                            supports_check_mode=True)
 
-    check_transport(module)
-
     warnings = list()
     check_args(module, warnings)
 
     result = {'changed': False, 'warnings': warnings}
 
     if module.params['backup']:
-        result['__backup__'] = get_config(module)
+        reply = get_configuration(module, format='set')
+        match = reply.find('.//configuration-set')
+        if match is None:
+            module.fail_json(msg='unable to retrieve device configuration')
+        result['__backup__'] = str(match.text).strip()
 
     if module.params['rollback']:
-        diff = get_diff(module)
+        if not module.check_mode:
-        update_result(module, result, diff)
+            diff = rollback(module)
+            if module._diff:
+                result['diff'] = {'prepared': diff}
+        result['changed'] = True
+
+    elif module.params['zeroize']:
+        if not module.check_mode:
+            zeroize(module)
+        result['changed'] = True
 
     else:
-        diff = load(module)
+        diff = configure_device(module)
-        update_result(module, result, diff)
+        if diff:
+            if module._diff:
+                result['diff'] = {'prepared': diff}
+            result['changed'] = True
 
     module.exit_json(**result)
 

lib/ansible/modules/network/junos/junos_netconf.py

@@ -80,6 +80,7 @@ import re
 from ansible.module_utils.junos import junos_argument_spec, check_args
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.connection import exec_command
+from ansible.module_utils.network_common import to_list
 from ansible.module_utils.six import iteritems
 
 USE_PERSISTENT_CONNECTION = True
@@ -97,10 +98,11 @@ def map_obj_to_commands(updates, module):
     elif want['state'] == 'absent' and have['state'] == 'present':
         commands.append('delete system services netconf')
 
-    elif want['netconf_port'] != have.get('netconf_port'):
+    elif want['state'] == 'present':
-        commands.append(
+        if want['netconf_port'] != have.get('netconf_port'):
-            'set system services netconf ssh port %s' % want['netconf_port']
+            commands.append(
-        )
+                'set system services netconf ssh port %s' % want['netconf_port']
+            )
 
     return commands
 
@@ -110,7 +112,12 @@ def parse_port(config):
         return int(match.group(1))
 
 def map_config_to_obj(module):
-    config = get_config(module, ['system services netconf'])
+    cmd = 'show configuration system services netconf'
+    rc, out, err = exec_command(module, cmd)
+    if rc != 0:
+        module.fail_json(msg='unable to retrieve current config', stderr=err)
+    config = str(out).strip()
+
     obj = {'state': 'absent'}
     if config:
         obj.update({
@@ -138,12 +145,6 @@ def map_params_to_obj(module):
 
     return obj
 
-def get_config(module, flags=[]):
-    rc, out, err = exec_command(module, cmd)
-    if rc != 0:
-        module.fail_json(msg='unable to retrieve current config', stderr=err)
-    return str(out).strip()
-
 def load_config(module, config, commit=False):
 
     exec_command(module, 'configure')

test/integration/targets/junos_config/tests/netconf/src_basic.yaml

@@ -17,8 +17,6 @@
 - assert:
     that:
       - "result.changed == true"
-# https://github.com/ansible/ansible-modules-core/issues/4807
-      - "result.updates is not defined"
 
 - name: check device with config
   junos_config:
@@ -29,7 +27,5 @@
 - assert:
     that:
       - "result.changed == false"
-# https://github.com/ansible/ansible-modules-core/issues/4807
-      - "result.updates is not defined"
 
 - debug: msg="END netconf/src_basic.yaml"

test/integration/targets/junos_netconf/tests/cli/changeport.yaml

@@ -33,17 +33,15 @@
       - "result.changed == false"
 
 - name: Ensure we can communicate over 8080
-  junos_config:
+  junos_command:
-    lines:
+    rpcs: get-software-information
-      - set system host-name {{ inventory_hostname_short }}
     provider: "{{ netconf }}"
     port: 8080
 
 # This protects against the port override above not being honoured and a bug setting the port
 - name: Ensure we can NOT communicate over default port
-  junos_config:
+  junos_command:
-    lines:
+    rpcs: get-software-information
-      - set system host-name {{ inventory_hostname_short }}
     provider: "{{ netconf }}"
   register: result
   ignore_errors: true
@@ -60,9 +58,8 @@
   register: result
 
 - name: Ensure we can communicate over netconf
-  junos_config:
+  junos_command:
-    lines:
+    rpcs: get-software-information
-      - set system host-name {{ inventory_hostname_short }}
     provider: "{{ netconf }}"
 
 - debug: msg="END netconf/changeport.yaml"

test/integration/targets/junos_netconf/tests/cli/netconf.yaml

@@ -21,9 +21,8 @@
 ###################################
 
 - name: Ensure we can communicate over netconf
-  junos_config:
+  junos_command:
-    lines:
+    rpcs: get-software-information
-      - set system host-name {{ inventory_hostname_short }}
     provider: "{{ netconf }}"
 
 # Disable netconf
@@ -48,11 +47,13 @@
     that:
       - "result.changed == false"
 
+- name: wait for persistent socket to timeout
+  pause:
+    seconds: 30
 
 - name: Ensure we can NOT talk via netconf
-  junos_config:
+  junos_command:
-    lines:
+    rpcs: get-software-information
-      - set system host-name {{ inventory_hostname_short }}
     provider: "{{ netconf }}"
   register: result
   ignore_errors: true