From 500704716266716f0b3079047238e430f4ed01c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20M=C3=A1gr?= Date: Tue, 13 May 2025 13:56:27 +0200 Subject: [PATCH] Deprecate disable-rbac option This patch makes --disable-rbac flag NOOP. Depends-On: I598fd222f7e41e80ad9fc7f0008c965190255b05 Change-Id: I01f0096d4fcfd0ce4bea2e1ee0d16ad2fccc841f --- .../tests/functional/test_cli.py | 126 ++++++------------ observabilityclient/tests/unit/test_cli.py | 10 +- .../tests/unit/test_python_api.py | 29 +--- observabilityclient/v1/base.py | 23 +++- observabilityclient/v1/python_api.py | 6 +- 5 files changed, 78 insertions(+), 116 deletions(-) diff --git a/observabilityclient/tests/functional/test_cli.py b/observabilityclient/tests/functional/test_cli.py index 4a8c053..9d3c14e 100644 --- a/observabilityclient/tests/functional/test_cli.py +++ b/observabilityclient/tests/functional/test_cli.py @@ -14,98 +14,56 @@ from observabilityclient.tests.functional import base import time -class CliTestFunctionalRBACDisabled(base.CliTestCase): - """Functional tests for cli commands with disabled RBAC.""" +class CliTestFunctionalRBACNOOP(base.CliTestCase): + """Functional tests for cli commands testing RBAC option.""" def test_list(self): - cmd_output = self.openstack( - 'metric list --disable-rbac', - parse_output=True, - ) - name_list = [item.get('metric_name') for item in cmd_output] - self.assertIn( - 'up', - name_list - ) + for cmdstr in ('metric list', 'metric list --disable-rbac'): + cmd_output = self.openstack( + cmdstr, + parse_output=True, + ) + name_list = [item.get('metric_name') for item in cmd_output] + self.assertIn( + 'ceilometer_image_size', + name_list + ) + self.assertIn( + 'up', + name_list + ) def test_show(self): - cmd_output = self.openstack( - 'metric show up --disable-rbac', - parse_output=True, - ) - for metric in cmd_output: - self.assertEqual( - "up", - metric["__name__"] - ) - self.assertEqual( - "1", - metric["value"] + for cmdstr in ('metric show up', 'metric show up --disable-rbac'): + cmd_output = self.openstack( + cmdstr, + parse_output=True, ) + for metric in cmd_output: + self.assertEqual( + "up", + metric["__name__"] + ) + self.assertEqual( + "1", + metric["value"] + ) def test_query(self): - cmd_output = self.openstack( - 'metric query up --disable-rbac', - parse_output=True, - ) - for metric in cmd_output: - self.assertEqual( - "up", - metric["__name__"] - ) - self.assertEqual( - "1", - metric["value"] - ) - - -class CliTestFunctionalRBACEnabled(base.CliTestCase): - """Functional tests for cli commands with enabled RBAC.""" - - def test_list(self): - cmd_output = self.openstack( - 'metric list', - parse_output=True, - ) - name_list = [item.get('metric_name') for item in cmd_output] - self.assertIn( - 'ceilometer_image_size', - name_list - ) - self.assertNotIn( - 'up', - name_list - ) - - def test_show(self): - cmd_output = self.openstack( - 'metric show ceilometer_image_size', - parse_output=True, - ) - for metric in cmd_output: - self.assertEqual( - "ceilometer_image_size", - metric["__name__"] - ) - self.assertEqual( - "custom", - metric["job"] - ) - - def test_query(self): - cmd_output = self.openstack( - 'metric query ceilometer_image_size', - parse_output=True, - ) - for metric in cmd_output: - self.assertEqual( - "ceilometer_image_size", - metric["__name__"] - ) - self.assertEqual( - "custom", - metric["job"] + for cmdstr in ('metric query up', 'metric query up --disable-rbac'): + cmd_output = self.openstack( + cmdstr, + parse_output=True, ) + for metric in cmd_output: + self.assertEqual( + "up", + metric["__name__"] + ) + self.assertEqual( + "1", + metric["value"] + ) class CliTestFunctionalAdminCommands(base.CliTestCase): diff --git a/observabilityclient/tests/unit/test_cli.py b/observabilityclient/tests/unit/test_cli.py index 536de62..a43fa6b 100644 --- a/observabilityclient/tests/unit/test_cli.py +++ b/observabilityclient/tests/unit/test_cli.py @@ -43,8 +43,9 @@ class CliTest(testtools.TestCase): return_value=self.client), \ mock.patch.object(self.client.query, 'list', return_value=metric_names) as m: + # NOTE: disable-rbac option is NOOP, RBAC is disabled by default ret1 = cli_list.take_action(test_parsed_args_enabled) - m.assert_called_with(disable_rbac=False) + m.assert_called_with(disable_rbac=True) ret2 = cli_list.take_action(test_parsed_args_disabled) m.assert_called_with(disable_rbac=True) @@ -75,9 +76,9 @@ class CliTest(testtools.TestCase): return_value=self.client), \ mock.patch.object(self.client.query, 'show', return_value=prom_metric) as m: - + # NOTE: disable-rbac option is NOOP, RBAC is disabled by default ret1 = cli_show.take_action(test_parsed_args_enabled) - m.assert_called_with('metric_name', disable_rbac=False) + m.assert_called_with('metric_name', disable_rbac=True) ret2 = cli_show.take_action(test_parsed_args_disabled) m.assert_called_with('metric_name', disable_rbac=True) @@ -113,8 +114,9 @@ class CliTest(testtools.TestCase): mock.patch.object(self.client.query, 'query', return_value=prom_metric) as m: + # NOTE: disable-rbac option is NOOP, RBAC is disabled by default ret1 = cli_query.take_action(test_parsed_args_enabled) - m.assert_called_with(query, disable_rbac=False) + m.assert_called_with(query, disable_rbac=True) ret2 = cli_query.take_action(test_parsed_args_disabled) m.assert_called_with(query, disable_rbac=True) diff --git a/observabilityclient/tests/unit/test_python_api.py b/observabilityclient/tests/unit/test_python_api.py index 5fdc9c1..f7fe74f 100644 --- a/observabilityclient/tests/unit/test_python_api.py +++ b/observabilityclient/tests/unit/test_python_api.py @@ -40,33 +40,16 @@ class QueryManagerTest(testtools.TestCase): self.client.query = self.manager def test_list(self): - returned_by_prom_rbac = { - 'data': [ - { - '__name__': 'metric1', - 'label1': 'foo' - }, - { - '__name__': 'test42', - 'anotherlabel': 'bar' - }, - { - '__name__': 'abc2', - } - ] - } - returned_by_prom_no_rbac = {'data': ['metric1', 'test42', 'abc2']} + returned_data = {'data': ['metric1', 'test42', 'abc2']} expected = ['abc2', 'metric1', 'test42'] with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get', - return_value=returned_by_prom_rbac): + return_value=returned_data): ret1 = self.manager.list() - self.assertEqual(expected, ret1) + self.assertEqual(expected, ret1) - with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get', - return_value=returned_by_prom_no_rbac): ret2 = self.manager.list(disable_rbac=True) - self.assertEqual(expected, ret2) + self.assertEqual(expected, ret2) def test_show(self): query = 'some_metric' @@ -90,7 +73,7 @@ class QueryManagerTest(testtools.TestCase): with mock.patch.object(prometheus_client.PrometheusAPIClient, '_get', return_value=returned_by_prom): ret1 = self.manager.show(query) - self.rbac.append_rbac_labels.assert_called_with(query) + self.rbac.append_rbac_labels.assert_not_called() self.assertThat(ret1, expected_matcher) self.assertThat(ret2, expected_matcher) @@ -118,7 +101,7 @@ class QueryManagerTest(testtools.TestCase): self.rbac.modify_query.assert_not_called() ret2 = self.manager.query(query) - self.rbac.modify_query.assert_called_with(query) + self.rbac.modify_query.assert_not_called() self.assertThat(ret1, expected_matcher) self.assertThat(ret2, expected_matcher) diff --git a/observabilityclient/v1/base.py b/observabilityclient/v1/base.py index 87c4dd3..2a37fb6 100644 --- a/observabilityclient/v1/base.py +++ b/observabilityclient/v1/base.py @@ -13,20 +13,39 @@ # under the License. # +import argparse +import sys + from osc_lib.command import command from observabilityclient.i18n import _ +class DeprecatedNOOPAction(argparse.Action): + def __init__(self, *args, **kwargs): + kwargs['help'] = f"[DEPRECATED] {kwargs['help']}" \ + if 'help' in kwargs else \ + "[DEPRECATED]" + kwargs['required'] = False + kwargs['nargs'] = 0 + super().__init__(*args, **kwargs) + + def __call__(self, parser, namespace, values, option_string=None): + if option_string is not None: + print(f"Note: {option_string} is deprecated option, " + "has no effect and is a subject for removal " + "in future OpenStack release.", file=sys.stderr) + + class ObservabilityBaseCommand(command.Command): """Base class for metric commands.""" def get_parser(self, prog_name): parser = super().get_parser(prog_name) - # TODO(jwysogla): Should this be restricted somehow? parser.add_argument( '--disable-rbac', - action='store_true', + action=DeprecatedNOOPAction, + default=True, help=_("Disable rbac injection") ) return parser diff --git a/observabilityclient/v1/python_api.py b/observabilityclient/v1/python_api.py index 8de70a3..5f6810e 100644 --- a/observabilityclient/v1/python_api.py +++ b/observabilityclient/v1/python_api.py @@ -17,7 +17,7 @@ from observabilityclient.v1 import base class QueryManager(base.Manager): - def list(self, disable_rbac=False): + def list(self, disable_rbac=True): """List metric names. :param disable_rbac: Disables rbac injection if set to True @@ -34,7 +34,7 @@ class QueryManager(base.Manager): unique_metric_names = list(set([m['__name__'] for m in metrics])) return sorted(unique_metric_names) - def show(self, name, disable_rbac=False): + def show(self, name, disable_rbac=True): """Show current values for metrics of a specified name. :param disable_rbac: Disables rbac injection if set to True @@ -48,7 +48,7 @@ class QueryManager(base.Manager): last_metric_query = f"last_over_time({query}[5m])" return self.prom.query(last_metric_query) - def query(self, query, disable_rbac=False): + def query(self, query, disable_rbac=True): """Send a query to prometheus. The query can be any PromQL query. Labels for enforcing