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

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

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(

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, {})
 

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(
+        find_floating_ip_mock.assert_called_once_with(
-            self.floating_ips[0].id, ignore_missing=False)
+            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(
+        find_floating_ip_mock.assert_any_call(
-            self.floating_ips[0].id, ignore_missing=False)
+            mock.ANY,
-        self.network.find_ip.assert_any_call(
+            [],
-            'unexist_floating_ip', ignore_missing=False)
+            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)

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 <https://bugs.launchpad.net/bugs/1656402>`_