Improved parameter handling on proxmox modules (#1765) (#1801)

* Improved parameter handling on proxmox modules

* removed unused imports

* rollback change in plugins/modules/cloud/misc/proxmox_user_info.py

* added changelog fragment

* Update changelogs/fragments/1765-proxmox-params.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 0a5f79724c)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

changelogs/fragments/1765-proxmox-params.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - proxmox* modules - refactored some parameter validation code into use of ``env_fallback``, ``required_if``, ``required_together``, ``required_one_of`` (https://github.com/ansible-collections/community.general/pull/1765).

plugins/modules/cloud/misc/proxmox.py

@@ -345,7 +345,6 @@ EXAMPLES = r'''
     state: absent
 '''
 
-import os
 import time
 import traceback
 from distutils.version import LooseVersion
@@ -356,7 +355,7 @@ try:
 except ImportError:
     HAS_PROXMOXER = False
 
-from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.basic import AnsibleModule, env_fallback
 from ansible.module_utils._text import to_native
 
 
@@ -481,7 +480,7 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             api_host=dict(required=True),
-            api_password=dict(no_log=True),
+            api_password=dict(no_log=True, fallback=(env_fallback, ['PROXMOX_PASSWORD'])),
             api_token_id=dict(no_log=True),
             api_token_secret=dict(no_log=True),
             api_user=dict(required=True),
@@ -514,7 +513,10 @@ def main():
             description=dict(type='str'),
             hookscript=dict(type='str'),
             proxmox_default_behavior=dict(type='str', choices=['compatibility', 'no_defaults']),
-        )
+        ),
+        required_if=[('state', 'present', ['node', 'hostname', 'password', 'ostemplate'])],
+        required_together=[('api_token_id', 'api_token_secret')],
+        required_one_of=[('api_password', 'api_token_id')],
     )
 
     if not HAS_PROXMOXER:
@@ -561,13 +563,7 @@ def main():
                 module.params[param] = value
 
     auth_args = {'user': api_user}
-    if not (api_token_id and api_token_secret):
-        # If password not set get it from PROXMOX_PASSWORD env
-        if not api_password:
-            try:
-                api_password = os.environ['PROXMOX_PASSWORD']
-            except KeyError as e:
-                module.fail_json(msg='You should set api_password param or use PROXMOX_PASSWORD environment variable')
+    if not api_token_id:
         auth_args['password'] = api_password
     else:
         auth_args['token_name'] = api_token_id
@@ -599,8 +595,6 @@ def main():
             # If no vmid was passed, there cannot be another VM named 'hostname'
             if not module.params['vmid'] and get_vmid(proxmox, hostname) and not module.params['force']:
                 module.exit_json(changed=False, msg="VM with hostname %s already exists and has ID number %s" % (hostname, get_vmid(proxmox, hostname)[0]))
-            elif not (node, module.params['hostname'] and module.params['password'] and module.params['ostemplate']):
-                module.fail_json(msg='node, hostname, password and ostemplate are mandatory for creating vm')
             elif not node_check(proxmox, node):
                 module.fail_json(msg="node '%s' not exists in cluster" % node)
             elif not content_check(proxmox, node, module.params['ostemplate'], template_store):

plugins/modules/cloud/misc/proxmox_kvm.py

@@ -769,7 +769,6 @@ status:
     }'
 '''
 
-import os
 import re
 import time
 import traceback
@@ -782,7 +781,7 @@ try:
 except ImportError:
     HAS_PROXMOXER = False
 
-from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.basic import AnsibleModule, env_fallback
 from ansible.module_utils._text import to_native
 
 
@@ -1015,7 +1014,7 @@ def main():
             agent=dict(type='bool'),
             args=dict(type='str'),
             api_host=dict(required=True),
-            api_password=dict(no_log=True),
+            api_password=dict(no_log=True, fallback=(env_fallback, ['PROXMOX_PASSWORD'])),
             api_token_id=dict(no_log=True),
             api_token_secret=dict(no_log=True),
             api_user=dict(required=True),
@@ -1028,12 +1027,12 @@ def main():
             cipassword=dict(type='str', no_log=True),
             citype=dict(type='str', choices=['nocloud', 'configdrive2']),
             ciuser=dict(type='str'),
-            clone=dict(type='str', default=None),
+            clone=dict(type='str'),
             cores=dict(type='int'),
             cpu=dict(type='str'),
             cpulimit=dict(type='int'),
             cpuunits=dict(type='int'),
-            delete=dict(type='str', default=None),
+            delete=dict(type='str'),
             description=dict(type='str'),
             digest=dict(type='str'),
             force=dict(type='bool'),
@@ -1056,7 +1055,7 @@ def main():
             name=dict(type='str'),
             nameservers=dict(type='list', elements='str'),
             net=dict(type='dict'),
-            newid=dict(type='int', default=None),
+            newid=dict(type='int'),
             node=dict(),
             numa=dict(type='dict'),
             numa_enabled=dict(type='bool'),
@@ -1092,13 +1091,14 @@ def main():
             vcpus=dict(type='int'),
             vga=dict(choices=['std', 'cirrus', 'vmware', 'qxl', 'serial0', 'serial1', 'serial2', 'serial3', 'qxl2', 'qxl3', 'qxl4']),
             virtio=dict(type='dict'),
-            vmid=dict(type='int', default=None),
+            vmid=dict(type='int'),
             watchdog=dict(),
             proxmox_default_behavior=dict(type='str', choices=['compatibility', 'no_defaults']),
         ),
         mutually_exclusive=[('delete', 'revert'), ('delete', 'update'), ('revert', 'update'), ('clone', 'update'), ('clone', 'delete'), ('clone', 'revert')],
-        required_one_of=[('name', 'vmid',)],
-        required_if=[('state', 'present', ['node'])]
+        required_together=[('api_token_id', 'api_token_secret')],
+        required_one_of=[('name', 'vmid'), ('api_password', 'api_token_id')],
+        required_if=[('state', 'present', ['node'])],
     )
 
     if not HAS_PROXMOXER:
@@ -1159,12 +1159,6 @@ def main():
 
     auth_args = {'user': api_user}
     if not (api_token_id and api_token_secret):
-        # If password not set get it from PROXMOX_PASSWORD env
-        if not api_password:
-            try:
-                api_password = os.environ['PROXMOX_PASSWORD']
-            except KeyError:
-                module.fail_json(msg='You should set api_password param or use PROXMOX_PASSWORD environment variable')
         auth_args['password'] = api_password
     else:
         auth_args['token_name'] = api_token_id

plugins/modules/cloud/misc/proxmox_snap.py

@@ -31,6 +31,7 @@ options:
       - The password to authenticate with.
       - You can use PROXMOX_PASSWORD environment variable.
     type: str
+    required: yes
   hostname:
     description:
       - The instance name.
@@ -106,7 +107,6 @@ EXAMPLES = r'''
 
 RETURN = r'''#'''
 
-import os
 import time
 import traceback
 
@@ -118,7 +118,7 @@ except ImportError:
     PROXMOXER_IMP_ERR = traceback.format_exc()
     HAS_PROXMOXER = False
 
-from ansible.module_utils.basic import AnsibleModule, missing_required_lib
+from ansible.module_utils.basic import AnsibleModule, missing_required_lib, env_fallback
 from ansible.module_utils._text import to_native
 
 
@@ -182,7 +182,7 @@ def main():
         argument_spec=dict(
             api_host=dict(required=True),
             api_user=dict(required=True),
-            api_password=dict(no_log=True),
+            api_password=dict(no_log=True, required=True, fallback=(env_fallback, ['PROXMOX_PASSWORD'])),
             vmid=dict(required=False),
             validate_certs=dict(type='bool', default='no'),
             hostname=dict(),
@@ -213,13 +213,6 @@ def main():
     force = module.params['force']
     vmstate = module.params['vmstate']
 
-    # If password not set get it from PROXMOX_PASSWORD env
-    if not api_password:
-        try:
-            api_password = os.environ['PROXMOX_PASSWORD']
-        except KeyError as e:
-            module.fail_json(msg='You should set api_password param or use PROXMOX_PASSWORD environment variable' % to_native(e))
-
     try:
         proxmox = setup_api(api_host, api_user, api_password, validate_certs)
 

plugins/modules/cloud/misc/proxmox_template.py

@@ -122,7 +122,7 @@ try:
 except ImportError:
     HAS_PROXMOXER = False
 
-from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.basic import AnsibleModule, env_fallback
 
 
 def get_template(proxmox, node, storage, content_type, template):
@@ -175,7 +175,7 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             api_host=dict(required=True),
-            api_password=dict(no_log=True),
+            api_password=dict(no_log=True, fallback=(env_fallback, ['PROXMOX_PASSWORD'])),
             api_token_id=dict(no_log=True),
             api_token_secret=dict(no_log=True),
             api_user=dict(required=True),
@@ -188,7 +188,10 @@ def main():
             timeout=dict(type='int', default=30),
             force=dict(type='bool', default=False),
             state=dict(default='present', choices=['present', 'absent']),
-        )
+        ),
+        required_together=[('api_token_id', 'api_token_secret')],
+        required_one_of=[('api_password', 'api_token_id')],
+        required_if=[('state', 'absent', ['template'])]
     )
 
     if not HAS_PROXMOXER:
@@ -207,12 +210,6 @@ def main():
 
     auth_args = {'user': api_user}
     if not (api_token_id and api_token_secret):
-        # If password not set get it from PROXMOX_PASSWORD env
-        if not api_password:
-            try:
-                api_password = os.environ['PROXMOX_PASSWORD']
-            except KeyError as e:
-                module.fail_json(msg='You should set api_password param or use PROXMOX_PASSWORD environment variable')
         auth_args['password'] = api_password
     else:
         auth_args['token_name'] = api_token_id
@@ -261,9 +258,7 @@ def main():
             content_type = module.params['content_type']
             template = module.params['template']
 
-            if not template:
-                module.fail_json(msg='template param is mandatory')
-            elif not get_template(proxmox, node, storage, content_type, template):
+            if not get_template(proxmox, node, storage, content_type, template):
                 module.exit_json(changed=False, msg='template with volid=%s:%s/%s is already deleted' % (storage, content_type, template))
 
             if delete_template(module, proxmox, node, storage, content_type, template, timeout):