Handle multiple ports in AddFloatingIPqueens-em3.14.3

AddFloatingIP refers to an old nova proxy API to neutron that was
deprecated in nova. The neutron API for floating IP associate requires
a port to be specified. Currently, the code is selecting the first port
if the server has multiple ports. But, an attempt to associate the
first port with a floating IP can fail if the first port is not on a
network that is attached to an external gateway.

In order to make the command work better for users who have a server
with multiple ports, we can:

  1. Select the port corresponding to the fixed_ip_address, if one was
     specified

  2. Try to associate the floating IP with each port until one of the
     attempts succeeds, else re-raise the last exception.
     (404 ExternalGatewayForFloatingIPNotFound from neutron)

This also fixes incorrect FakeFloatingIP attributes that were being set
in the TestServerAddFloatingIPNetwork unit tests, which were causing
the tests to use None as parsed args for ip-address and
--fixed-ip-address and thus bypassing code in the
'if parsed_args.fixed_ip_address:' block.

Task: 27800
Story: 2004263

Change-Id: I11fbcebf6b00f12a030b000c84dcf1d6b5e86250
(cherry picked from commit 013c9a4f3a44cb0b81fc7affe9b933e701cb5dba)
(cherry picked from commit e09c358e7b35ac938595aad90c5c42dfe402a42a)

3 files changed, 164 insertions, 15 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 135953aa..c591161a 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -24,6 +24,7 @@ import sys
 
 from novaclient import api_versions
 from novaclient.v2 import servers
+from openstack import exceptions as sdk_exceptions
 from osc_lib.cli import parseractions
 from osc_lib.command import command
 from osc_lib import exceptions
@@ -248,13 +249,16 @@ class AddFloatingIP(network_common.NetworkAndComputeCommand):
         parser.add_argument(
             "ip_address",
             metavar="<ip-address>",
-            help=_("Floating IP address to assign to server (IP only)"),
+            help=_("Floating IP address to assign to the first available "
+                   "server port (IP only)"),
         )
         parser.add_argument(
             "--fixed-ip-address",
             metavar="<ip-address>",
             help=_(
-                "Fixed IP address to associate with this floating IP address"
+                "Fixed IP address to associate with this floating IP address. "
+                "The first server port containing the fixed IP address will "
+                "be used"
             ),
         )
         return parser
@@ -271,12 +275,45 @@ class AddFloatingIP(network_common.NetworkAndComputeCommand):
             compute_client.servers,
             parsed_args.server,
         )
-        port = list(client.ports(device_id=server.id))[0]
-        attrs['port_id'] = port.id
+        ports = list(client.ports(device_id=server.id))
+        # If the fixed IP address was specified, we need to find the
+        # corresponding port.
         if parsed_args.fixed_ip_address:
-            attrs['fixed_ip_address'] = parsed_args.fixed_ip_address
-
-        client.update_ip(obj, **attrs)
+            fip_address = parsed_args.fixed_ip_address
+            attrs['fixed_ip_address'] = fip_address
+            for port in ports:
+                for ip in port.fixed_ips:
+                    if ip['ip_address'] == fip_address:
+                        attrs['port_id'] = port.id
+                        break
+                else:
+                    continue
+                break
+            if 'port_id' not in attrs:
+                msg = _('No port found for fixed IP address %s')
+                raise exceptions.CommandError(msg % fip_address)
+            client.update_ip(obj, **attrs)
+        else:
+            # It's possible that one or more ports are not connected to a
+            # router and thus could fail association with a floating IP.
+            # Try each port until one succeeds. If none succeed, re-raise the
+            # last exception.
+            error = None
+            for port in ports:
+                attrs['port_id'] = port.id
+                try:
+                    client.update_ip(obj, **attrs)
+                except sdk_exceptions.NotFoundException as exp:
+                    # 404 ExternalGatewayForFloatingIPNotFound from neutron
+                    LOG.info('Skipped port %s because it is not attached to '
+                             'an external gateway', port.id)
+                    error = exp
+                    continue
+                else:
+                    error = None
+                    break
+            if error:
+                raise error
 
     def take_action_compute(self, client, parsed_args):
         client.api.floating_ip_add(
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index d85c0799..172072f6 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -19,6 +19,7 @@ import getpass
 import mock
 from mock import call
 from novaclient import api_versions
+from openstack import exceptions as sdk_exceptions
 from osc_lib import exceptions
 from osc_lib import utils as common_utils
 from oslo_utils import timeutils
@@ -222,11 +223,11 @@ class TestServerAddFloatingIPNetwork(
         self.network.ports = mock.Mock(return_value=[_port])
         arglist = [
             _server.id,
-            _floating_ip['ip'],
+            _floating_ip['floating_ip_address'],
         ]
         verifylist = [
             ('server', _server.id),
-            ('ip_address', _floating_ip['ip']),
+            ('ip_address', _floating_ip['floating_ip_address']),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
@@ -237,7 +238,7 @@ class TestServerAddFloatingIPNetwork(
         }
 
         self.network.find_ip.assert_called_once_with(
-            _floating_ip['ip'],
+            _floating_ip['floating_ip_address'],
             ignore_missing=False,
         )
         self.network.ports.assert_called_once_with(
@@ -248,6 +249,64 @@ class TestServerAddFloatingIPNetwork(
             **attrs
         )
 
+    def test_server_add_floating_ip_default_no_external_gateway(self,
+                                                                success=False):
+        _server = compute_fakes.FakeServer.create_one_server()
+        self.servers_mock.get.return_value = _server
+        _port = network_fakes.FakePort.create_one_port()
+        _floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
+        self.network.find_ip = mock.Mock(return_value=_floating_ip)
+        return_value = [_port]
+        # In the success case, we'll have two ports, where the first port is
+        # not attached to an external gateway but the second port is.
+        if success:
+            return_value.append(_port)
+        self.network.ports = mock.Mock(return_value=return_value)
+        side_effect = [sdk_exceptions.NotFoundException()]
+        if success:
+            side_effect.append(None)
+        self.network.update_ip = mock.Mock(side_effect=side_effect)
+        arglist = [
+            _server.id,
+            _floating_ip['floating_ip_address'],
+        ]
+        verifylist = [
+            ('server', _server.id),
+            ('ip_address', _floating_ip['floating_ip_address']),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        if success:
+            self.cmd.take_action(parsed_args)
+        else:
+            self.assertRaises(sdk_exceptions.NotFoundException,
+                              self.cmd.take_action, parsed_args)
+
+        attrs = {
+            'port_id': _port.id,
+        }
+
+        self.network.find_ip.assert_called_once_with(
+            _floating_ip['floating_ip_address'],
+            ignore_missing=False,
+        )
+        self.network.ports.assert_called_once_with(
+            device_id=_server.id,
+        )
+        if success:
+            self.assertEqual(2, self.network.update_ip.call_count)
+            calls = [mock.call(_floating_ip, **attrs)] * 2
+            self.network.update_ip.assert_has_calls(calls)
+        else:
+            self.network.update_ip.assert_called_once_with(
+                _floating_ip,
+                **attrs
+            )
+
+    def test_server_add_floating_ip_default_one_external_gateway(self):
+        self.test_server_add_floating_ip_default_no_external_gateway(
+            success=True)
+
     def test_server_add_floating_ip_fixed(self):
         _server = compute_fakes.FakeServer.create_one_server()
         self.servers_mock.get.return_value = _server
@@ -255,26 +314,31 @@ class TestServerAddFloatingIPNetwork(
         _floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
         self.network.find_ip = mock.Mock(return_value=_floating_ip)
         self.network.ports = mock.Mock(return_value=[_port])
+        # The user has specified a fixed ip that matches one of the ports
+        # already attached to the instance.
         arglist = [
-            '--fixed-ip-address', _floating_ip['fixed_ip'],
+            '--fixed-ip-address', _port.fixed_ips[0]['ip_address'],
             _server.id,
-            _floating_ip['ip'],
+            _floating_ip['floating_ip_address'],
         ]
         verifylist = [
-            ('fixed_ip_address', _floating_ip['fixed_ip']),
+            ('fixed_ip_address', _port.fixed_ips[0]['ip_address']),
             ('server', _server.id),
-            ('ip_address', _floating_ip['ip']),
+            ('ip_address', _floating_ip['floating_ip_address']),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         self.cmd.take_action(parsed_args)
 
+        # We expect the update_ip call to specify a new fixed_ip_address which
+        # will overwrite the floating ip's existing fixed_ip_address.
         attrs = {
             'port_id': _port.id,
+            'fixed_ip_address': _port.fixed_ips[0]['ip_address'],
         }
 
         self.network.find_ip.assert_called_once_with(
-            _floating_ip['ip'],
+            _floating_ip['floating_ip_address'],
             ignore_missing=False,
         )
         self.network.ports.assert_called_once_with(
@@ -285,6 +349,40 @@ class TestServerAddFloatingIPNetwork(
             **attrs
         )
 
+    def test_server_add_floating_ip_fixed_no_port_found(self):
+        _server = compute_fakes.FakeServer.create_one_server()
+        self.servers_mock.get.return_value = _server
+        _port = network_fakes.FakePort.create_one_port()
+        _floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
+        self.network.find_ip = mock.Mock(return_value=_floating_ip)
+        self.network.ports = mock.Mock(return_value=[_port])
+        # The user has specified a fixed ip that does not match any of the
+        # ports already attached to the instance.
+        nonexistent_ip = '10.0.0.9'
+        arglist = [
+            '--fixed-ip-address', nonexistent_ip,
+            _server.id,
+            _floating_ip['floating_ip_address'],
+        ]
+        verifylist = [
+            ('fixed_ip_address', nonexistent_ip),
+            ('server', _server.id),
+            ('ip_address', _floating_ip['floating_ip_address']),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)
+
+        self.network.find_ip.assert_called_once_with(
+            _floating_ip['floating_ip_address'],
+            ignore_missing=False,
+        )
+        self.network.ports.assert_called_once_with(
+            device_id=_server.id,
+        )
+        self.network.update_ip.assert_not_called()
+
 
 class TestServerAddPort(TestServer):
 
diff --git a/releasenotes/notes/floating-ip-multi-port-9779e88f590cae23.yaml b/releasenotes/notes/floating-ip-multi-port-9779e88f590cae23.yaml
new file mode 100644
index 00000000..738190f9
--- /dev/null
+++ b/releasenotes/notes/floating-ip-multi-port-9779e88f590cae23.yaml
@@ -0,0 +1,14 @@
+---
+fixes:
+  - |
+    The ``openstack server add floating ip`` command has been fixed to handle
+    servers with multiple ports attached. Previously, the command was using
+    the first port in the port list when attempting to associate the floating
+    ip. This could fail if the server had multiple ports and the first port
+    in the list was not attached to an external gateway. Another way it could
+    fail is if the ``--fixed-ip-address`` option was passed and the first port
+    did not have the specified fixed IP address attached to it.
+    Now, the ``openstack server add floating ip`` command will find the port
+    attached to the specified ``--fixed-ip-address``, if provided, else it will
+    try multiple ports until one is found attached to an external gateway. If
+    a suitable port is not found in the port list, an error will be returned.