From 3ab808b23d17900cc859b75895ac1c7a8f2d7bb9 Mon Sep 17 00:00:00 2001 From: Eoghan Glynn Date: Mon, 23 Jul 2012 12:25:43 +0000 Subject: Setting root passwd no longer fails silently. Fixes LP 1022961 Previously an attempt to set the root password would fail silently if for example the driver did not support this action. To avoid the user being misled that the password change had succeeded, we now report any failure by changing the MEP for set_admin_password RPC from cast to call. Change-Id: I1fd7b925e9226d30892f3a7cc4ddb938ff678a55 --- nova/compute/api.py | 5 +++-- nova/compute/manager.py | 22 ++++++++++++------- nova/compute/rpcapi.py | 2 +- nova/exception.py | 6 ++++++ nova/tests/compute/test_compute.py | 43 +++++++++++++++++++++++++++++++------- nova/tests/compute/test_rpcapi.py | 2 +- 6 files changed, 61 insertions(+), 19 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index f78e13f20..860fd1f3a 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1509,8 +1509,9 @@ class API(base.Base): instance, task_state=task_states.UPDATING_PASSWORD) - self.compute_rpcapi.set_admin_password(context, instance=instance, - new_pass=password) + self.compute_rpcapi.set_admin_password(context, + instance=instance, + new_pass=password) @wrap_check_policy def inject_file(self, context, instance, path, file_contents): diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e76c603e5..f6bf8173b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1240,7 +1240,9 @@ class ComputeManager(manager.SchedulerDependentManager): task_state=None) _msg = _('Failed to set admin password. Instance %s is not' ' running') % instance["uuid"] - raise exception.Invalid(_msg) + raise exception.InstancePasswordSetFailed( + instance=instance_uuid, + reason=_msg) else: try: self.driver.set_admin_password(instance, new_pass) @@ -1252,23 +1254,29 @@ class ComputeManager(manager.SchedulerDependentManager): except NotImplementedError: # NOTE(dprince): if the driver doesn't implement # set_admin_password we break to avoid a loop - LOG.warn(_('set_admin_password is not implemented ' - 'by this driver.'), instance=instance) + _msg = _('set_admin_password is not implemented ' + 'by this driver.') + LOG.warn(_msg, instance=instance) self._instance_update(context, instance['uuid'], task_state=None) - break + raise exception.InstancePasswordSetFailed( + instance=instance_uuid, + reason=_msg) except Exception, e: # Catch all here because this could be anything. - LOG.exception(e, instance=instance) + LOG.exception(_('set_admin_password failed: %s') % e, + instance=instance) if i == max_tries - 1: self._set_instance_error_state(context, instance['uuid']) # We create a new exception here so that we won't # potentially reveal password information to the # API caller. The real exception is logged above - _msg = _('Error setting admin password') - raise exception.NovaException(_msg) + _msg = _('error setting admin password') + raise exception.InstancePasswordSetFailed( + instance=instance_uuid, + reason=_msg) time.sleep(1) continue diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py index 153348374..c4ad69c72 100644 --- a/nova/compute/rpcapi.py +++ b/nova/compute/rpcapi.py @@ -400,7 +400,7 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy): def set_admin_password(self, ctxt, instance, new_pass): instance_p = jsonutils.to_primitive(instance) - self.cast(ctxt, self.make_msg('set_admin_password', + return self.call(ctxt, self.make_msg('set_admin_password', instance=instance_p, new_pass=new_pass), topic=_compute_topic(self.topic, ctxt, None, instance), version='1.33') diff --git a/nova/exception.py b/nova/exception.py index 323f33c29..a5a7c0f1b 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -1085,6 +1085,12 @@ class InstanceTypeCreateFailed(NovaException): message = _("Unable to create instance type") +class InstancePasswordSetFailed(NovaException): + message = _("Failed to set admin password on %(instance)s " + "because %(reason)s") + safe = True + + class SolidFireAPIException(NovaException): message = _("Bad response from SolidFire API") diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 46f06e7b3..0ccd4607f 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -628,14 +628,15 @@ class ComputeTestCase(BaseTestCase): self.stubs.Set(nova.virt.fake.FakeDriver, 'get_info', fake_driver_get_info) - self.assertRaises(exception.Invalid, + self.assertRaises(exception.InstancePasswordSetFailed, self.compute.set_admin_password, self.context, instance=instance) self.compute.terminate_instance(self.context, instance['uuid']) - def test_set_admin_password_driver_error(self): - """Ensure error is raised admin password set""" + def _do_test_set_admin_password_driver_error(self, exc, expected_vm_state, + expected_task_state): + """Ensure expected exception is raised if set_admin_password fails""" def fake_sleep(_time): pass @@ -643,7 +644,7 @@ class ComputeTestCase(BaseTestCase): self.stubs.Set(time, 'sleep', fake_sleep) def fake_driver_set_pass(self2, _instance, _pwd): - raise exception.NotAuthorized(_('Internal error')) + raise exc self.stubs.Set(nova.virt.fake.FakeDriver, 'set_admin_password', fake_driver_set_pass) @@ -659,17 +660,37 @@ class ComputeTestCase(BaseTestCase): #error raised from the driver should not reveal internal information #so a new error is raised - self.assertRaises(exception.NovaException, + self.assertRaises(exception.InstancePasswordSetFailed, self.compute.set_admin_password, self.context, instance=jsonutils.to_primitive(inst_ref)) inst_ref = db.instance_get_by_uuid(self.context, instance['uuid']) - self.assertEqual(inst_ref['vm_state'], vm_states.ERROR) - self.assertEqual(inst_ref['task_state'], task_states.UPDATING_PASSWORD) + self.assertEqual(inst_ref['vm_state'], expected_vm_state) + self.assertEqual(inst_ref['task_state'], expected_task_state) self.compute.terminate_instance(self.context, inst_ref['uuid']) + def test_set_admin_password_driver_not_authorized(self): + """ + Ensure expected exception is raised if set_admin_password not + authorized. + """ + exc = exception.NotAuthorized(_('Internal error')) + self._do_test_set_admin_password_driver_error(exc, + vm_states.ERROR, + task_states.UPDATING_PASSWORD) + + def test_set_admin_password_driver_not_implemented(self): + """ + Ensure expected exception is raised if set_admin_password not + implemented by driver. + """ + exc = NotImplementedError() + self._do_test_set_admin_password_driver_error(exc, + vm_states.ACTIVE, + None) + def test_inject_file(self): """Ensure we can write a file to an instance""" called = {'inject': False} @@ -2700,11 +2721,17 @@ class ComputeAPITestCase(BaseTestCase): self.assertEqual(inst_ref['vm_state'], vm_states.ACTIVE) self.assertEqual(inst_ref['task_state'], None) + def fake_rpc_method(context, topic, msg, do_cast=True): + self.assertFalse(do_cast) + + self.stubs.Set(rpc, 'call', fake_rpc_method) + self.compute_api.set_admin_password(self.context, inst_ref) inst_ref = db.instance_get_by_uuid(self.context, instance_uuid) self.assertEqual(inst_ref['vm_state'], vm_states.ACTIVE) - self.assertEqual(inst_ref['task_state'], task_states.UPDATING_PASSWORD) + self.assertEqual(inst_ref['task_state'], + task_states.UPDATING_PASSWORD) self.compute.terminate_instance(self.context, instance_uuid) diff --git a/nova/tests/compute/test_rpcapi.py b/nova/tests/compute/test_rpcapi.py index cafd7fd55..7250c61d7 100644 --- a/nova/tests/compute/test_rpcapi.py +++ b/nova/tests/compute/test_rpcapi.py @@ -293,7 +293,7 @@ class ComputeRpcAPITestCase(test.TestCase): version='1.32') def test_set_admin_password(self): - self._test_compute_api('set_admin_password', 'cast', + self._test_compute_api('set_admin_password', 'call', instance=self.fake_instance, new_pass='pw', version='1.33') def test_set_host_enabled(self): -- cgit