From 4dc5a26293ef88e9b1bf8c79d45200ebb6b06032 Mon Sep 17 00:00:00 2001 From: Tim Rupp Date: Sat, 10 Nov 2018 21:16:00 -0800 Subject: [PATCH] Adds new params to bigip_node. Fix unit tests. (#48521) --- lib/ansible/modules/network/f5/bigip_node.py | 546 ++++++++++-------- .../modules/network/f5/test_bigip_node.py | 25 +- 2 files changed, 316 insertions(+), 255 deletions(-) diff --git a/lib/ansible/modules/network/f5/bigip_node.py b/lib/ansible/modules/network/f5/bigip_node.py index ef7adc8360..c0e8d10fcf 100644 --- a/lib/ansible/modules/network/f5/bigip_node.py +++ b/lib/ansible/modules/network/f5/bigip_node.py @@ -1,7 +1,7 @@ #!/usr/bin/python # -*- coding: utf-8 -*- # -# Copyright (c) 2016 F5 Networks Inc. +# Copyright: (c) 2016, 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 @@ -157,6 +157,25 @@ options: - When creating a new node, if this parameter is not specified, the default of C(1) will be used. version_added: 2.7 + availability_requirements: + description: + - Specifies, if you activate more than one health monitor, the number of health + monitors that must receive successful responses in order for the link to be + considered available. + suboptions: + type: + description: + - Monitor rule type when C(monitors) is specified. + - When creating a new pool, if this value is not specified, the default of + 'all' will be used. + choices: ['all', 'at_least'] + at_least: + description: + - Specifies the minimum number of active health monitors that must be successful + before the link is considered up. + - This parameter is only relevant when a C(type) of C(at_least) is used. + - This parameter will be ignored if a type of C(all) is used. + version_added: 2.8 partition: description: - Device partition to manage resources on. @@ -170,68 +189,64 @@ author: EXAMPLES = r''' - name: Add node bigip_node: - server: lb.mydomain.com - user: admin - password: secret - state: present - partition: Common host: 10.20.30.40 name: 10.20.30.40 + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost - name: Add node with a single 'ping' monitor bigip_node: - server: lb.mydomain.com - user: admin - password: secret - state: present - partition: Common host: 10.20.30.40 name: mytestserver monitors: - /Common/icmp + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost - name: Modify node description bigip_node: - server: lb.mydomain.com - user: admin - password: secret - state: present - partition: Common name: 10.20.30.40 description: Our best server yet + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost - name: Delete node bigip_node: - server: lb.mydomain.com - user: admin - password: secret state: absent - partition: Common name: 10.20.30.40 + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost - name: Force node offline bigip_node: - server: lb.mydomain.com - user: admin - password: secret state: disabled - partition: Common name: 10.20.30.40 + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost - name: Add node by their FQDN bigip_node: - server: lb.mydomain.com - user: admin - password: secret - state: present - partition: Common fqdn: foo.bar.com name: 10.20.30.40 + provider: + server: lb.mydomain.com + user: admin + password: secret delegate_to: localhost ''' @@ -286,22 +301,18 @@ try: from library.module_utils.network.f5.bigip import F5RestClient from library.module_utils.network.f5.common import F5ModuleError from library.module_utils.network.f5.common import AnsibleF5Parameters - from library.module_utils.network.f5.common import cleanup_tokens from library.module_utils.network.f5.common import fq_name from library.module_utils.network.f5.common import f5_argument_spec from library.module_utils.network.f5.common import transform_name - from library.module_utils.network.f5.common import exit_json - from library.module_utils.network.f5.common import fail_json + from library.module_utils.compat.ipaddress import ip_address except ImportError: from ansible.module_utils.network.f5.bigip import F5RestClient from ansible.module_utils.network.f5.common import F5ModuleError from ansible.module_utils.network.f5.common import AnsibleF5Parameters - from ansible.module_utils.network.f5.common import cleanup_tokens from ansible.module_utils.network.f5.common import fq_name from ansible.module_utils.network.f5.common import f5_argument_spec from ansible.module_utils.network.f5.common import transform_name - from ansible.module_utils.network.f5.common import exit_json - from ansible.module_utils.network.f5.common import fail_json + from ansible.module_utils.compat.ipaddress import ip_address class Parameters(AnsibleF5Parameters): @@ -312,29 +323,13 @@ class Parameters(AnsibleF5Parameters): } api_attributes = [ - # Leave the ``monitor`` attribute commented out - # - # This attribute is commented out to prevent it from trying to be - # sent to the API during a create or update request. This is because - # the field is **broken** and **will not work** if you send some - # formats of the monitor to the API. - # - # Specifically, the m_of_n types will not work because they include - # the brace ( ``{`` ) character and the API considers this character - # to be invalid. - # - # Monitors are handled in a special case within the ``update_one_device`` - # and ``create_one_device`` methods. Refer to them if you need to know - # what that special case is. - # - # 'monitor', - 'description', 'address', 'fqdn', 'ratio', 'connectionLimit', 'rateLimit', + 'monitor', # Used for changing state # @@ -349,11 +344,10 @@ class Parameters(AnsibleF5Parameters): ] returnables = [ - 'monitor_type', - 'quorum', 'monitors', 'description', 'fqdn', + 'address', 'session', 'state', 'fqdn_auto_populate', @@ -363,12 +357,11 @@ class Parameters(AnsibleF5Parameters): 'fqdn_name', 'connection_limit', 'ratio', - 'rate_limit' + 'rate_limit', + 'availability_requirements' ] updatables = [ - 'monitor_type', - 'quorum', 'monitors', 'description', 'state', @@ -379,7 +372,7 @@ class Parameters(AnsibleF5Parameters): 'fqdn_address_type', 'connection_limit', 'ratio', - 'rate_limit' + 'rate_limit', ] def to_return(self): @@ -392,28 +385,6 @@ class Parameters(AnsibleF5Parameters): except Exception: return result - @property - def monitors_list(self): - if self._values['monitors'] is None: - return [] - try: - result = re.findall(r'/\w+/[^\s}]+', self._values['monitors']) - return result - except Exception: - return self._values['monitors'] - - @property - def monitors(self): - if self._values['monitors'] is None: - return None - monitors = [fq_name(self.partition, x) for x in self.monitors_list] - if self.monitor_type == 'm_of_n': - monitors = ' '.join(monitors) - result = 'min %s of { %s }' % (self.quorum, monitors) - else: - result = ' and '.join(monitors).strip() - return result - @property def rate_limit(self): if self._values['rate_limit'] is None: @@ -439,35 +410,130 @@ class UsableChanges(Changes): result['autopopulate'] = self._values['fqdn_auto_populate'] if self._values['fqdn_name'] is not None: result['tmName'] = self._values['fqdn_name'] + if self._values['fqdn_address_type'] is not None: + result['addressFamily'] = self._values['fqdn_address_type'] if not result: return None return result + @property + def monitors(self): + monitor_string = self._values['monitors'] + if monitor_string is None: + return None + if '{' in monitor_string and '}': + tmp = monitor_string.strip('}').split('{') + monitor = ''.join(tmp).rstrip() + return monitor + return monitor_string + class ReportableChanges(Changes): - pass + @property + def monitors(self): + if self._values['monitors'] is None: + return [] + try: + result = re.findall(r'/\w+/[^\s}]+', self._values['monitors']) + result.sort() + return result + except Exception: + return self._values['monitors'] + + @property + def availability_requirement_type(self): + if self._values['monitors'] is None: + return None + if 'min ' in self._values['monitors']: + return 'at_least' + else: + return 'all' + + @property + def at_least(self): + """Returns the 'at least' value from the monitor string. + The monitor string for a Require monitor looks like this. + min 1 of { /Common/gateway_icmp } + This method parses out the first of the numeric values. This values represents + the "at_least" value that can be updated in the module. + Returns: + int: The at_least value if found. None otherwise. + """ + if self._values['monitors'] is None: + return None + pattern = r'min\s+(?P\d+)\s+of\s+' + matches = re.search(pattern, self._values['monitors']) + if matches is None: + return None + return int(matches.group('least')) + + @property + def availability_requirements(self): + if self._values['monitors'] is None: + return None + result = dict() + result['type'] = self.availability_requirement_type + result['at_least'] = self.at_least + return result class ModuleParameters(Parameters): - @property - def quorum(self): - if self._values['quorum'] is None: + def _get_availability_value(self, type): + if self._values['availability_requirements'] is None: return None - quorum = self._values['quorum'] - try: - if quorum is None: - return None - return int(quorum) - except ValueError: - raise F5ModuleError( - "The specified 'quorum' must be an integer." - ) + if self._values['availability_requirements'][type] is None: + return None + return int(self._values['availability_requirements'][type]) @property - def monitor_type(self): - if self._values['monitor_type'] is None: + def monitors_list(self): + if self._values['monitors'] is None: + return [] + try: + result = re.findall(r'/\w+/[^\s}]+', self._values['monitors']) + except Exception: + result = self._values['monitors'] + result.sort() + return result + + @property + def monitors(self): + if self._values['monitors'] is None: return None - return self._values['monitor_type'] + if len(self._values['monitors']) == 1 and self._values['monitors'][0] == '': + return '/Common/none' + monitors = [fq_name(self.partition, x) for x in self.monitors_list] + if self.availability_requirement_type == 'at_least': + if self.at_least > len(self.monitors_list): + raise F5ModuleError( + "The 'at_least' value must not exceed the number of 'monitors'." + ) + monitors = ' '.join(monitors) + result = 'min {0} of {{ {1} }}'.format(self.at_least, monitors) + else: + result = ' and '.join(monitors).strip() + + return result + + @property + def availability_requirement_type(self): + if self._values['monitor_type']: + if self._values['monitor_type'] in ['single', 'and_list']: + result = 'all' + else: + result = 'at_least' + self._values['availability_requirements'] = dict(type=None) + self._values['availability_requirements']['type'] = result + if self._values['availability_requirements'] is None: + return None + return self._values['availability_requirements']['type'] + + @property + def at_least(self): + if self._values['quorum']: + self._values['availability_requirements'] = dict(at_least=None) + self._values['availability_requirements']['at_least'] = self._values['quorum'] + return self._get_availability_value('at_least') @property def fqdn_up_interval(self): @@ -521,36 +587,6 @@ class ModuleParameters(Parameters): class ApiParameters(Parameters): - @property - def quorum(self): - if self._values['monitors'] is None: - return None - pattern = r'min\s+(?P\d+)\s+of' - matches = re.search(pattern, self._values['monitors']) - if matches: - quorum = matches.group('quorum') - else: - quorum = None - try: - if quorum is None: - return None - return int(quorum) - except ValueError: - raise F5ModuleError( - "The specified 'quorum' must be an integer." - ) - - @property - def monitor_type(self): - if self._values['monitors'] is None: - return None - pattern = r'min\s+\d+\s+of' - matches = re.search(pattern, self._values['monitors']) - if matches: - return 'm_of_n' - else: - return 'and_list' - @property def fqdn_up_interval(self): if self._values['fqdn'] is None: @@ -585,6 +621,62 @@ class ApiParameters(Parameters): return None return self._values['description'] + @property + def availability_requirement_type(self): + if self._values['monitors'] is None: + return None + if 'min ' in self._values['monitors']: + return 'at_least' + else: + return 'all' + + @property + def monitors_list(self): + if self._values['monitors'] is None: + return [] + try: + result = re.findall(r'/\w+/[^\s}]+', self._values['monitors']) + except Exception: + result = self._values['monitors'] + result.sort() + return result + + @property + def monitors(self): + if self._values['monitors'] is None: + return None + if self._values['monitors'] == 'default': + return 'default' + monitors = [fq_name(self.partition, x) for x in self.monitors_list] + if self.availability_requirement_type == 'at_least': + monitors = ' '.join(monitors) + result = 'min {0} of {{ {1} }}'.format(self.at_least, monitors) + else: + result = ' and '.join(monitors).strip() + return result + + @property + def at_least(self): + """Returns the 'at least' value from the monitor string. + + The monitor string for a Require monitor looks like this. + + min 1 of { /Common/gateway_icmp } + + This method parses out the first of the numeric values. This values represents + the "at_least" value that can be updated in the module. + + Returns: + int: The at_least value if found. None otherwise. + """ + if self._values['monitors'] is None: + return None + pattern = r'min\s+(?P\d+)\s+of\s+' + matches = re.search(pattern, self._values['monitors']) + if matches is None: + return None + return matches.group('least') + class Difference(object): def __init__(self, want, have=None): @@ -608,20 +700,8 @@ class Difference(object): return attr1 @property - def monitor_type(self): - if self.want.monitor_type is None: - self.want.update(dict(monitor_type=self.have.monitor_type)) - - if self.want.quorum is None: - self.want.update(dict(quorum=self.have.quorum)) - - if self.want.monitor_type == 'm_of_n' and self.want.quorum is None: - if self.want.quorum is None and self.have.quorum is None: - return None - raise F5ModuleError( - "Quorum value must be specified with monitor_type 'm_of_n'." - ) - elif self.want.monitor_type == 'single': + def monitors(self): + if self.want.monitor_type == 'single': if len(self.want.monitors_list) > 1: raise F5ModuleError( "When using a 'monitor_type' of 'single', only one monitor may be provided." @@ -633,27 +713,19 @@ class Difference(object): raise F5ModuleError( "A single monitor must be specified if more than one monitor currently exists on your pool." ) - # Update to 'and_list' here because the above checks are all that need - # to be done before we change the value back to what is expected by - # BIG-IP. - # - # Remember that 'single' is nothing more than a fancy way of saying - # "and_list plus some extra checks" - self.want.update(dict(monitor_type='and_list')) - if self.want.monitor_type != self.have.monitor_type: - return self.want.monitor_type - - @property - def monitors(self): - if self.want.monitor_type is None: - self.want.update(dict(monitor_type=self.have.monitor_type)) - if not self.want.monitors_list: - self.want.monitors = self.have.monitors_list - if not self.want.monitors and self.want.monitor_type is not None: - raise F5ModuleError( - "The 'monitors' parameter cannot be empty when 'monitor_type' parameter is specified" - ) - if self.want.monitors != self.have.monitors: + if self.want.monitors is None: + return None + if self.want.monitors == 'default' and self.have.monitors == 'default': + return None + if self.want.monitors == 'default' and self.have.monitors is None: + return None + if self.want.monitors == '/Common/none' and self.have.monitors == '/Common/none': + return None + if self.want.monitors == 'default' and len(self.have.monitors) > 0: + return 'default' + if self.have.monitors is None: + return self.want.monitors + if self.have.monitors != self.want.monitors: return self.want.monitors @property @@ -679,28 +751,6 @@ class Difference(object): ) return result - @property - def fqdn_auto_populate(self): - if self.want.fqdn_auto_populate is None: - return None - if self.want.fqdn_auto_populate != self.have.fqdn_auto_populate: - raise F5ModuleError( - "The 'fqdn_auto_populate' parameter cannot be changed." - ) - - @property - def fqdn_address_type(self): - if self.want.fqdn_address_type is None: - return None - if self.want.fqdn_address_type != self.have.fqdn_address_type: - raise F5ModuleError( - "The 'fqdn_address_type' parameter cannot be changed." - ) - - @property - def fqdn(self): - return None - @property def description(self): if self.want.description is None: @@ -718,6 +768,7 @@ class ModuleManager(object): self.have = None self.want = ModuleParameters(params=self.module.params) self.changes = UsableChanges() + self.client = F5RestClient(**self.module.params) def _set_changed_options(self): changed = {} @@ -745,7 +796,7 @@ class ModuleManager(object): return True return False - def _announce_deprecations(self): + def _announce_deprecations(self): # lgtm [py/similar-function] warnings = [] if self.want: warnings += self.want._values.get('__warnings', []) @@ -847,6 +898,23 @@ class ModuleManager(object): if self.module.check_mode: return True + # These are being set here because the ``create_on_device`` method + # uses ``self.changes`` (to get formatting of parameters correct) + # but these two parameters here cannot be changed and also it is + # not easy to get the current versions of them for comparison. + if self.want.address: + self.changes.update({'address': self.want.address}) + if self.want.fqdn_up_interval is not None: + self.changes.update({'fqdn_up_interval': self.want.fqdn_up_interval}) + if self.want.fqdn_down_interval is not None: + self.changes.update({'fqdn_down_interval': self.want.fqdn_down_interval}) + if self.want.fqdn_auto_populate is not None: + self.changes.update({'fqdn_auto_populate': self.want.fqdn_auto_populate}) + if self.want.fqdn_name is not None: + self.changes.update({'fqdn_name': self.want.fqdn_name}) + if self.want.fqdn_address_type is not None: + self.changes.update({'fqdn_address_type': self.want.fqdn_address_type}) + self.create_on_device() if not self.exists(): raise F5ModuleError("Failed to create the node") @@ -866,6 +934,18 @@ class ModuleManager(object): self.have = self.read_current_from_device() if not self.should_update(): return False + + if self.want.fqdn_auto_populate is not None: + if self.want.fqdn_auto_populate != self.have.fqdn_auto_populate: + raise F5ModuleError( + "The 'fqdn_auto_populate' parameter cannot be changed." + ) + if self.want.fqdn_address_type is not None: + if self.want.fqdn_address_type != self.have.fqdn_address_type: + raise F5ModuleError( + "The 'fqdn_address_type' parameter cannot be changed." + ) + if self.module.check_mode: return True @@ -906,7 +986,7 @@ class ModuleManager(object): raise F5ModuleError(resp.content) return ApiParameters(params=response) - def exists(self): + def exists(self): # lgtm [py/similar-function] uri = "https://{0}:{1}/mgmt/tm/ltm/node/{2}".format( self.client.provider['server'], self.client.provider['server_port'], @@ -962,12 +1042,9 @@ class ModuleManager(object): raise F5ModuleError(response['message']) else: raise F5ModuleError(resp.content) - if self.want.monitors: - self.update_monitors_on_device() def create_on_device(self): - params = self.want.api_params() - + params = self.changes.api_params() params['name'] = self.want.name params['partition'] = self.want.partition uri = "https://{0}:{1}/mgmt/tm/ltm/node/".format( @@ -985,8 +1062,6 @@ class ModuleManager(object): raise F5ModuleError(response['message']) else: raise F5ModuleError(resp.content) - if self.want.monitors: - self.update_monitors_on_device() self._wait_for_fqdn_checks() def _wait_for_fqdn_checks(self): @@ -1007,46 +1082,6 @@ class ModuleManager(object): if resp.status == 200: return True - def update_monitors_on_device(self): - """Updates the monitors string - - There is a long-standing bug in where the monitor value - is a string that includes braces. These braces cause the REST API to panic and - fail to update or create any resources that have an "at_least" or "require" - set of availability_requirements. - - This method exists to do a tmsh command to cause the update to take place on - the device. - - Preferably, this method can be removed and the bug be fixed. The API should - be working, obviously, but the more concerning issue is if tmsh commands change - over time, breaking this method. - """ - command = 'tmsh modify ltm node /{0}/{1} monitor {2}'.format( - self.want.partition, self.want.name, self.want.monitors - ) - params = { - "command": "run", - "utilCmdArgs": '-c "{0}"'.format(command) - } - uri = "https://{0}:{1}/mgmt/tm/util/bash".format( - self.client.provider['server'], - self.client.provider['server_port'] - ) - resp = self.client.api.post(uri, json=params) - try: - response = resp.json() - if 'commandResult' in response and len(response['commandResult'].strip()) > 0: - raise F5ModuleError(response['commandResult']) - except ValueError as ex: - raise F5ModuleError(str(ex)) - if 'code' in response and response['code'] in [400, 403]: - if 'message' in response: - raise F5ModuleError(response['message']) - else: - raise F5ModuleError(resp.content) - return True - class ArgumentSpec(object): def __init__(self): @@ -1060,13 +1095,6 @@ class ArgumentSpec(object): aliases=['hostname'] ), description=dict(), - monitor_type=dict( - choices=[ - 'and_list', 'm_of_n', 'single' - ] - ), - quorum=dict(type='int'), - monitors=dict(type='list'), state=dict( choices=['absent', 'present', 'enabled', 'disabled', 'offline'], default='present' @@ -1084,11 +1112,42 @@ class ArgumentSpec(object): connection_limit=dict(type='int'), rate_limit=dict(type='int'), ratio=dict(type='int'), - dynamic_ratio=dict(type='int') + dynamic_ratio=dict(type='int'), + availability_requirements=dict( + type='dict', + options=dict( + type=dict( + choices=['all', 'at_least'], + required=True + ), + at_least=dict(type='int'), + ), + required_if=[ + ['type', 'at_least', ['at_least']], + ] + ), + monitors=dict(type='list'), + + + # Deprecated parameters + monitor_type=dict( + choices=[ + 'and_list', 'm_of_n', 'single' + ], + removed_in_version=2.12, + ), + quorum=dict( + type='int', + removed_in_version=2.12, + ), + ) self.argument_spec = {} self.argument_spec.update(f5_argument_spec) self.argument_spec.update(argument_spec) + self.mutually_exclusive = [ + ['monitor_type', 'quorum', 'availability_requirements'] + ] def main(): @@ -1100,14 +1159,11 @@ def main(): ) try: - client = F5RestClient(**module.params) - mm = ModuleManager(module=module, client=client) + mm = ModuleManager(module=module) results = mm.exec_module() - cleanup_tokens(client) - exit_json(module, results, client) + module.exit_json(**results) except F5ModuleError as ex: - cleanup_tokens(client) - fail_json(module, ex, client) + module.fail_json(msg=str(ex)) if __name__ == '__main__': diff --git a/test/units/modules/network/f5/test_bigip_node.py b/test/units/modules/network/f5/test_bigip_node.py index 6427128d2b..fb14f3fc2b 100644 --- a/test/units/modules/network/f5/test_bigip_node.py +++ b/test/units/modules/network/f5/test_bigip_node.py @@ -14,9 +14,6 @@ from nose.plugins.skip import SkipTest if sys.version_info < (2, 7): raise SkipTest("F5 Ansible modules require Python >= 2.7") -from units.compat import unittest -from units.compat.mock import Mock -from units.compat.mock import patch from ansible.module_utils.basic import AnsibleModule try: @@ -25,9 +22,13 @@ try: from library.modules.bigip_node import ApiParameters from library.modules.bigip_node import ModuleManager from library.modules.bigip_node import ArgumentSpec - from library.module_utils.network.f5.common import F5ModuleError - from library.module_utils.network.f5.common import iControlUnexpectedHTTPError - from test.unit.modules.utils import set_module_args + + # In Ansible 2.8, Ansible changed import paths. + from test.units.compat import unittest + from test.units.compat.mock import Mock + from test.units.compat.mock import patch + + from test.units.modules.utils import set_module_args except ImportError: try: from ansible.modules.network.f5.bigip_node import Parameters @@ -35,8 +36,12 @@ except ImportError: from ansible.modules.network.f5.bigip_node import ApiParameters from ansible.modules.network.f5.bigip_node import ModuleManager from ansible.modules.network.f5.bigip_node import ArgumentSpec - from ansible.module_utils.network.f5.common import F5ModuleError - from ansible.module_utils.network.f5.common import iControlUnexpectedHTTPError + + # Ansible 2.8 imports + from units.compat import unittest + from units.compat.mock import Mock + from units.compat.mock import patch + from units.modules.utils import set_module_args except ImportError: raise SkipTest("F5 Ansible modules require the f5-sdk Python library") @@ -114,7 +119,7 @@ class TestManager(unittest.TestCase): assert results['changed'] is True - def test_create_selfip_idempotent(self, *args): + def test_create_node_idempotent(self, *args): set_module_args(dict( host='10.20.30.40', name='mytestserver', @@ -128,7 +133,7 @@ class TestManager(unittest.TestCase): user='admin' )) - current = Parameters(params=load_fixture('load_ltm_node_3.json')) + current = ApiParameters(params=load_fixture('load_ltm_node_3.json')) module = AnsibleModule( argument_spec=self.spec.argument_spec,