Split get_prometheus_client to multiple functions

Improvements:
- better readability
- get_prom_client_from_keystone can be used by watcher for
  PrometheusAPIClient initialization.
- A better split between how the client is created for Aetos and
  how it's created for Prometheus.

Changes to the previous behavior of get_prometheus_client:
- It's not possible to override PrometheusAPIClient arguments with
  environment variables if they're retrieved from Keystone.
- It's not possible to use config file / environment variables to
  connect to Aetos (the keystone session parameter won't be passed to
  PrometheusAPIClient and so it won't be able to authenticate)

I think even though, there are some changes to how Aetos can access
can be configured, it's getting it a bit closer to how other services
work (getting the endpoint from keystone and not requiring / allowing
additional configuration). Prometheus access configuration through
config file / env variables stays unchanged.

Change-Id: Icd08347056e92502992d4fee799bb3e06e03c0c9
Signed-off-by: Jaromir Wysoglad <jwysogla@redhat.com>

observabilityclient/tests/unit/test_utils.py

@@ -137,26 +137,6 @@ class GetPrometheusClientTest(testtools.TestCase):
         )
         ca_m.assert_called_with(True)
 
-    def test_get_prometheus_client_from_keystone_custom_ca(self):
-        prom_endpoint = "https://localhost:1234/prometheus"
-        keystone_session = session.Session()
-        config_data = 'ca_cert: "ca/path"'
-        config_file = mock.mock_open(read_data=config_data)("name", 'r')
-        with mock.patch.dict(os.environ, {}), \
-                mock.patch.object(metric_utils, 'get_config_file',
-                                  return_value=config_file), \
-                mock.patch.object(adapter.Adapter, 'get_endpoint',
-                                  return_value=prom_endpoint), \
-                mock.patch.object(prometheus_client.PrometheusAPIClient,
-                                  "__init__", return_value=None) as init_m, \
-                mock.patch.object(prometheus_client.PrometheusAPIClient,
-                                  "set_ca_cert") as ca_m:
-            metric_utils.get_prometheus_client(keystone_session)
-        init_m.assert_called_with(
-            "localhost:1234", keystone_session, "prometheus"
-        )
-        ca_m.assert_called_with("ca/path")
-
     def test_get_prometheus_client_from_env_vars_ipv6(self):
         patched_env = {'PROMETHEUS_HOST': '2607:5300:201:2000::654',
                        'PROMETHEUS_PORT': '1234'}

observabilityclient/utils/metric_utils.py

@@ -49,13 +49,33 @@ def get_config_file():
     return None
 
 
-def get_prometheus_client(session=None, adapter_options={}):
+def get_prom_client_from_keystone(session, adapter_options=None):
-    host = None
+    if adapter_options is None:
-    port = None
+        adapter_options = {}
-    ca_cert = None
+    endpoint = adapter.Adapter(
-    root_path = ""
+        session=session, **adapter_options
-    is_aetos = False
+    ).get_endpoint()
+    parsed_url = parse.urlparse(endpoint)
 
+    escaped_host = netutils.escape_ipv6(parsed_url.hostname)
+    root_path = parsed_url.path.strip('/')
+    tls = parsed_url.scheme == "https"
+
+    if parsed_url.port is not None:
+        url = f'{escaped_host}:{parsed_url.port}'
+    else:
+        url = escaped_host
+
+    client = PrometheusAPIClient(url, session, root_path)
+
+    if tls:
+        client.set_ca_cert(True)
+    return client
+
+
+def get_prom_client_from_file_or_env():
+    host = port = ca_cert = None
+    root_path = ''
     conf_file = get_config_file()
     if conf_file is not None:
         conf = yaml.safe_load(conf_file)
@@ -68,37 +88,6 @@ def get_prometheus_client(session=None, adapter_options={}):
         if 'root_path' in conf:
             root_path = conf['root_path']
         conf_file.close()
-
-    if session is not None and (host is None or port is None):
-        try:
-            endpoint = adapter.Adapter(
-                session=session, **adapter_options
-            ).get_endpoint()
-            parsed_url = parse.urlparse(endpoint)
-            host = parsed_url.hostname
-            root_path = parsed_url.path.strip('/')
-            if parsed_url.scheme == "https" and ca_cert is None:
-                # NOTE(jwysogla): Use the default CA certs if the scheme
-                # is https, but keep the original value if already set,
-                # so that a custom certificate can be set in the config
-                # file, while the endpoint is retrieved from keystone.
-                ca_cert = True
-            if parsed_url.port is not None:
-                port = parsed_url.port
-            elif parsed_url.scheme == "https":
-                port = 443
-            else:
-                port = 80
-            is_aetos = True
-        except keystone_exception.EndpointNotFound:
-            # NOTE(jwysogla): Don't do anything here. It's still possible
-            # to get the correct endpoint configuration from the env vars.
-            # If that doesn't work, the same error message is part of the
-            # exception raised below.
-            pass
-
-    # NOTE(jwysogla): We allow to overide the prometheus.yaml by
-    #                 the environment variables
     if 'PROMETHEUS_HOST' in os.environ:
         host = os.environ['PROMETHEUS_HOST']
     if 'PROMETHEUS_PORT' in os.environ:
@@ -109,22 +98,48 @@ def get_prometheus_client(session=None, adapter_options={}):
         root_path = os.environ['PROMETHEUS_ROOT_PATH']
     if host is None or port is None:
         raise ConfigurationError("Can't find prometheus host and "
-                                 "port configuration and endpoint for "
+                                 "port configuration in config file or "
-                                 "metric-storage not found.")
+                                 "environment variables.")
     escaped_host = netutils.escape_ipv6(host)
-    if is_aetos:
+    client = PrometheusAPIClient(
-        client = PrometheusAPIClient(
+        f"{escaped_host}:{port}", None, root_path
-            f"{escaped_host}:{port}", session, root_path
+    )
-        )
-    else:
-        client = PrometheusAPIClient(
-            f"{escaped_host}:{port}", None, root_path
-        )
     if ca_cert is not None:
         client.set_ca_cert(ca_cert)
     return client
 
 
+def get_prometheus_client(session=None, adapter_options=None):
+    if adapter_options is None:
+        adapter_options = {}
+
+    keystone_error = None
+    if session is not None:
+        try:
+            return get_prom_client_from_keystone(session, adapter_options)
+        except keystone_exception.EndpointNotFound as e:
+            # NOTE(jwysogla): Fallback to get the endpoint configuration from
+            # the config file and env vars. If that doesn't work, the error
+            # message is raised as ConfigurationError later.
+            keystone_error = e
+            LOG.debug("Aetos endpoint discovery from Keystone failed: %s", e)
+
+    # NOTE(jwysogla): Always fallback to the original method of discovery
+    # through config file and env variables for backwards compatibility.
+    try:
+        return get_prom_client_from_file_or_env()
+    except ConfigurationError as e:
+        if keystone_error is not None:
+            raise ConfigurationError(
+                f"Failed to configure Prometheus client. "
+                f"Aetos discovery from keystone failed: '{keystone_error}'. "
+                f"Prometheus configuration from config file and environment "
+                f"variables failed: '{e}'"
+            )
+        else:
+            raise e
+
+
 def get_client(obj):
     return obj.app.client_manager.observabilityclient
 

releasenotes/notes/disable-config-overrides-for-keystone-discovered-endpoints-0b36ad382f029fa0.yaml

@@ -0,0 +1,7 @@
+---
+upgrade:
+  - |
+    Aetos endpoint information discovered from keystone can no longer
+    be overridden by a config file or by environment variables. From
+    now on, Aetos access must be fully discoverable from Keystone
+    endpoints only.