Merge ~ack/maas:1922569-discover-projects-rpc-error into maas:master

[Alberto Donato]

LP: #1922569 - wrap errors from discover projects call in RPC handler

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1922569-discover-projects-rpc-error lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9691/console
COMMIT: e9d1d5e52c1c537c7708a43783d8b71d15c0b844

review: Needs Fixing

[Alberto Donato (ack)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1922569-discover-projects-rpc-error lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e9d1d5e52c1c537c7708a43783d8b71d15c0b844

review: Approve

[Adam Collard (adam-collard)]

review: Approve

diff --git a/src/provisioningserver/drivers/pod/lxd.py b/src/provisioningserver/drivers/pod/lxd.py
index 3683fe2..4095e85 100644
--- a/src/provisioningserver/drivers/pod/lxd.py
+++ b/src/provisioningserver/drivers/pod/lxd.py
@@ -11,7 +11,7 @@ from urllib.parse import urlparse
 import uuid
 
 from pylxd import Client
-from pylxd.exceptions import ClientConnectionFailed, NotFound
+from pylxd.exceptions import ClientConnectionFailed, LXDAPIException, NotFound
 import urllib3
 
 from provisioningserver.drivers import (
@@ -757,7 +757,10 @@ class LXDPodDriver(PodDriver):
             if not client.trusted:
                 password = context.get("password")
                 if password:
-                    client.authenticate(password)
+                    try:
+                        client.authenticate(password)
+                    except LXDAPIException as e:
+                        raise LXDPodError(f"Authentication failed: {e}")
                 else:
                     raise LXDPodError(
                         f"Pod {pod_id}: Certificate is not trusted and no password was given."
diff --git a/src/provisioningserver/drivers/pod/tests/test_lxd.py b/src/provisioningserver/drivers/pod/tests/test_lxd.py
index 0976151..0b41f13 100644
--- a/src/provisioningserver/drivers/pod/tests/test_lxd.py
+++ b/src/provisioningserver/drivers/pod/tests/test_lxd.py
@@ -5,6 +5,7 @@ from os.path import join
 import random
 from unittest.mock import ANY, Mock, PropertyMock, sentinel
 
+from pylxd.exceptions import LXDAPIException
 from testtools.matchers import Equals, IsInstance, MatchesAll, MatchesStructure
 from testtools.testcase import ExpectedException
 from twisted.internet.defer import inlineCallbacks
@@ -237,6 +238,20 @@ class TestLXDPodDriver(MAASTestCase):
         with ExpectedException(lxd_module.LXDPodError, error_msg):
             driver._get_client(pod_id, context)
 
+    def test_get_client_raises_error_when_authenticate_fails(self):
+        context = self.make_parameters_context()
+        pod_id = factory.make_name("pod_id")
+        Client = self.patch(lxd_module, "Client")
+        client = Client.return_value
+        client.trusted = False
+        mock_response = Mock(status_code=403)
+        mock_response.json.return_value = {"error": "auth failed"}
+        client.authenticate.side_effect = LXDAPIException(mock_response)
+        driver = lxd_module.LXDPodDriver()
+        error_msg = "Authentication failed: auth failed"
+        with ExpectedException(lxd_module.LXDPodError, error_msg):
+            driver._get_client(pod_id, context)
+
     def test_get_machine(self):
         context = self.make_parameters_context()
         driver = lxd_module.LXDPodDriver()
diff --git a/src/provisioningserver/rpc/pods.py b/src/provisioningserver/rpc/pods.py
index bbc422e..6ac985c 100644
--- a/src/provisioningserver/rpc/pods.py
+++ b/src/provisioningserver/rpc/pods.py
@@ -52,6 +52,9 @@ def discover_pod_projects(pod_type, context):
         }
 
     d.addCallback(convert)
+    d.addErrback(
+        convert_errors, log_message="Failed to discover VM host projects."
+    )
     return d
 
 
@@ -86,18 +89,8 @@ def discover_pod(pod_type, context, pod_id=None, name=None):
         else:
             return {"pod": result}
 
-    def catch_all(failure):
-        """Convert all failures into `PodActionFail` unless already a
-        `PodActionFail` or `NotImplementedError`."""
-        # Log locally to help debugging.
-        log.err(failure, "Failed to discover pod.")
-        if failure.check(NotImplementedError, PodActionFail):
-            return failure
-        else:
-            raise PodActionFail(get_error_message(failure.value))
-
     d.addCallback(convert)
-    d.addErrback(catch_all)
+    d.addErrback(convert_errors, log_message="Failed to discover VM host.")
     return d
 
 
@@ -141,23 +134,12 @@ def compose_machine(pod_type, context, request, pod_id, name):
                     "invalid result." % pod_type
                 )
 
-    def catch_all(failure):
-        """Convert all failures into `PodActionFail` unless already a
-        `PodActionFail`, `PodInvalidResources` or `NotImplementedError`."""
-        if failure.check(PodInvalidResources):
-            # Driver returned its own invalid resource exception instead of
-            # None. Just pass this onto the region.
-            return failure
-
-        # Log locally to help debugging.
-        log.err(failure, "%s: Failed to compose machine: %s" % (name, request))
-        if failure.check(NotImplementedError, PodActionFail):
-            return failure
-        else:
-            raise PodActionFail(get_error_message(failure.value))
-
     d.addCallback(convert)
-    d.addErrback(catch_all)
+    d.addErrback(
+        convert_errors,
+        log_message=f"{name}: Failed to compose machine: {request}",
+        keep_failures=[PodInvalidResources],
+    )
     return d
 
 
@@ -217,23 +199,15 @@ def send_pod_commissioning_results(
                     f"Unable to send Pod commissioning information for {name}({system_id}): {e.error}"
                 )
 
-    def catch_all(failure):
-        """Convert all failures into `PodActionFail` unless already a
-        `PodActionFail` or `NotImplementedError`."""
-        # Log locally to help debugging.
-        log.err(failure, "Failed to send_pod_commissioning_results.")
-        if failure.check(NotImplementedError, PodActionFail):
-            return failure
-        else:
-            raise PodActionFail(get_error_message(failure.value))
-
     d.addCallback(
         lambda commissioning_results: deferToThread(
             send_items, commissioning_results
         )
     )
     d.addCallback(lambda _: {})
-    d.addErrback(catch_all)
+    d.addErrback(
+        convert_errors, log_message="Failed to send_pod_commissioning_results."
+    )
     return d
 
 
@@ -261,16 +235,23 @@ def decompose_machine(pod_type, context, pod_id, name):
         else:
             return {"hints": result}
 
-    def catch_all(failure):
-        """Convert all failures into `PodActionFail` unless already a
-        `PodActionFail` or `NotImplementedError`."""
-        # Log locally to help debugging.
-        log.err(failure, "Failed to decompose machine.")
-        if failure.check(NotImplementedError, PodActionFail):
-            return failure
-        else:
-            raise PodActionFail(get_error_message(failure.value))
-
     d.addCallback(convert)
-    d.addErrback(catch_all)
+    d.addErrback(convert_errors, log_message="Failed to decompose machine.")
     return d
+
+
+def convert_errors(failure, log_message=None, keep_failures=None):
+    """Convert all failures into PodActionFail unless already a
+    PodActionFail or NotImplementedError.
+
+    Optionally, also log a failure message.
+    """
+    valid_failures = [NotImplementedError, PodActionFail]
+    if keep_failures:
+        valid_failures.extend(keep_failures)
+    if log_message:
+        log.err(failure, log_message)
+    if failure.check(*valid_failures):
+        return failure
+    else:
+        raise PodActionFail(get_error_message(failure.value))
diff --git a/src/provisioningserver/rpc/tests/test_pods.py b/src/provisioningserver/rpc/tests/test_pods.py
index aa8f547..0f8bb01 100644
--- a/src/provisioningserver/rpc/tests/test_pods.py
+++ b/src/provisioningserver/rpc/tests/test_pods.py
@@ -41,6 +41,15 @@ class TestDiscoverPodProjects(MAASTestCase):
             yield pods.discover_pod_projects(unknown_type, {})
 
     @inlineCallbacks
+    def test_converts_exceptions(self):
+        fake_driver = MagicMock()
+        fake_driver.name = factory.make_name("pod")
+        fake_driver.discover_projects.return_value = fail(Exception("fail!"))
+        self.patch(PodDriverRegistry, "get_item").return_value = fake_driver
+        with ExpectedException(exceptions.PodActionFail):
+            yield pods.discover_pod_projects(fake_driver.name, {})
+
+    @inlineCallbacks
     def test_return_projects(self):
         fake_driver = MagicMock()
         fake_driver.name = factory.make_name("pod")