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

* 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

@@ -370,7 +370,6 @@ EXAMPLES = r'''
     state: absent
 '''
 
-import os
 import time
 import traceback
 from distutils.version import LooseVersion
@@ -381,7 +380,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
 
 
@@ -506,7 +505,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),
@@ -538,7 +537,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:
@@ -585,13 +587,7 @@ def main():
                 module.params[param] = value
 
     auth_args = {'user': api_user}
-    if not (api_token_id and api_token_secret):
+    if not api_token_id:
-        # 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
@@ -623,8 +619,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

@@ -813,7 +813,6 @@ status:
     }'
 '''
 
-import os
 import re
 import time
 import traceback
@@ -826,7 +825,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
 
 
@@ -1059,7 +1058,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),
@@ -1072,12 +1071,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'),
@@ -1100,7 +1099,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'),
@@ -1136,13 +1135,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_together=[('api_token_id', 'api_token_secret')],
-        required_if=[('state', 'present', ['node'])]
+        required_one_of=[('name', 'vmid'), ('api_password', 'api_token_id')],
+        required_if=[('state', 'present', ['node'])],
     )
 
     if not HAS_PROXMOXER:
@@ -1203,12 +1203,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_template.py

@@ -152,7 +152,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):
@@ -205,7 +205,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),
@@ -218,7 +218,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:
@@ -237,12 +240,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
@@ -291,9 +288,7 @@ def main():
             content_type = module.params['content_type']
             template = module.params['template']
 
-            if not template:
+            if not get_template(proxmox, node, storage, content_type, template):
-                module.fail_json(msg='template param is mandatory')
-            elif 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):