From bc6ca87a6a01ecbda926a78d07cadbd4e3ab97e0 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Mon, 21 May 2018 16:44:59 -0400 Subject: [PATCH] Validate transport_url in nova-manage cell_v2 commands In the three commands that take a --transport-url option, or reads it from config, this will validate the tranport URL is valid by calling the parsing code in oslo.messaging and fail if the URL does not parse correctly. Change-Id: If60cdf697cab2f035cd22830303f5ecaba0f3969 Closes-Bug: #1770341 --- doc/source/cli/nova-manage.rst | 9 +++++---- nova/cmd/manage.py | 13 ++++++++++--- nova/tests/unit/test_nova_manage.py | 22 +++++++++++++--------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/doc/source/cli/nova-manage.rst b/doc/source/cli/nova-manage.rst index e22fc95ceb6..8637b59993d 100644 --- a/doc/source/cli/nova-manage.rst +++ b/doc/source/cli/nova-manage.rst @@ -142,8 +142,8 @@ Nova Cells v2 specified, it will use the one defined by ``[DEFAULT]/transport_url`` in the configuration file. Returns 0 if setup is completed (or has already been done), 1 if no hosts are reporting (and cannot be - mapped), 1 if the transport url is missing, and 2 if run in a cells v1 - environment. + mapped), 1 if the transport url is missing or invalid, and 2 if run in a + cells v1 environment. ``nova-manage cell_v2 map_cell0 [--database_connection ]`` Create a cell mapping to the database connection for the cell0 database. @@ -177,7 +177,7 @@ Nova Cells v2 use the one defined by ``[DEFAULT]/transport_url`` in the configuration file. This command is idempotent (can be run multiple times), and the verbose option will print out the resulting cell mapping uuid. Returns 0 - on successful completion, and 1 if the transport url is missing. + on successful completion, and 1 if the transport url is missing or invalid. ``nova-manage cell_v2 verify_instance --uuid [--quiet]`` Verify instance mapping to a cell. This command is useful to determine if @@ -204,7 +204,8 @@ Nova Cells v2 explained below: * Returns 0 if the cell mapping was successfully created. - * Returns 1 if the transport url or database connection was missing. + * Returns 1 if the transport url or database connection was missing + or invalid. * Returns 2 if another cell is already using that transport url and/or database connection combination. diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 8dfc4cf2081..087d7dbfaea 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -1022,6 +1022,14 @@ def _validate_transport_url(self, transport_url): if not transport_url: print('Must specify --transport-url if [DEFAULT]/transport_url ' 'is not set in the configuration file.') + return None + + try: + messaging.TransportURL.parse(conf=CONF, url=transport_url) + except (messaging.InvalidTransportURL, ValueError) as e: + print(_('Invalid transport URL: %s') % six.text_type(e)) + return None + return transport_url def _non_unique_transport_url_database_connection_checker(self, ctxt, @@ -1436,11 +1444,10 @@ def status_fn(msg): def create_cell(self, name=None, database_connection=None, transport_url=None, verbose=False, disabled=False): ctxt = context.get_context() - transport_url = transport_url or CONF.transport_url + transport_url = self._validate_transport_url(transport_url) if not transport_url: - print(_('Must specify --transport-url if [DEFAULT]/transport_url ' - 'is not set in the configuration file.')) return 1 + database_connection = database_connection or CONF.database.connection if not database_connection: print(_('Must specify --database_connection ' diff --git a/nova/tests/unit/test_nova_manage.py b/nova/tests/unit/test_nova_manage.py index 91be05dc813..f95f7e5bbea 100644 --- a/nova/tests/unit/test_nova_manage.py +++ b/nova/tests/unit/test_nova_manage.py @@ -1728,13 +1728,13 @@ def test_discover_hosts_by_service(self, mock_discover_hosts): True) def test_validate_transport_url_in_conf(self): - from_conf = 'fake://user:pass@host:port/' + from_conf = 'fake://user:pass@host:5672/' self.flags(transport_url=from_conf) self.assertEqual(from_conf, self.commands._validate_transport_url(None)) def test_validate_transport_url_on_command_line(self): - from_cli = 'fake://user:pass@host:port/' + from_cli = 'fake://user:pass@host:5672/' self.assertEqual(from_cli, self.commands._validate_transport_url(from_cli)) @@ -1742,11 +1742,15 @@ def test_validate_transport_url_missing(self): self.assertIsNone(self.commands._validate_transport_url(None)) def test_validate_transport_url_favors_command_line(self): - self.flags(transport_url='fake://user:pass@host:port/') - from_cli = 'fake://otheruser:otherpass@otherhost:otherport' + self.flags(transport_url='fake://user:pass@host:5672/') + from_cli = 'fake://otheruser:otherpass@otherhost:5673' self.assertEqual(from_cli, self.commands._validate_transport_url(from_cli)) + def test_validate_transport_url_invalid_url(self): + self.assertIsNone(self.commands._validate_transport_url('not-a-url')) + self.assertIn('Invalid transport URL', self.output.getvalue()) + def test_non_unique_transport_url_database_connection_checker(self): ctxt = context.RequestContext() cell1 = objects.CellMapping(context=ctxt, uuid=uuidsentinel.cell1, @@ -1780,7 +1784,7 @@ def test_create_cell_use_params(self): ctxt = context.get_context() kwargs = dict( name='fake-name', - transport_url='fake-transport-url', + transport_url='http://fake-transport-url', database_connection='fake-db-connection') status = self.commands.create_cell(verbose=True, **kwargs) self.assertEqual(0, status) @@ -1795,7 +1799,7 @@ def test_create_cell_use_params(self): def test_create_cell_use_config_values(self): settings = dict( - transport_url='fake-conf-transport-url', + transport_url='http://fake-conf-transport-url', database_connection='fake-conf-db-connection') self.flags(connection=settings['database_connection'], group='database') @@ -1814,7 +1818,7 @@ def test_create_cell_use_config_values(self): def test_create_cell_failed_if_non_unique(self): kwargs = dict( name='fake-name', - transport_url='fake-transport-url', + transport_url='http://fake-transport-url', database_connection='fake-db-connection') status1 = self.commands.create_cell(verbose=True, **kwargs) status2 = self.commands.create_cell(verbose=True, **kwargs) @@ -1829,7 +1833,7 @@ def test_create_cell_failed_if_no_transport_url(self): def test_create_cell_failed_if_no_database_connection(self): self.flags(connection=None, group='database') - status = self.commands.create_cell(transport_url='fake-transport-url') + status = self.commands.create_cell(transport_url='http://fake-url') self.assertEqual(1, status) self.assertIn('--database_connection', self.output.getvalue()) @@ -1837,7 +1841,7 @@ def test_create_cell_pre_disabled(self): ctxt = context.get_context() kwargs = dict( name='fake-name1', - transport_url='fake-transport-url1', + transport_url='http://fake-transport-url1', database_connection='fake-db-connection1') status1 = self.commands.create_cell(verbose=True, disabled=True, **kwargs)