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

Author: rtheis

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 @@ def take_action(self, parsed_args):
         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)

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 @@ def create_one_router(attrs={}):
             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 @@ def create_one_router(attrs={}):
         return router
 
     @staticmethod
-    def create_routers(attrs={}, count=2):
+    def create_routers(attrs=None, count=2):
         """Create multiple fake routers.
 
         :param Dictionary attrs:

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 @@ def test_remove_subnet_required_options(self):
 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 @@ def test_set_route(self):
         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,
     )
 

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