From 013c9a4f3a44cb0b81fc7affe9b933e701cb5dba Mon Sep 17 00:00:00 2001 From: melanie witt Date: Fri, 2 Nov 2018 22:27:55 +0000 Subject: [PATCH] Handle multiple ports in AddFloatingIP 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 --- openstackclient/compute/v2/server.py | 51 ++++++-- .../tests/unit/compute/v2/test_server.py | 114 ++++++++++++++++-- ...oating-ip-multi-port-9779e88f590cae23.yaml | 14 +++ 3 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/floating-ip-multi-port-9779e88f590cae23.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 4b0aedd7a..9051c8d25 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -23,6 +23,7 @@ import os 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 @@ -251,13 +252,16 @@ class AddFloatingIP(network_common.NetworkAndComputeCommand): parser.add_argument( "ip_address", metavar="", - 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="", 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 @@ -274,12 +278,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 928794fba..afa2e80a4 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 000000000..738190f90 --- /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.