From c8d5cb7ee2a3b4d9550a08dd231c9020fa5ccc18 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Wed, 29 Jun 2022 18:54:04 -0300 Subject: [PATCH 1/4] Fix handling of boolean values for FreeIPA 4.9.10+ FreeIPA 4.9.10+ and 4.10 use proper mapping for boolean values, and only searching for "TRUE" does not work anymore. This patch fix ipadnszone plugin and IPAParamMapping class handling of boolean values. --- plugins/module_utils/ansible_freeipa_module.py | 5 ++++- plugins/modules/ipaconfig.py | 6 +++++- plugins/modules/ipadnsforwardzone.py | 8 +++++++- plugins/modules/ipadnszone.py | 6 +++++- plugins/modules/ipahbacrule.py | 18 +++++++++++++----- plugins/modules/ipasudorule.py | 16 ++++++++++++---- 6 files changed, 46 insertions(+), 13 deletions(-) diff --git a/plugins/module_utils/ansible_freeipa_module.py b/plugins/module_utils/ansible_freeipa_module.py index 82f48e85..862aaabb 100644 --- a/plugins/module_utils/ansible_freeipa_module.py +++ b/plugins/module_utils/ansible_freeipa_module.py @@ -845,7 +845,10 @@ else: # Check if param_name is actually a param if param_name in self.ansible_module.params: value = self.ansible_module.params_get(param_name) - if isinstance(value, bool): + if ( + self.ansible_module.ipa_check_version("<", "4.9.10") + and isinstance(value, bool) + ): value = "TRUE" if value else "FALSE" # Since param wasn't a param check if it's a method name diff --git a/plugins/modules/ipaconfig.py b/plugins/modules/ipaconfig.py index f7901f2c..6731e37d 100644 --- a/plugins/modules/ipaconfig.py +++ b/plugins/modules/ipaconfig.py @@ -441,7 +441,11 @@ def main(): elif ( isinstance(value, (tuple, list)) and arg_type == "bool" ): - exit_args[k] = (value[0] == "TRUE") + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + exit_args[k] = (str(value[0]).upper() == "TRUE") else: if arg_type not in type_map: raise ValueError( diff --git a/plugins/modules/ipadnsforwardzone.py b/plugins/modules/ipadnsforwardzone.py index 99aa226d..f4c6c634 100644 --- a/plugins/modules/ipadnsforwardzone.py +++ b/plugins/modules/ipadnsforwardzone.py @@ -344,7 +344,13 @@ def main(): if state in ['enabled', 'disabled']: if existing_resource is not None: - is_enabled = existing_resource["idnszoneactive"][0] + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + is_enabled = ( + str(existing_resource["idnszoneactive"][0]).upper() + ) else: ansible_module.fail_json( msg="dnsforwardzone '%s' not found." % (name)) diff --git a/plugins/modules/ipadnszone.py b/plugins/modules/ipadnszone.py index ae9b7516..d64e6e0a 100644 --- a/plugins/modules/ipadnszone.py +++ b/plugins/modules/ipadnszone.py @@ -418,7 +418,11 @@ class DNSZoneModule(IPAAnsibleModule): is_zone_active = False else: zone = response["result"] - is_zone_active = "TRUE" in zone.get("idnszoneactive") + # FreeIPA 4.9.10+ and 4.10 use proper mapping for boolean vaalues. + # See: https://github.com/freeipa/freeipa/pull/6294 + is_zone_active = ( + str(zone.get("idnszoneactive")[0]).upper() == "TRUE" + ) return zone, is_zone_active diff --git a/plugins/modules/ipahbacrule.py b/plugins/modules/ipahbacrule.py index 002004ad..f1e0a5c4 100644 --- a/plugins/modules/ipahbacrule.py +++ b/plugins/modules/ipahbacrule.py @@ -472,18 +472,26 @@ def main(): # hbacrule_enable is not failing on an enabled hbacrule # Therefore it is needed to have a look at the ipaenabledflag # in res_find. - if "ipaenabledflag" not in res_find or \ - res_find["ipaenabledflag"][0] != "TRUE": + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + enabled_flag = str(res_find.get("ipaenabledflag", [False])[0]) + if enabled_flag.upper() != "TRUE": commands.append([name, "hbacrule_enable", {}]) elif state == "disabled": if res_find is None: ansible_module.fail_json(msg="No hbacrule '%s'" % name) - # hbacrule_disable is not failing on an disabled hbacrule + # hbacrule_disable is not failing on an enabled hbacrule # Therefore it is needed to have a look at the ipaenabledflag # in res_find. - if "ipaenabledflag" not in res_find or \ - res_find["ipaenabledflag"][0] != "FALSE": + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + enabled_flag = str(res_find.get("ipaenabledflag", [False])[0]) + if enabled_flag.upper() != "FALSE": commands.append([name, "hbacrule_disable", {}]) else: diff --git a/plugins/modules/ipasudorule.py b/plugins/modules/ipasudorule.py index 7d6cd860..fd3671ed 100644 --- a/plugins/modules/ipasudorule.py +++ b/plugins/modules/ipasudorule.py @@ -656,8 +656,12 @@ def main(): # sudorule_enable is not failing on an enabled sudorule # Therefore it is needed to have a look at the ipaenabledflag # in res_find. - if "ipaenabledflag" not in res_find or \ - res_find["ipaenabledflag"][0] != "TRUE": + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + enabled_flag = str(res_find.get("ipaenabledflag", [False])[0]) + if enabled_flag.upper() != "TRUE": commands.append([name, "sudorule_enable", {}]) elif state == "disabled": @@ -666,8 +670,12 @@ def main(): # sudorule_disable is not failing on an disabled sudorule # Therefore it is needed to have a look at the ipaenabledflag # in res_find. - if "ipaenabledflag" not in res_find or \ - res_find["ipaenabledflag"][0] != "FALSE": + # FreeIPA 4.9.10+ and 4.10 use proper mapping for + # boolean values, so we need to convert it to str + # for comparison. + # See: https://github.com/freeipa/freeipa/pull/6294 + enabled_flag = str(res_find.get("ipaenabledflag", [False])[0]) + if enabled_flag.upper() != "FALSE": commands.append([name, "sudorule_disable", {}]) else: From 87ff15a92ccbbed37ae213e40b83d461981fd15e Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Fri, 1 Jul 2022 10:03:55 -0300 Subject: [PATCH 2/4] api_check_ipa_version: Fix version comparison for more than one digit The fallback function used to compare IPA versions was spliting the version string into a tuple of strings, and the comparison of the tuple would fail if comparing a field with one digit aginst a two-digit one, for example, '8' with '10', as the string comparison would put '10' before the '8'. This patch forces the version fields to be converted to integers, so a numerical comparison will be performed. If a version string field cannot be converted to a number, than the string comparison will still be used. --- plugins/module_utils/ansible_freeipa_module.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/module_utils/ansible_freeipa_module.py b/plugins/module_utils/ansible_freeipa_module.py index 862aaabb..b6c17625 100644 --- a/plugins/module_utils/ansible_freeipa_module.py +++ b/plugins/module_utils/ansible_freeipa_module.py @@ -67,9 +67,15 @@ else: """ Split a version string A.B.C, into a tuple. - This will not work for `rc`, `dev` or similar version string. + This will not work for `rc`, `dev` or similar. """ - return tuple(re.split("[-_.]", version_str)) # noqa: W605 + try: + _version = tuple( + (int(x) for x in re.split("[-_.]", version_str)) + ) + except ValueError: + _version = tuple(re.split("[-_.]", version_str)) + return _version from ipalib import api from ipalib import errors as ipalib_errors # noqa From 8ab3aa06ff3297498760d11af4bdc36708f9c413 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Mon, 4 Jul 2022 12:15:40 -0300 Subject: [PATCH 3/4] pytest tests: Enhanced assertion for check_* methods. Checking if some output is present or absent from standard streams was done by simple string searching. Due to recent changes in FreeIPA, this search is not effective due to capitalization differences in boolean values output. Changing the string searching to regular expression searches fixes this behavior for current and previous versions of FreeIPA. This patch also adds more information on the assert tests in case of an error, so that it is easier to understand why the test failed. --- tests/utils.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index db22f973..c7e630a5 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -21,6 +21,7 @@ import os import pytest +import re import subprocess import tempfile import testinfra @@ -314,6 +315,10 @@ class AnsibleFreeIPATestCase(TestCase): expected_msg in result.stderr.decode("utf8") ) + @staticmethod + def __is_text_on_data(text, data): + return re.search(text, data) is not None + def check_details(self, expected_output, cmd, extra_cmds=None): cmd = "ipa " + cmd if extra_cmds: @@ -322,10 +327,16 @@ class AnsibleFreeIPATestCase(TestCase): res = self.master.run(cmd) if res.rc != 0: for output in expected_output: - assert output in res.stderr + assert self.__is_text_on_data(output, res.stderr), ( + f"\n{'='*40}\nExpected: {output}\n{'='*40}\n" + + f"Output:\n{res.stderr}{'='*40}\n" + ) else: for output in expected_output: - assert output in res.stdout + assert self.__is_text_on_data(output, res.stdout), ( + f"\n{'='*40}\nExpected: {output}\n{'='*40}\n" + + f"Output:\n{res.stdout}{'='*40}\n" + ) kdestroy(self.master) def check_notexists(self, members, cmd, extra_cmds=None): @@ -335,7 +346,10 @@ class AnsibleFreeIPATestCase(TestCase): kinit_admin(self.master) res = self.master.run(cmd) for member in members: - assert member not in res.stdout + assert not self.__is_text_on_data(member, res.stdout), ( + f"\n{'='*40}\nExpected: {member}\n{'='*40}\n" + + f"Output:\n{res.stdout}{'='*40}\n" + ) kdestroy(self.master) def mark_xfail_using_ansible_freeipa_version(self, version, reason): From a5306b2db5405ea9c9a9cd8a08e3850467ad8b13 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Tue, 5 Jul 2022 15:38:39 -0300 Subject: [PATCH 4/4] pytests/test_dnszone: Fix evaluation of boolean values Evaluating boolean values output by FreeIPA must use regular expressions to handle both "TRUE/FALSE" and "True/False". --- tests/pytests/dnszone/test_dnszone.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/pytests/dnszone/test_dnszone.py b/tests/pytests/dnszone/test_dnszone.py index 00978f0f..67a98548 100644 --- a/tests/pytests/dnszone/test_dnszone.py +++ b/tests/pytests/dnszone/test_dnszone.py @@ -64,18 +64,26 @@ class TestDNSZone(AnsibleFreeIPATestCase): def test_dnszone_disable(self): """TC-30: Disable DNS Zone.""" zone26 = "26testzone.test" - self.check_details(["Active zone: TRUE"], "dnszone-find", [zone26]) + self.check_details( + ["Active zone: (TRUE|True)"], "dnszone-find", [zone26] + ) # Disable dns zone self.run_playbook(BASE_PATH + "dnszone_disable.yaml") - self.check_details(["Active zone: FALSE"], "dnszone-find", [zone26]) + self.check_details( + ["Active zone: (FALSE|False)"], "dnszone-find", [zone26] + ) def test_dnszone_enable(self): """TC-31: Enable DNS Zone.""" zone26 = "26testzone.test" - self.check_details(["Active zone: FALSE"], "dnszone-find", [zone26]) + self.check_details( + ["Active zone: (FALSE|False)"], "dnszone-find", [zone26] + ) # Enable dns zone self.run_playbook(BASE_PATH + "dnszone_enable.yaml") - self.check_details(["Active zone: TRUE"], "dnszone-find", [zone26]) + self.check_details( + ["Active zone: (TRUE|True)"], "dnszone-find", [zone26] + ) def test_dnszone_name_from_ip(self): """TC-35: Add dns zone with reverse zone IP. Bug#1845056."""