Refactored bigip_device_dns (#32483)

Module was using old coding standards. This updates the module

lib/ansible/modules/network/f5/bigip_device_dns.py

@@ -4,11 +4,15 @@
 # Copyright (c) 2017 F5 Networks Inc.
 # GNU General Public License v3.0 (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+
 ANSIBLE_METADATA = {'metadata_version': '1.1',
                     'status': ['preview'],
                     'supported_by': 'community'}
 
-DOCUMENTATION = '''
+DOCUMENTATION = r'''
 ---
 module: bigip_device_dns
 short_description: Manage BIG-IP device DNS settings
@@ -22,17 +26,17 @@ options:
         operation each time a lookup is needed. Please note that this applies
         only to Access Policy Manager features, such as ACLs, web application
         rewrites, and authentication.
-    required: false
     default: disable
     choices:
-       - enable
-       - disable
+       - enabled
+       - disabled
   name_servers:
     description:
-      - A list of name serverz that the system uses to validate DNS lookups
+      - A list of name servers that the system uses to validate DNS lookups
   forwarders:
     description:
       - A list of BIND servers that the system can use to perform DNS lookups
+      - Deprecated in 2.4. Use the GUI or edit named.conf.
   search:
     description:
       - A list of domains that the system searches for local domain lookups,
@@ -40,7 +44,6 @@ options:
   ip_version:
     description:
       - Specifies whether the DNS specifies IP addresses using IPv4 or IPv6.
-    required: false
     choices:
       - 4
       - 6
@@ -48,14 +51,13 @@ options:
     description:
       - The state of the variable on the system. When C(present), guarantees
         that an existing variable is set to C(value).
-    required: false
     default: present
     choices:
       - absent
       - present
 notes:
   - Requires the f5-sdk Python package on the host. This is as easy as pip
-    install requests
+    install f5-sdk.
 extends_documentation_fragment: f5
 requirements:
   - f5-sdk
@@ -63,327 +65,326 @@ author:
   - Tim Rupp (@caphrim007)
 '''
 
-EXAMPLES = '''
+EXAMPLES = r'''
 - name: Set the DNS settings on the BIG-IP
   bigip_device_dns:
-      name_servers:
-          - 208.67.222.222
-          - 208.67.220.220
-      search:
-          - localdomain
-          - lab.local
-      state: present
-      password: "secret"
-      server: "lb.mydomain.com"
-      user: "admin"
-      validate_certs: "no"
+    name_servers:
+      - 208.67.222.222
+      - 208.67.220.220
+    search:
+      - localdomain
+      - lab.local
+    password: secret
+    server: lb.mydomain.com
+    user: admin
+    validate_certs: no
   delegate_to: localhost
 '''
 
-RETURN = '''
+RETURN = r'''
 cache:
-    description: The new value of the DNS caching
-    returned: changed
-    type: string
-    sample: "enabled"
+  description: The new value of the DNS caching
+  returned: changed
+  type: string
+  sample: enabled
 name_servers:
-    description: List of name servers that were added or removed
-    returned: changed
-    type: list
-    sample: "['192.0.2.10', '172.17.12.10']"
-forwarders:
-    description: List of forwarders that were added or removed
-    returned: changed
-    type: list
-    sample: "['192.0.2.10', '172.17.12.10']"
+  description: List of name servers that were set
+  returned: changed
+  type: list
+  sample: ['192.0.2.10', '172.17.12.10']
 search:
-    description: List of search domains that were added or removed
-    returned: changed
-    type: list
-    sample: "['192.0.2.10', '172.17.12.10']"
+  description: List of search domains that were set
+  returned: changed
+  type: list
+  sample: ['192.0.2.10', '172.17.12.10']
 ip_version:
-    description: IP version that was set that DNS will specify IP addresses in
-    returned: changed
-    type: int
-    sample: 4
+  description: IP version that was set that DNS will specify IP addresses in
+  returned: changed
+  type: int
+  sample: 4
+warnings:
+  description: The list of warnings (if any) generated by module based on arguments
+  returned: always
+  type: list
+  sample: ['...', '...']
 '''
 
+from ansible.module_utils.f5_utils import AnsibleF5Client
+from ansible.module_utils.f5_utils import AnsibleF5Parameters
+from ansible.module_utils.f5_utils import HAS_F5SDK
+from ansible.module_utils.f5_utils import F5ModuleError
+
 try:
-    from f5.bigip.contexts import TransactionContextManager
-    from f5.bigip import ManagementRoot
-    HAS_F5SDK = True
+    from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
 except ImportError:
     HAS_F5SDK = False
 
 
-REQUIRED = ['name_servers', 'search', 'forwarders', 'ip_version', 'cache']
-CACHE = ['disable', 'enable']
-IP = [4, 6]
+class Parameters(AnsibleF5Parameters):
+    api_map = {
+        'dhclient.mgmt': 'dhcp',
+        'dns.cache': 'cache',
+        'nameServers': 'name_servers',
+        'include': 'ip_version'
+    }
 
+    api_attributes = [
+        'nameServers', 'search', 'include'
+    ]
 
-class BigIpDeviceDns(object):
-    def __init__(self, *args, **kwargs):
-        if not HAS_F5SDK:
-            raise F5ModuleError("The python f5-sdk module is required")
+    updatables = [
+        'cache', 'name_servers', 'search', 'ip_version'
+    ]
 
-        # The params that change in the module
-        self.cparams = dict()
+    returnables = [
+        'cache', 'name_servers', 'search', 'ip_version'
+    ]
 
-        # Stores the params that are sent to the module
-        self.params = kwargs
-        self.api = ManagementRoot(kwargs['server'],
-                                  kwargs['user'],
-                                  kwargs['password'],
-                                  port=kwargs['server_port'])
+    absentables = [
+        'name_servers', 'search'
+    ]
 
-    def flush(self):
-        result = dict()
-        changed = False
-        state = self.params['state']
+    def to_return(self):
+        result = {}
+        for returnable in self.returnables:
+            result[returnable] = getattr(self, returnable)
+        result = self._filter_params(result)
+        return result
 
-        if self.dhcp_enabled():
+    def api_params(self):
+        result = {}
+        for api_attribute in self.api_attributes:
+            if self.api_map is not None and api_attribute in self.api_map:
+                result[api_attribute] = getattr(self, self.api_map[api_attribute])
+            else:
+                result[api_attribute] = getattr(self, api_attribute)
+        result = self._filter_params(result)
+        return result
+
+    @property
+    def search(self):
+        result = []
+        if self._values['search'] is None:
+            return None
+        for server in self._values['search']:
+            result.append(str(server))
+        return result
+
+    @property
+    def name_servers(self):
+        result = []
+        if self._values['name_servers'] is None:
+            return None
+        for server in self._values['name_servers']:
+            result.append(str(server))
+        return result
+
+    @property
+    def cache(self):
+        if str(self._values['cache']) in ['enabled', 'enable']:
+            return 'enable'
+        else:
+            return 'disable'
+
+    @property
+    def dhcp(self):
+        valid = ['enable', 'enabled']
+        return True if self._values['dhcp'] in valid else False
+
+    @property
+    def forwarders(self):
+        if self._values['forwarders'] is None:
+            return None
+        else:
             raise F5ModuleError(
-                "DHCP on the mgmt interface must be disabled to make use of " +
-                "this module"
+                "The modifying of forwarders is not supported."
             )
 
-        if state == 'absent':
-            changed = self.absent()
+    @property
+    def ip_version(self):
+        if self._values['ip_version'] in [6, '6', 'options inet6']:
+            return "options inet6"
+        elif self._values['ip_version'] in [4, '4', '']:
+            return ""
         else:
-            changed = self.present()
+            return None
 
-        result.update(**self.cparams)
+
+class ModuleManager(object):
+    def __init__(self, client):
+        self.client = client
+        self.have = None
+        self.want = Parameters(self.client.module.params)
+        self.changes = Parameters()
+
+    def _update_changed_options(self):
+        changed = {}
+        for key in Parameters.updatables:
+            if getattr(self.want, key) is not None:
+                attr1 = getattr(self.want, key)
+                attr2 = getattr(self.have, key)
+                if attr1 != attr2:
+                    changed[key] = attr1
+        if changed:
+            self.changes = Parameters(changed)
+            return True
+        return False
+
+    def exec_module(self):
+        changed = False
+        result = dict()
+        state = self.want.state
+
+        try:
+            if state == "present":
+                changed = self.update()
+            elif state == "absent":
+                changed = self.absent()
+        except iControlUnexpectedHTTPError as e:
+            raise F5ModuleError(str(e))
+
+        changes = self.changes.to_return()
+        result.update(**changes)
         result.update(dict(changed=changed))
         return result
 
-    def dhcp_enabled(self):
-        r = self.api.tm.sys.dbs.db.load(name='dhclient.mgmt')
-        if r.value == 'enable':
-            return True
-        else:
-            return False
-
-    def read(self):
+    def read_current_from_device(self):
+        want_keys = ['dns.cache']
         result = dict()
+        dbs = self.client.api.tm.sys.dbs.get_collection()
+        for db in dbs:
+            if db.name in want_keys:
+                result[db.name] = db.value
+        dns = self.client.api.tm.sys.dns.load()
+        attrs = dns.attrs
+        if 'include' not in attrs:
+            attrs['include'] = 4
+        result.update(attrs)
+        return Parameters(result)
 
-        cache = self.api.tm.sys.dbs.db.load(name='dns.cache')
-        proxy = self.api.tm.sys.dbs.db.load(name='dns.proxy.__iter__')
-        dns = self.api.tm.sys.dns.load()
-
-        result['cache'] = str(cache.value)
-        result['forwarders'] = str(proxy.value).split(' ')
-
-        if hasattr(dns, 'nameServers'):
-            result['name_servers'] = dns.nameServers
-        if hasattr(dns, 'search'):
-            result['search'] = dns.search
-        if hasattr(dns, 'include') and 'options inet6' in dns.include:
-            result['ip_version'] = 6
-        else:
-            result['ip_version'] = 4
-        return result
-
-    def present(self):
-        params = dict()
-        current = self.read()
-
-        # Temporary locations to hold the changed params
-        update = dict(
-            dns=None,
-            forwarders=None,
-            cache=None
-        )
-
-        nameservers = self.params['name_servers']
-        search_domains = self.params['search']
-        ip_version = self.params['ip_version']
-        forwarders = self.params['forwarders']
-        cache = self.params['cache']
-        check_mode = self.params['check_mode']
-
-        if nameservers:
-            if 'name_servers' in current:
-                if nameservers != current['name_servers']:
-                    params['nameServers'] = nameservers
-            else:
-                params['nameServers'] = nameservers
-
-        if search_domains:
-            if 'search' in current:
-                if search_domains != current['search']:
-                    params['search'] = search_domains
-            else:
-                params['search'] = search_domains
-
-        if ip_version:
-            if 'ip_version' in current:
-                if ip_version != int(current['ip_version']):
-                    if ip_version == 6:
-                        params['include'] = 'options inet6'
-                    elif ip_version == 4:
-                        params['include'] = ''
-            else:
-                if ip_version == 6:
-                    params['include'] = 'options inet6'
-                elif ip_version == 4:
-                    params['include'] = ''
-
-        if params:
-            self.cparams.update(camel_dict_to_snake_dict(params))
-
-            if 'include' in params:
-                del self.cparams['include']
-                if params['include'] == '':
-                    self.cparams['ip_version'] = 4
-                else:
-                    self.cparams['ip_version'] = 6
-
-            update['dns'] = params.copy()
-            params = dict()
-
-        if forwarders:
-            if 'forwarders' in current:
-                if forwarders != current['forwarders']:
-                    params['forwarders'] = forwarders
-            else:
-                params['forwarders'] = forwarders
-
-        if params:
-            self.cparams.update(camel_dict_to_snake_dict(params))
-            update['forwarders'] = ' '.join(params['forwarders'])
-            params = dict()
-
-        if cache:
-            if 'cache' in current:
-                if cache != current['cache']:
-                    params['cache'] = cache
-
-        if params:
-            self.cparams.update(camel_dict_to_snake_dict(params))
-            update['cache'] = params['cache']
-            params = dict()
-
-        if self.cparams:
-            changed = True
-            if check_mode:
-                return changed
-        else:
+    def update(self):
+        self.have = self.read_current_from_device()
+        if not self.should_update():
             return False
+        if self.client.check_mode:
+            return True
+        self.update_on_device()
+        return True
 
-        tx = self.api.tm.transactions.transaction
-        with TransactionContextManager(tx) as api:
-            cache = api.tm.sys.dbs.db.load(name='dns.cache')
-            proxy = api.tm.sys.dbs.db.load(name='dns.proxy.__iter__')
-            dns = api.tm.sys.dns.load()
+    def should_update(self):
+        result = self._update_changed_options()
+        if result:
+            return True
+        return False
 
-            # Empty values can be supplied, but you cannot supply the
-            # None value, so we check for that specifically
-            if update['cache'] is not None:
-                cache.update(value=update['cache'])
-            if update['forwarders'] is not None:
-                proxy.update(value=update['forwarders'])
-            if update['dns'] is not None:
-                dns.update(**update['dns'])
-        return changed
+    def update_on_device(self):
+        params = self.want.api_params()
+        cache = self.client.api.tm.sys.dbs.db.load(name='dns.cache')
+        dns = self.client.api.tm.sys.dns.load()
+
+        # Empty values can be supplied, but you cannot supply the
+        # None value, so we check for that specifically
+        if self.want.cache is not None:
+            cache.update(value=self.want.cache)
+        if params:
+            dns.update(**params)
+
+    def _absent_changed_options(self):
+        changed = {}
+        for key in Parameters.absentables:
+            if getattr(self.want, key) is not None:
+                set_want = set(getattr(self.want, key))
+                set_have = set(getattr(self.have, key))
+                set_new = set_have - set_want
+                if set_new != set_have:
+                    changed[key] = list(set_new)
+        if changed:
+            self.changes = Parameters(changed)
+            return True
+        return False
+
+    def should_absent(self):
+        result = self._absent_changed_options()
+        if result:
+            return True
+        return False
 
     def absent(self):
-        params = dict()
-        current = self.read()
-
-        # Temporary locations to hold the changed params
-        update = dict(
-            dns=None,
-            forwarders=None
-        )
-
-        nameservers = self.params['name_servers']
-        search_domains = self.params['search']
-        forwarders = self.params['forwarders']
-        check_mode = self.params['check_mode']
-
-        if forwarders and 'forwarders' in current:
-            set_current = set(current['forwarders'])
-            set_new = set(forwarders)
-
-            forwarders = set_current - set_new
-            if forwarders != set_current:
-                forwarders = list(forwarders)
-                params['forwarders'] = ' '.join(forwarders)
-
-        if params:
-            changed = True
-            self.cparams.update(camel_dict_to_snake_dict(params))
-            update['forwarders'] = params['forwarders']
-            params = dict()
-
-        if nameservers and 'name_servers' in current:
-            set_current = set(current['name_servers'])
-            set_new = set(nameservers)
-
-            nameservers = set_current - set_new
-            if nameservers != set_current:
-                params['nameServers'] = list(nameservers)
-
-        if search_domains and 'search' in current:
-            set_current = set(current['search'])
-            set_new = set(search_domains)
-
-            search_domains = set_current - set_new
-            if search_domains != set_current:
-                params['search'] = list(search_domains)
-
-        if params:
-            changed = True
-            self.cparams.update(camel_dict_to_snake_dict(params))
-            update['dns'] = params.copy()
-            params = dict()
-
-        if not self.cparams:
+        self.have = self.read_current_from_device()
+        if not self.should_absent():
             return False
+        if self.client.check_mode:
+            return True
+        self.absent_on_device()
+        return True
 
-        if check_mode:
-            return changed
+    def absent_on_device(self):
+        params = self.changes.api_params()
+        resource = self.client.api.tm.sys.dns.load()
+        resource.update(**params)
 
-        tx = self.api.tm.transactions.transaction
-        with TransactionContextManager(tx) as api:
-            proxy = api.tm.sys.dbs.db.load(name='dns.proxy.__iter__')
-            dns = api.tm.sys.dns.load()
 
-            if update['forwarders'] is not None:
-                proxy.update(value=update['forwarders'])
-            if update['dns'] is not None:
-                dns.update(**update['dns'])
-        return changed
+class ArgumentSpec(object):
+    def __init__(self):
+        self.supports_check_mode = True
+        self.argument_spec = dict(
+            cache=dict(
+                required=False,
+                choices=['disabled', 'enabled', 'disable', 'enable'],
+                default=None
+            ),
+            name_servers=dict(
+                required=False,
+                default=None,
+                type='list'
+            ),
+            forwarders=dict(
+                required=False,
+                default=None,
+                type='list'
+            ),
+            search=dict(
+                required=False,
+                default=None,
+                type='list'
+            ),
+            ip_version=dict(
+                required=False,
+                default=None,
+                choices=[4, 6],
+                type='int'
+            ),
+            state=dict(
+                required=False,
+                default='present',
+                choices=['absent', 'present']
+            )
+        )
+        self.required_one_of = [
+            ['name_servers', 'search', 'forwarders', 'ip_version', 'cache']
+        ]
+        self.f5_product_name = 'bigip'
 
 
 def main():
-    argument_spec = f5_argument_spec()
+    if not HAS_F5SDK:
+        raise F5ModuleError("The python f5-sdk module is required")
 
-    meta_args = dict(
-        cache=dict(required=False, choices=CACHE, default=None),
-        name_servers=dict(required=False, default=None, type='list'),
-        forwarders=dict(required=False, default=None, type='list'),
-        search=dict(required=False, default=None, type='list'),
-        ip_version=dict(required=False, default=None, choices=IP, type='int')
-    )
-    argument_spec.update(meta_args)
-    module = AnsibleModule(
-        argument_spec=argument_spec,
-        required_one_of=[REQUIRED],
-        supports_check_mode=True
+    spec = ArgumentSpec()
+
+    client = AnsibleF5Client(
+        argument_spec=spec.argument_spec,
+        supports_check_mode=spec.supports_check_mode,
+        f5_product_name=spec.f5_product_name,
+        required_one_of=spec.required_one_of
     )
 
     try:
-        obj = BigIpDeviceDns(check_mode=module.check_mode, **module.params)
-        result = obj.flush()
-
-        module.exit_json(**result)
+        mm = ModuleManager(client)
+        results = mm.exec_module()
+        client.module.exit_json(**results)
     except F5ModuleError as e:
-        module.fail_json(msg=str(e))
-
-from ansible.module_utils.basic import *
-from ansible.module_utils.ec2 import camel_dict_to_snake_dict
-from ansible.module_utils.f5_utils import *
+        client.module.fail_json(msg=str(e))
 
 if __name__ == '__main__':
     main()

test/units/modules/network/f5/test_bigip_device_dns.py

@@ -0,0 +1,143 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (c) 2017 F5 Networks Inc.
+# GNU General Public License v3.0 (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import os
+import json
+import pytest
+import sys
+
+from nose.plugins.skip import SkipTest
+if sys.version_info < (2, 7):
+    raise SkipTest("F5 Ansible modules require Python >= 2.7")
+
+from ansible.compat.tests import unittest
+from ansible.compat.tests.mock import patch, Mock
+from ansible.module_utils import basic
+from ansible.module_utils._text import to_bytes
+from ansible.module_utils.f5_utils import AnsibleF5Client
+from ansible.module_utils.f5_utils import F5ModuleError
+
+try:
+    from library.bigip_device_dns import Parameters
+    from library.bigip_device_dns import ModuleManager
+    from library.bigip_device_dns import ArgumentSpec
+    from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
+except ImportError:
+    try:
+        from ansible.modules.network.f5.bigip_device_dns import Parameters
+        from ansible.modules.network.f5.bigip_device_dns import ModuleManager
+        from ansible.modules.network.f5.bigip_device_dns import ArgumentSpec
+        from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
+    except ImportError:
+        raise SkipTest("F5 Ansible modules require the f5-sdk Python library")
+
+fixture_path = os.path.join(os.path.dirname(__file__), 'fixtures')
+fixture_data = {}
+
+
+def set_module_args(args):
+    args = json.dumps({'ANSIBLE_MODULE_ARGS': args})
+    basic._ANSIBLE_ARGS = to_bytes(args)
+
+
+def load_fixture(name):
+    path = os.path.join(fixture_path, name)
+
+    if path in fixture_data:
+        return fixture_data[path]
+
+    with open(path) as f:
+        data = f.read()
+
+    try:
+        data = json.loads(data)
+    except Exception:
+        pass
+
+    fixture_data[path] = data
+    return data
+
+
+class TestParameters(unittest.TestCase):
+    def test_module_parameters(self):
+        args = dict(
+            cache='disable',
+            forwarders=['12.12.12.12', '13.13.13.13'],
+            ip_version=4,
+            name_servers=['10.10.10.10', '11.11.11.11'],
+            search=['14.14.14.14', '15.15.15.15'],
+            server='localhost',
+            user='admin',
+            password='password'
+        )
+        p = Parameters(args)
+        assert p.cache == 'disable'
+        assert p.name_servers == ['10.10.10.10', '11.11.11.11']
+        assert p.search == ['14.14.14.14', '15.15.15.15']
+
+        # BIG-IP considers "ipv4" to be an empty value
+        assert p.ip_version == ''
+
+    def test_ipv6_parameter(self):
+        args = dict(
+            ip_version=6
+        )
+        p = Parameters(args)
+        assert p.ip_version == 'options inet6'
+
+    def test_ensure_forwards_raises_exception(self):
+        args = dict(
+            forwarders=['12.12.12.12', '13.13.13.13'],
+        )
+        p = Parameters(args)
+        with pytest.raises(F5ModuleError) as ex:
+            foo = p.forwarders
+        assert 'The modifying of forwarders is not supported' in str(ex)
+
+
+class TestManager(unittest.TestCase):
+
+    def setUp(self):
+        self.spec = ArgumentSpec()
+
+    @patch('ansible.module_utils.f5_utils.AnsibleF5Client._get_mgmt_root',
+           return_value=True)
+    def test_update_settings(self, *args):
+        set_module_args(dict(
+            cache='disable',
+            forwarders=['12.12.12.12', '13.13.13.13'],
+            ip_version=4,
+            name_servers=['10.10.10.10', '11.11.11.11'],
+            search=['14.14.14.14', '15.15.15.15'],
+            server='localhost',
+            user='admin',
+            password='password'
+        ))
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            dict(
+                cache='enable'
+            )
+        )
+
+        client = AnsibleF5Client(
+            argument_spec=self.spec.argument_spec,
+            supports_check_mode=self.spec.supports_check_mode,
+            f5_product_name=self.spec.f5_product_name
+        )
+        mm = ModuleManager(client)
+
+        # Override methods to force specific logic in the module to happen
+        mm.update_on_device = Mock(return_value=True)
+        mm.read_current_from_device = Mock(return_value=current)
+
+        results = mm.exec_module()
+
+        assert results['changed'] is True