Fix "access rule" commands to only use ID
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 f4748f4d25
)
This commit is contained in:
parent
bf758a6385
commit
560f19b894
|
@ -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
|
||||
|
||||
|
|
|
@ -37,7 +37,7 @@ class DeleteAccessRule(command.Command):
|
|||
'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 @@ class DeleteAccessRule(command.Command):
|
|||
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 @@ class ShowAccessRule(command.ShowOne):
|
|||
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)
|
||||
|
||||
|
|
|
@ -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 @@ class TestAccessRuleDelete(TestAccessRule):
|
|||
)
|
||||
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 @@ class TestAccessRuleDelete(TestAccessRule):
|
|||
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)
|
||||
|
||||
|
|
|
@ -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.
|
Loading…
Reference in New Issue