From 2f31846d73c044740ccaaa4204720f0b94d64145 Mon Sep 17 00:00:00 2001 From: Jaromir Wysoglad Date: Thu, 24 Jul 2025 13:42:58 -0400 Subject: [PATCH] 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 --- observabilityclient/tests/unit/test_utils.py | 20 ---- observabilityclient/utils/metric_utils.py | 109 ++++++++++-------- ...discovered-endpoints-0b36ad382f029fa0.yaml | 7 ++ 3 files changed, 69 insertions(+), 67 deletions(-) create mode 100644 releasenotes/notes/disable-config-overrides-for-keystone-discovered-endpoints-0b36ad382f029fa0.yaml diff --git a/observabilityclient/tests/unit/test_utils.py b/observabilityclient/tests/unit/test_utils.py index 5d98c29..15c3204 100644 --- a/observabilityclient/tests/unit/test_utils.py +++ b/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'} diff --git a/observabilityclient/utils/metric_utils.py b/observabilityclient/utils/metric_utils.py index 0548fb6..4584072 100644 --- a/observabilityclient/utils/metric_utils.py +++ b/observabilityclient/utils/metric_utils.py @@ -49,13 +49,33 @@ def get_config_file(): return None -def get_prometheus_client(session=None, adapter_options={}): - host = None - port = None - ca_cert = None - root_path = "" - is_aetos = False +def get_prom_client_from_keystone(session, adapter_options=None): + if adapter_options is None: + adapter_options = {} + endpoint = adapter.Adapter( + session=session, **adapter_options + ).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 " - "metric-storage not found.") + "port configuration in config file or " + "environment variables.") escaped_host = netutils.escape_ipv6(host) - if is_aetos: - client = PrometheusAPIClient( - f"{escaped_host}:{port}", session, root_path - ) - else: - client = PrometheusAPIClient( - f"{escaped_host}:{port}", None, root_path - ) + 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 diff --git a/releasenotes/notes/disable-config-overrides-for-keystone-discovered-endpoints-0b36ad382f029fa0.yaml b/releasenotes/notes/disable-config-overrides-for-keystone-discovered-endpoints-0b36ad382f029fa0.yaml new file mode 100644 index 0000000..9ae2776 --- /dev/null +++ b/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.