Skip to content

Commit 010da92

Browse files
melwittmelwitt
committed
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 (cherry picked from commit 013c9a4) (cherry picked from commit e09c358)
1 parent 28797fe commit 010da92

3 files changed

Lines changed: 164 additions & 15 deletions

File tree

‎openstackclient/compute/v2/server.py‎

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
from novaclient import api_versions
2626
from novaclient.v2 import servers
27+
from openstack import exceptions as sdk_exceptions
2728
from osc_lib.cli import parseractions
2829
from osc_lib.command import command
2930
from osc_lib import exceptions
@@ -248,13 +249,16 @@ def update_parser_common(self, parser):
248249
parser.add_argument(
249250
"ip_address",
250251
metavar="<ip-address>",
251-
help=_("Floating IP address to assign to server (IP only)"),
252+
help=_("Floating IP address to assign to the first available "
253+
"server port (IP only)"),
252254
)
253255
parser.add_argument(
254256
"--fixed-ip-address",
255257
metavar="<ip-address>",
256258
help=_(
257-
"Fixed IP address to associate with this floating IP address"
259+
"Fixed IP address to associate with this floating IP address. "
260+
"The first server port containing the fixed IP address will "
261+
"be used"
258262
),
259263
)
260264
return parser
@@ -271,12 +275,45 @@ def take_action_network(self, client, parsed_args):
271275
compute_client.servers,
272276
parsed_args.server,
273277
)
274-
port = list(client.ports(device_id=server.id))[0]
275-
attrs['port_id'] = port.id
278+
ports = list(client.ports(device_id=server.id))
279+
# If the fixed IP address was specified, we need to find the
280+
# corresponding port.
276281
if parsed_args.fixed_ip_address:
277-
attrs['fixed_ip_address'] = parsed_args.fixed_ip_address
278-
279-
client.update_ip(obj, **attrs)
282+
fip_address = parsed_args.fixed_ip_address
283+
attrs['fixed_ip_address'] = fip_address
284+
for port in ports:
285+
for ip in port.fixed_ips:
286+
if ip['ip_address'] == fip_address:
287+
attrs['port_id'] = port.id
288+
break
289+
else:
290+
continue
291+
break
292+
if 'port_id' not in attrs:
293+
msg = _('No port found for fixed IP address %s')
294+
raise exceptions.CommandError(msg % fip_address)
295+
client.update_ip(obj, **attrs)
296+
else:
297+
# It's possible that one or more ports are not connected to a
298+
# router and thus could fail association with a floating IP.
299+
# Try each port until one succeeds. If none succeed, re-raise the
300+
# last exception.
301+
error = None
302+
for port in ports:
303+
attrs['port_id'] = port.id
304+
try:
305+
client.update_ip(obj, **attrs)
306+
except sdk_exceptions.NotFoundException as exp:
307+
# 404 ExternalGatewayForFloatingIPNotFound from neutron
308+
LOG.info('Skipped port %s because it is not attached to '
309+
'an external gateway', port.id)
310+
error = exp
311+
continue
312+
else:
313+
error = None
314+
break
315+
if error:
316+
raise error
280317

281318
def take_action_compute(self, client, parsed_args):
282319
client.api.floating_ip_add(

‎openstackclient/tests/unit/compute/v2/test_server.py‎

Lines changed: 106 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import mock
2020
from mock import call
2121
from novaclient import api_versions
22+
from openstack import exceptions as sdk_exceptions
2223
from osc_lib import exceptions
2324
from osc_lib import utils as common_utils
2425
from oslo_utils import timeutils
@@ -222,11 +223,11 @@ def test_server_add_floating_ip_default(self):
222223
self.network.ports = mock.Mock(return_value=[_port])
223224
arglist = [
224225
_server.id,
225-
_floating_ip['ip'],
226+
_floating_ip['floating_ip_address'],
226227
]
227228
verifylist = [
228229
('server', _server.id),
229-
('ip_address', _floating_ip['ip']),
230+
('ip_address', _floating_ip['floating_ip_address']),
230231
]
231232
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
232233

@@ -237,7 +238,7 @@ def test_server_add_floating_ip_default(self):
237238
}
238239

239240
self.network.find_ip.assert_called_once_with(
240-
_floating_ip['ip'],
241+
_floating_ip['floating_ip_address'],
241242
ignore_missing=False,
242243
)
243244
self.network.ports.assert_called_once_with(
@@ -248,33 +249,96 @@ def test_server_add_floating_ip_default(self):
248249
**attrs
249250
)
250251

252+
def test_server_add_floating_ip_default_no_external_gateway(self,
253+
success=False):
254+
_server = compute_fakes.FakeServer.create_one_server()
255+
self.servers_mock.get.return_value = _server
256+
_port = network_fakes.FakePort.create_one_port()
257+
_floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
258+
self.network.find_ip = mock.Mock(return_value=_floating_ip)
259+
return_value = [_port]
260+
# In the success case, we'll have two ports, where the first port is
261+
# not attached to an external gateway but the second port is.
262+
if success:
263+
return_value.append(_port)
264+
self.network.ports = mock.Mock(return_value=return_value)
265+
side_effect = [sdk_exceptions.NotFoundException()]
266+
if success:
267+
side_effect.append(None)
268+
self.network.update_ip = mock.Mock(side_effect=side_effect)
269+
arglist = [
270+
_server.id,
271+
_floating_ip['floating_ip_address'],
272+
]
273+
verifylist = [
274+
('server', _server.id),
275+
('ip_address', _floating_ip['floating_ip_address']),
276+
]
277+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
278+
279+
if success:
280+
self.cmd.take_action(parsed_args)
281+
else:
282+
self.assertRaises(sdk_exceptions.NotFoundException,
283+
self.cmd.take_action, parsed_args)
284+
285+
attrs = {
286+
'port_id': _port.id,
287+
}
288+
289+
self.network.find_ip.assert_called_once_with(
290+
_floating_ip['floating_ip_address'],
291+
ignore_missing=False,
292+
)
293+
self.network.ports.assert_called_once_with(
294+
device_id=_server.id,
295+
)
296+
if success:
297+
self.assertEqual(2, self.network.update_ip.call_count)
298+
calls = [mock.call(_floating_ip, **attrs)] * 2
299+
self.network.update_ip.assert_has_calls(calls)
300+
else:
301+
self.network.update_ip.assert_called_once_with(
302+
_floating_ip,
303+
**attrs
304+
)
305+
306+
def test_server_add_floating_ip_default_one_external_gateway(self):
307+
self.test_server_add_floating_ip_default_no_external_gateway(
308+
success=True)
309+
251310
def test_server_add_floating_ip_fixed(self):
252311
_server = compute_fakes.FakeServer.create_one_server()
253312
self.servers_mock.get.return_value = _server
254313
_port = network_fakes.FakePort.create_one_port()
255314
_floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
256315
self.network.find_ip = mock.Mock(return_value=_floating_ip)
257316
self.network.ports = mock.Mock(return_value=[_port])
317+
# The user has specified a fixed ip that matches one of the ports
318+
# already attached to the instance.
258319
arglist = [
259-
'--fixed-ip-address', _floating_ip['fixed_ip'],
320+
'--fixed-ip-address', _port.fixed_ips[0]['ip_address'],
260321
_server.id,
261-
_floating_ip['ip'],
322+
_floating_ip['floating_ip_address'],
262323
]
263324
verifylist = [
264-
('fixed_ip_address', _floating_ip['fixed_ip']),
325+
('fixed_ip_address', _port.fixed_ips[0]['ip_address']),
265326
('server', _server.id),
266-
('ip_address', _floating_ip['ip']),
327+
('ip_address', _floating_ip['floating_ip_address']),
267328
]
268329
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
269330

270331
self.cmd.take_action(parsed_args)
271332

333+
# We expect the update_ip call to specify a new fixed_ip_address which
334+
# will overwrite the floating ip's existing fixed_ip_address.
272335
attrs = {
273336
'port_id': _port.id,
337+
'fixed_ip_address': _port.fixed_ips[0]['ip_address'],
274338
}
275339

276340
self.network.find_ip.assert_called_once_with(
277-
_floating_ip['ip'],
341+
_floating_ip['floating_ip_address'],
278342
ignore_missing=False,
279343
)
280344
self.network.ports.assert_called_once_with(
@@ -285,6 +349,40 @@ def test_server_add_floating_ip_fixed(self):
285349
**attrs
286350
)
287351

352+
def test_server_add_floating_ip_fixed_no_port_found(self):
353+
_server = compute_fakes.FakeServer.create_one_server()
354+
self.servers_mock.get.return_value = _server
355+
_port = network_fakes.FakePort.create_one_port()
356+
_floating_ip = network_fakes.FakeFloatingIP.create_one_floating_ip()
357+
self.network.find_ip = mock.Mock(return_value=_floating_ip)
358+
self.network.ports = mock.Mock(return_value=[_port])
359+
# The user has specified a fixed ip that does not match any of the
360+
# ports already attached to the instance.
361+
nonexistent_ip = '10.0.0.9'
362+
arglist = [
363+
'--fixed-ip-address', nonexistent_ip,
364+
_server.id,
365+
_floating_ip['floating_ip_address'],
366+
]
367+
verifylist = [
368+
('fixed_ip_address', nonexistent_ip),
369+
('server', _server.id),
370+
('ip_address', _floating_ip['floating_ip_address']),
371+
]
372+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
373+
374+
self.assertRaises(exceptions.CommandError, self.cmd.take_action,
375+
parsed_args)
376+
377+
self.network.find_ip.assert_called_once_with(
378+
_floating_ip['floating_ip_address'],
379+
ignore_missing=False,
380+
)
381+
self.network.ports.assert_called_once_with(
382+
device_id=_server.id,
383+
)
384+
self.network.update_ip.assert_not_called()
385+
288386

289387
class TestServerAddPort(TestServer):
290388

‎releasenotes/notes/floating-ip-multi-port-9779e88f590cae23.yaml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
fixes:
3+
- |
4+
The ``openstack server add floating ip`` command has been fixed to handle
5+
servers with multiple ports attached. Previously, the command was using
6+
the first port in the port list when attempting to associate the floating
7+
ip. This could fail if the server had multiple ports and the first port
8+
in the list was not attached to an external gateway. Another way it could
9+
fail is if the ``--fixed-ip-address`` option was passed and the first port
10+
did not have the specified fixed IP address attached to it.
11+
Now, the ``openstack server add floating ip`` command will find the port
12+
attached to the specified ``--fixed-ip-address``, if provided, else it will
13+
try multiple ports until one is found attached to an external gateway. If
14+
a suitable port is not found in the port list, an error will be returned.

0 commit comments

Comments
 (0)