From 339af2c20bfe44a772a1e39fc8b769db112b75ce Mon Sep 17 00:00:00 2001 From: Dean Troyer Date: Fri, 13 Jan 2017 14:05:04 -0600 Subject: [PATCH] Fix floating IP delete and show by IP The floating IP delete and show commands did not work using IP addresses as the selector, only ID. The SDK floating_ip resource does not support but OSC does, so we have to do it ourselves. Now with more SDK 0.9.10 support! Change-Id: Iea1b57cded6b16a56a06af87ab8f1fa001a3485e Closes-bug: 1656402 --- openstackclient/network/v2/floating_ip.py | 81 ++++++++++++++++- .../functional/network/v2/test_floating_ip.py | 6 -- .../tests/unit/network/v2/fakes.py | 1 + .../tests/unit/network/v2/test_floating_ip.py | 90 +++++++++++++++---- .../notes/bug-1656402-88b12760fb2d4ef9.yaml | 6 ++ 5 files changed, 157 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/bug-1656402-88b12760fb2d4ef9.yaml diff --git a/openstackclient/network/v2/floating_ip.py b/openstackclient/network/v2/floating_ip.py index 8202b3fae2..980c41c716 100644 --- a/openstackclient/network/v2/floating_ip.py +++ b/openstackclient/network/v2/floating_ip.py @@ -15,6 +15,8 @@ import logging +from openstack import exceptions as sdk_exceptions +from openstack.network.v2 import floating_ip as _floating_ip from osc_lib import utils from openstackclient.i18n import _ @@ -79,6 +81,58 @@ def _get_attrs(client_manager, parsed_args): return attrs +def _find_floating_ip( + session, + ip_cache, + name_or_id, + ignore_missing=True, + **params +): + """Find a floating IP by IP or ID + + The SDK's find_ip() can only locate a floating IP by ID so we have + to do this ourselves. + """ + + def _get_one_match(name_or_id): + """Given a list of results, return the match""" + the_result = None + for maybe_result in ip_cache: + id_value = maybe_result.id + ip_value = maybe_result.floating_ip_address + + if (id_value == name_or_id) or (ip_value == name_or_id): + # Only allow one resource to be found. If we already + # found a match, raise an exception to show it. + if the_result is None: + the_result = maybe_result + else: + msg = "More than one %s exists with the name '%s'." + msg = (msg % (_floating_ip.FloatingIP, name_or_id)) + raise sdk_exceptions.DuplicateResource(msg) + + return the_result + + # Try to short-circuit by looking directly for a matching ID. + try: + match = _floating_ip.FloatingIP.existing(id=name_or_id, **params) + return (match.get(session), ip_cache) + except sdk_exceptions.NotFoundException: + pass + + if len(ip_cache) == 0: + ip_cache = list(_floating_ip.FloatingIP.list(session, **params)) + + result = _get_one_match(name_or_id) + if result is not None: + return (result, ip_cache) + + if ignore_missing: + return (None, ip_cache) + raise sdk_exceptions.ResourceNotFound( + "No %s found for %s" % (_floating_ip.FloatingIP.__name__, name_or_id)) + + class CreateFloatingIP(common.NetworkAndComputeShowOne): _description = _("Create floating IP") @@ -186,13 +240,28 @@ class DeleteFloatingIP(common.NetworkAndComputeDelete): return parser def take_action_network(self, client, parsed_args): - obj = client.find_ip(self.r, ignore_missing=False) + (obj, self.ip_cache) = _find_floating_ip( + client.session, + self.ip_cache, + self.r, + ignore_missing=False, + ) client.delete_ip(obj) def take_action_compute(self, client, parsed_args): obj = utils.find_resource(client.floating_ips, self.r) client.floating_ips.delete(obj.id) + def take_action(self, parsed_args): + """Implements a naive cache for the list of floating IPs""" + + # NOTE(dtroyer): This really only prevents multiple list() + # calls when performing multiple resource deletes + # in a single command. In an interactive session + # each delete command will call list(). + self.ip_cache = [] + super(DeleteFloatingIP, self).take_action(parsed_args) + class DeleteIPFloating(DeleteFloatingIP): _description = _("Delete floating IP(s)") @@ -390,6 +459,9 @@ class ListIPFloating(ListFloatingIP): class ShowFloatingIP(common.NetworkAndComputeShowOne): _description = _("Display floating IP details") + # ip_cache is unused here but is a side effect of _find_floating_ip() + ip_cache = [] + def update_parser_common(self, parser): parser.add_argument( 'floating_ip', @@ -399,7 +471,12 @@ class ShowFloatingIP(common.NetworkAndComputeShowOne): return parser def take_action_network(self, client, parsed_args): - obj = client.find_ip(parsed_args.floating_ip, ignore_missing=False) + (obj, self.ip_cache) = _find_floating_ip( + client.session, + [], + parsed_args.floating_ip, + ignore_missing=False, + ) display_columns, columns = _get_network_columns(obj) data = utils.get_item_properties(obj, columns) return (display_columns, data) diff --git a/openstackclient/tests/functional/network/v2/test_floating_ip.py b/openstackclient/tests/functional/network/v2/test_floating_ip.py index 5f642f0415..f0b12bc7a8 100644 --- a/openstackclient/tests/functional/network/v2/test_floating_ip.py +++ b/openstackclient/tests/functional/network/v2/test_floating_ip.py @@ -14,8 +14,6 @@ import random import re import uuid -import testtools - from openstackclient.tests.functional import base @@ -25,7 +23,6 @@ class FloatingIpTests(base.TestCase): NETWORK_NAME = uuid.uuid4().hex @classmethod - @testtools.skip('broken SDK testing') def setUpClass(cls): # Set up some regex for matching below cls.re_id = re.compile("id\s+\|\s+(\S+)") @@ -62,7 +59,6 @@ class FloatingIpTests(base.TestCase): raw_output = cls.openstack('network delete ' + cls.NETWORK_NAME) cls.assertOutput('', raw_output) - @testtools.skip('broken SDK testing') def test_floating_ip_delete(self): """Test create, delete multiple""" raw_output = self.openstack( @@ -93,7 +89,6 @@ class FloatingIpTests(base.TestCase): raw_output = self.openstack('floating ip delete ' + ip1 + ' ' + ip2) self.assertOutput('', raw_output) - @testtools.skip('broken SDK testing') def test_floating_ip_list(self): """Test create defaults, list filters, delete""" raw_output = self.openstack( @@ -135,7 +130,6 @@ class FloatingIpTests(base.TestCase): # TODO(dtroyer): add more filter tests - @testtools.skip('broken SDK testing') def test_floating_ip_show(self): """Test show""" raw_output = self.openstack( diff --git a/openstackclient/tests/unit/network/v2/fakes.py b/openstackclient/tests/unit/network/v2/fakes.py index fe0422face..98d5dea37b 100644 --- a/openstackclient/tests/unit/network/v2/fakes.py +++ b/openstackclient/tests/unit/network/v2/fakes.py @@ -52,6 +52,7 @@ VALID_DSCP_MARKS = [0, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30, 32, class FakeNetworkV2Client(object): def __init__(self, **kwargs): + self.session = mock.Mock() self.extensions = mock.Mock() self.extensions.resource_class = fakes.FakeResource(None, {}) diff --git a/openstackclient/tests/unit/network/v2/test_floating_ip.py b/openstackclient/tests/unit/network/v2/test_floating_ip.py index 63d22bf84c..e395300d99 100644 --- a/openstackclient/tests/unit/network/v2/test_floating_ip.py +++ b/openstackclient/tests/unit/network/v2/test_floating_ip.py @@ -208,13 +208,19 @@ class TestDeleteFloatingIPNetwork(TestFloatingIPNetwork): super(TestDeleteFloatingIPNetwork, self).setUp() self.network.delete_ip = mock.Mock(return_value=None) - self.network.find_ip = ( - network_fakes.FakeFloatingIP.get_floating_ips(self.floating_ips)) # Get the command object to test self.cmd = floating_ip.DeleteFloatingIP(self.app, self.namespace) - def test_floating_ip_delete(self): + @mock.patch( + "openstackclient.tests.unit.network.v2.test_floating_ip." + + "floating_ip._find_floating_ip" + ) + def test_floating_ip_delete(self, find_floating_ip_mock): + find_floating_ip_mock.side_effect = [ + (self.floating_ips[0], []), + (self.floating_ips[1], []), + ] arglist = [ self.floating_ips[0].id, ] @@ -225,12 +231,24 @@ class TestDeleteFloatingIPNetwork(TestFloatingIPNetwork): result = self.cmd.take_action(parsed_args) - self.network.find_ip.assert_called_once_with( - self.floating_ips[0].id, ignore_missing=False) + find_floating_ip_mock.assert_called_once_with( + mock.ANY, + [], + self.floating_ips[0].id, + ignore_missing=False, + ) self.network.delete_ip.assert_called_once_with(self.floating_ips[0]) self.assertIsNone(result) - def test_multi_floating_ips_delete(self): + @mock.patch( + "openstackclient.tests.unit.network.v2.test_floating_ip." + + "floating_ip._find_floating_ip" + ) + def test_floating_ip_delete_multi(self, find_floating_ip_mock): + find_floating_ip_mock.side_effect = [ + (self.floating_ips[0], []), + (self.floating_ips[1], []), + ] arglist = [] verifylist = [] @@ -243,13 +261,37 @@ class TestDeleteFloatingIPNetwork(TestFloatingIPNetwork): result = self.cmd.take_action(parsed_args) + calls = [ + call( + mock.ANY, + [], + self.floating_ips[0].id, + ignore_missing=False, + ), + call( + mock.ANY, + [], + self.floating_ips[1].id, + ignore_missing=False, + ), + ] + find_floating_ip_mock.assert_has_calls(calls) + calls = [] for f in self.floating_ips: calls.append(call(f)) self.network.delete_ip.assert_has_calls(calls) self.assertIsNone(result) - def test_multi_floating_ips_delete_with_exception(self): + @mock.patch( + "openstackclient.tests.unit.network.v2.test_floating_ip." + + "floating_ip._find_floating_ip" + ) + def test_floating_ip_delete_multi_exception(self, find_floating_ip_mock): + find_floating_ip_mock.side_effect = [ + (self.floating_ips[0], []), + exceptions.CommandError, + ] arglist = [ self.floating_ips[0].id, 'unexist_floating_ip', @@ -260,21 +302,24 @@ class TestDeleteFloatingIPNetwork(TestFloatingIPNetwork): ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - find_mock_result = [self.floating_ips[0], exceptions.CommandError] - self.network.find_ip = ( - mock.Mock(side_effect=find_mock_result) - ) - try: self.cmd.take_action(parsed_args) self.fail('CommandError should be raised.') except exceptions.CommandError as e: self.assertEqual('1 of 2 floating_ips failed to delete.', str(e)) - self.network.find_ip.assert_any_call( - self.floating_ips[0].id, ignore_missing=False) - self.network.find_ip.assert_any_call( - 'unexist_floating_ip', ignore_missing=False) + find_floating_ip_mock.assert_any_call( + mock.ANY, + [], + self.floating_ips[0].id, + ignore_missing=False, + ) + find_floating_ip_mock.assert_any_call( + mock.ANY, + [], + 'unexist_floating_ip', + ignore_missing=False, + ) self.network.delete_ip.assert_called_once_with( self.floating_ips[0] ) @@ -534,7 +579,12 @@ class TestShowFloatingIPNetwork(TestFloatingIPNetwork): # Get the command object to test self.cmd = floating_ip.ShowFloatingIP(self.app, self.namespace) - def test_floating_ip_show(self): + @mock.patch( + "openstackclient.tests.unit.network.v2.test_floating_ip." + + "floating_ip._find_floating_ip" + ) + def test_floating_ip_show(self, find_floating_ip_mock): + find_floating_ip_mock.return_value = (self.floating_ip, []) arglist = [ self.floating_ip.id, ] @@ -545,9 +595,11 @@ class TestShowFloatingIPNetwork(TestFloatingIPNetwork): columns, data = self.cmd.take_action(parsed_args) - self.network.find_ip.assert_called_once_with( + find_floating_ip_mock.assert_called_once_with( + mock.ANY, + [], self.floating_ip.id, - ignore_missing=False + ignore_missing=False, ) self.assertEqual(self.columns, columns) self.assertEqual(self.data, data) diff --git a/releasenotes/notes/bug-1656402-88b12760fb2d4ef9.yaml b/releasenotes/notes/bug-1656402-88b12760fb2d4ef9.yaml new file mode 100644 index 0000000000..c8a302982a --- /dev/null +++ b/releasenotes/notes/bug-1656402-88b12760fb2d4ef9.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fix ``floating ip delete`` and ``floating ip show`` to accept IP addresses + in addition to IDs to select floating IPs to delete or show. + [Bug `1656402 `_