From c7d582379ad6b22c6dd8b7334b34a51ec59b69d4 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 10 Dec 2020 15:06:03 +0000 Subject: [PATCH] compute: Improve 'server create --nic' option parsing Simplify the parsing of this option by making use of a custom action. Change-Id: I670ff5109522d533ef4e62a79116e49a35c4e8fa Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 221 +++++++++++------- .../tests/unit/compute/v2/test_server.py | 104 ++++----- 2 files changed, 185 insertions(+), 140 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 7e17d0d043..1277c34130 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -98,16 +98,6 @@ def _get_ip_address(addresses, address_type, ip_address_family): ) -def _prefix_checked_value(prefix): - def func(value): - if ',' in value or '=' in value: - msg = _("Invalid argument %s, " - "characters ',' and '=' are not allowed") % value - raise argparse.ArgumentTypeError(msg) - return prefix + value - return func - - def _prep_server_detail(compute_client, image_client, server, refresh=True): """Prepare the detailed server dict for printing @@ -611,6 +601,81 @@ class AddServerVolume(command.Command): ) +# TODO(stephenfin): Replace with 'MultiKeyValueAction' when we no longer +# support '--nic=auto' and '--nic=none' +class NICAction(argparse.Action): + + def __init__( + self, + option_strings, + dest, + nargs=None, + const=None, + default=None, + type=None, + choices=None, + required=False, + help=None, + metavar=None, + key=None, + ): + self.key = key + super().__init__( + option_strings=option_strings, dest=dest, nargs=nargs, const=const, + default=default, type=type, choices=choices, required=required, + help=help, metavar=metavar, + ) + + def __call__(self, parser, namespace, values, option_string=None): + # Make sure we have an empty dict rather than None + if getattr(namespace, self.dest, None) is None: + setattr(namespace, self.dest, []) + + # Handle the special auto/none cases + if values in ('auto', 'none'): + getattr(namespace, self.dest).append(values) + return + + if self.key: + if ',' in values or '=' in values: + msg = _( + "Invalid argument %s; characters ',' and '=' are not " + "allowed" + ) + raise argparse.ArgumentTypeError(msg % values) + + values = '='.join([self.key, values]) + + info = { + 'net-id': '', + 'port-id': '', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', + } + + for kv_str in values.split(','): + k, sep, v = kv_str.partition("=") + + if k not in info or not v: + msg = _( + "Invalid argument %s; argument must be of form " + "'net-id=net-uuid,v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr," + "port-id=port-uuid'" + ) + raise argparse.ArgumentTypeError(msg % values) + + info[k] = v + + if info['net-id'] and info['port-id']: + msg = _( + 'Invalid argument %s; either network or port should be ' + 'specified but not both' + ) + raise argparse.ArgumentTypeError(msg % values) + + getattr(namespace, self.dest).append(info) + + class CreateServer(command.ShowOne): _description = _("Create a new server") @@ -701,9 +766,10 @@ class CreateServer(command.ShowOne): parser.add_argument( '--network', metavar="", - action='append', - dest='nic', - type=_prefix_checked_value('net-id='), + dest='nics', + default=[], + action=NICAction, + key='net-id', # NOTE(RuiChen): Add '\n' to the end of line to improve formatting; # see cliff's _SmartHelpFormatter for more details. help=_( @@ -719,9 +785,10 @@ class CreateServer(command.ShowOne): parser.add_argument( '--port', metavar="", - action='append', - dest='nic', - type=_prefix_checked_value('port-id='), + dest='nics', + default=[], + action=NICAction, + key='port-id', help=_( "Create a NIC on the server and connect it to port. " "Specify option multiple times to create multiple NICs. " @@ -733,21 +800,28 @@ class CreateServer(command.ShowOne): ) parser.add_argument( '--nic', - metavar="", - action='append', + metavar="", + action=NICAction, + dest='nics', + default=[], help=_( - "Create a NIC on the server. " - "Specify option multiple times to create multiple NICs. " - "Either net-id or port-id must be provided, but not both. " - "net-id: attach NIC to network with this UUID, " - "port-id: attach NIC to port with this UUID, " - "v4-fixed-ip: IPv4 fixed address for NIC (optional), " - "v6-fixed-ip: IPv6 fixed address for NIC (optional), " - "none: (v2.37+) no network is attached, " + "Create a NIC on the server.\n" + "NIC in the format:\n" + "net-id=: attach NIC to network with this UUID,\n" + "port-id=: attach NIC to port with this UUID,\n" + "v4-fixed-ip=: IPv4 fixed address for NIC (optional)," + "\n" + "v6-fixed-ip=: IPv6 fixed address for NIC (optional)," + "\n" + "none: (v2.37+) no network is attached,\n" "auto: (v2.37+) the compute service will automatically " - "allocate a network. Specifying a --nic of auto or none " - "cannot be used with any other --nic value." + "allocate a network.\n" + "\n" + "Specify option multiple times to create multiple NICs.\n" + "Specifying a --nic of auto or none cannot be used with any " + "other --nic value.\n" + "Either net-id or port-id must be provided, but not both." ), ) parser.add_argument( @@ -1103,84 +1177,55 @@ class CreateServer(command.ShowOne): raise exceptions.CommandError(msg) block_device_mapping_v2.append(mapping) - nics = [] - auto_or_none = False - if parsed_args.nic is None: - parsed_args.nic = [] - for nic_str in parsed_args.nic: - # Handle the special auto/none cases - if nic_str in ('auto', 'none'): - auto_or_none = True - nics.append(nic_str) - else: - nic_info = { - 'net-id': '', - 'v4-fixed-ip': '', - 'v6-fixed-ip': '', - 'port-id': '', - } - for kv_str in nic_str.split(","): - k, sep, v = kv_str.partition("=") - if k in nic_info and v: - nic_info[k] = v - else: - msg = _( - "Invalid nic argument '%s'. Nic arguments " - "must be of the form --nic ." - ) - raise exceptions.CommandError(msg % k) + nics = parsed_args.nics - if bool(nic_info["net-id"]) == bool(nic_info["port-id"]): - msg = _( - 'Either network or port should be specified ' - 'but not both' - ) - raise exceptions.CommandError(msg) + if 'auto' in nics or 'none' in nics: + if len(nics) > 1: + msg = _( + 'Specifying a --nic of auto or none cannot ' + 'be used with any other --nic, --network ' + 'or --port value.' + ) + raise exceptions.CommandError(msg) + nics = nics[0] + else: + for nic in nics: if self.app.client_manager.is_network_endpoint_enabled(): network_client = self.app.client_manager.network - if nic_info["net-id"]: + + if nic['net-id']: net = network_client.find_network( - nic_info["net-id"], ignore_missing=False) - nic_info["net-id"] = net.id - if nic_info["port-id"]: + nic['net-id'], ignore_missing=False, + ) + nic['net-id'] = net.id + + if nic['port-id']: port = network_client.find_port( - nic_info["port-id"], ignore_missing=False) - nic_info["port-id"] = port.id + nic['port-id'], ignore_missing=False, + ) + nic['port-id'] = port.id else: - if nic_info["net-id"]: - nic_info["net-id"] = compute_client.api.network_find( - nic_info["net-id"] + if nic['net-id']: + nic['net-id'] = compute_client.api.network_find( + nic['net-id'], )['id'] - if nic_info["port-id"]: + + if nic['port-id']: msg = _( "Can't create server with port specified " "since network endpoint not enabled" ) raise exceptions.CommandError(msg) - nics.append(nic_info) - - if nics: - if auto_or_none: - if len(nics) > 1: - msg = _( - 'Specifying a --nic of auto or none cannot ' - 'be used with any other --nic, --network ' - 'or --port value.' - ) - raise exceptions.CommandError(msg) - nics = nics[0] - else: + if not nics: # Compute API version >= 2.37 requires a value, so default to # 'auto' to maintain legacy behavior if a nic wasn't specified. if compute_client.api_version >= api_versions.APIVersion('2.37'): nics = 'auto' else: - # Default to empty list if nothing was specified, let nova - # side to decide the default behavior. + # Default to empty list if nothing was specified and let nova + # decide the default behavior. nics = [] # Check security group exist and convert ID to name diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 9a01758c8c..e8db7f596f 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1311,8 +1311,28 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), - ('nic', ['net-id=net1', 'net-id=net1,v4-fixed-ip=10.0.0.2', - 'port-id=port1', 'net-id=net1', 'port-id=port2']), + ('nics', [ + { + 'net-id': 'net1', 'port-id': '', + 'v4-fixed-ip': '', 'v6-fixed-ip': '', + }, + { + 'net-id': 'net1', 'port-id': '', + 'v4-fixed-ip': '10.0.0.2', 'v6-fixed-ip': '', + }, + { + 'net-id': '', 'port-id': 'port1', + 'v4-fixed-ip': '', 'v6-fixed-ip': '', + }, + { + 'net-id': 'net1', 'port-id': '', + 'v4-fixed-ip': '', 'v6-fixed-ip': '', + }, + { + 'net-id': '', 'port-id': 'port2', + 'v4-fixed-ip': '', 'v6-fixed-ip': '', + }, + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -1413,7 +1433,7 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), - ('nic', ['auto']), + ('nics', ['auto']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -1509,7 +1529,7 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), - ('nic', ['none']), + ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -1557,7 +1577,14 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), - ('nic', ['none', 'auto', 'port-id=port1']), + ('nics', [ + 'none', + 'auto', + { + 'net-id': '', 'port-id': 'port1', + 'v4-fixed-ip': '', 'v6-fixed-ip': '', + }, + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -1587,17 +1614,10 @@ class TestServerCreate(TestServer): '--nic', 'abcdefgh', self.new_server.name, ] - verifylist = [ - ('image', 'image1'), - ('flavor', 'flavor1'), - ('nic', ['abcdefgh']), - ('config_drive', False), - ('server_name', self.new_server.name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.assertRaises(exceptions.CommandError, - self.cmd.take_action, parsed_args) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) self.assertNotCalled(self.servers_mock.create) def test_server_create_with_invalid_network_key(self): @@ -1607,17 +1627,10 @@ class TestServerCreate(TestServer): '--nic', 'abcdefgh=12324', self.new_server.name, ] - verifylist = [ - ('image', 'image1'), - ('flavor', 'flavor1'), - ('nic', ['abcdefgh=12324']), - ('config_drive', False), - ('server_name', self.new_server.name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.assertRaises(exceptions.CommandError, - self.cmd.take_action, parsed_args) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) self.assertNotCalled(self.servers_mock.create) def test_server_create_with_empty_network_key_value(self): @@ -1627,17 +1640,10 @@ class TestServerCreate(TestServer): '--nic', 'net-id=', self.new_server.name, ] - verifylist = [ - ('image', 'image1'), - ('flavor', 'flavor1'), - ('nic', ['net-id=']), - ('config_drive', False), - ('server_name', self.new_server.name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.assertRaises(exceptions.CommandError, - self.cmd.take_action, parsed_args) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) self.assertNotCalled(self.servers_mock.create) def test_server_create_with_only_network_key(self): @@ -1647,17 +1653,11 @@ class TestServerCreate(TestServer): '--nic', 'net-id', self.new_server.name, ] - verifylist = [ - ('image', 'image1'), - ('flavor', 'flavor1'), - ('nic', ['net-id']), - ('config_drive', False), - ('server_name', self.new_server.name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) - self.assertRaises(exceptions.CommandError, - self.cmd.take_action, parsed_args) self.assertNotCalled(self.servers_mock.create) @mock.patch.object(common_utils, 'wait_for_status', return_value=True) @@ -2230,7 +2230,7 @@ class TestServerCreate(TestServer): verifylist = [ ('image_properties', {'hypervisor_type': 'qemu'}), ('flavor', 'flavor1'), - ('nic', ['none']), + ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2286,7 +2286,7 @@ class TestServerCreate(TestServer): ('image_properties', {'hypervisor_type': 'qemu', 'hw_disk_bus': 'ide'}), ('flavor', 'flavor1'), - ('nic', ['none']), + ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2342,7 +2342,7 @@ class TestServerCreate(TestServer): ('image_properties', {'hypervisor_type': 'qemu', 'hw_disk_bus': 'virtio'}), ('flavor', 'flavor1'), - ('nic', ['none']), + ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2374,7 +2374,7 @@ class TestServerCreate(TestServer): ('image_properties', {'owner_specified.openstack.object': 'image/cirros'}), ('flavor', 'flavor1'), - ('nic', ['none']), + ('nics', ['none']), ('server_name', self.new_server.name), ] # create a image_info as the side_effect of the fake image_list()