From a90c824e0407f931b5c45df53103b43aa564de12 Mon Sep 17 00:00:00 2001 From: Richard Theis Date: Tue, 5 Apr 2016 13:27:42 -0500 Subject: [PATCH] Fix router set --route option Fix the "--route" option on the "os router set" command. The option did not properly format the new routes to set which resulted in a "HttpException: Bad Request" error. In addition, the output for routes was fixed to improve readability and to align with the "--route" option on the "os router set" command. Change-Id: I9c514153ec201e2feae32be6dd281771e3298b9c Closes-Bug: #1564460 --- openstackclient/network/v2/router.py | 26 +++++++++++++++---- openstackclient/tests/network/v2/fakes.py | 6 +++-- .../tests/network/v2/test_router.py | 15 ++++++----- .../notes/bug-1564460-ab7ad35c02392cb4.yaml | 9 +++++++ 4 files changed, 43 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml diff --git a/openstackclient/network/v2/router.py b/openstackclient/network/v2/router.py index 56630a230..a32ab5ea7 100644 --- a/openstackclient/network/v2/router.py +++ b/openstackclient/network/v2/router.py @@ -34,11 +34,20 @@ def _format_external_gateway_info(info): return '' +def _format_routes(routes): + # Map the route keys to match --route option. + for route in routes: + if 'nexthop' in route: + route['gateway'] = route.pop('nexthop') + return utils.format_list_of_dicts(routes) + + _formatters = { 'admin_state_up': _format_admin_state, 'external_gateway_info': _format_external_gateway_info, 'availability_zones': utils.format_list, 'availability_zone_hints': utils.format_list, + 'routes': _format_routes, } @@ -67,11 +76,6 @@ def _get_attrs(client_manager, parsed_args): and parsed_args.availability_zone_hints is not None): attrs['availability_zone_hints'] = parsed_args.availability_zone_hints - if 'clear_routes' in parsed_args and parsed_args.clear_routes: - attrs['routes'] = [] - elif 'routes' in parsed_args and parsed_args.routes is not None: - attrs['routes'] = parsed_args.routes - # "router set" command doesn't support setting project. if 'project' in parsed_args and parsed_args.project is not None: identity_client = client_manager.identity @@ -393,7 +397,19 @@ class SetRouter(command.Command): client = self.app.client_manager.network obj = client.find_router(parsed_args.router, ignore_missing=False) + # Get the common attributes. attrs = _get_attrs(self.app.client_manager, parsed_args) + + # Get the route attributes. + if parsed_args.clear_routes: + attrs['routes'] = [] + elif parsed_args.routes is not None: + # Map the route keys and append to the current routes. + # The REST API will handle route validation and duplicates. + for route in parsed_args.routes: + route['nexthop'] = route.pop('gateway') + attrs['routes'] = obj.routes + parsed_args.routes + if attrs == {}: msg = "Nothing specified to be set" raise exceptions.CommandError(msg) diff --git a/openstackclient/tests/network/v2/fakes.py b/openstackclient/tests/network/v2/fakes.py index 5fd7ea3b8..73c6b09eb 100644 --- a/openstackclient/tests/network/v2/fakes.py +++ b/openstackclient/tests/network/v2/fakes.py @@ -328,7 +328,7 @@ class FakeRouter(object): """Fake one or more routers.""" @staticmethod - def create_one_router(attrs={}): + def create_one_router(attrs=None): """Create a fake router. :param Dictionary attrs: @@ -337,6 +337,8 @@ class FakeRouter(object): A FakeResource object, with id, name, admin_state_up, status, tenant_id """ + attrs = attrs or {} + # Set default attributes. router_attrs = { 'id': 'router-id-' + uuid.uuid4().hex, @@ -364,7 +366,7 @@ class FakeRouter(object): return router @staticmethod - def create_routers(attrs={}, count=2): + def create_routers(attrs=None, count=2): """Create multiple fake routers. :param Dictionary attrs: diff --git a/openstackclient/tests/network/v2/test_router.py b/openstackclient/tests/network/v2/test_router.py index e80414986..e3f32e4ae 100644 --- a/openstackclient/tests/network/v2/test_router.py +++ b/openstackclient/tests/network/v2/test_router.py @@ -138,7 +138,7 @@ class TestCreateRouter(TestRouter): new_router.id, new_router.name, new_router.tenant_id, - new_router.routes, + router._format_routes(new_router.routes), new_router.status, ) @@ -268,7 +268,7 @@ class TestListRouter(TestRouter): r = routers[i] data_long.append( data[i] + ( - r.routes, + router._format_routes(r.routes), router._format_external_gateway_info(r.external_gateway_info), osc_utils.format_list(r.availability_zones), ) @@ -399,7 +399,10 @@ class TestRemoveSubnetFromRouter(TestRouter): class TestSetRouter(TestRouter): # The router to set. - _router = network_fakes.FakeRouter.create_one_router() + _default_route = {'destination': '10.20.20.0/24', 'nexthop': '10.20.30.1'} + _router = network_fakes.FakeRouter.create_one_router( + attrs={'routes': [_default_route]} + ) def setUp(self): super(TestSetRouter, self).setUp() @@ -491,8 +494,8 @@ class TestSetRouter(TestRouter): result = self.cmd.take_action(parsed_args) attrs = { - 'routes': [{'destination': '10.20.30.0/24', - 'gateway': '10.20.30.1'}], + 'routes': self._router.routes + [{'destination': '10.20.30.0/24', + 'nexthop': '10.20.30.1'}], } self.network.update_router.assert_called_once_with( self._router, **attrs) @@ -572,7 +575,7 @@ class TestShowRouter(TestRouter): _router.id, _router.name, _router.tenant_id, - _router.routes, + router._format_routes(_router.routes), _router.status, ) diff --git a/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml b/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml new file mode 100644 index 000000000..54b9bdd7b --- /dev/null +++ b/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - Fixed the ``--route`` option on the ``router set`` command + which did not properly format the new routes to set resulting + in a ``Bad Request`` error. In addition, the ``router create``, + ``router list`` and ``router show`` command output for routes + was fixed to improve readability and to align with the + ``--route`` option on the ``router set`` command. + [Bug `1564460 `_]