From dd700d956bb3d418314561570dbd54634186ac4d Mon Sep 17 00:00:00 2001 From: Rafael Guterres Jeffman <rjeffman@redhat.com> Date: Wed, 5 Jan 2022 18:22:41 -0300 Subject: [PATCH] Fixed automountkey code review issues. Fixed several issues found during code review and change AutomountkeyModule to use IPAAnsibleModule instead of deprecated FreeIPABaseModule. --- README-automountkey.md | 54 ++-- playbooks/automount/automountkey-present.yml | 13 + playbooks/automount/automountkey-renamed.yml | 13 + playbooks/automount/automoutkey-absent.yml | 12 + plugins/modules/ipaautomountkey.py | 218 +++++++------ tests/automount/test_automountkey.yml | 288 ++++++++---------- .../test_automountkey_client_context.yml | 41 +++ 7 files changed, 359 insertions(+), 280 deletions(-) create mode 100644 playbooks/automount/automountkey-present.yml create mode 100644 playbooks/automount/automountkey-renamed.yml create mode 100644 playbooks/automount/automoutkey-absent.yml create mode 100644 tests/automount/test_automountkey_client_context.yml diff --git a/README-automountkey.md b/README-automountkey.md index a4c1ab69..cb49b5b5 100644 --- a/README-automountkey.md +++ b/README-automountkey.md @@ -4,7 +4,7 @@ Automountkey module Description ----------- -The automountkey module allows the addition and removal of keys within an automount map. +The automountkey module allows management of keys within an automount map. It is desgined to follow the IPA api as closely as possible while ensuring ease of use. @@ -38,16 +38,15 @@ ipaserver.test.local ``` -Example playbook to ensure presence of an automount map: +Example playbook to ensure presence of an automount key: ```yaml --- -- name: Playbook to add an automount map +- name: Playbook to manage automount key hosts: ipaserver - become: true tasks: - - name: create key TestKey + - name: ensure automount key TestKey is present ipaautomountkey: ipaadmin_password: SomeADMINpassword location: TestLocation @@ -55,14 +54,6 @@ Example playbook to ensure presence of an automount map: key: TestKey info: 192.168.122.1:/exports state: present - - - name: ensure key TestKey is absent - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - location: TestLocation - mapname: TestMap - key: TestKey - state: absent ``` Example playbook to rename an automount map: @@ -70,33 +61,50 @@ Example playbook to rename an automount map: ```yaml --- - name: Playbook to add an automount map - - name: ensure key TestKey has been renamed to NewKeyName + hosts: ipaserver + + tasks: + - name: ensure aumount key TestKey is renamed to NewKeyName ipaautomountkey: ipaadmin_password: password01 automountlocationcn: TestLocation automountmapname: TestMap automountkey: TestKey newname: NewKeyName - state: rename + state: renamed +``` + +Example playbook to ensure an automount key is absent: + +```yaml +--- +- name: Playbook to manage an automount key + hosts: ipaserver + + tasks: + - name: ensure automount key TestKey is absent + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + mapname: TestMap + key: TestKey + state: absent ``` Variables ========= -ipaautomountkey -------- - Variable | Description | Required -------- | ----------- | -------- `ipaadmin_principal` | The admin principal is a string and defaults to `admin` | no `ipaadmin_password` | The admin password is a string and is required if there is no admin ticket available on the node | no -`location` \| `automountlocationcn` \| `automountlocation` | Location name. | yes -`mapname` \| `map` \| `automountmapname` \| `automountmap` | Map the key belongs to | yes -`key` \| `name` \| `automountkey` | Automount key to manage | yes -`newkey` \| newname` \| `newautomountkey` | the name to change the key to if state is `rename` | yes when state is `rename` +`location` \| `automountlocationcn` \| `automountlocation` | Location name. | yes +`mapname` \| `map` \| `automountmapname` \| `automountmap` | Map the key belongs to | yes +`key` \| `name` \| `automountkey` | Automount key to manage | yes +`rename` \| `new_name` \| `newautomountkey` | the name to change the key to if state is `renamed` | yes when state is `renamed` `info` \| `information` \| `automountinformation` | Mount information for the key | yes when state is `present` -`state` | The state to ensure. It can be one of `present`, or `absent`, default: `present`. | no +`state` | The state to ensure. It can be one of `present`, `absent` or `renamed`, default: `present`. | no Authors ======= diff --git a/playbooks/automount/automountkey-present.yml b/playbooks/automount/automountkey-present.yml new file mode 100644 index 00000000..50f51bc3 --- /dev/null +++ b/playbooks/automount/automountkey-present.yml @@ -0,0 +1,13 @@ +--- +- name: Playbook to manage an automout key + hosts: ipaserver + + tasks: + - name: Ensure autmount key is present + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + mapname: TestMap + key: TestKey + info: 192.168.122.1:/exports + state: present diff --git a/playbooks/automount/automountkey-renamed.yml b/playbooks/automount/automountkey-renamed.yml new file mode 100644 index 00000000..ff60d117 --- /dev/null +++ b/playbooks/automount/automountkey-renamed.yml @@ -0,0 +1,13 @@ +--- +- name: Playbook to manage an automount key + hosts: ipaserver + + tasks: + - name: Ensure aumount key TestKey is renamed to NewKeyName + ipaautomountkey: + ipaadmin_password: password01 + automountlocationcn: TestLocation + automountmapname: TestMap + automountkey: TestKey + newname: NewKeyName + state: renamed diff --git a/playbooks/automount/automoutkey-absent.yml b/playbooks/automount/automoutkey-absent.yml new file mode 100644 index 00000000..99eadbe3 --- /dev/null +++ b/playbooks/automount/automoutkey-absent.yml @@ -0,0 +1,12 @@ +--- +- name: Playbook to manage an automount key + hosts: ipaserver + + tasks: + - name: Ensure autmount key is present + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + mapname: TestMap + key: TestKey + state: absent diff --git a/plugins/modules/ipaautomountkey.py b/plugins/modules/ipaautomountkey.py index 772ff2ed..8eac6896 100644 --- a/plugins/modules/ipaautomountkey.py +++ b/plugins/modules/ipaautomountkey.py @@ -20,6 +20,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see <http://www.gnu.org/licenses/>. +from __future__ import (absolute_import, division, print_function) + +__metaclass__ = type + ANSIBLE_METADATA = { "metadata_version": "1.0", "supported_by": "community", @@ -54,18 +58,18 @@ options: required: True choices: ["name", "automountkey"] newkey: - description: key to change to if state=rename + description: key to change to if state is 'renamed' required: True choices: ["newname", "newautomountkey"] info: description: Mount information for the key required: True - choices: ["information", "newinfo", "automountinformation"] + choices: ["information", "automountinformation"] state: description: State to ensure required: False default: present - choices: ["present", "absent", "rename"] + choices: ["present", "absent", "renamed"] ''' EXAMPLES = ''' @@ -91,126 +95,140 @@ RETURN = ''' ''' from ansible.module_utils.ansible_freeipa_module import ( - FreeIPABaseModule, ipalib_errors + IPAAnsibleModule, ipalib_errors ) -class AutomountKey(FreeIPABaseModule): +class AutomountKey(IPAAnsibleModule): - ipa_param_mapping = { - 'automountkey': "key", - 'automountmapautomountmapname': "mapname", - } + def __init__(self, *args, **kwargs): + # pylint: disable=super-with-arguments + super(AutomountKey, self).__init__(*args, **kwargs) + self.commands = [] - def get_key(self, location, mapname, keyname): - resp = dict() + def get_key(self, location, mapname, key): try: - resp = self.api_command("automountkey_show", - location, - {"automountmapautomountmapname": mapname, - "automountkey": keyname}) + args = { + "automountmapautomountmapname": mapname, + "automountkey": key, + "all": True, + } + resp = self.ipa_command("automountkey_show", location, args) except ipalib_errors.NotFound: - pass - - return resp.get("result", None) + return None + else: + return resp.get("result") def check_ipa_params(self): - if not self.ipa_params.info and self.ipa_params.state == "present": - self.fail_json(msg="Value required for argument 'info'") - - if self.ipa_params.state == "rename" and \ - self.ipa_params.newname is None: - self.fail_json(msg="newname is required if state = 'rename'") + invalid = [] + state = self.params_get("state") + if state == "present": + invalid = ["rename"] + if not self.params_get("info"): + self.fail_json(msg="Value required for argument 'info'") + + if state == "rename": + invalid = ["info"] + if not self.params_get("rename"): + self.fail_json(msg="Value required for argument 'renamed'") + + if state == "absent": + invalid = ["info", "rename"] + + self.params_fail_used_invalid(invalid, state) + + @staticmethod + def get_args(mapname, key, info, rename): + _args = {} + if mapname: + _args["automountmapautomountmapname"] = mapname + if key: + _args["automountkey"] = key + if info: + _args["automountinformation"] = info + if rename: + _args["rename"] = rename + return _args def define_ipa_commands(self): - args = self.get_ipa_command_args() - key = self.get_key(self.ipa_params.location, - self.ipa_params.mapname, - self.ipa_params.key) + state = self.params_get("state") + location = self.params_get("location") + mapname = self.params_get("mapname") + key = self.params_get("key") + info = self.params_get("info") + rename = self.params_get("rename") + + args = self.get_args(mapname, key, info, rename) + + res_find = self.get_key(location, mapname, key) - if self.ipa_params.state == "present": - if key is None: + if state == "present": + if res_find is None: # does not exist and is wanted - args["automountinformation"] = self.ipa_params.info - self.add_ipa_command( - "automountkey_add", - name=self.ipa_params.location, - args=args - ) - elif key is not None: + self.commands.append([location, "automountkey_add", args]) + else: # exists and is wanted, check for changes - if self.ipa_params.info != \ - key.get('automountinformation', [None])[0]: - args["newautomountinformation"] = self.ipa_params.info - self.add_ipa_command( - "automountkey_mod", - name=self.ipa_params.location, - args=args - ) - elif self.ipa_params.state == "rename": - if key is not None: - newkey = self.get_key(self.ipa_params.location, - self.ipa_params.mapname, - self.ipa_params.newname) - - args["rename"] = self.ipa_params.newname - if newkey is None: - self.add_ipa_command( - "automountkey_mod", - name=self.ipa_params.location, - args=args + if info not in res_find.get("automountinformation"): + self.commands.append([location, "automountkey_mod", args]) + + if state == "renamed": + if res_find is None: + self.fail_json( + msg=( + "Cannot rename inexistent key: '%s', '%s', '%s'" + % (location, mapname, key) ) - else: - # if key exists and self.ipa_params.state == "absent": - if key is not None: - self.add_ipa_command( - "automountkey_del", - name=self.ipa_params.location, - args=args ) + self.commands.append([location, "automountkey_mod", args]) + + if state == "absent": + # if key exists and self.ipa_params.state == "absent": + if res_find is not None: + self.commands.append([location, "automountkey_del", args]) def main(): ipa_module = AutomountKey( argument_spec=dict( - ipaadmin_principal=dict(type="str", - default="admin" - ), - ipaadmin_password=dict(type="str", - required=False, - no_log=True - ), - state=dict(type='str', - default='present', - choices=['present', 'absent', 'rename'] - ), - location=dict(type="str", - aliases=["automountlocationcn", "automountlocation"], - default=None, - required=True - ), - newname=dict(type="str", - aliases=["newkey", "new_name", - "new_key", "newautomountkey"], - default=None, - required=False - ), - mapname=dict(type="str", - aliases=["map", "automountmapname", "automountmap"], - default=None, - required=True - ), - key=dict(type="str", - required=True, - aliases=["name", "automountkey"] - ), - info=dict(type="str", - aliases=["information", "newinfo", - "automountinformation"] - ), + state=dict( + type='str', + choices=['present', 'absent', 'renamed'], + required=None, + default='present', + ), + location=dict( + type="str", + aliases=["automountlocationcn", "automountlocation"], + required=True, + ), + rename=dict( + type="str", + aliases=["new_name", "newautomountkey"], + required=False, + ), + mapname=dict( + type="str", + aliases=["map", "automountmapname", "automountmap"], + required=True, + ), + key=dict( + type="str", + aliases=["name", "automountkey"], + required=True, + ), + info=dict( + type="str", + aliases=["information", "automountinformation"], + required=False, + ), ), ) - ipa_module.ipa_run() + ipaapi_context = ipa_module.params_get("ipaapi_context") + with ipa_module.ipa_connect(context=ipaapi_context): + ipa_module.check_ipa_params() + ipa_module.define_ipa_commands() + changed = ipa_module.execute_ipa_commands(ipa_module.commands) + ipa_module.exit_json(changed=changed) if __name__ == "__main__": diff --git a/tests/automount/test_automountkey.yml b/tests/automount/test_automountkey.yml index 19c8922a..6ab6beba 100644 --- a/tests/automount/test_automountkey.yml +++ b/tests/automount/test_automountkey.yml @@ -1,180 +1,154 @@ --- - name: Test automountmap - hosts: ipaserver - become: true - gather_facts: false + hosts: "{{ ipa_test_host | default('ipaserver') }}" + become: no + gather_facts: no tasks: - - name: ensure location TestLocation is absent + - name: ensure test location TestLocation is present ipaautomountlocation: ipaadmin_password: SomeADMINpassword name: TestLocation - state: absent - -# - name: ensure map TestMap is absent - # ipaautomountmap: - # ipaadmin_password: SomeADMINpassword - # name: TestMap - # location: TestLocation - # state: absent -# -# - name: ensure key TestKey is absent -# ipaautomountkey: -# ipaadmin_password: SomeADMINpassword -# automountlocationcn: TestLocation -# automountmapname: TestMap -# automountkey: TestKey -# state: absent - - - name: create location TestLocation - ipaautomountlocation: - ipaadmin_password: SomeADMINpassword - name: TestLocation - state: present - - name: create map TestMap + - name: ensure test map TestMap is present ipaautomountmap: ipaadmin_password: SomeADMINpassword name: TestMap location: TestLocation - desc: "this is a test map that should be deleted by the test" - -### test the key creation, and modification - - name: ensure key TestKey is present - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/exports - state: present - register: result - failed_when: result.failed or not result.changed - - - name: ensure key TestKey is present again - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/exports - state: present - register: result - failed_when: result.failed or result.changed - -## modify the key - - name: ensure key TestKey information has been updated - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/nfsshare - state: present - register: result - failed_when: result.failed or not result.changed - - - name: ensure key TestKey information has been updated again - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/nfsshare - state: present - register: result - failed_when: result.failed or result.changed - -## modify the name - - name: ensure key TestKey has been renamed to NewKeyName - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/nfsshare - newname: NewKeyName - state: rename - register: result - failed_when: result.failed or not result.changed - - - name: ensure key TestKey does not exist - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: NewKeyName - automountinformation: 192.168.122.1:/nfsshare - state: present - register: result - failed_when: result.failed or result.changed - - - name: ensure key NewKeyName does exist - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: NewKeyName - automountinformation: 192.168.122.1:/nfsshare - state: present - register: result - failed_when: result.failed or result.changed - - - name: ensure key TestKey has been renamed to NewKeyName again - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/nfsshare - newname: NewKeyName - state: rename - register: result - failed_when: result.failed or result.changed - - - name: ensure failure when state=present and newname is not set - ipaautomountkey: - ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: TestKey - automountinformation: 192.168.122.1:/nfsshare - state: rename - register: result - failed_when: not result.failed - -### cleanup after the tests - name: ensure key NewKeyName is absent ipaautomountkey: ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: NewKeyName + location: TestLocation + map: TestMap + key: NewKeyName state: absent - register: result - failed_when: result.failed or not result.changed - - name: ensure key TestKey is absent again + - name: ensure key TestKey is absent ipaautomountkey: ipaadmin_password: SomeADMINpassword - automountlocationcn: TestLocation - automountmapname: TestMap - automountkey: NewKeyName - state: absent - register: result - failed_when: result.failed or result.changed - - - name: ensure map TestMap is removed - ipaautomountmap: - ipaadmin_password: SomeADMINpassword - name: TestMap location: TestLocation + map: TestMap + key: NewKeyName state: absent - - name: ensure location TestLocation is removed - ipaautomountlocation: - ipaadmin_password: SomeADMINpassword - name: TestLocation - state: absent - + - block: + ### test the key creation, and modification + - name: ensure key TestKey is present + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + info: 192.168.122.1:/exports + state: present + register: result + failed_when: result.failed or not result.changed + + - name: ensure key TestKey is present again + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + info: 192.168.122.1:/exports + state: present + register: result + failed_when: result.failed or result.changed + + ## modify the key + - name: ensure key TestKey information has been updated + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + info: 192.168.122.1:/nfsshare + state: present + register: result + failed_when: result.failed or not result.changed + + - name: ensure key TestKey information has been updated again + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + info: 192.168.122.1:/nfsshare + state: present + register: result + failed_when: result.failed or result.changed + + ## modify the name + - name: ensure key TestKey has been renamed to NewKeyName + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + new_name: NewKeyName + state: renamed + register: result + failed_when: result.failed or not result.changed + + - name: ensure key TestKey is absent + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + state: absent + register: result + failed_when: result.failed or result.changed + + - name: ensure key NewKeyName is present + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: NewKeyName + info: 192.168.122.1:/nfsshare + state: present + register: result + failed_when: result.failed or result.changed + + - name: ensure failure when state is renamed and newname is not set + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: TestKey + state: renamed + register: result + failed_when: not result.failed + + ### cleanup after the tests + always: + - name: ensure key NewKeyName is absent + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: NewKeyName + state: absent + + - name: ensure key TestKey is absent + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + location: TestLocation + map: TestMap + key: NewKeyName + state: absent + + - name: ensure map TestMap is absent + ipaautomountmap: + ipaadmin_password: SomeADMINpassword + name: TestMap + location: TestLocation + state: absent + + - name: ensure location TestLocation is absent + ipaautomountlocation: + ipaadmin_password: SomeADMINpassword + name: TestLocation + state: absent diff --git a/tests/automount/test_automountkey_client_context.yml b/tests/automount/test_automountkey_client_context.yml new file mode 100644 index 00000000..e6d611b2 --- /dev/null +++ b/tests/automount/test_automountkey_client_context.yml @@ -0,0 +1,41 @@ +--- +- name: Test automountkey + hosts: ipaclients, ipaserver + become: no + gather_facts: no + + tasks: + - name: Include FreeIPA facts. + include_tasks: ../env_freeipa_facts.yml + + # Test will only be executed if host is not a server. + - name: Execute with server context in the client. + ipaautomountkey: + ipaadmin_password: SomeADMINpassword + ipaapi_context: server + location: NoLocation + map: NoMap + key: ThisShouldNotWork + register: result + failed_when: not (result.failed and result.msg is regex("No module named '*ipaserver'*")) + when: ipa_host_is_client + +# Import basic module tests, and execute with ipa_context set to 'client'. +# If ipaclients is set, it will be executed using the client, if not, +# ipaserver will be used. +# +# With this setup, tests can be executed against an IPA client, against +# an IPA server using "client" context, and ensure that tests are executed +# in upstream CI. + +- name: Test automountlocation using client context, in client host. + import_playbook: test_automountkey.yml + when: groups['ipaclients'] + vars: + ipa_test_host: ipaclients + +- name: Test automountlocation using client context, in server host. + import_playbook: test_automountkey.yml + when: groups['ipaclients'] is not defined or not groups['ipaclients'] + vars: + ipa_context: client -- GitLab