Repository: proxmoxer/proxmoxer

Issue:
proxmoxer doesn't distinguish between error types
Hi. I'd like to use proxmoxer for an ansible collection I'm writing that implements all of the PVE API paths. Ansible modules (generally) get run on the target host, which means using the local backend and the pvesh.

The problem I'm running into is that when pvesh returns non-zero, proxmoxer throws a `proxmoxer.core.ResourceException` with a `500 Internal Server Error` regardless of the pvesh error code. pvesh itself, though, distinguishes between different error types. For example, it returns 1 if there is no handler defined for a given API path (e.g. if the path doesn't exist), but it returns 2 if the handler exists, but the parameter doesn't. See below for an example.

This is an important distinction for something like ansible because the parameter not existing isn't necessarily an error. It could be that we want to check if it exists before creating/updating. But if the API path is wrong, or some other issue occurs, we'd want the module to also throw an error.

I don't really want to parse the exception text in my module, so I'm hoping you'd be open to addressing this in the library. Happy to contribute the fix.

```python
>>> p.get("/access/domains/pve")
{'comment': 'Proxmox VE authentication server', 'digest': 'da39a3ee5e6b4b0d3255bfef95601890afd80709', 'type': 'pve'}

>>> p.get("/access/domains/pvse")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/root/.venv/lib/python3.11/site-packages/proxmoxer/core.py", line 167, in get
    return self(args)._request("GET", params=params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/.venv/lib/python3.11/site-packages/proxmoxer/core.py", line 156, in _request
    raise ResourceException(
proxmoxer.core.ResourceException: 500 Internal Server Error: domain 'pvse' does not exist

>>> p.get("/access/domaisns/pve")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/root/.venv/lib/python3.11/site-packages/proxmoxer/core.py", line 167, in get
    return self(args)._request("GET", params=params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/.venv/lib/python3.11/site-packages/proxmoxer/core.py", line 156, in _request
    raise ResourceException(
proxmoxer.core.ResourceException: 500 Internal Server Error: No 'get' handler defined for '/access/domaisns/pve'
```

Discussion:

Patch:
diff --git a/proxmoxer/backends/command_base.py b/proxmoxer/backends/command_base.py
index 4d8e7b0..661bc1c 100644
--- a/proxmoxer/backends/command_base.py
+++ b/proxmoxer/backends/command_base.py
@@ -30,8 +30,9 @@ except ImportError:
 
 
 class Response:
-    def __init__(self, content, status_code):
+    def __init__(self, content, status_code, exit_code):
         self.status_code = status_code
+        self.exit_code = exit_code
         self.content = content
         self.text = str(content)
         self.headers = {"content-type": "application/json"}
@@ -110,7 +111,7 @@ class CommandBaseSession:
         if self.sudo:
             full_cmd = ["sudo"] + full_cmd
 
-        stdout, stderr = self._exec(full_cmd)
+        stdout, stderr, exit_code = self._exec(full_cmd)
 
         def is_http_status_string(s):
             return re.match(r"\d\d\d [a-zA-Z]", str(s))
@@ -135,8 +136,8 @@ class CommandBaseSession:
         else:
             status_code = 200
         if stdout:
-            return Response(stdout, status_code)
-        return Response(stderr, status_code)
+            return Response(stdout, status_code, exit_code)
+        return Response(stderr, status_code, exit_code)
 
     def upload_file_obj(self, file_obj, remote_path):
         raise NotImplementedError()
diff --git a/proxmoxer/backends/local.py b/proxmoxer/backends/local.py
index 00bb1cd..dbb8776 100644
--- a/proxmoxer/backends/local.py
+++ b/proxmoxer/backends/local.py
@@ -12,7 +12,7 @@ class LocalSession(CommandBaseSession):
     def _exec(self, cmd):
         proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
         stdout, stderr = proc.communicate(timeout=self.timeout)
-        return stdout.decode(), stderr.decode()
+        return stdout.decode(), stderr.decode(), proc.returncode
 
     def upload_file_obj(self, file_obj, remote_path):
         with open(remote_path, "wb") as dest_fp:
diff --git a/proxmoxer/backends/openssh.py b/proxmoxer/backends/openssh.py
index d9cd7d1..170edf1 100644
--- a/proxmoxer/backends/openssh.py
+++ b/proxmoxer/backends/openssh.py
@@ -55,7 +55,7 @@ class OpenSSHSession(CommandBaseSession):
 
     def _exec(self, cmd):
         ret = self.ssh_client.run(shell_join(cmd), forward_ssh_agent=self.forward_ssh_agent)
-        return ret.stdout, ret.stderr
+        return ret.stdout, ret.stderr, ret.returncode
 
     def upload_file_obj(self, file_obj, remote_path):
         self.ssh_client.scp((file_obj,), target=remote_path)
diff --git a/proxmoxer/backends/ssh_paramiko.py b/proxmoxer/backends/ssh_paramiko.py
index 0d0aa8c..ebb2e1f 100644
--- a/proxmoxer/backends/ssh_paramiko.py
+++ b/proxmoxer/backends/ssh_paramiko.py
@@ -63,7 +63,7 @@ class SshParamikoSession(CommandBaseSession):
         session.exec_command(shell_join(cmd))
         stdout = session.makefile("rb", -1).read().decode()
         stderr = session.makefile_stderr("rb", -1).read().decode()
-        return stdout, stderr
+        return stdout, stderr, session.recv_exit_status()
 
     def upload_file_obj(self, file_obj, remote_path):
         sftp = self.ssh_client.open_sftp()
diff --git a/proxmoxer/core.py b/proxmoxer/core.py
index 73fb1a9..23afca7 100644
--- a/proxmoxer/core.py
+++ b/proxmoxer/core.py
@@ -54,7 +54,7 @@ class ResourceException(Exception):
     An Exception thrown when an Proxmox API call failed
     """
 
-    def __init__(self, status_code, status_message, content, errors=None):
+    def __init__(self, status_code, status_message, content, errors=None, exit_code=None):
         """
         Create a new ResourceException
 
@@ -71,6 +71,7 @@ class ResourceException(Exception):
         self.status_message = status_message
         self.content = content
         self.errors = errors
+        self.exit_code = exit_code
         if errors is not None:
             content += f" - {errors}"
         message = f"{status_code} {status_message}: {content}".strip()
@@ -151,6 +152,7 @@ class ProxmoxResource:
                     ),
                     resp.reason,
                     errors=(self._store["serializer"].loads_errors(resp)),
+                    exit_code=resp.exit_code if hasattr(resp, "exit_code") else None,
                 )
             else:
                 raise ResourceException(
@@ -159,6 +161,7 @@ class ProxmoxResource:
                         resp.status_code, ANYEVENT_HTTP_STATUS_CODES.get(resp.status_code)
                     ),
                     resp.text,
+                    exit_code=resp.exit_code if hasattr(resp, "exit_code") else None,
                 )
         elif 200 <= resp.status_code <= 299:
             return self._store["serializer"].loads(resp)


Test patch:
diff --git a/tests/api_mock.py b/tests/api_mock.py
index 0488545..c64d9ca 100644
--- a/tests/api_mock.py
+++ b/tests/api_mock.py
@@ -8,7 +8,6 @@ from urllib.parse import parse_qsl, urlparse
 
 import pytest
 import responses
-from requests_toolbelt import MultipartEncoder
 
 
 @pytest.fixture()
@@ -141,8 +140,6 @@ class PVERegistry(responses.registries.FirstMatchRegistry):
     def _cb_echo(self, request):
         body = request.body
         if body is not None:
-            if isinstance(body, MultipartEncoder):
-                body = body.to_string()  # really, to byte string
             body = body if isinstance(body, str) else str(body, "utf-8")
 
         resp = {
diff --git a/tests/test_command_base.py b/tests/test_command_base.py
index 6306eed..733403f 100644
--- a/tests/test_command_base.py
+++ b/tests/test_command_base.py
@@ -16,11 +16,12 @@ from .api_mock import PVERegistry
 
 class TestResponse:
     def test_init_all_args(self):
-        resp = command_base.Response(b"content", 200)
+        resp = command_base.Response(b"content", 200, 201)
 
         assert resp.content == b"content"
         assert resp.text == "b'content'"
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.headers == {"content-type": "application/json"}
         assert str(resp) == "Response (200) b'content'"
 
@@ -48,6 +49,7 @@ class TestCommandBaseSession:
         resp = self._session.request("GET", self.base_url + "/fake/echo")
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "pvesh",
             "get",
@@ -60,6 +62,7 @@ class TestCommandBaseSession:
         resp = self._session.request("GET", self.base_url + "/stdout")
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert (
             resp.content == "UPID:node:003094EA:095F1EFE:63E88772:download:file.iso:root@pam:done"
         )
@@ -67,6 +70,7 @@ class TestCommandBaseSession:
         resp_stderr = self._session.request("GET", self.base_url + "/stderr")
 
         assert resp_stderr.status_code == 200
+        assert resp.exit_code == 201
         assert (
             resp_stderr.content
             == "UPID:node:003094EA:095F1EFE:63E88772:download:file.iso:root@pam:done"
@@ -79,6 +83,7 @@ class TestCommandBaseSession:
         )
 
         assert resp.status_code == 403
+        assert resp.exit_code == 501
         assert (
             resp.content
             == "pvesh\nget\nhttps://1.2.3.4:1234/api2/json/fake/echo\n-thing\n403 Unauthorized\n--output-format\njson"
@@ -88,6 +93,7 @@ class TestCommandBaseSession:
         resp = self._session.request("GET", self.base_url + "/fake/echo", data={"thing": "failure"})
 
         assert resp.status_code == 500
+        assert resp.exit_code == 501
         assert (
             resp.content
             == "pvesh\nget\nhttps://1.2.3.4:1234/api2/json/fake/echo\n-thing\nfailure\n--output-format\njson"
@@ -99,6 +105,7 @@ class TestCommandBaseSession:
         )
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "sudo",
             "pvesh",
@@ -112,6 +119,7 @@ class TestCommandBaseSession:
         resp = self._session.request("GET", self.base_url + "/fake/echo", data={"key": "value"})
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "pvesh",
             "get",
@@ -128,6 +136,7 @@ class TestCommandBaseSession:
         )
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "pvesh",
             "get",
@@ -146,6 +155,7 @@ class TestCommandBaseSession:
         )
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "pvesh",
             "create",
@@ -166,6 +176,7 @@ class TestCommandBaseSession:
         )
 
         assert resp.status_code == 200
+        assert resp.exit_code == 201
         assert resp.content == [
             "pvesh",
             "create",
@@ -187,6 +198,7 @@ class TestCommandBaseSession:
             )
 
             assert resp.status_code == 200
+            assert resp.exit_code == 201
             assert resp.content == [
                 "pvesh",
                 "create",
@@ -209,7 +221,7 @@ class TestJsonSimpleSerializer:
         input_str = '{"key1": "value1", "key2": "value2"}'
         exp_output = {"key1": "value1", "key2": "value2"}
 
-        response = command_base.Response(input_str.encode("utf-8"), 200)
+        response = command_base.Response(input_str.encode("utf-8"), 200, 201)
 
         act_output = self._serializer.loads(response)
 
@@ -219,7 +231,7 @@ class TestJsonSimpleSerializer:
         input_str = "There was an error with the request"
         exp_output = {"errors": b"There was an error with the request"}
 
-        response = command_base.Response(input_str.encode("utf-8"), 200)
+        response = command_base.Response(input_str.encode("utf-8"), 200, 201)
 
         act_output = self._serializer.loads(response)
 
@@ -229,7 +241,7 @@ class TestJsonSimpleSerializer:
         input_str = '{"data": {"key1": "value1", "key2": "value2"}, "errors": {}}\x80'
         exp_output = {"errors": input_str.encode("utf-8")}
 
-        response = command_base.Response(input_str.encode("utf-8"), 200)
+        response = command_base.Response(input_str.encode("utf-8"), 200, 201)
 
         act_output = self._serializer.loads(response)
 
@@ -267,21 +279,21 @@ def _exec_echo(_, cmd):
         "import tempfile; import sys; tf = tempfile.NamedTemporaryFile(); sys.stdout.write(tf.name)",
     ]:
         return b"/tmp/tmpasdfasdf", None
-    return cmd, None
+    return cmd, None, 201
 
 
 @classmethod
 def _exec_err(_, cmd):
-    return None, "\n".join(cmd)
+    return None, "\n".join(cmd), 501
 
 
 @classmethod
 def _exec_task(_, cmd):
     upid = "UPID:node:003094EA:095F1EFE:63E88772:download:file.iso:root@pam:done"
     if "stderr" in cmd[2]:
-        return None, upid
+        return None, upid, 501
     else:
-        return upid, None
+        return upid, None, 201
 
 
 @classmethod
diff --git a/tests/test_core.py b/tests/test_core.py
index c2a2f5f..db41185 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -58,6 +58,16 @@ class TestResourceException:
             == "ResourceException('500 Internal Error: Unable to do the thing - functionality not found')"
         )
 
+    def test_init_exit_code(self):
+        e = core.ResourceException(500, "Internal Error", "Unable to do the thing", exit_code=255)
+
+        assert e.status_code == 500
+        assert e.status_message == "Internal Error"
+        assert e.content == "Unable to do the thing"
+        assert e.exit_code == 255
+        assert str(e) == "500 Internal Error: Unable to do the thing"
+        assert repr(e) == "ResourceException('500 Internal Error: Unable to do the thing')"
+
 
 class TestProxmoxResource:
     obj = core.ProxmoxResource()
@@ -183,6 +193,7 @@ class TestProxmoxResource:
             ),
         ]
         assert exc_info.value.status_code == 500
+        assert exc_info.value.exit_code == 501
         assert exc_info.value.status_message == "Internal Server Error"
         assert exc_info.value.content == str(b"this is the error")
         assert exc_info.value.errors is None
@@ -206,6 +217,7 @@ class TestProxmoxResource:
             ),
         ]
         assert exc_info.value.status_code == 500
+        assert exc_info.value.exit_code == 501
         assert exc_info.value.status_message == "Internal Server Error"
         assert exc_info.value.content == "this is the reason"
         assert exc_info.value.errors == {"errors": b"this is the error"}
@@ -380,12 +392,12 @@ class MockSession:
         self.url = url
 
         if "fail" in url:
-            r = Response(b"this is the error", 500)
+            r = Response(b"this is the error", 500, 501)
             if "reason" in url:
                 r.reason = "this is the reason"
             return r
         else:
-            return Response(b'{"data": {"key": "value"}}', 200)
+            return Response(b'{"data": {"key": "value"}}', 200, 201)
 
 
 @pytest.fixture
diff --git a/tests/test_https.py b/tests/test_https.py
index f4d3ad8..c3dc3b5 100644
--- a/tests/test_https.py
+++ b/tests/test_https.py
@@ -358,7 +358,7 @@ class TestProxmoxHttpSession:
         assert m is not None  # content matches multipart for the created file
         assert content["headers"]["Content-Type"] == "multipart/form-data; boundary=" + m[1]
 
-    def test_request_streaming(self, toolbelt_on_off, caplog, mock_pve):
+    def test_request_streaming(self, shrink_thresholds, toolbelt_on_off, caplog, mock_pve):
         caplog.set_level(logging.INFO, logger=MODULE_LOGGER_NAME)
 
         size = https.STREAMING_SIZE_THRESHOLD + 1
diff --git a/tests/test_local.py b/tests/test_local.py
index fd7359c..adac994 100644
--- a/tests/test_local.py
+++ b/tests/test_local.py
@@ -54,7 +54,8 @@ class TestLocalSession:
             'import sys; sys.stdout.write("stdout content"); sys.stderr.write("stderr content")',
         ]
 
-        stdout, stderr = self._session._exec(cmd)
+        stdout, stderr, exit_code = self._session._exec(cmd)
 
         assert stdout == "stdout content"
         assert stderr == "stderr content"
+        assert exit_code == 0
diff --git a/tests/test_openssh.py b/tests/test_openssh.py
index 9bd3f97..7d57222 100644
--- a/tests/test_openssh.py
+++ b/tests/test_openssh.py
@@ -53,10 +53,11 @@ class TestOpenSSHSession:
             "world",
         ]
 
-        stdout, stderr = mock_session._exec(cmd)
+        stdout, stderr, exit_code = mock_session._exec(cmd)
 
         assert stdout == "stdout content"
         assert stderr == "stderr content"
+        assert exit_code == 0
         mock_session.ssh_client.run.assert_called_once_with(
             "echo hello world",
             forward_ssh_agent=True,
@@ -83,7 +84,7 @@ def _get_mock_ssh_conn(_):
 
     ssh_conn.run = mock.Mock(
         # spec=openssh_wrapper.SSHConnection.run,
-        return_value=mock.Mock(stdout="stdout content", stderr="stderr content"),
+        return_value=mock.Mock(stdout="stdout content", stderr="stderr content", returncode=0),
     )
 
     ssh_conn.scp = mock.Mock()
diff --git a/tests/test_paramiko.py b/tests/test_paramiko.py
index 5205089..5030204 100644
--- a/tests/test_paramiko.py
+++ b/tests/test_paramiko.py
@@ -100,10 +100,11 @@ class TestSshParamikoSession:
         sess = ssh_paramiko.SshParamikoSession("host", "user")
         sess.ssh_client = mock_client
 
-        stdout, stderr = sess._exec(["echo", "hello", "world"])
+        stdout, stderr, exit_code = sess._exec(["echo", "hello", "world"])
 
         assert stdout == "stdout contents"
         assert stderr == "stderr contents"
+        assert exit_code == 0
         mock_session.exec_command.assert_called_once_with("echo hello world")
 
     def test_upload_file_obj(self, mock_ssh_client):
@@ -147,6 +148,7 @@ def mock_ssh_client():
     mock_stderr.read.return_value = b"stderr contents"
     mock_channel.makefile.return_value = mock_stdout
     mock_channel.makefile_stderr.return_value = mock_stderr
+    mock_channel.recv_exit_status.return_value = 0
 
     mock_transport.open_session.return_value = mock_channel
     mock_client.get_transport.return_value = mock_transport
diff --git a/tests/tools/test_tasks.py b/tests/tools/test_tasks.py
index fde50c3..e248d78 100644
--- a/tests/tools/test_tasks.py
+++ b/tests/tools/test_tasks.py
@@ -115,7 +115,7 @@ class TestBlockingStatus:
         status = Tasks.blocking_status(
             mocked_prox,
             "UPID:node1:000FF1FD:10F9374C:630D702C:vzdump:110:root@pam:keep-running",
-            timeout=0.021,
+            timeout=0.025,
             polling_interval=0.01,
         )