openstack/python-observabilityclient
Code Issues Proposed changes

Refactor obsC rbac module

Browse Source 
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
This commit is contained in:
Jaromir Wysoglad
2025-04-09 07:53:38 -04:00
parent d69cb08a77
commit 72dcb9b9d4
6 changed files with 106 additions and 141 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
observabilityclient/v1/rbac.py → observabilityclient/rbac.py
View File

@@ -14,32 +14,15 @@
import re import re
from keystoneauth1.exceptions.auth_plugins import MissingAuthPlugin
from observabilityclient.utils.metric_utils import format_labels from observabilityclient.utils.metric_utils import format_labels
class ObservabilityRbacError(Exception): class PromQLRbac(object):
pass def __init__(self, prom_api_client, project_id):
self.client = prom_api_client
self.labels = {
class Rbac(object): "project": project_id
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
def _find_label_value_end(self, query, start, quote_char): def _find_label_value_end(self, query, start, quote_char):
end = start end = start
@@ -92,29 +75,22 @@ class Rbac(object):
# this indicates illegal format without closing } . # this indicates illegal format without closing } .
return -1 return -1
def _insert_labels( def _insert_labels(self, query, location, comma=False, braces=False):
self, query, location, labels, comma=False, braces=False
):
comma_str = ", " if comma else "" 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}" return (f"{query[:location]}{comma_str}"
f"{labels_str}" f"{labels_str}"
f"{query[location:]}") f"{query[location:]}")
def enrich_query(self, query, disable_rbac=False): def modify_query(self, query):
"""Add rbac labels to queries. """Add rbac labels to a query.
:param query: The query to enrich :param query: The query to modify
:type query: str :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 # 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 # We need to detect the locations of metric names
# inside the query # inside the query
@@ -145,7 +121,6 @@ class Rbac(object):
query = self._insert_labels( query = self._insert_labels(
query, query,
labels_end, labels_end,
format_labels(labels),
comma=False, comma=False,
braces=False braces=False
) )
@@ -153,7 +128,6 @@ class Rbac(object):
query = self._insert_labels( query = self._insert_labels(
query, query,
labels_end, labels_end,
format_labels(labels),
comma=True, comma=True,
braces=False braces=False
) )
@@ -161,26 +135,23 @@ class Rbac(object):
query = self._insert_labels( query = self._insert_labels(
query, query,
name_end_location, name_end_location,
format_labels(labels),
comma=False, comma=False,
braces=True braces=True
) )
return query return query
def append_rbac(self, query, disable_rbac=False): def append_rbac_labels(self, query):
"""Append rbac labels to queries. """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 appends the labels at the end of the query string. For proper handling
of complex queries, where metric names might occure elsewhere than 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 :param query: The query to append to
:type query: str :type query: str
:param disable_rbac: Disables rbac injection if set to True
:type disable_rbac: boolean
""" """
labels = self.default_labels if any(c in query for c in "{}"):
if disable_rbac: return self.modify_query(query)
return query else:
return f"{query}{{{format_labels(labels)}}}" return f"{query}{{{format_labels(self.labels)}}}"

32
observabilityclient/tests/unit/test_python_api.py
View File

@@ -17,11 +17,11 @@ from unittest import mock
import testtools import testtools
from observabilityclient import prometheus_client from observabilityclient import prometheus_client
from observabilityclient import rbac
from observabilityclient.tests.unit.test_prometheus_client import ( from observabilityclient.tests.unit.test_prometheus_client import (
MetricListMatcher MetricListMatcher
) )
from observabilityclient.v1 import python_api from observabilityclient.v1 import python_api
from observabilityclient.v1 import rbac
class QueryManagerTest(testtools.TestCase): class QueryManagerTest(testtools.TestCase):
@@ -31,10 +31,8 @@ class QueryManagerTest(testtools.TestCase):
prom_client = prometheus_client.PrometheusAPIClient("somehost") prom_client = prometheus_client.PrometheusAPIClient("somehost")
self.client.prometheus_client = prom_client self.client.prometheus_client = prom_client
self.rbac = mock.Mock(wraps=rbac.Rbac(self.client, mock.Mock())) self.rbac = mock.Mock(wraps=rbac.PromQLRbac(prom_client, mock.Mock()))
self.rbac.default_labels = {'project': 'project_id'} self.rbac.labels = {'project': 'project_id'}
self.rbac.rbac_init_succesful = True
self.rbac.disable_rbac = False
self.manager = python_api.QueryManager(self.client) self.manager = python_api.QueryManager(self.client)
@@ -83,15 +81,16 @@ class QueryManagerTest(testtools.TestCase):
} }
expected = [prometheus_client.PrometheusMetric(returned_by_prom)] expected = [prometheus_client.PrometheusMetric(returned_by_prom)]
expected_matcher = MetricListMatcher(expected) 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', with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get',
return_value=returned_by_prom): return_value=returned_by_prom):
ret1 = self.manager.show(query) ret1 = self.manager.show(query)
self.rbac.append_rbac.assert_called_with(query, self.rbac.append_rbac_labels.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.assertThat(ret1, expected_matcher) self.assertThat(ret1, expected_matcher)
self.assertThat(ret2, expected_matcher) self.assertThat(ret2, expected_matcher)
@@ -114,13 +113,12 @@ class QueryManagerTest(testtools.TestCase):
return_value=returned_by_prom), \ return_value=returned_by_prom), \
mock.patch.object(python_api.QueryManager, 'list', mock.patch.object(python_api.QueryManager, 'list',
return_value=queried_metric_name): 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) ret1 = self.manager.query(query, disable_rbac=True)
self.rbac.enrich_query.assert_called_with(query, self.rbac.modify_query.assert_not_called()
disable_rbac=True)
ret2 = self.manager.query(query)
self.rbac.modify_query.assert_called_with(query)
self.assertThat(ret1, expected_matcher) self.assertThat(ret1, expected_matcher)
self.assertThat(ret2, expected_matcher) self.assertThat(ret2, expected_matcher)

110
observabilityclient/tests/unit/test_rbac.py
View File

@@ -14,24 +14,20 @@
from unittest import mock from unittest import mock
from keystoneauth1.exceptions.auth_plugins import MissingAuthPlugin
from keystoneauth1 import session
import testtools import testtools
from observabilityclient.v1 import rbac from observabilityclient import rbac
class RbacTest(testtools.TestCase): class PromQLRbacTest(testtools.TestCase):
def setUp(self): def setUp(self):
super(RbacTest, self).setUp() super(PromQLRbacTest, self).setUp()
self.rbac = rbac.Rbac(mock.Mock(), mock.Mock()) self.project_id = "project123"
self.rbac.project_id = "secret_id" self.rbac = rbac.PromQLRbac(mock.Mock(), mock.Mock())
self.rbac.default_labels = { self.rbac.labels = {
"project": self.rbac.project_id "project": "project123"
} }
self.rbac.disable_rbac = False self.rbac.client.label_values = lambda label: [
self.rbac.client.query.list = lambda disable_rbac: [
'test_query', 'test_query',
'cpu_temp_celsius', 'cpu_temp_celsius',
'http_requests', 'http_requests',
@@ -43,42 +39,42 @@ class RbacTest(testtools.TestCase):
self.test_cases = [ self.test_cases = [
( (
"test_query", "test_query",
f"test_query{{project='{self.rbac.project_id}'}}" f"test_query{{project='{self.project_id}'}}"
), ( ), (
"test_query{somelabel='value'}", "test_query{somelabel='value'}",
(f"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'}", "test_query{somelabel='value', label2='value2'}",
(f"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/-_#~$&%\\'}", "test_query{somelabel='unicode{}{ \t/-_#~$&%\\'}",
(f"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'}", "test_query{somelabel='s p a c e'}",
(f"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\"'}", "test_query{somelabel='doublequotes\"'}",
(f"test_query{{somelabel='doublequotes\"', " (f"test_query{{somelabel='doublequotes\"', "
f"project='{self.rbac.project_id}'}}") f"project='{self.project_id}'}}")
), ( ), (
'test_query{somelabel="singlequotes\'"}', 'test_query{somelabel="singlequotes\'"}',
(f'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'}", "test_query{doesnt_match_regex!~'regex'}",
(f"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]) - " "delta(cpu_temp_celsius{host='zeus'}[2h]) - "
"sum(http_requests) + " "sum(http_requests) + "
@@ -86,81 +82,81 @@ class RbacTest(testtools.TestCase):
"sum(http_requests{or_regex=~'smth1|something2|3'})", "sum(http_requests{or_regex=~'smth1|something2|3'})",
(f"delta(cpu_temp_celsius{{host='zeus', " (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"sum(http_requests"
f"{{project='{self.rbac.project_id}'}}) + " f"{{project='{self.project_id}'}}) + "
f"sum(http_requests{{instance=~'.*', " 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"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)", "round(test_query{label='something'},5)",
(f"round(test_query{{label='something', " (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'})", "sum by (foo) (test_query{label_1='baz'})",
(f"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{} + avg without (application, group) "
"(test:query:with:colon:{label='baz'})", "(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"avg without (application, group) "
f"(test:query:with:colon:{{label='baz', " 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) " "test_query{label1='foo'} + on (label1,label2) "
"avg by (label3) (test_query_with_digit1{label='baz'," "avg by (label3) (test_query_with_digit1{label='baz',"
"label1='foo',label2='bar'})", "label1='foo',label2='bar'})",
(f"test_query{{label1='foo', " (f"test_query{{label1='foo', "
f"project='{self.rbac.project_id}'}} " f"project='{self.project_id}'}} "
f"+ on (label1,label2) avg by (label3) " f"+ on (label1,label2) avg by (label3) "
f"(test_query_with_digit1{{label='baz'," f"(test_query_with_digit1{{label='baz',"
f"label1='foo',label2='bar', " f"label1='foo',label2='bar', "
f"project='{self.rbac.project_id}'}})") f"project='{self.project_id}'}})")
), ( ), (
"{label='no-metric'}", "{label='no-metric'}",
(f"{{label='no-metric', " (f"{{label='no-metric', "
f"project='{self.rbac.project_id}'}}") f"project='{self.project_id}'}}")
), ( ), (
"http_requests{environment=~" "http_requests{environment=~"
"'staging|testing|development',method!='GET'}", "'staging|testing|development',method!='GET'}",
(f"http_requests{{environment=~" (f"http_requests{{environment=~"
f"'staging|testing|development',method!='GET', " f"'staging|testing|development',method!='GET', "
f"project='{self.rbac.project_id}'}}") f"project='{self.project_id}'}}")
), ( ), (
"http_requests{replica!='rep-a',replica=~'rep.*'}", "http_requests{replica!='rep-a',replica=~'rep.*'}",
(f"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:.*'}", "{__name__=~'job:.*'}",
(f"{{__name__=~'job:.*', " (f"{{__name__=~'job:.*', "
f"project='{self.rbac.project_id}'}}") f"project='{self.project_id}'}}")
), ( ), (
"http_requests offset 5m", "http_requests offset 5m",
(f"http_requests" (f"http_requests"
f"{{project='{self.rbac.project_id}'}} " f"{{project='{self.project_id}'}} "
f"offset 5m") f"offset 5m")
), ( ), (
"rate(http_requests[5m] offset -1w)", "rate(http_requests[5m] offset -1w)",
(f"rate(http_requests" (f"rate(http_requests"
f"{{project='{self.rbac.project_id}'}}" f"{{project='{self.project_id}'}}"
f"[5m] offset -1w)") f"[5m] offset -1w)")
), ( ), (
"http_requests @ 1609746000", "http_requests @ 1609746000",
(f"http_requests" (f"http_requests"
f"{{project='{self.rbac.project_id}'}} " f"{{project='{self.project_id}'}} "
f"@ 1609746000") f"@ 1609746000")
), ( ), (
"histogram_quantile(0.9, sum by (le) " "histogram_quantile(0.9, sum by (le) "
@@ -168,45 +164,25 @@ class RbacTest(testtools.TestCase):
(f"histogram_quantile(0.9, sum by (le) " (f"histogram_quantile(0.9, sum by (le) "
f"(rate(http_requests" f"(rate(http_requests"
f"{{project='{self.rbac.project_id}'}}" f"{{project='{self.project_id}'}}"
f"[10m])))" f"[10m])))"
) )
) )
] ]
def test_constructor(self): def test_constructor(self):
with mock.patch.object(session.Session, 'get_project_id', r = rbac.PromQLRbac("client", "123")
return_value="123"): self.assertEqual(r.labels, {
r = rbac.Rbac("client", session.Session(), False) "project": "123"
self.assertEqual(r.project_id, "123") })
self.assertEqual(r.default_labels, {
"project": "123"
})
def test_constructor_error(self): def test_modify_query(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):
for query, expected in self.test_cases: for query, expected in self.test_cases:
ret = self.rbac.enrich_query(query) ret = self.rbac.modify_query(query)
self.assertEqual(expected, ret) self.assertEqual(expected, ret)
def test_enrich_query_disable(self): def test_append_rbac_labels(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):
query = "test_query" query = "test_query"
expected = f"{query}{{project='{self.rbac.project_id}'}}" expected = f"{query}{{project='{self.project_id}'}}"
ret = self.rbac.append_rbac(query) ret = self.rbac.append_rbac_labels(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)
self.assertEqual(expected, ret) self.assertEqual(expected, ret)

7
observabilityclient/v1/client.py
View File

@@ -14,9 +14,9 @@
import keystoneauth1.session import keystoneauth1.session
from observabilityclient import rbac
from observabilityclient.utils.metric_utils import get_prometheus_client from observabilityclient.utils.metric_utils import get_prometheus_client
from observabilityclient.v1 import python_api from observabilityclient.v1 import python_api
from observabilityclient.v1 import rbac
class Client(object): class Client(object):
@@ -40,4 +40,7 @@ class Client(object):
self.prometheus_client = get_prometheus_client() self.prometheus_client = get_prometheus_client()
self.query = python_api.QueryManager(self) 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()
)

16
observabilityclient/v1/python_api.py
View File

@@ -23,11 +23,11 @@ class QueryManager(base.Manager):
:param disable_rbac: Disables rbac injection if set to True :param disable_rbac: Disables rbac injection if set to True
:type disable_rbac: boolean :type disable_rbac: boolean
""" """
if disable_rbac or self.client.rbac.disable_rbac: if disable_rbac:
metric_names = self.prom.label_values("__name__") metric_names = self.prom.label_values("__name__")
return sorted(metric_names) return sorted(metric_names)
else: else:
match = f"{{{format_labels(self.client.rbac.default_labels)}}}" match = f"{{{format_labels(self.client.rbac.labels)}}}"
metrics = self.prom.series(match) metrics = self.prom.series(match)
if metrics == []: if metrics == []:
return [] return []
@@ -40,9 +40,12 @@ class QueryManager(base.Manager):
:param disable_rbac: Disables rbac injection if set to True :param disable_rbac: Disables rbac injection if set to True
:type disable_rbac: boolean :type disable_rbac: boolean
""" """
enriched = self.client.rbac.append_rbac(name, query = ""
disable_rbac=disable_rbac) if disable_rbac:
last_metric_query = f"last_over_time({enriched}[5m])" 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) return self.prom.query(last_metric_query)
def query(self, query, disable_rbac=False): 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 :param disable_rbac: Disables rbac injection if set to True
:type disable_rbac: boolean :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) return self.prom.query(query)
def delete(self, matches, start=None, end=None): def delete(self, matches, start=None, end=None):

13
releasenotes/notes/refactor-rbac-967ed5edcc1ef2a4.yaml Normal file
View File

@@ -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.