Refactors bigip_device_ntp (#33472)

* Refactors bigip_device_ntp

To be inline with current f5 conventions

* Fixed incorrect key

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

@@ -3,31 +3,31 @@
 #
 # 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_ntp
 short_description: Manage NTP servers on a BIG-IP
 description:
-  - Manage NTP servers on a BIG-IP
+  - Manage NTP servers on a BIG-IP.
 version_added: "2.2"
 options:
   ntp_servers:
     description:
       - A list of NTP servers to set on the device. At least one of C(ntp_servers)
         or C(timezone) is required.
-    required: false
-    default: []
   state:
     description:
       - The state of the NTP servers on the system. When C(present), guarantees
         that the NTP servers are set on the system. When C(absent), removes the
         specified NTP servers from the device configuration.
-    required: false
     default: present
     choices:
       - absent
@@ -37,7 +37,6 @@ options:
       - The timezone to set for NTP lookups. At least one of C(ntp_servers) or
         C(timezone) is required.
     default: UTC
-    required: false
 notes:
   - Requires the f5-sdk Python package on the host. This is as easy as pip
     install f5-sdk.
@@ -46,204 +45,231 @@ requirements:
   - f5-sdk
 author:
   - Tim Rupp (@caphrim007)
+  - Wojciech Wypior (@wojtek0806)
 '''
 
-EXAMPLES = '''
+EXAMPLES = r'''
 - name: Set NTP server
   bigip_device_ntp:
-      ntp_servers:
-          - "192.0.2.23"
-      password: "secret"
-      server: "lb.mydomain.com"
-      user: "admin"
-      validate_certs: "no"
+    ntp_servers:
+      - 192.0.2.23
+    password: secret
+    server: lb.mydomain.com
+    user: admin
+    validate_certs: no
   delegate_to: localhost
 
 - name: Set timezone
   bigip_device_ntp:
-      password: "secret"
-      server: "lb.mydomain.com"
-      timezone: "America/Los_Angeles"
-      user: "admin"
-      validate_certs: "no"
+    password: secret
+    server: lb.mydomain.com
+    timezone: America/Los_Angeles
+    user: admin
+    validate_certs: no
   delegate_to: localhost
 '''
 
-RETURN = '''
+RETURN = r'''
 ntp_servers:
-    description: The NTP servers that were set on the device
-    returned: changed
-    type: list
-    sample: ["192.0.2.23", "192.0.2.42"]
+  description: The NTP servers that were set on the device
+  returned: changed
+  type: list
+  sample: ["192.0.2.23", "192.0.2.42"]
 timezone:
-    description: The timezone that was set on the device
-    returned: changed
-    type: string
-    sample: "true"
+  description: The timezone that was set on the device
+  returned: changed
+  type: string
+  sample: true
 '''
 
+
+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 import ManagementRoot
-    from icontrol.session import iControlUnexpectedHTTPError
-    HAS_F5SDK = True
+    from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
 except ImportError:
     HAS_F5SDK = False
 
 
-class BigIpDeviceNtp(object):
-    def __init__(self, *args, **kwargs):
-        if not HAS_F5SDK:
-            raise F5ModuleError("The python f5-sdk module is required")
+class Parameters(AnsibleF5Parameters):
+    api_map = {
+        'servers': 'ntp_servers'
+    }
 
-        # The params that change in the module
-        self.cparams = dict()
+    api_attributes = [
+        'servers', 'timezone',
+    ]
 
-        # 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'])
+    updatables = [
+        'ntp_servers', 'timezone'
+    ]
 
-    def flush(self):
-        result = dict()
+    returnables = [
+        'ntp_servers', 'timezone'
+    ]
+
+    absentables = [
+        'ntp_servers'
+    ]
+
+    def to_return(self):
+        result = {}
+        for returnable in self.returnables:
+            result[returnable] = getattr(self, returnable)
+        result = self._filter_params(result)
+        return result
+
+    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
+
+
+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 _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))
+                if set_want != set_have:
+                    changed[key] = list(set_want)
+        if changed:
+            self.changes = Parameters(changed)
+            return True
+        return False
+
+    def exec_module(self):
         changed = False
-        state = self.params['state']
+        result = dict()
+        state = self.want.state
 
         try:
             if state == "present":
-                changed = self.present()
+                changed = self.update()
             elif state == "absent":
                 changed = self.absent()
         except iControlUnexpectedHTTPError as e:
             raise F5ModuleError(str(e))
 
-        if 'servers' in self.cparams:
-            self.cparams['ntp_servers'] = self.cparams.pop('servers')
-
-        result.update(**self.cparams)
+        changes = self.changes.to_return()
+        result.update(**changes)
         result.update(dict(changed=changed))
         return result
 
-    def read(self):
-        """Read information and transform it
+    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
 
-        The values that are returned by BIG-IP in the f5-sdk can have encoding
-        attached to them as well as be completely missing in some cases.
+    def should_update(self):
+        result = self._update_changed_options()
+        if result:
+            return True
+        return False
 
-        Therefore, this method will transform the data from the BIG-IP into a
-        format that is more easily consumable by the rest of the class and the
-        parameters that are supported by the module.
-        """
-        p = dict()
-        r = self.api.tm.sys.ntp.load()
-
-        if hasattr(r, 'servers'):
-            # Deliberately using sets to suppress duplicates
-            p['servers'] = set([str(x) for x in r.servers])
-        if hasattr(r, 'timezone'):
-            p['timezone'] = str(r.timezone)
-        return p
-
-    def present(self):
-        changed = False
-        params = dict()
-        current = self.read()
-
-        check_mode = self.params['check_mode']
-        ntp_servers = self.params['ntp_servers']
-        timezone = self.params['timezone']
-
-        # NTP servers can be set independently
-        if ntp_servers is not None:
-            if 'servers' in current:
-                items = set(ntp_servers)
-                if items != current['servers']:
-                    params['servers'] = list(ntp_servers)
-            else:
-                params['servers'] = ntp_servers
-
-        # Timezone can be set independently
-        if timezone is not None:
-            if 'timezone' in current and current['timezone'] != timezone:
-                params['timezone'] = timezone
-
-        if params:
-            changed = True
-            self.cparams = camel_dict_to_snake_dict(params)
-            if check_mode:
-                return changed
-        else:
-            return changed
-
-        r = self.api.tm.sys.ntp.load()
-        r.update(**params)
-        r.refresh()
-
-        return changed
+    def should_absent(self):
+        result = self._absent_changed_options()
+        if result:
+            return True
+        return False
 
     def absent(self):
-        changed = False
-        params = dict()
-        current = self.read()
+        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
 
-        check_mode = self.params['check_mode']
-        ntp_servers = self.params['ntp_servers']
+    def update_on_device(self):
+        params = self.want.api_params()
+        resource = self.client.api.tm.sys.ntp.load()
+        resource.update(**params)
 
-        if not ntp_servers:
-            raise F5ModuleError(
-                "Absent can only be used when removing NTP servers"
+    def read_current_from_device(self):
+        resource = self.client.api.tm.sys.ntp.load()
+        result = resource.attrs
+        return Parameters(result)
+
+    def absent_on_device(self):
+        params = self.changes.api_params()
+        resource = self.client.api.tm.sys.ntp.load()
+        resource.update(**params)
+
+
+class ArgumentSpec(object):
+    def __init__(self):
+        self.supports_check_mode = True
+        self.argument_spec = dict(
+            ntp_servers=dict(
+                required=False,
+                default=None,
+                type='list',
+            ),
+            timezone=dict(
+                required=False,
+                default=None,
             )
-
-        if ntp_servers and 'servers' in current:
-            servers = current['servers']
-            new_servers = [x for x in servers if x not in ntp_servers]
-
-            if servers != new_servers:
-                params['servers'] = new_servers
-
-        if params:
-            changed = True
-            self.cparams = camel_dict_to_snake_dict(params)
-            if check_mode:
-                return changed
-        else:
-            return changed
-
-        r = self.api.tm.sys.ntp.load()
-        r.update(**params)
-        r.refresh()
-        return changed
+        )
+        self.required_one_of = [
+            ['ntp_servers', 'timezone']
+        ]
+        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(
-        ntp_servers=dict(required=False, type='list', default=None),
-        timezone=dict(default=None, required=False)
-    )
-    argument_spec.update(meta_args)
+    spec = ArgumentSpec()
 
-    module = AnsibleModule(
-        argument_spec=argument_spec,
-        required_one_of=[
-            ['ntp_servers', 'timezone']
-        ],
-        supports_check_mode=True
+    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 = BigIpDeviceNtp(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/fixtures/load_ntp.json

@@ -0,0 +1,13 @@
+{
+  "kind": "tm:sys:ntp:ntpstate",
+  "selfLink": "https://localhost/mgmt/tm/sys/ntp?ver=12.1.0",
+  "servers": [
+    "192.168.1.1",
+    "192.168.1.2"
+  ],
+  "timezone": "America/Los_Angeles",
+  "restrictReference": {
+    "link": "https://localhost/mgmt/tm/sys/ntp/restrict?ver=12.1.0",
+    "isSubcollection": true
+  }
+}

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

@@ -0,0 +1,252 @@
+# -*- 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 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 Mock
+from ansible.compat.tests.mock import patch
+from ansible.module_utils.f5_utils import AnsibleF5Client
+
+try:
+    from library.bigip_device_ntp import Parameters
+    from library.bigip_device_ntp import ModuleManager
+    from library.bigip_device_ntp import ArgumentSpec
+    from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
+    from test.unit.modules.utils import set_module_args
+except ImportError:
+    try:
+        from ansible.modules.network.f5.bigip_device_ntp import Parameters
+        from ansible.modules.network.f5.bigip_device_ntp import ModuleManager
+        from ansible.modules.network.f5.bigip_device_ntp import ArgumentSpec
+        from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError
+        from units.modules.utils import set_module_args
+    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 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):
+        ntp = ['192.168.1.1', '192.168.1.2']
+        args = dict(
+            ntp_servers=ntp,
+            timezone='Arctic/Longyearbyen'
+        )
+
+        p = Parameters(args)
+        assert p.ntp_servers == ntp
+        assert p.timezone == 'Arctic/Longyearbyen'
+
+    def test_api_parameters(self):
+        ntp = ['192.168.1.1', '192.168.1.2']
+        args = dict(
+            servers=ntp,
+            timezone='Arctic/Longyearbyen'
+        )
+
+        p = Parameters(args)
+        assert p.ntp_servers == ntp
+        assert p.timezone == 'Arctic/Longyearbyen'
+
+
+@patch('ansible.module_utils.f5_utils.AnsibleF5Client._get_mgmt_root',
+       return_value=True)
+class TestModuleManager(unittest.TestCase):
+
+    def setUp(self):
+        self.spec = ArgumentSpec()
+
+    def test_update_ntp_servers(self, *args):
+        ntp = ['10.1.1.1', '10.1.1.2']
+        set_module_args(
+            dict(
+                ntp_servers=ntp,
+                server='localhost',
+                user='admin',
+                password='password'
+            )
+        )
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            load_fixture('load_ntp.json')
+        )
+
+        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
+        assert results['ntp_servers'] == ntp
+
+    def test_update_timezone(self, *args):
+        set_module_args(
+            dict(
+                timezone='Arctic/Longyearbyen',
+                server='localhost',
+                user='admin',
+                password='password'
+            )
+        )
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            load_fixture('load_ntp.json')
+        )
+
+        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
+        assert results['timezone'] == 'Arctic/Longyearbyen'
+
+    def test_update_ntp_servers_and_timezone(self, *args):
+        ntp = ['10.1.1.1', '10.1.1.2']
+        set_module_args(
+            dict(
+                ntp_servers=ntp,
+                timezone='Arctic/Longyearbyen',
+                server='localhost',
+                user='admin',
+                password='password'
+            )
+        )
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            load_fixture('load_ntp.json')
+        )
+
+        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
+        assert results['ntp_servers'] == ntp
+        assert results['timezone'] == 'Arctic/Longyearbyen'
+
+    def test_absent_ntp_servers(self, *args):
+        ntp = []
+        set_module_args(
+            dict(
+                ntp_servers=ntp,
+                timezone='America/Los_Angeles',
+                server='localhost',
+                user='admin',
+                password='password',
+                state='absent'
+            )
+        )
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            load_fixture('load_ntp.json')
+        )
+
+        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.absent_on_device = Mock(return_value=True)
+        mm.read_current_from_device = Mock(return_value=current)
+
+        results = mm.exec_module()
+        assert results['changed'] is True
+        assert results['ntp_servers'] == ntp
+        assert not hasattr(results, 'timezone')
+
+    def test_absent_timezone(self, *args):
+        set_module_args(
+            dict(
+                timezone='',
+                server='localhost',
+                user='admin',
+                password='password',
+                state='absent'
+            )
+        )
+
+        # Configure the parameters that would be returned by querying the
+        # remote device
+        current = Parameters(
+            load_fixture('load_ntp.json')
+        )
+
+        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.absent_on_device = Mock(return_value=True)
+        mm.read_current_from_device = Mock(return_value=current)
+
+        results = mm.exec_module()
+        assert results['changed'] is False