CVE-2017-16239

@@ -0,0 +1,13 @@
+---
+security:
+  - |
+    `OSSA-2017-005`_: Nova Filter Scheduler bypass through rebuild action
+
+    By rebuilding an instance, an authenticated user may be able to circumvent
+    the FilterScheduler bypassing imposed filters (for example, the
+    ImagePropertiesFilter or the IsolatedHostsFilter). All setups using the
+    FilterScheduler (or CachingScheduler) are affected.
+
+    The fix is in the `nova-api` and `nova-conductor` services.
+
+    .. _OSSA-2017-005: https://security.openstack.org/ossa/OSSA-2017-005.html
\ No newline at end of file

@@ -2949,9 +2949,24 @@ def _reset_image_metadata():
         # sure that all our instances are currently migrated to have an
         # attached RequestSpec object but let's consider that the operator only
         # half migrated all their instances in the meantime.
+        host = instance.host
         try:
             request_spec = objects.RequestSpec.get_by_instance_uuid(
                 context, instance.uuid)
+            # If a new image is provided on rebuild, we will need to run
+            # through the scheduler again, but we want the instance to be
+            # rebuilt on the same host it's already on.
+            if orig_image_ref != image_href:
+                request_spec.requested_destination = objects.Destination(
+                    host=instance.host,
+                    node=instance.node)
+                # We have to modify the request spec that goes to the scheduler
+                # to contain the new image. We persist this since we've already
+                # changed the instance.image_ref above so we're being
+                # consistent.
+                request_spec.image = objects.ImageMeta.from_dict(image)
+                request_spec.save()
+                host = None     # This tells conductor to call the scheduler.
         except exception.RequestSpecNotFound:
             # Some old instances can still have no RequestSpec object attached
             # to them, we need to support the old way
@@ -2961,7 +2976,7 @@ def _reset_image_metadata():
                 new_pass=admin_password, injected_files=files_to_inject,
                 image_ref=image_href, orig_image_ref=orig_image_ref,
                 orig_sys_metadata=orig_sys_metadata, bdms=bdms,
-                preserve_ephemeral=preserve_ephemeral, host=instance.host,
+                preserve_ephemeral=preserve_ephemeral, host=host,
                 request_spec=request_spec,
                 kwargs=kwargs)
 

@@ -801,7 +801,8 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
 
             # The host variable is passed in two cases:
             # 1. rebuild - the instance.host is passed to rebuild on the
-            #       same host and bypass the scheduler.
+            #       same host and bypass the scheduler *unless* a new image
+            #       was specified
             # 2. evacuate with specified host and force=True - the specified
             #       host is passed and is meant to bypass the scheduler.
             # NOTE(mriedem): This could be a lot more straight-forward if we
@@ -815,10 +816,13 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                     self._allocate_for_evacuate_dest_host(
                         context, instance, host, request_spec)
             else:
-                # At this point, either the user is doing a rebuild on the
-                # same host (not evacuate), or they are evacuating and
-                # specified a host but are not forcing it. The API passes
-                # host=None in this case but sets up the
+                # At this point, the user is either:
+                #
+                # 1. Doing a rebuild on the same host (not evacuate) and
+                #    specified a new image.
+                # 2. Evacuating and specified a host but are not forcing it.
+                #
+                # In either case, the API passes host=None but sets up the
                 # RequestSpec.requested_destination field for the specified
                 # host.
                 if not request_spec:
@@ -833,7 +837,7 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                         image_meta, [instance])
                     request_spec = objects.RequestSpec.from_primitives(
                         context, request_spec, filter_properties)
-                else:
+                elif recreate:
                     # NOTE(sbauza): Augment the RequestSpec object by excluding
                     # the source host for avoiding the scheduler to pick it
                     request_spec.ignore_hosts = request_spec.ignore_hosts or []

@@ -306,6 +306,16 @@ def post_image(self, image):
     def delete_image(self, image_id):
         return self.api_delete('/images/%s' % image_id)
 
+    def put_image_meta_key(self, image_id, key, value):
+        """Creates or updates a given image metadata key/value pair."""
+        req_body = {
+            'meta': {
+                key: value
+            }
+        }
+        return self.api_put('/images/%s/metadata/%s' % (image_id, key),
+                            req_body)
+
     def get_flavor(self, flavor_id):
         return self.api_get('/flavors/%s' % flavor_id).body['flavor']
 

@@ -270,19 +270,21 @@ def _wait_until_deleted(self, server):
             return
 
     def _wait_for_action_fail_completion(
-            self, server, expected_action, event_name):
+            self, server, expected_action, event_name, api=None):
         """Polls instance action events for the given instance, action and
         action event name until it finds the action event with an error
         result.
         """
+        if api is None:
+            api = self.api
         completion_event = None
         for attempt in range(10):
-            actions = self.api.get_instance_actions(server['id'])
+            actions = api.get_instance_actions(server['id'])
             # Look for the migrate action.
             for action in actions:
                 if action['action'] == expected_action:
                     events = (
-                        self.api.api_get(
+                        api.api_get(
                             '/servers/%s/os-instance-actions/%s' %
                             (server['id'], action['request_id'])
                         ).body['instanceAction']['events'])

@@ -1059,6 +1059,68 @@ def test_attach_detach_vol_to_shelved_offloaded_server(self):
         self._delete_server(server_id)
 
 
+class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
+                            integrated_helpers.InstanceHelperMixin):
+    api_major_version = 'v2.1'
+    # We have to cap the microversion at 2.38 because that's the max we
+    # can use to update image metadata via our compute images proxy API.
+    microversion = '2.38'
+
+    def test_rebuild_with_image_novalidhost(self):
+        """Creates a server with an image that is valid for the single compute
+        that we have. Then rebuilds the server, passing in an image with
+        metadata that does not fit the single compute which should result in
+        a NoValidHost error. The ImagePropertiesFilter filter is enabled by
+        default so that should filter out the host based on the image meta.
+        """
+        server_req_body = {
+            'server': {
+                # We hard-code from a fake image since we can't get images
+                # via the compute /images proxy API with microversion > 2.35.
+                'imageRef': '155d900f-4e14-4e4c-a73d-069cbf4541e6',
+                'flavorRef': '1',   # m1.tiny from DefaultFlavorsFixture,
+                'name': 'test_rebuild_with_image_novalidhost',
+                # We don't care about networking for this test. This requires
+                # microversion >= 2.37.
+                'networks': 'none'
+            }
+        }
+        server = self.api.post_server(server_req_body)
+        self._wait_for_state_change(self.api, server, 'ACTIVE')
+        # Now update the image metadata to be something that won't work with
+        # the fake compute driver we're using since the fake driver has an
+        # "x86_64" architecture.
+        rebuild_image_ref = (
+            nova.tests.unit.image.fake.AUTO_DISK_CONFIG_ENABLED_IMAGE_UUID)
+        self.api.put_image_meta_key(
+            rebuild_image_ref, 'hw_architecture', 'unicore32')
+        # Now rebuild the server with that updated image and it should result
+        # in a NoValidHost failure from the scheduler.
+        rebuild_req_body = {
+            'rebuild': {
+                'imageRef': rebuild_image_ref
+            }
+        }
+        # Since we're using the CastAsCall fixture, the NoValidHost error
+        # should actually come back to the API and result in a 500 error.
+        # Normally the user would get a 202 response because nova-api RPC casts
+        # to nova-conductor which RPC calls the scheduler which raises the
+        # NoValidHost. We can mimic the end user way to figure out the failure
+        # by looking for the failed 'rebuild' instance action event.
+        self.api.api_post('/servers/%s/action' % server['id'],
+                          rebuild_req_body, check_response_status=[500])
+        # Look for the failed rebuild action.
+        self._wait_for_action_fail_completion(
+            server, instance_actions.REBUILD, 'rebuild_server',
+            # Before microversion 2.51 events are only returned for instance
+            # actions if you're an admin.
+            self.api_fixture.admin_api)
+        # Unfortunately the server's image_ref is updated to be the new image
+        # even though the rebuild should not work.
+        server = self.api.get_server(server['id'])
+        self.assertEqual(rebuild_image_ref, server['image']['id'])
+
+
 class ProviderUsageBaseTestCase(test.TestCase,
                                 integrated_helpers.InstanceHelperMixin):
     """Base test class for functional tests that check provider usage

@@ -3183,6 +3183,7 @@ def test_rebuild(self, _record_action_start,
         bdm_get_by_instance_uuid.assert_called_once_with(
             self.context, instance.uuid)
 
+    @mock.patch.object(objects.RequestSpec, 'save')
     @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')
     @mock.patch.object(objects.Instance, 'save')
     @mock.patch.object(objects.Instance, 'get_flavor')
@@ -3194,7 +3195,7 @@ def test_rebuild(self, _record_action_start,
     def test_rebuild_change_image(self, _record_action_start,
             _checks_for_create_and_rebuild, _check_auto_disk_config,
             _get_image, bdm_get_by_instance_uuid, get_flavor, instance_save,
-            req_spec_get_by_inst_uuid):
+            req_spec_get_by_inst_uuid, req_spec_save):
         orig_system_metadata = {}
         get_flavor.return_value = test_flavor.fake_flavor
         orig_image_href = 'orig_image'
@@ -3241,8 +3242,15 @@ def get_image(context, image_href):
                     injected_files=files_to_inject, image_ref=new_image_href,
                     orig_image_ref=orig_image_href,
                     orig_sys_metadata=orig_system_metadata, bdms=bdms,
-                    preserve_ephemeral=False, host=instance.host,
+                    preserve_ephemeral=False, host=None,
                     request_spec=fake_spec, kwargs={})
+            # assert the request spec was modified so the scheduler picks
+            # the existing instance host/node
+            req_spec_save.assert_called_once_with()
+            self.assertIn('requested_destination', fake_spec)
+            requested_destination = fake_spec.requested_destination
+            self.assertEqual(instance.host, requested_destination.host)
+            self.assertEqual(instance.node, requested_destination.node)
 
         _check_auto_disk_config.assert_called_once_with(image=new_image)
         _checks_for_create_and_rebuild.assert_called_once_with(self.context,

@@ -1361,7 +1361,10 @@ def test_rebuild_instance_with_request_spec(self):
             self.conductor_manager.rebuild_instance(context=self.context,
                                             instance=inst_obj,
                                             **rebuild_args)
-            reset_fd.assert_called_once_with()
+            if rebuild_args['recreate']:
+                reset_fd.assert_called_once_with()
+            else:
+                reset_fd.assert_not_called()
             select_dest_mock.assert_called_once_with(self.context,
                     fake_spec, [inst_obj.uuid])
             compute_args['host'] = expected_host

@@ -3051,9 +3051,24 @@ def _reset_image_metadata():
         # sure that all our instances are currently migrated to have an
         # attached RequestSpec object but let's consider that the operator only
         # half migrated all their instances in the meantime.
+        host = instance.host
         try:
             request_spec = objects.RequestSpec.get_by_instance_uuid(
                 context, instance.uuid)
+            # If a new image is provided on rebuild, we will need to run
+            # through the scheduler again, but we want the instance to be
+            # rebuilt on the same host it's already on.
+            if orig_image_ref != image_href:
+                request_spec.requested_destination = objects.Destination(
+                    host=instance.host,
+                    node=instance.node)
+                # We have to modify the request spec that goes to the scheduler
+                # to contain the new image. We persist this since we've already
+                # changed the instance.image_ref above so we're being
+                # consistent.
+                request_spec.image = objects.ImageMeta.from_dict(image)
+                request_spec.save()
+                host = None     # This tells conductor to call the scheduler.
         except exception.RequestSpecNotFound:
             # Some old instances can still have no RequestSpec object attached
             # to them, we need to support the old way
@@ -3063,7 +3078,7 @@ def _reset_image_metadata():
                 new_pass=admin_password, injected_files=files_to_inject,
                 image_ref=image_href, orig_image_ref=orig_image_ref,
                 orig_sys_metadata=orig_sys_metadata, bdms=bdms,
-                preserve_ephemeral=preserve_ephemeral, host=instance.host,
+                preserve_ephemeral=preserve_ephemeral, host=host,
                 request_spec=request_spec,
                 kwargs=kwargs)
 

@@ -705,7 +705,7 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                     filter_properties = {'ignore_hosts': [instance.host]}
                     request_spec = scheduler_utils.build_request_spec(
                             context, image_ref, [instance])
-                else:
+                elif recreate:
                     # NOTE(sbauza): Augment the RequestSpec object by excluding
                     # the source host for avoiding the scheduler to pick it
                     request_spec.ignore_hosts = request_spec.ignore_hosts or []
@@ -720,6 +720,10 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                     filter_properties = request_spec.\
                         to_legacy_filter_properties_dict()
                     request_spec = request_spec.to_legacy_request_spec_dict()
+                else:
+                    filter_properties = request_spec. \
+                        to_legacy_filter_properties_dict()
+                    request_spec = request_spec.to_legacy_request_spec_dict()
                 try:
                     hosts = self._schedule_instances(
                             context, request_spec, filter_properties)

@@ -303,6 +303,16 @@ def post_image(self, image):
     def delete_image(self, image_id):
         return self.api_delete('/images/%s' % image_id)
 
+    def put_image_meta_key(self, image_id, key, value):
+        """Creates or updates a given image metadata key/value pair."""
+        req_body = {
+            'meta': {
+                key: value
+            }
+        }
+        return self.api_put('/images/%s/metadata/%s' % (image_id, key),
+                            req_body)
+
     def get_flavor(self, flavor_id):
         return self.api_get('/flavors/%s' % flavor_id).body['flavor']
 

@@ -77,10 +77,11 @@ def setUp(self):
         self.flags(use_neutron=self.USE_NEUTRON)
 
         nova.tests.unit.image.fake.stub_out_image_service(self)
-        self._setup_services()
 
         self.useFixture(cast_as_call.CastAsCall(self.stubs))
 
+        self._setup_services()
+
         self.addCleanup(nova.tests.unit.image.fake.FakeImageService_reset)
 
     def _setup_compute_service(self):
@@ -248,3 +249,40 @@ def _build_minimal_create_server_request(self, api, name, image_uuid=None,
         if networks is not None:
             server['networks'] = networks
         return server
+
+    def _wait_for_action_fail_completion(
+            self, server, expected_action, event_name, api=None):
+        """Polls instance action events for the given instance, action and
+        action event name until it finds the action event with an error
+        result.
+        """
+        if api is None:
+            api = self.api
+        completion_event = None
+        for attempt in range(10):
+            actions = api.get_instance_actions(server['id'])
+            # Look for the migrate action.
+            for action in actions:
+                if action['action'] == expected_action:
+                    events = (
+                        api.api_get(
+                            '/servers/%s/os-instance-actions/%s' %
+                            (server['id'], action['request_id'])
+                        ).body['instanceAction']['events'])
+                    # Look for the action event being in error state.
+                    for event in events:
+                        if (event['event'] == event_name and
+                                event['result'] is not None and
+                                event['result'].lower() == 'error'):
+                            completion_event = event
+                            # Break out of the events loop.
+                            break
+                    if completion_event:
+                        # Break out of the actions loop.
+                        break
+            # We didn't find the completion event yet, so wait a bit.
+            time.sleep(0.5)
+
+        if completion_event is None:
+            self.fail('Timed out waiting for %s failure event. Current '
+                      'instance actions: %s' % (event_name, actions))

@@ -23,17 +23,20 @@
 from oslo_utils import timeutils
 
 from nova.compute import api as compute_api
+from nova.compute import instance_actions
 from nova.compute import rpcapi
 from nova import context
 from nova import exception
 from nova import objects
 from nova.objects import block_device as block_device_obj
 from nova import test
+from nova.tests import fixtures as nova_fixtures
 from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
 from nova.tests.unit.api.openstack import fakes
 from nova.tests.unit import fake_block_device
 from nova.tests.unit import fake_network
+import nova.tests.unit.image.fake
 from nova import volume
 
 
@@ -907,3 +910,75 @@ def test_attach_detach_vol_to_shelved_offloaded_server(self):
             self.assertTrue(mock_clean_vols.called)
 
         self._delete_server(server_id)
+
+
+class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
+                            integrated_helpers.InstanceHelperMixin):
+    api_major_version = 'v2.1'
+    # We have to cap the microversion at 2.38 because that's the max we
+    # can use to update image metadata via our compute images proxy API.
+    microversion = '2.38'
+
+    # We need the ImagePropertiesFilter so override the base class setup
+    # which configures to use the chance_scheduler.
+    def _setup_scheduler_service(self):
+        # Make sure we're using the PlacementFixture before starting the
+        # scheduler since the FilterScheduler relies on Placement.
+        self.useFixture(nova_fixtures.PlacementFixture())
+        self.flags(enabled_filters=['ImagePropertiesFilter'],
+                   group='filter_scheduler')
+        return self.start_service('scheduler')
+
+    def test_rebuild_with_image_novalidhost(self):
+        """Creates a server with an image that is valid for the single compute
+        that we have. Then rebuilds the server, passing in an image with
+        metadata that does not fit the single compute which should result in
+        a NoValidHost error. The ImagePropertiesFilter filter is enabled by
+        default so that should filter out the host based on the image meta.
+        """
+        server_req_body = {
+            'server': {
+                # We hard-code from a fake image since we can't get images
+                # via the compute /images proxy API with microversion > 2.35.
+                'imageRef': '155d900f-4e14-4e4c-a73d-069cbf4541e6',
+                'flavorRef': '1',   # m1.tiny from DefaultFlavorsFixture,
+                'name': 'test_rebuild_with_image_novalidhost',
+                # We don't care about networking for this test. This requires
+                # microversion >= 2.37.
+                'networks': 'none'
+            }
+        }
+        server = self.api.post_server(server_req_body)
+        self._wait_for_state_change(self.api, server, 'ACTIVE')
+        # Now update the image metadata to be something that won't work with
+        # the fake compute driver we're using since the fake driver has an
+        # "x86_64" architecture.
+        rebuild_image_ref = (
+            nova.tests.unit.image.fake.AUTO_DISK_CONFIG_ENABLED_IMAGE_UUID)
+        self.api.put_image_meta_key(
+            rebuild_image_ref, 'hw_architecture', 'unicore32')
+        # Now rebuild the server with that updated image and it should result
+        # in a NoValidHost failure from the scheduler.
+        rebuild_req_body = {
+            'rebuild': {
+                'imageRef': rebuild_image_ref
+            }
+        }
+        # Since we're using the CastAsCall fixture, the NoValidHost error
+        # should actually come back to the API and result in a 500 error.
+        # Normally the user would get a 202 response because nova-api RPC casts
+        # to nova-conductor which RPC calls the scheduler which raises the
+        # NoValidHost. We can mimic the end user way to figure out the failure
+        # by looking for the failed 'rebuild' instance action event.
+        self.api.api_post('/servers/%s/action' % server['id'],
+                          rebuild_req_body, check_response_status=[500])
+        # Look for the failed rebuild action.
+        self._wait_for_action_fail_completion(
+            server, instance_actions.REBUILD, 'rebuild_server',
+            # Before microversion 2.51 events are only returned for instance
+            # actions if you're an admin.
+            self.api_fixture.admin_api)
+        # Unfortunately the server's image_ref is updated to be the new image
+        # even though the rebuild should not work.
+        server = self.api.get_server(server['id'])
+        self.assertEqual(rebuild_image_ref, server['image']['id'])

@@ -3182,6 +3182,7 @@ def test_rebuild(self, _record_action_start,
                 None, image, flavor, {}, [], None)
         self.assertNotEqual(orig_system_metadata, instance.system_metadata)
 
+    @mock.patch.object(objects.RequestSpec, 'save')
     @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')
     @mock.patch.object(objects.Instance, 'save')
     @mock.patch.object(objects.Instance, 'get_flavor')
@@ -3193,7 +3194,7 @@ def test_rebuild(self, _record_action_start,
     def test_rebuild_change_image(self, _record_action_start,
             _checks_for_create_and_rebuild, _check_auto_disk_config,
             _get_image, bdm_get_by_instance_uuid, get_flavor, instance_save,
-            req_spec_get_by_inst_uuid):
+            req_spec_get_by_inst_uuid, req_spec_save):
         orig_system_metadata = {}
         get_flavor.return_value = test_flavor.fake_flavor
         orig_image_href = 'orig_image'
@@ -3240,8 +3241,15 @@ def get_image(context, image_href):
                     injected_files=files_to_inject, image_ref=new_image_href,
                     orig_image_ref=orig_image_href,
                     orig_sys_metadata=orig_system_metadata, bdms=bdms,
-                    preserve_ephemeral=False, host=instance.host,
+                    preserve_ephemeral=False, host=None,
                     request_spec=fake_spec, kwargs={})
+            # assert the request spec was modified so the scheduler picks
+            # the existing instance host/node
+            req_spec_save.assert_called_once_with()
+            self.assertIn('requested_destination', fake_spec)
+            requested_destination = fake_spec.requested_destination
+            self.assertEqual(instance.host, requested_destination.host)
+            self.assertEqual(instance.node, requested_destination.node)
 
         _check_auto_disk_config.assert_called_once_with(image=new_image)
         _checks_for_create_and_rebuild.assert_called_once_with(self.context,

@@ -1378,7 +1378,10 @@ def test_rebuild_instance_with_request_spec(self):
             self.conductor_manager.rebuild_instance(context=self.context,
                                             instance=inst_obj,
                                             **rebuild_args)
-            reset_fd.assert_called_once_with()
+            if rebuild_args['recreate']:
+                reset_fd.assert_called_once_with()
+            else:
+                reset_fd.assert_not_called()
             to_reqspec.assert_called_once_with()
             to_filtprops.assert_called_once_with()
             fp_mock.assert_called_once_with(self.context, request_spec,

@@ -2978,9 +2978,24 @@ def _reset_image_metadata():
         # sure that all our instances are currently migrated to have an
         # attached RequestSpec object but let's consider that the operator only
         # half migrated all their instances in the meantime.
+        host = instance.host
         try:
             request_spec = objects.RequestSpec.get_by_instance_uuid(
                 context, instance.uuid)
+            # If a new image is provided on rebuild, we will need to run
+            # through the scheduler again, but we want the instance to be
+            # rebuilt on the same host it's already on.
+            if orig_image_ref != image_href:
+                request_spec.requested_destination = objects.Destination(
+                    host=instance.host,
+                    node=instance.node)
+                # We have to modify the request spec that goes to the scheduler
+                # to contain the new image. We persist this since we've already
+                # changed the instance.image_ref above so we're being
+                # consistent.
+                request_spec.image = objects.ImageMeta.from_dict(image)
+                request_spec.save()
+                host = None     # This tells conductor to call the scheduler.
         except exception.RequestSpecNotFound:
             # Some old instances can still have no RequestSpec object attached
             # to them, we need to support the old way
@@ -2990,7 +3005,7 @@ def _reset_image_metadata():
                 new_pass=admin_password, injected_files=files_to_inject,
                 image_ref=image_href, orig_image_ref=orig_image_ref,
                 orig_sys_metadata=orig_sys_metadata, bdms=bdms,
-                preserve_ephemeral=preserve_ephemeral, host=instance.host,
+                preserve_ephemeral=preserve_ephemeral, host=host,
                 request_spec=request_spec,
                 kwargs=kwargs)
 

@@ -819,7 +819,8 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
 
             # The host variable is passed in two cases:
             # 1. rebuild - the instance.host is passed to rebuild on the
-            #       same host and bypass the scheduler.
+            #       same host and bypass the scheduler *unless* a new image
+            #       was specified
             # 2. evacuate with specified host and force=True - the specified
             #       host is passed and is meant to bypass the scheduler.
             # NOTE(mriedem): This could be a lot more straight-forward if we
@@ -833,10 +834,13 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                     self._allocate_for_evacuate_dest_host(
                         context, instance, host, request_spec)
             else:
-                # At this point, either the user is doing a rebuild on the
-                # same host (not evacuate), or they are evacuating and
-                # specified a host but are not forcing it. The API passes
-                # host=None in this case but sets up the
+                # At this point, the user is either:
+                #
+                # 1. Doing a rebuild on the same host (not evacuate) and
+                #    specified a new image.
+                # 2. Evacuating and specified a host but are not forcing it.
+                #
+                # In either case, the API passes host=None but sets up the
                 # RequestSpec.requested_destination field for the specified
                 # host.
                 if not request_spec:
@@ -850,7 +854,7 @@ def rebuild_instance(self, context, instance, orig_image_ref, image_ref,
                             context, image_ref, [instance])
                     request_spec = objects.RequestSpec.from_primitives(
                         context, request_spec, filter_properties)
-                else:
+                elif recreate:
                     # NOTE(sbauza): Augment the RequestSpec object by excluding
                     # the source host for avoiding the scheduler to pick it
                     request_spec.ignore_hosts = request_spec.ignore_hosts or []

@@ -306,6 +306,16 @@ def post_image(self, image):
     def delete_image(self, image_id):
         return self.api_delete('/images/%s' % image_id)
 
+    def put_image_meta_key(self, image_id, key, value):
+        """Creates or updates a given image metadata key/value pair."""
+        req_body = {
+            'meta': {
+                key: value
+            }
+        }
+        return self.api_put('/images/%s/metadata/%s' % (image_id, key),
+                            req_body)
+
     def get_flavor(self, flavor_id):
         return self.api_get('/flavors/%s' % flavor_id).body['flavor']
 

@@ -77,11 +77,12 @@ def setUp(self):
         self.flags(use_neutron=self.USE_NEUTRON)
 
         nova.tests.unit.image.fake.stub_out_image_service(self)
-        self._setup_services()
 
         self.useFixture(cast_as_call.CastAsCall(self))
         self.useFixture(nova_fixtures.PlacementFixture())
 
+        self._setup_services()
+
         self.addCleanup(nova.tests.unit.image.fake.FakeImageService_reset)
 
     def _setup_compute_service(self):
@@ -270,19 +271,21 @@ def _wait_until_deleted(self, server):
             return
 
     def _wait_for_action_fail_completion(
-            self, server, expected_action, event_name):
+            self, server, expected_action, event_name, api=None):
         """Polls instance action events for the given instance, action and
         action event name until it finds the action event with an error
         result.
         """
+        if api is None:
+            api = self.api
         completion_event = None
         for attempt in range(10):
-            actions = self.api.get_instance_actions(server['id'])
+            actions = api.get_instance_actions(server['id'])
             # Look for the migrate action.
             for action in actions:
                 if action['action'] == expected_action:
                     events = (
-                        self.api.api_get(
+                        api.api_get(
                             '/servers/%s/os-instance-actions/%s' %
                             (server['id'], action['request_id'])
                         ).body['instanceAction']['events'])

@@ -1057,6 +1057,73 @@ def test_attach_detach_vol_to_shelved_offloaded_server(self):
         self._delete_server(server_id)
 
 
+class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
+                            integrated_helpers.InstanceHelperMixin):
+    api_major_version = 'v2.1'
+    # We have to cap the microversion at 2.38 because that's the max we
+    # can use to update image metadata via our compute images proxy API.
+    microversion = '2.38'
+
+    # We need the ImagePropertiesFilter so override the base class setup
+    # which configures to use the chance_scheduler.
+    def _setup_scheduler_service(self):
+        return self.start_service('scheduler')
+
+    def test_rebuild_with_image_novalidhost(self):
+        """Creates a server with an image that is valid for the single compute
+        that we have. Then rebuilds the server, passing in an image with
+        metadata that does not fit the single compute which should result in
+        a NoValidHost error. The ImagePropertiesFilter filter is enabled by
+        default so that should filter out the host based on the image meta.
+        """
+        server_req_body = {
+            'server': {
+                # We hard-code from a fake image since we can't get images
+                # via the compute /images proxy API with microversion > 2.35.
+                'imageRef': '155d900f-4e14-4e4c-a73d-069cbf4541e6',
+                'flavorRef': '1',   # m1.tiny from DefaultFlavorsFixture,
+                'name': 'test_rebuild_with_image_novalidhost',
+                # We don't care about networking for this test. This requires
+                # microversion >= 2.37.
+                'networks': 'none'
+            }
+        }
+        server = self.api.post_server(server_req_body)
+        self._wait_for_state_change(self.api, server, 'ACTIVE')
+        # Now update the image metadata to be something that won't work with
+        # the fake compute driver we're using since the fake driver has an
+        # "x86_64" architecture.
+        rebuild_image_ref = (
+            nova.tests.unit.image.fake.AUTO_DISK_CONFIG_ENABLED_IMAGE_UUID)
+        self.api.put_image_meta_key(
+            rebuild_image_ref, 'hw_architecture', 'unicore32')
+        # Now rebuild the server with that updated image and it should result
+        # in a NoValidHost failure from the scheduler.
+        rebuild_req_body = {
+            'rebuild': {
+                'imageRef': rebuild_image_ref
+            }
+        }
+        # Since we're using the CastAsCall fixture, the NoValidHost error
+        # should actually come back to the API and result in a 500 error.
+        # Normally the user would get a 202 response because nova-api RPC casts
+        # to nova-conductor which RPC calls the scheduler which raises the
+        # NoValidHost. We can mimic the end user way to figure out the failure
+        # by looking for the failed 'rebuild' instance action event.
+        self.api.api_post('/servers/%s/action' % server['id'],
+                          rebuild_req_body, check_response_status=[500])
+        # Look for the failed rebuild action.
+        self._wait_for_action_fail_completion(
+            server, instance_actions.REBUILD, 'rebuild_server',
+            # Before microversion 2.51 events are only returned for instance
+            # actions if you're an admin.
+            self.api_fixture.admin_api)
+        # Unfortunately the server's image_ref is updated to be the new image
+        # even though the rebuild should not work.
+        server = self.api.get_server(server['id'])
+        self.assertEqual(rebuild_image_ref, server['image']['id'])
+
+
 class ProviderUsageBaseTestCase(test.TestCase,
                                 integrated_helpers.InstanceHelperMixin):
     """Base test class for functional tests that check provider usage

@@ -3159,6 +3159,7 @@ def test_rebuild(self, _record_action_start,
                 None, image, flavor, {}, [], None)
         self.assertNotEqual(orig_system_metadata, instance.system_metadata)
 
+    @mock.patch.object(objects.RequestSpec, 'save')
     @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')
     @mock.patch.object(objects.Instance, 'save')
     @mock.patch.object(objects.Instance, 'get_flavor')
@@ -3170,7 +3171,7 @@ def test_rebuild(self, _record_action_start,
     def test_rebuild_change_image(self, _record_action_start,
             _checks_for_create_and_rebuild, _check_auto_disk_config,
             _get_image, bdm_get_by_instance_uuid, get_flavor, instance_save,
-            req_spec_get_by_inst_uuid):
+            req_spec_get_by_inst_uuid, req_spec_save):
         orig_system_metadata = {}
         get_flavor.return_value = test_flavor.fake_flavor
         orig_image_href = 'orig_image'
@@ -3217,8 +3218,15 @@ def get_image(context, image_href):
                     injected_files=files_to_inject, image_ref=new_image_href,
                     orig_image_ref=orig_image_href,
                     orig_sys_metadata=orig_system_metadata, bdms=bdms,
-                    preserve_ephemeral=False, host=instance.host,
+                    preserve_ephemeral=False, host=None,
                     request_spec=fake_spec, kwargs={})
+            # assert the request spec was modified so the scheduler picks
+            # the existing instance host/node
+            req_spec_save.assert_called_once_with()
+            self.assertIn('requested_destination', fake_spec)
+            requested_destination = fake_spec.requested_destination
+            self.assertEqual(instance.host, requested_destination.host)
+            self.assertEqual(instance.node, requested_destination.node)
 
         _check_auto_disk_config.assert_called_once_with(image=new_image)
         _checks_for_create_and_rebuild.assert_called_once_with(self.context,

@@ -1359,7 +1359,10 @@ def test_rebuild_instance_with_request_spec(self):
             self.conductor_manager.rebuild_instance(context=self.context,
                                             instance=inst_obj,
                                             **rebuild_args)
-            reset_fd.assert_called_once_with()
+            if rebuild_args['recreate']:
+                reset_fd.assert_called_once_with()
+            else:
+                reset_fd.assert_not_called()
             select_dest_mock.assert_called_once_with(self.context,
                     fake_spec, [inst_obj.uuid])
             compute_args['host'] = expected_host