diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 36265cc8..9a50bacd 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~~~ diff --git a/rally_openstack/osclients.py b/rally_openstack/osclients.py index 457a35c0..787336ed 100644 --- a/rally_openstack/osclients.py +++ b/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: - if logging.is_debug(): + except Exception as original_e: + 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, - project=self.credential.tenant_name, - url=self.credential.auth_url, - etype=e.__class__.__name__, - error=str(e)) + + raise e return self.cache["keystone_auth_ref"] def get_session(self, version=None): diff --git a/rally_openstack/platforms/existing.py b/rally_openstack/platforms/existing.py index 33714125..78f61ee4 100644 --- a/rally_openstack/platforms/existing.py +++ b/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" - % json.dumps(d, indent=2, sort_keys=True)), + "Bad %s creds: \n%s" + % (user_role, + json.dumps(d, indent=2, sort_keys=True))), "traceback": traceback.format_exc() } diff --git a/tests/functional/test_cli_deployment.py b/tests/functional/test_cli_deployment.py index 68bc03b4..2aec84b0 100644 --- a/tests/functional/test_cli_deployment.py +++ b/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" - " project %(tenant_name)s" % - {"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"]}, + "AuthenticationFailed: Unable to establish connection to " + "%s" % TEST_ENV["OS_AUTH_URL"], str(e)) else: self.fail("rally deployment fails to raise error for wrong" diff --git a/tests/unit/platforms/test_existing.py b/tests/unit/platforms/test_existing.py index 752341f2..c71773cf 100644 --- a/tests/unit/platforms/test_existing.py +++ b/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"][1]), mock.call().keystone()]) + [mock.call(pdata["users"][0]), 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( diff --git a/tests/unit/test_osclients.py b/tests/unit/test_osclients.py index bbfb4f7d..50ae645c 100644 --- a/tests/unit/test_osclients.py +++ b/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") - def test_auth_ref_fails(self, mock_get_access): - mock_get_access.side_effect = Exception + @mock.patch("%s.LOG.exception" % PATH) + @mock.patch("%s.logging.is_debug" % PATH) + 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: - keystone.auth_ref - except exceptions.AuthenticationFailed: - pass - else: - self.fail("keystone.auth_ref didn't raise" - " exceptions.AuthenticationFailed") + 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) @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_debug(self, mock_get_access, - mock_is_debug, mock_log_exception): + def test_auth_ref_fails_debug(self, 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: - keystone.auth_ref - except exceptions.AuthenticationFailed: - pass - else: - self.fail("keystone.auth_ref didn't raise" - " exceptions.AuthenticationFailed") + self.assertRaises(osclients.AuthenticationFailed, + lambda: keystone.auth_ref) 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(': " + "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(': " + "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 diff --git a/tests/unit/test_workarounds.py b/tests/unit/test_workarounds.py index b57a867e..77ac2f87 100644 --- a/tests/unit/test_workarounds.py +++ b/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" ]) ]