Merge "Do not mute errors on check health of existing@openstack"

CHANGELOG.rst

@@ -29,6 +29,7 @@ Changed
 
 * Our requirements are updated as like upper-constraints (the list of
   suggested tested versions to use)
+* Error messages become more user-friendly in ``rally env check``.
 
 Removed
 ~~~~~~~

rally_openstack/osclients.py

@@ -31,6 +31,50 @@ LOG = logging.getLogger(__name__)
 CONF = cfg.CONF
 
 
+class AuthenticationFailed(exceptions.AuthenticationFailed):
+    error_code = 220
+
+    msg_fmt = ("Failed to authenticate to %(url)s for user '%(username)s'"
+               " in project '%(project)s': %(message)s")
+    msg_fmt_2 = "%(message)s"
+
+    def __init__(self, error, url, username, project):
+        kwargs = {
+            "error": error,
+            "url": url,
+            "username": username,
+            "project": project
+        }
+        self._helpful_trace = False
+
+        from keystoneauth1 import exceptions as ks_exc
+
+        if isinstance(error, ks_exc.ConnectionError):
+            # this type of errors is general for all users no need to include
+            # username, project name. The original error message should be
+            # self-sufficient
+            self.msg_fmt = self.msg_fmt_2
+            message = error.message
+            if message.startswith("Unable to establish connection to"):
+                # this message contains too much info.
+                if "Max retries exceeded with url" in message:
+                    if "HTTPConnectionPool" in message:
+                        splitter = ": HTTPConnectionPool"
+                    else:
+                        splitter = ": HTTPSConnectionPool"
+                    message = message.split(splitter, 1)[0]
+        elif isinstance(error, ks_exc.Unauthorized):
+            message = error.message.split(" (HTTP 401)", 1)[0]
+        else:
+            # something unexpected. include exception class as well.
+            self._helpful_trace = True
+            message = "[%s] %s" % (error.__class__.__name__, str(error))
+        super(AuthenticationFailed, self).__init__(message=message, **kwargs)
+
+    def is_trace_helpful(self):
+        return self._helpful_trace
+
+
 def configure(name, default_version=None, default_service_type=None,
               supported_versions=None):
     """OpenStack client class wrapper.
@@ -230,19 +274,21 @@ class Keystone(OSClient):
             if "keystone_auth_ref" not in self.cache:
                 sess, plugin = self.get_session()
                 self.cache["keystone_auth_ref"] = plugin.get_access(sess)
-        except Exception as e:
+        except Exception as original_e:
-            if logging.is_debug():
+            e = AuthenticationFailed(
+                error=original_e,
+                username=self.credential.username,
+                project=self.credential.tenant_name,
+                url=self.credential.auth_url
+            )
+            if logging.is_debug() and e.is_trace_helpful():
                 LOG.exception("Unable to authenticate for user"
                               " %(username)s in project"
                               " %(tenant_name)s" %
                               {"username": self.credential.username,
                                "tenant_name": self.credential.tenant_name})
-            raise exceptions.AuthenticationFailed(
+
-                username=self.credential.username,
+            raise e
-                project=self.credential.tenant_name,
-                url=self.credential.auth_url,
-                etype=e.__class__.__name__,
-                error=str(e))
         return self.cache["keystone_auth_ref"]
 
     def get_session(self, version=None):

rally_openstack/platforms/existing.py

@@ -144,32 +144,42 @@ class OpenStack(platform.Platform):
 
     def check_health(self):
         """Check whatever platform is alive."""
-        if self.platform_data["admin"]:
-            try:
-                osclients.Clients(
-                    self.platform_data["admin"]).verified_keystone()
-            except Exception:
-                d = copy.deepcopy(self.platform_data["admin"])
-                d["password"] = "***"
-                return {
-                    "available": False,
-                    "message": (
-                        "Bad admin creds: \n%s"
-                        % json.dumps(d, indent=2, sort_keys=True)),
-                    "traceback": traceback.format_exc()
-                }
 
-        for user in self.platform_data["users"]:
+        users_to_check = self.platform_data["users"]
+        if self.platform_data["admin"]:
+            users_to_check.append(self.platform_data["admin"])
+
+        for user in users_to_check:
             try:
-                osclients.Clients(user).keystone()
+                if self.platform_data["admin"] == user:
+                    osclients.Clients(user).verified_keystone()
+                else:
+                    osclients.Clients(user).keystone()
+            except osclients.exceptions.RallyException as e:
+                # all rally native exceptions should provide user-friendly
+                # messages
+                return {"available": False, "message": e.format_message(),
+                        # traceback is redundant here. Remove as soon as min
+                        #   required rally version will be updated
+                        #   More details here:
+                        #       https://review.openstack.org/597197
+                        "traceback": traceback.format_exc()}
             except Exception:
                 d = copy.deepcopy(user)
                 d["password"] = "***"
+                if logging.is_debug():
+                    LOG.exception("Something unexpected had happened while "
+                                  "validating OpenStack credentials.")
+                if self.platform_data["admin"] == user:
+                    user_role = "admin"
+                else:
+                    user_role = "user"
                 return {
                     "available": False,
                     "message": (
-                        "Bad user creds: \n%s"
+                        "Bad %s creds: \n%s"
-                        % json.dumps(d, indent=2, sort_keys=True)),
+                        % (user_role,
+                           json.dumps(d, indent=2, sort_keys=True))),
                     "traceback": traceback.format_exc()
                 }
 

tests/functional/test_cli_deployment.py

@@ -86,17 +86,8 @@ class DeploymentTestCase(unittest.TestCase):
             rally("--debug deployment check")
         except utils.RallyCliError as e:
             self.assertIn(
-                "[-] Unable to authenticate for user %(username)s in"
+                "AuthenticationFailed: Unable to establish connection to "
-                " project %(tenant_name)s" %
+                "%s" % TEST_ENV["OS_AUTH_URL"],
-                {"username": TEST_ENV["OS_USERNAME"],
-                 "tenant_name": TEST_ENV["OS_TENANT_NAME"]},
-                str(e))
-            self.assertIn(
-                "AuthenticationFailed: Failed to authenticate to %(auth_url)s"
-                " for user '%(username)s' in project '%(tenant_name)s'" %
-                {"auth_url": TEST_ENV["OS_AUTH_URL"],
-                 "username": TEST_ENV["OS_USERNAME"],
-                 "tenant_name": TEST_ENV["OS_TENANT_NAME"]},
                 str(e))
         else:
             self.fail("rally deployment fails to raise error for wrong"

tests/unit/platforms/test_existing.py

@@ -18,6 +18,7 @@ import jsonschema
 import mock
 from rally.env import env_mgr
 from rally.env import platform
+from rally import exceptions
 
 from rally_openstack.platforms import existing
 from tests.unit import test
@@ -245,14 +246,27 @@ class ExistingPlatformTestCase(PlatformBaseTestCase):
         self._check_health_schema(result)
         self.assertEqual({"available": True}, result)
         mock_clients.assert_has_calls(
-            [mock.call(pdata["admin"]), mock.call().verified_keystone(),
+            [mock.call(pdata["users"][0]), mock.call().keystone(),
-             mock.call(pdata["users"][0]), mock.call().keystone(),
+             mock.call(pdata["users"][1]), mock.call().keystone(),
-             mock.call(pdata["users"][1]), mock.call().keystone()])
+             mock.call(pdata["admin"]), mock.call().verified_keystone()])
+
+    @mock.patch("rally_openstack.osclients.Clients")
+    def test_check_failed_with_native_rally_exc(self, mock_clients):
+        e = exceptions.RallyException("foo")
+        mock_clients.return_value.keystone.side_effect = e
+        pdata = {"admin": None,
+                 "users": [{"username": "balbab", "password": "12345"}]}
+        result = existing.OpenStack({}, platform_data=pdata).check_health()
+        self._check_health_schema(result)
+        self.assertEqual({"available": False, "message": e.format_message(),
+                          "traceback": mock.ANY},
+                         result)
 
     @mock.patch("rally_openstack.osclients.Clients")
     def test_check_failed_admin(self, mock_clients):
         mock_clients.return_value.verified_keystone.side_effect = Exception
-        pdata = {"admin": {"username": "balbab", "password": "12345"}}
+        pdata = {"admin": {"username": "balbab", "password": "12345"},
+                 "users": []}
         result = existing.OpenStack({}, platform_data=pdata).check_health()
         self._check_health_schema(result)
         self.assertEqual(

tests/unit/test_osclients.py

@@ -283,38 +283,123 @@ class TestCreateKeystoneClient(test.TestCase, OSClientTestCaseUtils):
         keystone.auth_ref
         mock_keystone_get_session.assert_called_once_with()
 
-    @mock.patch("keystoneauth1.identity.base.BaseIdentityPlugin.get_access")
+    @mock.patch("%s.LOG.exception" % PATH)
-    def test_auth_ref_fails(self, mock_get_access):
+    @mock.patch("%s.logging.is_debug" % PATH)
-        mock_get_access.side_effect = Exception
+    def test_auth_ref_fails(self, mock_is_debug, mock_log_exception):
+        mock_is_debug.return_value = False
         keystone = osclients.Keystone(self.credential, {}, {})
+        session = mock.Mock()
+        auth_plugin = mock.Mock()
+        auth_plugin.get_access.side_effect = Exception
+        keystone.get_session = mock.Mock(return_value=(session, auth_plugin))
 
-        try:
+        self.assertRaises(osclients.AuthenticationFailed,
-            keystone.auth_ref
+                          lambda: keystone.auth_ref)
-        except exceptions.AuthenticationFailed:
+
-            pass
+        self.assertFalse(mock_log_exception.called)
-        else:
+        mock_is_debug.assert_called_once_with()
-            self.fail("keystone.auth_ref didn't raise"
+        auth_plugin.get_access.assert_called_once_with(session)
-                      " exceptions.AuthenticationFailed")
 
     @mock.patch("%s.LOG.exception" % PATH)
     @mock.patch("%s.logging.is_debug" % PATH)
-    @mock.patch("keystoneauth1.identity.base.BaseIdentityPlugin.get_access")
+    def test_auth_ref_fails_debug(self, mock_is_debug, mock_log_exception):
-    def test_auth_ref_debug(self, mock_get_access,
-                            mock_is_debug, mock_log_exception):
         mock_is_debug.return_value = True
-        mock_get_access.side_effect = Exception
         keystone = osclients.Keystone(self.credential, {}, {})
+        session = mock.Mock()
+        auth_plugin = mock.Mock()
+        auth_plugin.get_access.side_effect = Exception
+        keystone.get_session = mock.Mock(return_value=(session, auth_plugin))
 
-        try:
+        self.assertRaises(osclients.AuthenticationFailed,
-            keystone.auth_ref
+                          lambda: keystone.auth_ref)
-        except exceptions.AuthenticationFailed:
-            pass
-        else:
-            self.fail("keystone.auth_ref didn't raise"
-                      " exceptions.AuthenticationFailed")
 
         mock_log_exception.assert_called_once_with(mock.ANY)
         mock_is_debug.assert_called_once_with()
+        auth_plugin.get_access.assert_called_once_with(session)
+
+    @mock.patch("%s.LOG.exception" % PATH)
+    @mock.patch("%s.logging.is_debug" % PATH)
+    def test_auth_ref_fails_debug_with_native_keystone_error(
+            self, mock_is_debug, mock_log_exception):
+        from keystoneauth1 import exceptions as ks_exc
+
+        mock_is_debug.return_value = True
+        keystone = osclients.Keystone(self.credential, {}, {})
+        session = mock.Mock()
+        auth_plugin = mock.Mock()
+        auth_plugin.get_access.side_effect = ks_exc.ConnectFailure("foo")
+        keystone.get_session = mock.Mock(return_value=(session, auth_plugin))
+
+        self.assertRaises(osclients.AuthenticationFailed,
+                          lambda: keystone.auth_ref)
+
+        self.assertFalse(mock_log_exception.called)
+        mock_is_debug.assert_called_once_with()
+        auth_plugin.get_access.assert_called_once_with(session)
+
+    def test_authentication_failed_exception(self):
+        from keystoneauth1 import exceptions as ks_exc
+
+        original_e = KeyError("Oops")
+        e = osclients.AuthenticationFailed(
+            url="https://example.com", username="foo", project="project",
+            error=original_e
+        )
+        self.assertEqual(
+            "Failed to authenticate to https://example.com for user 'foo' in "
+            "project 'project': [KeyError] 'Oops'",
+            e.format_message())
+
+        original_e = ks_exc.Unauthorized("The request you have made requires "
+                                         "authentication.", request_id="ID")
+        e = osclients.AuthenticationFailed(
+            url="https://example.com", username="foo", project="project",
+            error=original_e
+        )
+        self.assertEqual(
+            "Failed to authenticate to https://example.com for user 'foo' in "
+            "project 'project': The request you have made requires "
+            "authentication.",
+            e.format_message())
+
+        original_e = ks_exc.ConnectionError("Some user-friendly native error")
+        e = osclients.AuthenticationFailed(
+            url="https://example.com", username="foo", project="project",
+            error=original_e
+        )
+        self.assertEqual("Some user-friendly native error",
+                         e.format_message())
+
+        original_e = ks_exc.ConnectionError(
+            "Unable to establish connection to https://example.com:500: "
+            "HTTPSConnectionPool(host='example.com', port=500): Max retries "
+            "exceeded with url: / (Caused by NewConnectionError('<urllib3."
+            "connection.VerifiedHTTPSConnection object at 0x7fb87a48e510>: "
+            "Failed to establish a new connection: [Errno 101] Network "
+            "is unreachable")
+        e = osclients.AuthenticationFailed(
+            url="https://example.com", username="foo", project="project",
+            error=original_e
+        )
+        self.assertEqual(
+            "Unable to establish connection to https://example.com:500",
+            e.format_message())
+
+        original_e = ks_exc.ConnectionError(
+            "Unable to establish connection to https://example.com:500: "
+            # another pool class
+            "HTTPConnectionPool(host='example.com', port=500): Max retries "
+            "exceeded with url: / (Caused by NewConnectionError('<urllib3."
+            "connection.VerifiedHTTPSConnection object at 0x7fb87a48e510>: "
+            "Failed to establish a new connection: [Errno 101] Network "
+            "is unreachable")
+        e = osclients.AuthenticationFailed(
+            url="https://example.com", username="foo", project="project",
+            error=original_e
+        )
+        self.assertEqual(
+            "Unable to establish connection to https://example.com:500",
+            e.format_message())
 
 
 @ddt.ddt

tests/unit/test_workarounds.py

@@ -38,6 +38,11 @@ class WorkaroundTestCase(test.TestCase):
             "'rally_openstack.validators' module has a check to do not "
             "register 'required_platforms@openstack' validator for old Rally "
             "releases."
+        ]),
+        ([1, 2], [
+            "'existing@openstack' platform puts 'traceback' in check method "
+            "in case of native keystone errors. It is redundant. "
+            "See https://review.openstack.org/597197"
         ])
     ]