From 6c97c340eaf9047a39747323a816eb3f3bf4a8f6 Mon Sep 17 00:00:00 2001 From: Tim Rupp Date: Wed, 3 Jan 2018 21:18:34 -0800 Subject: [PATCH] Various bigip_gtm_datacenter fixes (#34440) Code refactor to use f5 coding conventions. Removed deprecated "enabled/disabled" params (this is now a state). Adds token cleanup for cases where many api calls are made. --- .../network/f5/bigip_gtm_datacenter.py | 288 ++++++++++-------- .../network/f5/test_bigip_gtm_datacenter.py | 86 +----- 2 files changed, 171 insertions(+), 203 deletions(-) diff --git a/lib/ansible/modules/network/f5/bigip_gtm_datacenter.py b/lib/ansible/modules/network/f5/bigip_gtm_datacenter.py index 11ed595195..37373560e6 100644 --- a/lib/ansible/modules/network/f5/bigip_gtm_datacenter.py +++ b/lib/ansible/modules/network/f5/bigip_gtm_datacenter.py @@ -29,15 +29,6 @@ options: description: description: - The description of the data center. - enabled: - description: - - Whether the data center should be enabled. At least one of C(state) and - C(enabled) are required. - - Deprecated in 2.4. Use C(state) and either C(enabled) or C(disabled) - instead. - choices: - - yes - - no location: description: - The location of the data center. @@ -97,6 +88,16 @@ enabled: returned: changed type: bool sample: true +disabled: + description: Whether the datacenter is disabled or not. + returned: changed + type: bool + sample: true +state: + description: State of the datacenter. + returned: changed + type: string + sample: disabled location: description: The location that is set for the datacenter. returned: changed @@ -104,8 +105,6 @@ location: sample: 222 West 23rd ''' -from ansible.module_utils.parsing.convert_bool import BOOLEANS_TRUE -from ansible.module_utils.parsing.convert_bool import BOOLEANS_FALSE 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 @@ -121,97 +120,17 @@ class Parameters(AnsibleF5Parameters): api_map = {} updatables = [ - 'location', 'description', 'contact', - - # TODO: Remove this method in v2.5 - 'enabled' + 'location', 'description', 'contact', 'state' ] returnables = [ - 'location', 'description', 'contact', - - # TODO: Remove this method in v2.5 - 'enabled' + 'location', 'description', 'contact', 'state', 'enabled', 'disabled' ] api_attributes = [ - 'enabled', 'location', 'description', 'contact' + 'enabled', 'location', 'description', 'contact', 'disabled' ] - @property - def disabled(self): - if self._values['state'] == 'disabled': - return True - # TODO: Remove this method in v2.5 - elif self._values['disabled'] in BOOLEANS_TRUE: - return True - # TODO: Remove this method in v2.5 - elif self._values['disabled'] in BOOLEANS_FALSE: - return False - # TODO: Remove this method in v2.5 - elif self._values['enabled'] in BOOLEANS_FALSE: - return True - # TODO: Remove this method in v2.5 - elif self._values['enabled'] in BOOLEANS_TRUE: - return False - elif self._values['state'] == 'enabled': - return False - else: - return None - - @property - def enabled(self): - if self._values['state'] == 'enabled': - return True - # TODO: Remove this method in v2.5 - elif self._values['enabled'] in BOOLEANS_TRUE: - return True - # TODO: Remove this method in v2.5 - elif self._values['enabled'] in BOOLEANS_FALSE: - return False - # TODO: Remove this method in v2.5 - elif self._values['disabled'] in BOOLEANS_FALSE: - return True - # TODO: Remove this method in v2.5 - elif self._values['disabled'] in BOOLEANS_TRUE: - return False - elif self._values['state'] == 'disabled': - return False - else: - return None - - @property - def state(self): - if self.enabled and self._values['state'] != 'present': - return 'enabled' - elif self.disabled and self._values['state'] != 'present': - return 'disabled' - else: - return self._values['state'] - - # TODO: Remove this method in v2.5 - @state.setter - def state(self, value): - self._values['state'] = value - - # Only do this if not using legacy params - if self._values['enabled'] is None: - if self._values['state'] in ['enabled', 'present']: - self._values['enabled'] = True - self._values['disabled'] = False - elif self._values['state'] == 'disabled': - self._values['enabled'] = False - self._values['disabled'] = True - else: - if self._values['__warnings'] is None: - self._values['__warnings'] = [] - self._values['__warnings'].append( - dict( - msg="Usage of the 'enabled' parameter is deprecated", - version='2.4' - ) - ) - def to_return(self): result = {} for returnable in self.returnables: @@ -231,42 +150,119 @@ class Parameters(AnsibleF5Parameters): return result -class Changes(Parameters): +class ApiParameters(Parameters): + @property + def disabled(self): + if self._values['disabled'] is True: + return True + return None + @property def enabled(self): - if self._values['enabled'] in BOOLEANS_TRUE: + if self._values['enabled'] is True: + return True + return None + + +class ModuleParameters(Parameters): + @property + def disabled(self): + if self._values['state'] == 'disabled': return True else: + return None + + @property + def enabled(self): + if self._values['state'] in ['enabled', 'present']: + return True + return None + + @property + def state(self): + if self.enabled and self._values['state'] != 'present': + return 'enabled' + elif self.disabled and self._values['state'] != 'present': + return 'disabled' + else: + return self._values['state'] + + +class Changes(Parameters): + def to_return(self): + result = {} + try: + for returnable in self.returnables: + result[returnable] = getattr(self, returnable) + result = self._filter_params(result) + except Exception: + pass + return result + + +class UsableChanges(Changes): + pass + + +class ReportableChanges(Changes): + @property + def disabled(self): + if self._values['state'] == 'disabled': + return True + elif self._values['state'] in ['enabled', 'present']: return False + return None + + @property + def enabled(self): + if self._values['state'] in ['enabled', 'present']: + return True + elif self._values['state'] == 'disabled': + return False + return None + + +class Difference(object): + def __init__(self, want, have=None): + self.want = want + self.have = have + + def compare(self, param): + try: + result = getattr(self, param) + return result + except AttributeError: + return self.__default(param) + + def __default(self, param): + attr1 = getattr(self.want, param) + try: + attr2 = getattr(self.have, param) + if attr1 != attr2: + return attr1 + except AttributeError: + return attr1 + + @property + def state(self): + if self.want.enabled != self.have.enabled: + return dict( + state=self.want.state, + enabled=self.want.enabled + ) + if self.want.disabled != self.have.disabled: + return dict( + state=self.want.state, + disabled=self.want.disabled + ) class ModuleManager(object): def __init__(self, client): self.client = client - self.have = None - self.want = Parameters(self.client.module.params) - self.changes = Changes() - - def _set_changed_options(self): - changed = {} - for key in Parameters.returnables: - if getattr(self.want, key) is not None: - changed[key] = getattr(self.want, key) - if changed: - self.changes = Changes(changed) - - 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 = Changes(changed) - return True - return False + self.want = ModuleParameters(params=self.client.module.params) + self.have = ApiParameters() + self.changes = UsableChanges() def exec_module(self): changed = False @@ -281,24 +277,39 @@ class ModuleManager(object): except iControlUnexpectedHTTPError as e: raise F5ModuleError(str(e)) - changes = self.changes.to_return() + reportable = ReportableChanges(self.changes.to_return()) + changes = reportable.to_return() result.update(**changes) result.update(dict(changed=changed)) - self._announce_deprecations() + self._announce_deprecations(result) return result - def _announce_deprecations(self): - warnings = [] - if self.want: - warnings += self.want._values.get('__warnings', []) - if self.have: - warnings += self.have._values.get('__warnings', []) + def _announce_deprecations(self, result): + warnings = result.pop('__warnings', []) for warning in warnings: self.client.module.deprecate( msg=warning['msg'], version=warning['version'] ) + def _update_changed_options(self): + diff = Difference(self.want, self.have) + updatables = Parameters.updatables + changed = dict() + for k in updatables: + change = diff.compare(k) + if change is None: + continue + else: + if isinstance(change, dict): + changed.update(change) + else: + changed[k] = change + if changed: + self.changes = UsableChanges(changed) + return True + return False + def should_update(self): result = self._update_changed_options() if result: @@ -323,7 +334,7 @@ class ModuleManager(object): partition=self.want.partition ) result = resource.attrs - return Parameters(result) + return ApiParameters(result) def exists(self): result = self.client.api.tm.gtm.datacenters.datacenter.exists( @@ -350,7 +361,8 @@ class ModuleManager(object): resource.modify(**params) def create(self): - self._set_changed_options() + self.have = ApiParameters() + self.should_update() if self.client.check_mode: return True self.create_on_device() @@ -389,9 +401,6 @@ class ArgumentSpec(object): self.argument_spec = dict( contact=dict(), description=dict(), - enabled=dict( - type='bool', - ), location=dict(), name=dict(required=True), state=dict( @@ -403,6 +412,16 @@ class ArgumentSpec(object): self.f5_product_name = 'bigip' +def cleanup_tokens(client): + try: + resource = client.api.shared.authz.tokens_s.token.load( + name=client.api.icrs.token + ) + resource.delete() + except Exception: + pass + + def main(): if not HAS_F5SDK: raise F5ModuleError("The python f5-sdk module is required") @@ -418,9 +437,12 @@ def main(): try: mm = ModuleManager(client) results = mm.exec_module() + cleanup_tokens(client) client.module.exit_json(**results) except F5ModuleError as e: + cleanup_tokens(client) client.module.fail_json(msg=str(e)) + if __name__ == '__main__': main() diff --git a/test/units/modules/network/f5/test_bigip_gtm_datacenter.py b/test/units/modules/network/f5/test_bigip_gtm_datacenter.py index 0a7f0c586c..cbff193f2c 100644 --- a/test/units/modules/network/f5/test_bigip_gtm_datacenter.py +++ b/test/units/modules/network/f5/test_bigip_gtm_datacenter.py @@ -20,14 +20,16 @@ from ansible.compat.tests.mock import patch from ansible.module_utils.f5_utils import AnsibleF5Client try: - from library.bigip_gtm_datacenter import Parameters + from library.bigip_gtm_datacenter import ApiParameters + from library.bigip_gtm_datacenter import ModuleParameters from library.bigip_gtm_datacenter import ModuleManager from library.bigip_gtm_datacenter 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_gtm_datacenter import Parameters + from ansible.modules.network.f5.bigip_gtm_datacenter import ApiParameters + from ansible.modules.network.f5.bigip_gtm_datacenter import ModuleParameters from ansible.modules.network.f5.bigip_gtm_datacenter import ModuleManager from ansible.modules.network.f5.bigip_gtm_datacenter import ArgumentSpec from ansible.module_utils.f5_utils import iControlUnexpectedHTTPError @@ -66,19 +68,19 @@ class TestParameters(unittest.TestCase): location='baz', name='datacenter' ) - p = Parameters(args) + p = ModuleParameters(args) assert p.state == 'present' def test_api_parameters(self): args = load_fixture('load_gtm_datacenter_default.json') - p = Parameters(args) + p = ApiParameters(args) assert p.name == 'asd' def test_module_parameters_state_present(self): args = dict( state='present' ) - p = Parameters(args) + p = ModuleParameters(args) assert p.state == 'present' assert p.enabled is True @@ -86,25 +88,25 @@ class TestParameters(unittest.TestCase): args = dict( state='absent' ) - p = Parameters(args) + p = ModuleParameters(args) assert p.state == 'absent' def test_module_parameters_state_enabled(self): args = dict( state='enabled' ) - p = Parameters(args) + p = ModuleParameters(args) assert p.state == 'enabled' assert p.enabled is True - assert p.disabled is False + assert p.disabled is None def test_module_parameters_state_disabled(self): args = dict( state='disabled' ) - p = Parameters(args) + p = ModuleParameters(args) assert p.state == 'disabled' - assert p.enabled is False + assert p.enabled is None assert p.disabled is True @@ -137,6 +139,7 @@ class TestManager(unittest.TestCase): results = mm.exec_module() assert results['changed'] is True + assert results['state'] == 'present' def test_create_disabled_datacenter(self, *args): set_module_args(dict( @@ -161,6 +164,7 @@ class TestManager(unittest.TestCase): results = mm.exec_module() assert results['changed'] is True assert results['enabled'] is False + assert results['disabled'] is True def test_create_enabled_datacenter(self, *args): set_module_args(dict( @@ -185,6 +189,7 @@ class TestManager(unittest.TestCase): results = mm.exec_module() assert results['changed'] is True assert results['enabled'] is True + assert results['disabled'] is False def test_idempotent_disable_datacenter(self, *args): set_module_args(dict( @@ -201,7 +206,7 @@ class TestManager(unittest.TestCase): f5_product_name=self.spec.f5_product_name ) - current = Parameters(load_fixture('load_gtm_datacenter_disabled.json')) + current = ApiParameters(load_fixture('load_gtm_datacenter_disabled.json')) mm = ModuleManager(client) @@ -212,62 +217,3 @@ class TestManager(unittest.TestCase): results = mm.exec_module() assert results['changed'] is False - assert results['enabled'] is False - - -@patch('ansible.module_utils.f5_utils.AnsibleF5Client._get_mgmt_root', - return_value=True) -class TestLegacyManager(unittest.TestCase): - - def setUp(self): - self.spec = ArgumentSpec() - - def test_legacy_disable_datacenter(self, *args): - set_module_args(dict( - state='present', - enabled='no', - password='admin', - server='localhost', - user='admin', - name='foo' - )) - - 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.exists = Mock(side_effect=[False, True]) - mm.create_on_device = Mock(return_value=True) - - results = mm.exec_module() - assert results['changed'] is True - assert results['enabled'] is False - - def test_legacy_enable_datacenter(self, *args): - set_module_args(dict( - state='present', - enabled='yes', - password='admin', - server='localhost', - user='admin', - name='foo' - )) - - 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.exists = Mock(side_effect=[False, True]) - mm.create_on_device = Mock(return_value=True) - - results = mm.exec_module() - assert results['changed'] is True - assert results['enabled'] is True