Fix "access rule" commands to only use ID

dmendiza · dmendiza · commit a03e3dbf75c5 · 2023-07-06T15:46:17.000Z
This patch modifies the access rule commands to use only the resource ID. The previous logic incorrectly assumed that access rules have a "name" property, which resulted in unexpected behaviors. For example, "access rule delete {non-existent-id}" now results in a "not found" error instead of sometimes deleting an unrelated rule. Story: 2010775 Task: 48163 Change-Id: Ib5c3b7f86acf1dfe7cc76dfa99fa4c118388bd71 (cherry picked from commit bc60e3b)
diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py
@@ -87,6 +87,18 @@ def get_resource(manager, name_type_or_id):
         raise exceptions.CommandError(msg % name_type_or_id)
 
 
+def get_resource_by_id(manager, resource_id):
+    """Get resource by ID
+
+    Raises CommandError if the resource is not found
+    """
+    try:
+        return manager.get(resource_id)
+    except identity_exc.NotFound:
+        msg = _("Resource with id {} not found")
+        raise exceptions.CommandError(msg.format(resource_id))
+
+
 def _get_token_resource(client, resource, parsed_name, parsed_domain=None):
     """Peek into the user's auth token to get resource IDs
 
diff --git a/openstackclient/identity/v3/access_rule.py b/openstackclient/identity/v3/access_rule.py
@@ -37,7 +37,7 @@ def get_parser(self, prog_name):
             'access_rule',
             metavar='<access-rule>',
             nargs="+",
-            help=_('Access rule(s) to delete (name or ID)'),
+            help=_('Access rule ID(s) to delete'),
         )
         return parser
 
@@ -47,8 +47,9 @@ def take_action(self, parsed_args):
         errors = 0
         for ac in parsed_args.access_rule:
             try:
-                access_rule = utils.find_resource(
-                    identity_client.access_rules, ac)
+                access_rule = common.get_resource_by_id(
+                    identity_client.access_rules, ac
+                )
                 identity_client.access_rules.delete(access_rule.id)
             except Exception as e:
                 errors += 1
@@ -103,14 +104,15 @@ def get_parser(self, prog_name):
         parser.add_argument(
             'access_rule',
             metavar='<access-rule>',
-            help=_('Access rule to display (name or ID)'),
+            help=_('Access rule ID to display'),
         )
         return parser
 
     def take_action(self, parsed_args):
         identity_client = self.app.client_manager.identity
-        access_rule = utils.find_resource(identity_client.access_rules,
-                                          parsed_args.access_rule)
+        access_rule = common.get_resource_by_id(
+            identity_client.access_rules, parsed_args.access_rule
+        )
 
         access_rule._info.pop('links', None)
 
diff --git a/openstackclient/tests/unit/identity/v3/test_access_rule.py b/openstackclient/tests/unit/identity/v3/test_access_rule.py
@@ -14,10 +14,9 @@
 #
 
 import copy
-from unittest import mock
 
+from keystoneclient import exceptions as identity_exc
 from osc_lib import exceptions
-from osc_lib import utils
 
 from openstackclient.identity.v3 import access_rule
 from openstackclient.tests.unit import fakes
@@ -69,10 +68,13 @@ def test_access_rule_delete(self):
         )
         self.assertIsNone(result)
 
-    @mock.patch.object(utils, 'find_resource')
-    def test_delete_multi_access_rules_with_exception(self, find_mock):
-        find_mock.side_effect = [self.access_rules_mock.get.return_value,
-                                 exceptions.CommandError]
+    def test_delete_multi_access_rules_with_exception(self):
+        # mock returns for common.get_resource_by_id
+        mock_get = self.access_rules_mock.get
+        mock_get.side_effect = [
+            mock_get.return_value,
+            identity_exc.NotFound,
+        ]
         arglist = [
             identity_fakes.access_rule_id,
             'nonexistent_access_rule',
@@ -89,12 +91,10 @@ def test_delete_multi_access_rules_with_exception(self, find_mock):
             self.assertEqual('1 of 2 access rules failed to'
                              ' delete.', str(e))
 
-        find_mock.assert_any_call(self.access_rules_mock,
-                                  identity_fakes.access_rule_id)
-        find_mock.assert_any_call(self.access_rules_mock,
-                                  'nonexistent_access_rule')
+        mock_get.assert_any_call(identity_fakes.access_rule_id)
+        mock_get.assert_any_call('nonexistent_access_rule')
 
-        self.assertEqual(2, find_mock.call_count)
+        self.assertEqual(2, mock_get.call_count)
         self.access_rules_mock.delete.assert_called_once_with(
             identity_fakes.access_rule_id)
 
diff --git a/releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml b/releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixed a bug in "access rule" subcommands where the client logic incorrectly
+    assumed that access rules have a "name" property which resulted in
+    unpredictable behaviors.  e.g. "access rule delete {non-existent-id}" now
+    results in a not-found error instead of sometimes deleting an unrelated
+    rule.
