Merge ~zzehring/charm-openstack-service-checks:bugfix/lp1882822-keystone-client-errors-block-resolving-hooks into ~llama-charmers/charm-openstack-service-checks:master

[Zachary Zehring]

[Andrea Ieri (aieri)]

looks good, but there are a couple of things that might be done a little differently (see in-line comments).

The workload_status of OSCKeystoneClientError must be fixed, but the other comments are related more to taste.

review: Needs Fixing

[Adam Dyess (addyess)]

Proposed a new MR with these changes + Merge Review changes

https://code.launchpad.net/~addyess/charm-openstack-service-checks/+git/charm-openstack-service-checks/+merge/386897

diff --git a/lib/lib_openstack_service_checks.py b/lib/lib_openstack_service_checks.py
index 47a136b..99a0706 100644
--- a/lib/lib_openstack_service_checks.py
+++ b/lib/lib_openstack_service_checks.py
@@ -22,8 +22,31 @@ class OSCCredentialsError(Exception):
     pass
 
 
-class OSCEndpointError(OSCCredentialsError):
-    pass
+class OSCKeystoneError(Exception):
+    @property
+    def workload_status(self):
+        raise NotImplementedError
+
+
+class OSCKeystoneServerError(OSCKeystoneError):
+    @property
+    def workload_status(self):
+        return 'Keystone server error encountered trying to list keystone resources. ' \
+               'Check keystone server health. View juju logs for more info.'
+
+
+class OSCKeystoneClientError(OSCKeystoneError):
+    @property
+    def workload_status(self):
+        return 'Keystone client request error encountered trying to keystone resources. ' \
+               'Check keystone auth creds and url. View juju logs for more info.'
+
+
+class OSCSslError(OSCKeystoneError):
+    @property
+    def workload_status(self):
+        return 'SSL error encountered when requesting Keystone for resource list. ' \
+               'Check trusted_ssl_ca config option. View juju logs for more info.'
 
 
 class OSCHelper():
@@ -409,25 +432,38 @@ class OSCHelper():
         self._keystone_client = client.Client(session=sess)
 
         if self._keystone_client is None:
-            raise OSCEndpointError('Unable to list the endpoint errors, yet: '
-                                   'could not connect to the Identity Service')
+            raise OSCKeystoneServerError('Unable to list the endpoints yet: '
+                                         'could not connect to the Identity Service')
 
     @property
     def keystone_endpoints(self):
-        try:
-            endpoints = self._keystone_client.endpoints.list()
-            hookenv.log("Endpoints from keystone: {}".format(endpoints))
-            return endpoints
-        except keystoneauth1.exceptions.http.InternalServerError as error:
-            raise OSCEndpointError(
-                'Unable to list the keystone endpoints, yet: {}'.format(error))
+        endpoints = self._safe_keystone_client_command(self._keystone_client.endpoints.list,
+                                                       object_type='endpoints')
+        hookenv.log("Endpoints from keystone: {}".format(endpoints))
+        return endpoints
 
     @property
     def keystone_services(self):
-        services = self._keystone_client.services.list()
+        services = self._safe_keystone_client_command(self._keystone_client.services.list,
+                                                      object_type='services')
         hookenv.log("Services from keystone: {}".format(services))
         return services
 
+    @staticmethod
+    def _safe_keystone_client_command(client_command, object_type='objects'):
+        try:
+            response = client_command()
+        except (keystoneauth1.exceptions.http.InternalServerError,
+                keystoneauth1.exceptions.connection.ConnectFailure) as server_error:
+            raise OSCKeystoneServerError(
+                'Keystone server unable to list keystone {}: {}'.format(server_error, object_type))
+        except keystoneauth1.exceptions.http.BadRequest as client_error:
+            raise OSCKeystoneClientError(
+                'Keystone client error when listing {}: {}'.format(client_error, object_type))
+        except keystoneauth1.exceptions.connection.SSLError as ssl_error:
+            raise OSCSslError('Keystone ssl error when listing {}: {}'.format(ssl_error, object_type))
+        return response
+
     @property
     def _load_envvars(self, novarc='/var/lib/nagios/nagios.novarc'):
         if not os.path.exists(novarc):
diff --git a/reactive/openstack_service_checks.py b/reactive/openstack_service_checks.py
index 95dfaf0..cce8b28 100644
--- a/reactive/openstack_service_checks.py
+++ b/reactive/openstack_service_checks.py
@@ -24,7 +24,7 @@ from charms.reactive import any_flags_set, clear_flag, is_flag_set, set_flag, wh
 from lib_openstack_service_checks import (
     OSCHelper,
     OSCCredentialsError,
-    OSCEndpointError
+    OSCKeystoneError
 )
 
 CERT_FILE = '/usr/local/share/ca-certificates/openstack-service-checks.crt'
@@ -176,8 +176,8 @@ def render_config():
     try:
         helper.render_checks(creds)
         set_flag('openstack-service-checks.endpoints.configured')
-    except OSCEndpointError as error:
-        hookenv.log(error)
+    except OSCKeystoneError as keystone_error:
+        _set_keystone_error_workload_status(keystone_error)
 
     if not helper.deploy_rally():
         # Rally could not be installed (if enabled). No further actions taken
@@ -207,8 +207,8 @@ def configure_nrpe_endpoints():
         helper.create_endpoint_checks(creds)
         set_flag('openstack-service-checks.endpoints.configured')
         clear_flag('openstack-service-checks.started')
-    except OSCEndpointError as error:
-        hookenv.log(error)
+    except OSCKeystoneError as keystone_error:
+        _set_keystone_error_workload_status(keystone_error)
 
 
 @when('identity-notifications.available.updated')
@@ -221,10 +221,15 @@ def endpoints_changed():
 def do_restart():
     hookenv.log('Reloading nagios-nrpe-server')
     host.service_restart('nagios-nrpe-server')
-    hookenv.status_set('active', 'Unit is ready')
     set_flag('openstack-service-checks.started')
 
 
+@when('openstack-service-checks.started')
+@when('openstack-service-checks.endpoints.configured')
+def set_active():
+    hookenv.status_set('active', 'Unit is ready')
+
+
 @when('nrpe-external-master.available')
 def do_reconfigure_nrpe():
     os_credentials_flag = 'config.changed.os-credentials'
@@ -285,3 +290,9 @@ def parse_hooks():
 
         # render configs again
         do_reconfigure_nrpe()
+
+
+def _set_keystone_error_workload_status(keystone_error):
+    error_status_message = 'Failed to create endpoint checks due issue communicating with Keystone'
+    hookenv.log('{}. Error:\n{}'.format(error_status_message, keystone_error), level=hookenv.ERROR)
+    hookenv.status_set('blocked', keystone_error.workload_status)
diff --git a/tests/unit/test_lib.py b/tests/unit/test_lib.py
index effd5ea..e925ee2 100644
--- a/tests/unit/test_lib.py
+++ b/tests/unit/test_lib.py
@@ -1,4 +1,9 @@
+from unittest.mock import MagicMock
+
 import pytest
+import keystoneauth1
+
+from lib_openstack_service_checks import OSCKeystoneServerError, OSCKeystoneClientError, OSCSslError
 
 
 def test_openstackservicechecks_common_properties(openstackservicechecks):
@@ -61,3 +66,41 @@ def test_get_rally_checks_context(skip_rally, result, openstackservicechecks):
     expected = {comp: result[num]
                 for num, comp in enumerate('cinder glance nova neutron'.split())}
     assert openstackservicechecks._get_rally_checks_context() == expected
+
+
[e-mail]('keystone_auth_exception,expected_raised_exception', [
+    (keystoneauth1.exceptions.http.InternalServerError, OSCKeystoneServerError),
+    (keystoneauth1.exceptions.connection.ConnectFailure, OSCKeystoneServerError),
+    (keystoneauth1.exceptions.http.BadRequest, OSCKeystoneClientError),
+    (keystoneauth1.exceptions.connection.SSLError, OSCSslError),
+])
+def test_keystone_endpoints_client_exceptions(
+        keystone_auth_exception, expected_raised_exception, openstackservicechecks):
+    mock_keystone_client = MagicMock()
+    mock_keystone_client.endpoints.list.side_effect = keystone_auth_exception
+    openstackservicechecks._keystone_client = mock_keystone_client
+    try:
+        openstackservicechecks.keystone_endpoints
+    except Exception as e:
+        assert isinstance(e, expected_raised_exception)
+    else:
+        assert False, 'Error should have been raised.'
+
+
[e-mail]('keystone_auth_exception,expected_raised_exception', [
+    (keystoneauth1.exceptions.http.InternalServerError, OSCKeystoneServerError),
+    (keystoneauth1.exceptions.connection.ConnectFailure, OSCKeystoneServerError),
+    (keystoneauth1.exceptions.http.BadRequest, OSCKeystoneClientError),
+    (keystoneauth1.exceptions.connection.SSLError, OSCSslError),
+])
+def test_keystone_services_client_exceptions(
+        keystone_auth_exception, expected_raised_exception, openstackservicechecks):
+    mock_keystone_client = MagicMock()
+    mock_keystone_client.services.list.side_effect = keystone_auth_exception
+    openstackservicechecks._keystone_client = mock_keystone_client
+    try:
+        openstackservicechecks.keystone_services
+    except Exception as e:
+        assert isinstance(e, expected_raised_exception)
+    else:
+        assert False, 'Error should have been raised.'