From 0121e725bbb89048fa378e6bd4be384349c57c66 Mon Sep 17 00:00:00 2001 From: pjcreath Date: Fri, 24 Jun 2022 01:38:33 +0000 Subject: [PATCH] Fix authentication error handling and incomplete error messages in proxmox salt-cloud driver Fixes #62211 --- changelog/62211.fixed | 1 + salt/cloud/clouds/proxmox.py | 12 ++- tests/unit/cloud/clouds/test_proxmox.py | 109 +++++++++++++++++++++++- 3 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 changelog/62211.fixed diff --git a/changelog/62211.fixed b/changelog/62211.fixed new file mode 100644 index 00000000000..92a021023a1 --- /dev/null +++ b/changelog/62211.fixed @@ -0,0 +1 @@ +Improved error handling and diagnostics in the proxmox salt-cloud driver diff --git a/salt/cloud/clouds/proxmox.py b/salt/cloud/clouds/proxmox.py index 8e7745d17db..ccbd5133bf1 100644 --- a/salt/cloud/clouds/proxmox.py +++ b/salt/cloud/clouds/proxmox.py @@ -136,7 +136,9 @@ def _authenticate(): connect_data = {"username": username, "password": passwd} full_url = "https://{}:{}/api2/json/access/ticket".format(url, port) - returned_data = requests.post(full_url, verify=verify_ssl, data=connect_data).json() + response = requests.post(full_url, verify=verify_ssl, data=connect_data) + response.raise_for_status() + returned_data = response.json() ticket = {"PVEAuthCookie": returned_data["data"]["ticket"]} csrf = str(returned_data["data"]["CSRFPreventionToken"]) @@ -655,12 +657,18 @@ def create(vm_): newid = _get_next_vmid() data = create_node(vm_, newid) except Exception as exc: # pylint: disable=broad-except + msg = str(exc) + if ( + isinstance(exc, requests.exceptions.RequestException) + and exc.response is not None + ): + msg = msg + "\n" + exc.response.text log.error( "Error creating %s on PROXMOX\n\n" "The following exception was thrown when trying to " "run the initial deployment: \n%s", vm_["name"], - exc, + msg, # Show the traceback if the debug logging level is enabled exc_info_on_loglevel=logging.DEBUG, ) diff --git a/tests/unit/cloud/clouds/test_proxmox.py b/tests/unit/cloud/clouds/test_proxmox.py index efd17771588..8bb4eaefcaa 100644 --- a/tests/unit/cloud/clouds/test_proxmox.py +++ b/tests/unit/cloud/clouds/test_proxmox.py @@ -2,13 +2,35 @@ :codeauthor: Tyler Johnson """ +import io import textwrap +import requests +from salt import config from salt.cloud.clouds import proxmox +from tests.support.helpers import TstSuiteLoggingHandler from tests.support.mixins import LoaderModuleMockMixin from tests.support.mock import ANY, MagicMock, patch from tests.support.unit import TestCase +PROFILE = { + "my_proxmox": { + "provider": "my_proxmox", + "image": "local:some_image.tgz", + } +} +PROVIDER_CONFIG = { + "my_proxmox": { + "proxmox": { + "driver": "proxmox", + "url": "pve@domain.com", + "user": "cloud@pve", + "password": "verybadpass", + "profiles": PROFILE, + } + } +} + class ProxmoxTest(TestCase, LoaderModuleMockMixin): def setup_loader_modules(self): @@ -22,8 +44,8 @@ class ProxmoxTest(TestCase, LoaderModuleMockMixin): "__opts__": { "sock_dir": True, "transport": True, - "providers": {"my_proxmox": {}}, - "profiles": {"my_proxmox": {}}, + "providers": PROVIDER_CONFIG, + "profiles": PROFILE, }, "__active_provider_name__": "my_proxmox:proxmox", } @@ -262,3 +284,86 @@ class ProxmoxTest(TestCase, LoaderModuleMockMixin): proxmox._import_api() self.assertEqual(proxmox.api, [{"info": {}}]) return + + def test__authenticate_success(self): + response = requests.Response() + response.status_code = 200 + response.reason = "OK" + response.raw = io.BytesIO( + b"""{"data":{"CSRFPreventionToken":"01234567:dG9rZW4=","ticket":"PVE:cloud@pve:01234567::dGlja2V0"}}""" + ) + with patch("requests.post", return_value=response): + proxmox._authenticate() + assert proxmox.csrf and proxmox.ticket + return + + def test__authenticate_failure(self): + """ + Confirm that authentication failure raises an exception. + """ + response = requests.Response() + response.status_code = 401 + response.reason = "authentication failure" + response.raw = io.BytesIO(b"""{"data":null}""") + with patch("requests.post", return_value=response): + self.assertRaises(requests.exceptions.HTTPError, proxmox._authenticate) + return + + def test_creation_failure_logging(self): + """ + Test detailed logging on HTTP errors during VM creation. + """ + vm_ = { + "profile": "my_proxmox", + "name": "vm4", + "technology": "lxc", + "host": "127.0.0.1", + "image": "local:some_image.tgz", + "onboot": True, + } + self.assertEqual( + config.is_profile_configured( + proxmox.__opts__, "my_proxmox:proxmox", "my_proxmox", vm_=vm_ + ), + True, + ) + + response = requests.Response() + response.status_code = 400 + response.reason = "Parameter verification failed." + response.raw = io.BytesIO( + b"""{"data":null,"errors":{"onboot":"type check ('boolean') failed - got 'True'"}}""" + ) + + def mock_query_response(conn_type, option, post_data=None): + if conn_type == "get" and option == "cluster/nextid": + return 104 + if conn_type == "post" and option == "nodes/127.0.0.1/lxc": + response.raise_for_status() + return response + return None + + with patch.object( + proxmox, "query", side_effect=mock_query_response + ), patch.object( + proxmox, "_get_properties", return_value=set() + ), TstSuiteLoggingHandler() as log_handler: + self.assertEqual(proxmox.create(vm_), False) + + # Search for these messages in a multi-line log entry. + missing = { + "{} Client Error: {} for url:".format( + response.status_code, response.reason + ), + response.text, + } + for required in list(missing): + for message in log_handler.messages: + if required in message: + missing.remove(required) + break + if missing: + raise AssertionError( + "Did not find error messages: {}".format(sorted(list(missing))) + ) + return