Deprecate disable-rbac option

This patch makes --disable-rbac flag NOOP.

Depends-On: I598fd222f7e41e80ad9fc7f0008c965190255b05
Change-Id: I01f0096d4fcfd0ce4bea2e1ee0d16ad2fccc841f

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):

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)

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)

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

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