From 734b2ac57d77e50ab6dc3d00030e8eba50b21a12 Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman Date: Thu, 28 Dec 2023 18:08:52 -0300 Subject: [PATCH] ipahostgroup: Fix idempotence issues due to capitalization ipahostgroup parameters 'host', 'hostgroup', 'membermanager_user' and 'membermanager_group' must be compared in a case insensitive manner and stored as lower case strings. This patch fixes the comparison and storage of this parameters, and change the handling of members to use the same structure as in newer modules. Two new tests files were added: tests/hostgroup/test_hostgroup_case_insensitive.yml tests/hostgroup/test_hostgroup_membermanager_case_insensitive.yml --- plugins/modules/ipahostgroup.py | 228 ++++++++---------- .../test_hostgroup_case_insensitive.yml | 162 +++++++++++++ ...stgroup_membermanager_case_insensitive.yml | 227 +++++++++++++++++ 3 files changed, 496 insertions(+), 121 deletions(-) create mode 100644 tests/hostgroup/test_hostgroup_case_insensitive.yml create mode 100644 tests/hostgroup/test_hostgroup_membermanager_case_insensitive.yml diff --git a/plugins/modules/ipahostgroup.py b/plugins/modules/ipahostgroup.py index 7a0ca3c4fe..5ca1ae6d9e 100644 --- a/plugins/modules/ipahostgroup.py +++ b/plugins/modules/ipahostgroup.py @@ -149,7 +149,7 @@ from ansible.module_utils.ansible_freeipa_module import \ IPAAnsibleModule, compare_args_ipa, gen_add_del_lists, gen_add_list, \ - gen_intersection_list, ListOf, Hostname + gen_intersection_list, CaseInsensitive, Hostname, ListOf def find_hostgroup(module, name): @@ -302,6 +302,12 @@ def main(): commands = [] for name in names: + # clean add/del lists + host_add, host_del = [], [] + hostgroup_add, hostgroup_del = [], [] + membermanager_user_add, membermanager_user_del = [], [] + membermanager_group_add, membermanager_group_del = [], [] + # Make sure hostgroup exists res_find = find_hostgroup(ansible_module, name) @@ -324,63 +330,31 @@ def main(): # Set res_find to empty dict for next step res_find = {} - member_args = gen_member_args(host, hostgroup) - if not compare_args_ipa(ansible_module, member_args, - res_find): - # Generate addition and removal lists - host_add, host_del = gen_add_del_lists( - host, res_find.get("member_host")) - - hostgroup_add, hostgroup_del = gen_add_del_lists( - hostgroup, res_find.get("member_hostgroup")) - - # Add members - if len(host_add) > 0 or len(hostgroup_add) > 0: - commands.append([name, "hostgroup_add_member", - { - "host": host_add, - "hostgroup": hostgroup_add, - }]) - # Remove members - if len(host_del) > 0 or len(hostgroup_del) > 0: - commands.append([name, "hostgroup_remove_member", - { - "host": host_del, - "hostgroup": hostgroup_del, - }]) - - membermanager_user_add, membermanager_user_del = \ - gen_add_del_lists( - membermanager_user, - res_find.get("membermanager_user") - ) + # Generate addition and removal lists + host_add, host_del = gen_add_del_lists( + host, res_find.get("member_host"), + attr_datatype=( + Hostname(ansible_module.ipa_get_domain())), + ) - membermanager_group_add, membermanager_group_del = \ - gen_add_del_lists( - membermanager_group, - res_find.get("membermanager_group") - ) + hostgroup_add, hostgroup_del = gen_add_del_lists( + hostgroup, res_find.get("member_hostgroup"), + attr_datatype=CaseInsensitive(), + ) if has_add_membermanager: - # Add membermanager users and groups - if len(membermanager_user_add) > 0 or \ - len(membermanager_group_add) > 0: - commands.append( - [name, "hostgroup_add_member_manager", - { - "user": membermanager_user_add, - "group": membermanager_group_add, - }] + membermanager_user_add, membermanager_user_del = \ + gen_add_del_lists( + membermanager_user, + res_find.get("membermanager_user"), + attr_datatype=CaseInsensitive(), ) - # Remove member manager - if len(membermanager_user_del) > 0 or \ - len(membermanager_group_del) > 0: - commands.append( - [name, "hostgroup_remove_member_manager", - { - "user": membermanager_user_del, - "group": membermanager_group_del, - }] + + membermanager_group_add, membermanager_group_del = \ + gen_add_del_lists( + membermanager_group, + res_find.get("membermanager_group"), + attr_datatype=CaseInsensitive(), ) elif action == "member": @@ -390,45 +364,30 @@ def main(): # Reduce add lists for member_host and member_hostgroup, # to new entries only that are not in res_find. - if host is not None and "member_host" in res_find: - host = gen_add_list(host, res_find["member_host"]) - if hostgroup is not None \ - and "member_hostgroup" in res_find: - hostgroup = gen_add_list( - hostgroup, res_find["member_hostgroup"]) - - # Ensure members are present - commands.append([name, "hostgroup_add_member", - { - "host": host, - "hostgroup": hostgroup, - }]) + host_add = gen_add_list( + host, res_find.get("member_host"), + attr_datatype=( + Hostname(ansible_module.ipa_get_domain())), + ) + hostgroup_add = gen_add_list( + hostgroup, res_find.get("member_hostgroup"), + attr_datatype=CaseInsensitive(), + ) if has_add_membermanager: # Reduce add list for membermanager_user and # membermanager_group to new entries only that are # not in res_find. - if membermanager_user is not None \ - and "membermanager_user" in res_find: - membermanager_user = gen_add_list( - membermanager_user, - res_find["membermanager_user"]) - if membermanager_group is not None \ - and "membermanager_group" in res_find: - membermanager_group = gen_add_list( - membermanager_group, - res_find["membermanager_group"]) - - # Add membermanager users and groups - if membermanager_user is not None or \ - membermanager_group is not None: - commands.append( - [name, "hostgroup_add_member_manager", - { - "user": membermanager_user, - "group": membermanager_group, - }] - ) + membermanager_user_add = gen_add_list( + membermanager_user, + res_find.get("membermanager_user"), + attr_datatype=CaseInsensitive(), + ) + membermanager_group_add = gen_add_list( + membermanager_group, + res_find.get("membermanager_group"), + attr_datatype=CaseInsensitive(), + ) elif state == "renamed": if res_find is not None: @@ -459,46 +418,73 @@ def main(): # Reduce del lists of member_host and member_hostgroup, # to the entries only that are in res_find. if host is not None: - host = gen_intersection_list( - host, res_find.get("member_host")) + host_del = gen_intersection_list( + host, res_find.get("member_host"), + attr_datatype=( + Hostname(ansible_module.ipa_get_domain())), + ) if hostgroup is not None: - hostgroup = gen_intersection_list( - hostgroup, res_find.get("member_hostgroup")) - - # Ensure members are absent - commands.append([name, "hostgroup_remove_member", - { - "host": host, - "hostgroup": hostgroup, - }]) + hostgroup_del = gen_intersection_list( + hostgroup, res_find.get("member_hostgroup"), + attr_datatype=CaseInsensitive(), + ) if has_add_membermanager: - # Reduce del lists of membermanager_user and - # membermanager_group to the entries only that are - # in res_find. - if membermanager_user is not None: - membermanager_user = gen_intersection_list( - membermanager_user, - res_find.get("membermanager_user")) - if membermanager_group is not None: - membermanager_group = gen_intersection_list( - membermanager_group, - res_find.get("membermanager_group")) - - # Remove membermanager users and groups - if membermanager_user is not None or \ - membermanager_group is not None: - commands.append( - [name, "hostgroup_remove_member_manager", - { - "user": membermanager_user, - "group": membermanager_group, - }] - ) + # Get lists of membermanager users that exist + # in IPA and should be removed. + membermanager_user_del = gen_intersection_list( + membermanager_user, + res_find.get("membermanager_user"), + attr_datatype=CaseInsensitive(), + ) + membermanager_group_del = gen_intersection_list( + membermanager_group, + res_find.get("membermanager_group"), + attr_datatype=CaseInsensitive(), + ) else: ansible_module.fail_json(msg="Unkown state '%s'" % state) + # Manage members + + # Add members + if host_add or hostgroup_add: + commands.append([name, "hostgroup_add_member", + { + "host": host_add, + "hostgroup": hostgroup_add, + }]) + + # Remove members + if host_del or hostgroup_del: + commands.append([name, "hostgroup_remove_member", + { + "host": host_del, + "hostgroup": hostgroup_del, + }]) + + # Manage membermanager users and groups + if has_add_membermanager: + # Add membermanager users and groups + if membermanager_user_add or membermanager_group_add: + commands.append( + [name, "hostgroup_add_member_manager", + { + "user": membermanager_user_add, + "group": membermanager_group_add, + }] + ) + # Remove membermanager users and groups + if membermanager_user_del or membermanager_group_del: + commands.append( + [name, "hostgroup_remove_member_manager", + { + "user": membermanager_user_del, + "group": membermanager_group_del, + }] + ) + # Execute commands changed = ansible_module.execute_ipa_commands( diff --git a/tests/hostgroup/test_hostgroup_case_insensitive.yml b/tests/hostgroup/test_hostgroup_case_insensitive.yml new file mode 100644 index 0000000000..20a7d8a739 --- /dev/null +++ b/tests/hostgroup/test_hostgroup_case_insensitive.yml @@ -0,0 +1,162 @@ +--- +- name: Test hostgroup members case insensitive + hosts: ipaserver + become: true + gather_facts: false + module_defaults: + ipagroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + ipahost: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + ipahostgroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + + vars: + # Hostnames are supposed to have first letter + # capitalized for this test. + test_hosts: + - Host1 + - Host2 + test_hostgroups: + - testhostgroup1 + # TestHostgrop2 is meant to use CamelCase here. + - TestHostgroup2 + + tasks: + - name: Test in all supported versions of IPA + block: + # setup environment + - name: Ensure domain name is set + ansible.builtin.set_fact: + ipa_domain: "test.local" + when: ipa_domain is not defined + + - name: Ensure hostgroup testhostgroup1 and testhostgroup2 are absent + ipahostgroup: + name: "{{ test_hostgroups }}" + state: absent + + - name: Ensure test hosts are present + ipahost: + name: "{{ item }}.{{ ipa_domain }}" + force: true + loop: "{{ test_hosts }}" + + - name: Ensure hostgroup testhostgroup2 is present + ipahostgroup: + name: testhostgroup2 + + # tests + - name: Test hostgroup presence with single host and action hostgroup + vars: + test_cases: + - { value: "{{ test_hosts[0] | lower }}", expected: true } + - { value: "{{ test_hosts[0] | upper }}", expected: false } + - { value: "{{ test_hosts[0] }}", expected: false } + block: + - name: "Ensure hostgroup testhostgroup with host 'host1'" + ipahostgroup: + name: testhostgroup1 + host: "{{ item.value }}" + register: output + loop: "{{ test_cases }}" + loop_control: + index_var: index + label: "{{ item }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test hostgroup presence with multiple hosts and action hostgroup + vars: + test_cases: + - { value: "{{ test_hosts | lower }}", expected: true } + - { value: "{{ test_hosts | upper }}", expected: false } + - { value: "{{ test_hosts }}", expected: false } + - { value: "{{ test_hosts[1] }}", expected: true } + - { value: "{{ test_hosts[1] | lower }}", expected: false } + - { value: "{{ test_hosts[1] | upper }}", expected: false } + - { value: "{{ test_hosts[0] }}", expected: true } + - { value: "{{ test_hosts[0] | lower }}", expected: false } + - { value: "{{ test_hosts[0] | upper }}", expected: false } + block: + - name: "Ensure hostgroup testhostgroup with host 'host1'" + ipahostgroup: + name: testhostgroup1 + host: "{{ item.value }}" + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test hostgroup with multiple hosts and action member + vars: + test_cases: + - { value: "{{ test_hosts | lower }}", state: "absent", expected: true } + - { value: "{{ test_hosts | upper }}", state: "absent", expected: false } + - { value: "{{ test_hosts }}", state: "present", expected: true } + - { value: "{{ test_hosts[1] }}", state: "absent", expected: true } + - { value: "{{ test_hosts[1] | lower }}", state: "absent", expected: false } + - { value: "{{ test_hosts[1] | upper }}", state: "absent", expected: false } + - { value: "{{ test_hosts[0] | lower }}", state: "present", expected: false } + - { value: "{{ test_hosts[0] }}", state: "present", expected: false } + - { value: "{{ test_hosts[0] | upper }}", state: "present", expected: false } + - { value: "{{ test_hosts | upper }}", state: "present", expected: true } + - { value: "{{ test_hosts[1] }}", state: "present", expected: false } + - { value: "{{ test_hosts[0] | lower }}", state: "present", expected: false } + - { value: "{{ test_hosts[0] }}", state: "absent", expected: true } + - { value: "{{ test_hosts[0] | lower }}", state: "absent", expected: false } + - { value: "{{ test_hosts[0] | upper }}", state: "absent", expected: false } + block: + - name: "Ensure hostgroup testhostgroup with host 'host1'" + ipahostgroup: + name: testhostgroup1 + host: "{{ item.value }}" + action: member + state: "{{ item.state }}" + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + always: + # cleanup + - name: Ensure hostgroup testhostgroup is absent + ipahostgroup: + name: "{{ test_hostgroups }}" + state: absent + + - name: Ensure test hosts are absent + ipahost: + name: "{{ test_hosts | product([ipa_domain]) | map('join') | list }}" + state: absent diff --git a/tests/hostgroup/test_hostgroup_membermanager_case_insensitive.yml b/tests/hostgroup/test_hostgroup_membermanager_case_insensitive.yml new file mode 100644 index 0000000000..cddc2bd79e --- /dev/null +++ b/tests/hostgroup/test_hostgroup_membermanager_case_insensitive.yml @@ -0,0 +1,227 @@ +--- +- name: Test hostgroup membermanagers + hosts: ipaserver + become: true + gather_facts: false + module_defaults: + ipagroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + ipauser: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + ipahostgroup: + ipaadmin_password: SomeADMINpassword + ipaapi_context: "{{ ipa_context | default(omit) }}" + + tasks: + - name: Include tasks ../env_freeipa_facts.yml + ansible.builtin.include_tasks: ../env_freeipa_facts.yml + + - name: Tests requiring IPA version 4.8.4+ + when: ipa_version is version('4.8.4', '>=') + block: + # setup environment + - name: Ensure host-group testhostgroup is absent + ipahostgroup: + name: testhostgroup + state: absent + + - name: Ensure user manageruser1 and manageruser2 are present + ipauser: + users: + - name: manageruser1 + first: manageruser1 + last: Last1 + - name: manageruser2 + first: manageruser2 + last: Last2 + + - name: Ensure managergroup1 and managergroup2 are present + ipagroup: + groups: + - name: managergroup1 + - name: managergroup2 + + # tests + - name: Ensure host-group testhostgroup is present + ipahostgroup: + name: testhostgroup + + - name: Test membermanager_user parameter presence + vars: + test_cases: + - { value: "{{ 'ManagerUser1' | lower }}", expected: true } + - { value: "{{ 'ManagerUser1' | upper }}", expected: false } + - { value: 'ManagerUser1', expected: false } + block: + - name: "Ensure membermanager_user 'manageruser1' is present for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_user: "{{ item.value }}" + action: member + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test membermanager_group parameter presence + vars: + test_cases: + - { value: "{{ 'ManagerGroup1' | upper }}", expected: true } + - { value: "{{ 'ManagerGroup1' | lower }}", expected: false } + - { value: 'ManagerGroup1', expected: false } + block: + - name: "Ensure membermanager_group 'managergroup1' is present for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_group: "{{ item.value }}" + action: member + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item.value }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test membermanager_group and membermanager_user parameters presence + vars: + test_cases: + - { user: 'ManagerUser2', group: 'ManagerGroup2', expected: true } + - { user: "{{ 'ManagerUser2' | upper }}", group: "{{ 'ManagerGroup2' | upper }}", expected: false } + - { user: "{{ 'ManagerUser2' | lower }}", group: "{{ 'ManagerGroup2' | lower }}", expected: false } + block: + - name: "Ensure membermanager_group 'managergroup2' and membermanager_user 'manageruser2' are present for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_group: "{{ item.group }}" + membermanager_user: "{{ item.user }}" + action: member + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item.user }}, group: {{ item.group }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test membermanager_group parameter absence + vars: + test_cases: + - { value: 'ManagerGroup1', expected: true } + - { value: "{{ 'ManagerGroup1' | lower }}", expected: false } + - { value: "{{ 'ManagerGroup1' | upper }}", expected: false } + block: + - name: "Ensure membermanager_group 'managergroup1' is absent for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_group: "{{ item.value }}" + action: member + state: absent + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item.value }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test membermanager_user parameter absence + vars: + test_cases: + - { value: 'ManagerUser1', expected: true } + - { value: "{{ 'ManagerUser1' | lower }}", expected: false } + - { value: "{{ 'ManagerUser1' | upper }}", expected: false } + block: + - name: "Ensure membermanager_user 'manageruser1' is absent for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_user: "{{ item.value }}" + action: member + state: absent + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item.value }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + - name: Test membermanager_group and membermanager_user parameters absence + vars: + test_cases: + - { user: "{{ 'ManagerUser2' | lower }}", group: "{{ 'ManagerGroup2' | lower }}", expected: true } + - { user: 'ManagerUser2', group: 'ManagerGroup2', expected: false } + - { user: "{{ 'ManagerUser2' | upper }}", group: "{{ 'ManagerGroup2' | upper }}", expected: false } + block: + - name: "Ensure membermanager_user 'manageruser2' and membermanager_group 'managergroup2' are absent for testhostgroup" + ipahostgroup: + name: testhostgroup + membermanager_group: "{{ item.group }}" + membermanager_user: "{{ item.user }}" + action: member + state: absent + register: output + loop: "{{ test_cases }}" + loop_control: + label: "{{ item.user }}, group: {{ item.group }}" + - name: "Verify results" + ansible.builtin.assert: + that: run.changed == run.item.expected or run.failed + fail_msg: "{{ run.msg | default('Failed condition: expected=' ~ run.item.expected ~ ', observed=' ~ run.changed) }}" + quiet: true + loop: "{{ output.results }}" + loop_control: + loop_var: run + label: "{{ run.item }}, output={'changed': {{ run.changed }}, 'failed': {{ run.failed}} }" + + always: + # cleanup + - name: Ensure host-group testhostgroup is absent + ipahostgroup: + name: testhostgroup + state: absent + + - name: Ensure user manangeruser1 and manageruser2 is absent + ipauser: + name: manageruser1,manageruser2 + state: absent + + - name: Ensure group managergroup1 and managergroup2 are absent + ipagroup: + name: managergroup1,managergroup2 + state: absent