From 72dcb9b9d4fc8c7534a91ec900ec91230cbbb274 Mon Sep 17 00:00:00 2001 From: Jaromir Wysoglad Date: Wed, 9 Apr 2025 07:53:38 -0400 Subject: [PATCH] Refactor obsC rbac module The idea is to make the code usable from outside of the observabilityclient by Aetos and services like Aodh or Watcher. This includes: - Rename of the class to a more descriptive "PromQLRbac" - Renaming PromQLRbac functions to better names - Getting rid of the "disable_rbac" logic in the rbac module - Using the PrometheusAPIClient directly for Prometheus requests - Ability to specify any project_id - Moving the code out of v1/ folder Change-Id: I920b4730be110e82cd577b81a30e3a5af6280c98 --- observabilityclient/{v1 => }/rbac.py | 69 ++++------- .../tests/unit/test_python_api.py | 32 +++-- observabilityclient/tests/unit/test_rbac.py | 110 +++++++----------- observabilityclient/v1/client.py | 7 +- observabilityclient/v1/python_api.py | 16 ++- .../notes/refactor-rbac-967ed5edcc1ef2a4.yaml | 13 +++ 6 files changed, 106 insertions(+), 141 deletions(-) rename observabilityclient/{v1 => }/rbac.py (74%) create mode 100644 releasenotes/notes/refactor-rbac-967ed5edcc1ef2a4.yaml diff --git a/observabilityclient/v1/rbac.py b/observabilityclient/rbac.py similarity index 74% rename from observabilityclient/v1/rbac.py rename to observabilityclient/rbac.py index 6481bc9..39741c4 100644 --- a/observabilityclient/v1/rbac.py +++ b/observabilityclient/rbac.py @@ -14,32 +14,15 @@ import re -from keystoneauth1.exceptions.auth_plugins import MissingAuthPlugin - from observabilityclient.utils.metric_utils import format_labels -class ObservabilityRbacError(Exception): - pass - - -class Rbac(object): - def __init__(self, client, session, disable_rbac=False): - self.client = client - self.session = session - self.disable_rbac = disable_rbac - try: - self.project_id = self.session.get_project_id() - self.default_labels = { - "project": self.project_id - } - self.rbac_init_successful = True - except MissingAuthPlugin: - self.project_id = None - self.default_labels = { - "project": "no-project" - } - self.rbac_init_successful = False +class PromQLRbac(object): + def __init__(self, prom_api_client, project_id): + self.client = prom_api_client + self.labels = { + "project": project_id + } def _find_label_value_end(self, query, start, quote_char): end = start @@ -92,29 +75,22 @@ class Rbac(object): # this indicates illegal format without closing } . return -1 - def _insert_labels( - self, query, location, labels, comma=False, braces=False - ): + def _insert_labels(self, query, location, comma=False, braces=False): comma_str = ", " if comma else "" - labels_str = f"{{{labels}}}" if braces else labels + formatted_labels = format_labels(self.labels) + labels_str = f"{{{formatted_labels}}}" if braces else formatted_labels return (f"{query[:location]}{comma_str}" f"{labels_str}" f"{query[location:]}") - def enrich_query(self, query, disable_rbac=False): - """Add rbac labels to queries. + def modify_query(self, query): + """Add rbac labels to a query. - :param query: The query to enrich + :param query: The query to modify :type query: str - :param disable_rbac: Disables rbac injection if set to True - :type disable_rbac: boolean """ - if disable_rbac: - return query - labels = self.default_labels - # We need to get all metric names, no matter the rbac - metric_names = self.client.query.list(disable_rbac=False) + metric_names = self.client.label_values("__name__") # We need to detect the locations of metric names # inside the query @@ -145,7 +121,6 @@ class Rbac(object): query = self._insert_labels( query, labels_end, - format_labels(labels), comma=False, braces=False ) @@ -153,7 +128,6 @@ class Rbac(object): query = self._insert_labels( query, labels_end, - format_labels(labels), comma=True, braces=False ) @@ -161,26 +135,23 @@ class Rbac(object): query = self._insert_labels( query, name_end_location, - format_labels(labels), comma=False, braces=True ) return query - def append_rbac(self, query, disable_rbac=False): + def append_rbac_labels(self, query): """Append rbac labels to queries. - It's a simplified and faster version of enrich_query(). This just + It's a simplified and faster version of modify_query(). This just appends the labels at the end of the query string. For proper handling of complex queries, where metric names might occure elsewhere than - just at the end, please use the enrich_query() function. + just at the end, please use the modify_query() function. :param query: The query to append to :type query: str - :param disable_rbac: Disables rbac injection if set to True - :type disable_rbac: boolean """ - labels = self.default_labels - if disable_rbac: - return query - return f"{query}{{{format_labels(labels)}}}" + if any(c in query for c in "{}"): + return self.modify_query(query) + else: + return f"{query}{{{format_labels(self.labels)}}}" diff --git a/observabilityclient/tests/unit/test_python_api.py b/observabilityclient/tests/unit/test_python_api.py index ea2c2f4..5fdc9c1 100644 --- a/observabilityclient/tests/unit/test_python_api.py +++ b/observabilityclient/tests/unit/test_python_api.py @@ -17,11 +17,11 @@ from unittest import mock import testtools from observabilityclient import prometheus_client +from observabilityclient import rbac from observabilityclient.tests.unit.test_prometheus_client import ( MetricListMatcher ) from observabilityclient.v1 import python_api -from observabilityclient.v1 import rbac class QueryManagerTest(testtools.TestCase): @@ -31,10 +31,8 @@ class QueryManagerTest(testtools.TestCase): prom_client = prometheus_client.PrometheusAPIClient("somehost") self.client.prometheus_client = prom_client - self.rbac = mock.Mock(wraps=rbac.Rbac(self.client, mock.Mock())) - self.rbac.default_labels = {'project': 'project_id'} - self.rbac.rbac_init_succesful = True - self.rbac.disable_rbac = False + self.rbac = mock.Mock(wraps=rbac.PromQLRbac(prom_client, mock.Mock())) + self.rbac.labels = {'project': 'project_id'} self.manager = python_api.QueryManager(self.client) @@ -83,15 +81,16 @@ class QueryManagerTest(testtools.TestCase): } expected = [prometheus_client.PrometheusMetric(returned_by_prom)] expected_matcher = MetricListMatcher(expected) + + with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get', + return_value=returned_by_prom): + ret2 = self.manager.show(query, disable_rbac=True) + self.rbac.append_rbac_labels.assert_not_called() + with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get', return_value=returned_by_prom): ret1 = self.manager.show(query) - self.rbac.append_rbac.assert_called_with(query, - disable_rbac=False) - - ret2 = self.manager.show(query, disable_rbac=True) - self.rbac.append_rbac.assert_called_with(query, - disable_rbac=True) + self.rbac.append_rbac_labels.assert_called_with(query) self.assertThat(ret1, expected_matcher) self.assertThat(ret2, expected_matcher) @@ -114,13 +113,12 @@ class QueryManagerTest(testtools.TestCase): return_value=returned_by_prom), \ mock.patch.object(python_api.QueryManager, 'list', return_value=queried_metric_name): - ret1 = self.manager.query(query) - self.rbac.enrich_query.assert_called_with(query, - disable_rbac=False) - ret2 = self.manager.query(query, disable_rbac=True) - self.rbac.enrich_query.assert_called_with(query, - disable_rbac=True) + ret1 = self.manager.query(query, disable_rbac=True) + self.rbac.modify_query.assert_not_called() + + ret2 = self.manager.query(query) + self.rbac.modify_query.assert_called_with(query) self.assertThat(ret1, expected_matcher) self.assertThat(ret2, expected_matcher) diff --git a/observabilityclient/tests/unit/test_rbac.py b/observabilityclient/tests/unit/test_rbac.py index 6d82216..d5f3bd6 100644 --- a/observabilityclient/tests/unit/test_rbac.py +++ b/observabilityclient/tests/unit/test_rbac.py @@ -14,24 +14,20 @@ from unittest import mock -from keystoneauth1.exceptions.auth_plugins import MissingAuthPlugin -from keystoneauth1 import session import testtools -from observabilityclient.v1 import rbac +from observabilityclient import rbac -class RbacTest(testtools.TestCase): +class PromQLRbacTest(testtools.TestCase): def setUp(self): - super(RbacTest, self).setUp() - self.rbac = rbac.Rbac(mock.Mock(), mock.Mock()) - self.rbac.project_id = "secret_id" - self.rbac.default_labels = { - "project": self.rbac.project_id + super(PromQLRbacTest, self).setUp() + self.project_id = "project123" + self.rbac = rbac.PromQLRbac(mock.Mock(), mock.Mock()) + self.rbac.labels = { + "project": "project123" } - self.rbac.disable_rbac = False - - self.rbac.client.query.list = lambda disable_rbac: [ + self.rbac.client.label_values = lambda label: [ 'test_query', 'cpu_temp_celsius', 'http_requests', @@ -43,42 +39,42 @@ class RbacTest(testtools.TestCase): self.test_cases = [ ( "test_query", - f"test_query{{project='{self.rbac.project_id}'}}" + f"test_query{{project='{self.project_id}'}}" ), ( "test_query{somelabel='value'}", (f"test_query{{somelabel='value', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "test_query{somelabel='value', label2='value2'}", (f"test_query{{somelabel='value', label2='value2', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "test_query{somelabel='unicode{}{ \t/-_#~$&%\\'}", (f"test_query{{somelabel='unicode{{}}{{ \t/-_#~$&%\\', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "test_query{somelabel='s p a c e'}", (f"test_query{{somelabel='s p a c e', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "test_query{somelabel='doublequotes\"'}", (f"test_query{{somelabel='doublequotes\"', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( 'test_query{somelabel="singlequotes\'"}', (f'test_query{{somelabel="singlequotes\'", ' - f'project=\'{self.rbac.project_id}\'}}') + f'project=\'{self.project_id}\'}}') ), ( "test_query{doesnt_match_regex!~'regex'}", (f"test_query{{doesnt_match_regex!~'regex', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "delta(cpu_temp_celsius{host='zeus'}[2h]) - " "sum(http_requests) + " @@ -86,81 +82,81 @@ class RbacTest(testtools.TestCase): "sum(http_requests{or_regex=~'smth1|something2|3'})", (f"delta(cpu_temp_celsius{{host='zeus', " - f"project='{self.rbac.project_id}'}}[2h]) - " + f"project='{self.project_id}'}}[2h]) - " f"sum(http_requests" - f"{{project='{self.rbac.project_id}'}}) + " + f"{{project='{self.project_id}'}}) + " f"sum(http_requests{{instance=~'.*', " - f"project='{self.rbac.project_id}'}}) + " + f"project='{self.project_id}'}}) + " f"sum(http_requests{{or_regex=~'smth1|something2|3', " - f"project='{self.rbac.project_id}'}})") + f"project='{self.project_id}'}})") ), ( "round(test_query{label='something'},5)", (f"round(test_query{{label='something', " - f"project='{self.rbac.project_id}'}},5)") + f"project='{self.project_id}'}},5)") ), ( "sum by (foo) (test_query{label_1='baz'})", (f"sum by (foo) (test_query{{label_1='baz', " - f"project='{self.rbac.project_id}'}})") + f"project='{self.project_id}'}})") ), ( "test_query{} + avg without (application, group) " "(test:query:with:colon:{label='baz'})", - (f"test_query{{project='{self.rbac.project_id}'}} + " + (f"test_query{{project='{self.project_id}'}} + " f"avg without (application, group) " f"(test:query:with:colon:{{label='baz', " - f"project='{self.rbac.project_id}'}})") + f"project='{self.project_id}'}})") ), ( "test_query{label1='foo'} + on (label1,label2) " "avg by (label3) (test_query_with_digit1{label='baz'," "label1='foo',label2='bar'})", (f"test_query{{label1='foo', " - f"project='{self.rbac.project_id}'}} " + f"project='{self.project_id}'}} " f"+ on (label1,label2) avg by (label3) " f"(test_query_with_digit1{{label='baz'," f"label1='foo',label2='bar', " - f"project='{self.rbac.project_id}'}})") + f"project='{self.project_id}'}})") ), ( "{label='no-metric'}", (f"{{label='no-metric', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "http_requests{environment=~" "'staging|testing|development',method!='GET'}", (f"http_requests{{environment=~" f"'staging|testing|development',method!='GET', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "http_requests{replica!='rep-a',replica=~'rep.*'}", (f"http_requests{{replica!='rep-a',replica=~'rep.*', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "{__name__=~'job:.*'}", (f"{{__name__=~'job:.*', " - f"project='{self.rbac.project_id}'}}") + f"project='{self.project_id}'}}") ), ( "http_requests offset 5m", (f"http_requests" - f"{{project='{self.rbac.project_id}'}} " + f"{{project='{self.project_id}'}} " f"offset 5m") ), ( "rate(http_requests[5m] offset -1w)", (f"rate(http_requests" - f"{{project='{self.rbac.project_id}'}}" + f"{{project='{self.project_id}'}}" f"[5m] offset -1w)") ), ( "http_requests @ 1609746000", (f"http_requests" - f"{{project='{self.rbac.project_id}'}} " + f"{{project='{self.project_id}'}} " f"@ 1609746000") ), ( "histogram_quantile(0.9, sum by (le) " @@ -168,45 +164,25 @@ class RbacTest(testtools.TestCase): (f"histogram_quantile(0.9, sum by (le) " f"(rate(http_requests" - f"{{project='{self.rbac.project_id}'}}" + f"{{project='{self.project_id}'}}" f"[10m])))" ) ) ] def test_constructor(self): - with mock.patch.object(session.Session, 'get_project_id', - return_value="123"): - r = rbac.Rbac("client", session.Session(), False) - self.assertEqual(r.project_id, "123") - self.assertEqual(r.default_labels, { - "project": "123" - }) + r = rbac.PromQLRbac("client", "123") + self.assertEqual(r.labels, { + "project": "123" + }) - def test_constructor_error(self): - with mock.patch.object(session.Session, 'get_project_id', - side_effect=MissingAuthPlugin()): - r = rbac.Rbac("client", session.Session(), False) - self.assertIsNone(r.project_id) - - def test_enrich_query(self): + def test_modify_query(self): for query, expected in self.test_cases: - ret = self.rbac.enrich_query(query) + ret = self.rbac.modify_query(query) self.assertEqual(expected, ret) - def test_enrich_query_disable(self): - for query, expected in self.test_cases: - ret = self.rbac.enrich_query(query, disable_rbac=True) - self.assertEqual(query, ret) - - def test_append_rbac(self): + def test_append_rbac_labels(self): query = "test_query" - expected = f"{query}{{project='{self.rbac.project_id}'}}" - ret = self.rbac.append_rbac(query) - self.assertEqual(expected, ret) - - def test_append_rbac_disable(self): - query = "test_query" - expected = query - ret = self.rbac.append_rbac(query, disable_rbac=True) + expected = f"{query}{{project='{self.project_id}'}}" + ret = self.rbac.append_rbac_labels(query) self.assertEqual(expected, ret) diff --git a/observabilityclient/v1/client.py b/observabilityclient/v1/client.py index d0ed250..77176f7 100644 --- a/observabilityclient/v1/client.py +++ b/observabilityclient/v1/client.py @@ -14,9 +14,9 @@ import keystoneauth1.session +from observabilityclient import rbac from observabilityclient.utils.metric_utils import get_prometheus_client from observabilityclient.v1 import python_api -from observabilityclient.v1 import rbac class Client(object): @@ -40,4 +40,7 @@ class Client(object): self.prometheus_client = get_prometheus_client() self.query = python_api.QueryManager(self) - self.rbac = rbac.Rbac(self, self.session, disable_rbac) + self.rbac = rbac.PromQLRbac( + self.prometheus_client, + self.session.get_project_id() + ) diff --git a/observabilityclient/v1/python_api.py b/observabilityclient/v1/python_api.py index 0eb9ae7..8de70a3 100644 --- a/observabilityclient/v1/python_api.py +++ b/observabilityclient/v1/python_api.py @@ -23,11 +23,11 @@ class QueryManager(base.Manager): :param disable_rbac: Disables rbac injection if set to True :type disable_rbac: boolean """ - if disable_rbac or self.client.rbac.disable_rbac: + if disable_rbac: metric_names = self.prom.label_values("__name__") return sorted(metric_names) else: - match = f"{{{format_labels(self.client.rbac.default_labels)}}}" + match = f"{{{format_labels(self.client.rbac.labels)}}}" metrics = self.prom.series(match) if metrics == []: return [] @@ -40,9 +40,12 @@ class QueryManager(base.Manager): :param disable_rbac: Disables rbac injection if set to True :type disable_rbac: boolean """ - enriched = self.client.rbac.append_rbac(name, - disable_rbac=disable_rbac) - last_metric_query = f"last_over_time({enriched}[5m])" + query = "" + if disable_rbac: + query = name + else: + query = self.client.rbac.append_rbac_labels(name) + last_metric_query = f"last_over_time({query}[5m])" return self.prom.query(last_metric_query) def query(self, query, disable_rbac=False): @@ -63,7 +66,8 @@ class QueryManager(base.Manager): :param disable_rbac: Disables rbac injection if set to True :type disable_rbac: boolean """ - query = self.client.rbac.enrich_query(query, disable_rbac=disable_rbac) + if not disable_rbac: + query = self.client.rbac.modify_query(query) return self.prom.query(query) def delete(self, matches, start=None, end=None): diff --git a/releasenotes/notes/refactor-rbac-967ed5edcc1ef2a4.yaml b/releasenotes/notes/refactor-rbac-967ed5edcc1ef2a4.yaml new file mode 100644 index 0000000..0678083 --- /dev/null +++ b/releasenotes/notes/refactor-rbac-967ed5edcc1ef2a4.yaml @@ -0,0 +1,13 @@ +--- +upgrade: + - | + The rbac module was refactored. The `Rbac` class was renamed to + `PromQLRbac` and it was moved from `observabilityclient.v1.rbac` to + `observabilityclient.rbac`. The `enrich_query` function was renamed to + `modify_query` and the `append_rbac` function was renamed to + `append_rbac_labels`. All "disable_rbac" logic was removed from the + PromQLRbac. This functionality should be handled externaly from the class + if needed. The PromQLRbac constructor now requires an instance of the + PrometheusAPIClient instead of an instance of observabilityclient. It + also requires an explicitly specified project id instead of trying to + figure it out from a provided session object.