Fix "access rule" commands to only use ID

dmendiza · dmendiza · commit bc60e3bb908a · 2023-06-05T12:03:15.000-04:00
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
diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py
@@ -92,6 +92,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,7 +47,7 @@ def take_action(self, parsed_args):
         errors = 0
         for ac in parsed_args.access_rule:
             try:
-                access_rule = utils.find_resource(
+                access_rule = common.get_resource_by_id(
                     identity_client.access_rules, ac
                 )
                 identity_client.access_rules.delete(access_rule.id)
@@ -114,13 +114,13 @@ 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(
+        access_rule = common.get_resource_by_id(
             identity_client.access_rules, parsed_args.access_rule
         )
 
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
@@ -64,11 +63,12 @@ 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,
@@ -87,14 +87,10 @@ def test_delete_multi_access_rules_with_exception(self, find_mock):
                 '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.
