Merge "Refined fix for validating image on rebuild"

nova/compute/api.py

@@ -2941,16 +2941,27 @@ class API(base.Base):
             # 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.
+                if 'scheduler_hints' not in request_spec:
+                    request_spec.scheduler_hints = {}
+                # Nuke the id on this so we can't accidentally save
+                # this hint hack later
+                del request_spec.id
+
+                # NOTE(danms): Passing host=None tells conductor to
+                # call the scheduler. The _nova_check_type hint
+                # requires that the scheduler returns only the same
+                # host that we are currently on and only checks
+                # rebuild-related filters.
+                request_spec.scheduler_hints['_nova_check_type'] = ['rebuild']
+                request_spec.force_hosts = [instance.host]
+                request_spec.force_nodes = [instance.node]
+                host = None
         except exception.RequestSpecNotFound:
             # Some old instances can still have no RequestSpec object attached
             # to them, we need to support the old way

nova/scheduler/filters/__init__.py

@@ -21,9 +21,27 @@ from nova import filters
 
 class BaseHostFilter(filters.BaseFilter):
     """Base class for host filters."""
-    def _filter_one(self, obj, filter_properties):
+
+    # This is set to True if this filter should be run for rebuild.
+    # For example, with rebuild, we need to ask the scheduler if the
+    # existing host is still legit for a rebuild with the new image and
+    # other parameters. We care about running policy filters (i.e.
+    # ImagePropertiesFilter) but not things that check usage on the
+    # existing compute node, etc.
+    RUN_ON_REBUILD = False
+
+    def _filter_one(self, obj, spec):
         """Return True if the object passes the filter, otherwise False."""
-        return self.host_passes(obj, filter_properties)
+        # Do this here so we don't get scheduler.filters.utils
+        from nova.scheduler import utils
+        if not self.RUN_ON_REBUILD and utils.request_is_rebuild(spec):
+            # If we don't filter, default to passing the host.
+            return True
+        else:
+            # We are either a rebuild filter, in which case we always run,
+            # or this request is not rebuild in which case all filters
+            # should run.
+            return self.host_passes(obj, spec)
 
     def host_passes(self, host_state, filter_properties):
         """Return True if the HostState passes the filter, otherwise False.

nova/scheduler/filters/affinity_filter.py

@@ -29,6 +29,8 @@ class DifferentHostFilter(filters.BaseHostFilter):
     # The hosts the instances are running on doesn't change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         affinity_uuids = spec_obj.get_scheduler_hint('different_host')
         if affinity_uuids:
@@ -45,6 +47,8 @@ class SameHostFilter(filters.BaseHostFilter):
     # The hosts the instances are running on doesn't change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         affinity_uuids = spec_obj.get_scheduler_hint('same_host')
         if affinity_uuids:
@@ -59,6 +63,8 @@ class SimpleCIDRAffinityFilter(filters.BaseHostFilter):
     # The address of a host doesn't change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         affinity_cidr = spec_obj.get_scheduler_hint('cidr', '/24')
         affinity_host_addr = spec_obj.get_scheduler_hint('build_near_host_ip')
@@ -77,6 +83,9 @@ class _GroupAntiAffinityFilter(filters.BaseHostFilter):
     """Schedule the instance on a different host from a set of group
     hosts.
     """
+
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         # Only invoke the filter if 'anti-affinity' is configured
         policies = (spec_obj.instance_group.policies
@@ -110,6 +119,9 @@ class ServerGroupAntiAffinityFilter(_GroupAntiAffinityFilter):
 class _GroupAffinityFilter(filters.BaseHostFilter):
     """Schedule the instance on to host from a set of group hosts.
     """
+
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         # Only invoke the filter if 'affinity' is configured
         policies = (spec_obj.instance_group.policies

nova/scheduler/filters/aggregate_image_properties_isolation.py

@@ -32,6 +32,8 @@ class AggregateImagePropertiesIsolation(filters.BaseHostFilter):
     # Aggregate data and instance type does not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = True
+
     def host_passes(self, host_state, spec_obj):
         """Checks a host in an aggregate that metadata key/value match
         with image properties.

nova/scheduler/filters/aggregate_instance_extra_specs.py

@@ -33,6 +33,8 @@ class AggregateInstanceExtraSpecsFilter(filters.BaseHostFilter):
     # Aggregate data and instance type does not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         """Return a list of hosts that can create instance_type
 

nova/scheduler/filters/aggregate_multitenancy_isolation.py

@@ -28,6 +28,8 @@ class AggregateMultiTenancyIsolation(filters.BaseHostFilter):
     # Aggregate data and tenant do not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         """If a host is in an aggregate that has the metadata key
         "filter_tenant_id" it can only create instances from that tenant(s).

nova/scheduler/filters/all_hosts_filter.py

@@ -23,5 +23,7 @@ class AllHostsFilter(filters.BaseHostFilter):
     # list of hosts doesn't change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         return True

nova/scheduler/filters/availability_zone_filter.py

@@ -35,6 +35,8 @@ class AvailabilityZoneFilter(filters.BaseHostFilter):
     # Availability zones do not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         availability_zone = spec_obj.availability_zone
 

nova/scheduler/filters/compute_capabilities_filter.py

@@ -30,6 +30,8 @@ class ComputeCapabilitiesFilter(filters.BaseHostFilter):
     # Instance type and host capabilities do not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def _get_capabilities(self, host_state, scope):
         cap = host_state
         for index in range(0, len(scope)):

nova/scheduler/filters/compute_filter.py

@@ -25,6 +25,8 @@ LOG = logging.getLogger(__name__)
 class ComputeFilter(filters.BaseHostFilter):
     """Filter on active Compute nodes."""
 
+    RUN_ON_REBUILD = False
+
     def __init__(self):
         self.servicegroup_api = servicegroup.API()
 

nova/scheduler/filters/core_filter.py

@@ -26,6 +26,8 @@ LOG = logging.getLogger(__name__)
 
 class BaseCoreFilter(filters.BaseHostFilter):
 
+    RUN_ON_REBUILD = False
+
     def _get_cpu_allocation_ratio(self, host_state, spec_obj):
         raise NotImplementedError
 

nova/scheduler/filters/disk_filter.py

@@ -28,6 +28,8 @@ CONF = nova.conf.CONF
 class DiskFilter(filters.BaseHostFilter):
     """Disk Filter with over subscription flag."""
 
+    RUN_ON_REBUILD = False
+
     def _get_disk_allocation_ratio(self, host_state, spec_obj):
         return host_state.disk_allocation_ratio
 
@@ -80,6 +82,8 @@ class AggregateDiskFilter(DiskFilter):
     found.
     """
 
+    RUN_ON_REBUILD = False
+
     def _get_disk_allocation_ratio(self, host_state, spec_obj):
         aggregate_vals = utils.aggregate_values_from_key(
             host_state,

nova/scheduler/filters/exact_core_filter.py

@@ -25,6 +25,8 @@ LOG = logging.getLogger(__name__)
 class ExactCoreFilter(filters.BaseHostFilter):
     """Exact Core Filter."""
 
+    RUN_ON_REBUILD = False
+
     def __init__(self, *args, **kwargs):
         super(ExactCoreFilter, self).__init__(*args, **kwargs)
         LOG.warning('ExactCoreFilter is deprecated in Pike and will be '

nova/scheduler/filters/exact_disk_filter.py

@@ -23,6 +23,8 @@ LOG = logging.getLogger(__name__)
 class ExactDiskFilter(filters.BaseHostFilter):
     """Exact Disk Filter."""
 
+    RUN_ON_REBUILD = False
+
     def __init__(self, *args, **kwargs):
         super(ExactDiskFilter, self).__init__(*args, **kwargs)
         LOG.warning('ExactDiskFilter is deprecated in Pike and will be '

nova/scheduler/filters/exact_ram_filter.py

@@ -23,6 +23,8 @@ LOG = logging.getLogger(__name__)
 class ExactRamFilter(filters.BaseHostFilter):
     """Exact RAM Filter."""
 
+    RUN_ON_REBUILD = False
+
     def __init__(self, *args, **kwargs):
         super(ExactRamFilter, self).__init__(*args, **kwargs)
         LOG.warning('ExactRamFilter is deprecated in Pike and will be '

nova/scheduler/filters/image_props_filter.py

@@ -35,6 +35,8 @@ class ImagePropertiesFilter(filters.BaseHostFilter):
     contained in the image dictionary in the request_spec.
     """
 
+    RUN_ON_REBUILD = True
+
     # Image Properties and Compute Capabilities do not change within
     # a request
     run_filter_once_per_request = True

nova/scheduler/filters/io_ops_filter.py

@@ -28,6 +28,8 @@ CONF = nova.conf.CONF
 class IoOpsFilter(filters.BaseHostFilter):
     """Filter out hosts with too many concurrent I/O operations."""
 
+    RUN_ON_REBUILD = False
+
     def _get_max_io_ops_per_host(self, host_state, spec_obj):
         return CONF.filter_scheduler.max_io_ops_per_host
 

nova/scheduler/filters/isolated_hosts_filter.py

@@ -25,6 +25,8 @@ class IsolatedHostsFilter(filters.BaseHostFilter):
     # The configuration values do not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = True
+
     def host_passes(self, host_state, spec_obj):
         """Result Matrix with 'restrict_isolated_hosts_to_isolated_images' set
         to True::

nova/scheduler/filters/json_filter.py

@@ -26,6 +26,9 @@ class JsonFilter(filters.BaseHostFilter):
     """Host Filter to allow simple JSON-based grammar for
     selecting hosts.
     """
+
+    RUN_ON_REBUILD = False
+
     def _op_compare(self, args, op):
         """Returns True if the specified operator can successfully
         compare the first item in the args with all the rest. Will

nova/scheduler/filters/metrics_filter.py

@@ -32,6 +32,8 @@ class MetricsFilter(filters.BaseHostFilter):
     these hosts.
     """
 
+    RUN_ON_REBUILD = False
+
     def __init__(self):
         super(MetricsFilter, self).__init__()
         opts = utils.parse_options(CONF.metrics.weight_setting,

nova/scheduler/filters/num_instances_filter.py

@@ -28,6 +28,8 @@ CONF = nova.conf.CONF
 class NumInstancesFilter(filters.BaseHostFilter):
     """Filter out hosts with too many instances."""
 
+    RUN_ON_REBUILD = False
+
     def _get_max_instances_per_host(self, host_state, spec_obj):
         return CONF.filter_scheduler.max_instances_per_host
 

nova/scheduler/filters/numa_topology_filter.py

@@ -23,6 +23,8 @@ LOG = logging.getLogger(__name__)
 class NUMATopologyFilter(filters.BaseHostFilter):
     """Filter on requested NUMA topology."""
 
+    RUN_ON_REBUILD = True
+
     def _satisfies_cpu_policy(self, host_state, extra_specs, image_props):
         """Check that the host_state provided satisfies any available
         CPU policy requirements.

nova/scheduler/filters/pci_passthrough_filter.py

@@ -40,6 +40,8 @@ class PciPassthroughFilter(filters.BaseHostFilter):
 
     """
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         """Return true if the host has the required PCI devices."""
         pci_requests = spec_obj.pci_requests

nova/scheduler/filters/ram_filter.py

@@ -25,6 +25,8 @@ LOG = logging.getLogger(__name__)
 
 class BaseRamFilter(filters.BaseHostFilter):
 
+    RUN_ON_REBUILD = False
+
     def _get_ram_allocation_ratio(self, host_state, spec_obj):
         raise NotImplementedError
 

nova/scheduler/filters/retry_filter.py

@@ -26,6 +26,10 @@ class RetryFilter(filters.BaseHostFilter):
     purposes
     """
 
+    # NOTE(danms): This does not affect _where_ an instance lands, so not
+    # related to rebuild.
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         """Skip nodes that have already been attempted."""
         retry = spec_obj.retry

nova/scheduler/filters/trusted_filter.py

@@ -230,6 +230,8 @@ class ComputeAttestation(object):
 class TrustedFilter(filters.BaseHostFilter):
     """Trusted filter to support Trusted Compute Pools."""
 
+    RUN_ON_REBUILD = False
+
     def __init__(self):
         self.compute_attestation = ComputeAttestation()
         msg = _LW('The TrustedFilter is deprecated as it has been marked '

nova/scheduler/filters/type_filter.py

@@ -30,6 +30,8 @@ class TypeAffinityFilter(filters.BaseHostFilter):
     (spread) set to 1 (default).
     """
 
+    RUN_ON_REBUILD = False
+
     def __init__(self):
         super(TypeAffinityFilter, self).__init__()
         LOG.warning('TypeAffinityFilter is deprecated for removal in the '
@@ -64,6 +66,8 @@ class AggregateTypeAffinityFilter(filters.BaseHostFilter):
     # Aggregate data does not change within a request
     run_filter_once_per_request = True
 
+    RUN_ON_REBUILD = False
+
     def host_passes(self, host_state, spec_obj):
         instance_type = spec_obj.flavor
 

nova/scheduler/host_manager.py

@@ -577,8 +577,13 @@ class HostManager(object):
                 _match_forced_hosts(name_to_cls_map, force_hosts)
             if force_nodes:
                 _match_forced_nodes(name_to_cls_map, force_nodes)
-            if force_hosts or force_nodes:
-                # NOTE(deva): Skip filters when forcing host or node
+            check_type = ('scheduler_hints' in spec_obj and
+                          spec_obj.scheduler_hints.get('_nova_check_type'))
+            if not check_type and (force_hosts or force_nodes):
+                # NOTE(deva,dansmith): Skip filters when forcing host or node
+                # unless we've declared the internal check type flag, in which
+                # case we're asking for a specific host and for filtering to
+                # be done.
                 if name_to_cls_map:
                     return name_to_cls_map.values()
                 else:

nova/scheduler/utils.py

@@ -736,3 +736,16 @@ def retry_on_timeout(retries=1):
     return outer
 
 retry_select_destinations = retry_on_timeout(CONF.scheduler.max_attempts - 1)
+
+
+def request_is_rebuild(spec_obj):
+    """Returns True if request is for a rebuild.
+
+    :param spec_obj: An objects.RequestSpec to examine (or None).
+    """
+    if not spec_obj:
+        return False
+    if 'scheduler_hints' not in spec_obj:
+        return False
+    check_type = spec_obj.scheduler_hints.get('_nova_check_type')
+    return check_type == ['rebuild']

nova/tests/functional/test_servers.py

@@ -1066,6 +1066,16 @@ class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
     # can use to update image metadata via our compute images proxy API.
     microversion = '2.38'
 
+    def _disable_compute_for(self, server):
+        # Refresh to get its host
+        server = self.api.get_server(server['id'])
+        host = server['OS-EXT-SRV-ATTR:host']
+
+        # Disable the service it is on
+        self.api_fixture.admin_api.put_service('disable',
+                                               {'host': host,
+                                                'binary': 'nova-compute'})
+
     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
@@ -1073,6 +1083,12 @@ class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
         a NoValidHost error. The ImagePropertiesFilter filter is enabled by
         default so that should filter out the host based on the image meta.
         """
+
+        fake.set_nodes(['host2'])
+        self.addCleanup(fake.restore_nodes)
+        self.flags(host='host2')
+        self.compute2 = self.start_service('compute', host='host2')
+
         server_req_body = {
             'server': {
                 # We hard-code from a fake image since we can't get images
@@ -1087,6 +1103,11 @@ class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase,
         }
         server = self.api.post_server(server_req_body)
         self._wait_for_state_change(self.api, server, 'ACTIVE')
+
+        # Disable the host we're on so ComputeFilter would have ruled it out
+        # normally
+        self._disable_compute_for(server)
+
         # 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.

nova/tests/unit/compute/test_compute_api.py

@@ -3218,6 +3218,7 @@ class _ComputeAPIUnitTestMixIn(object):
                 system_metadata=orig_system_metadata,
                 expected_attrs=['system_metadata'],
                 image_ref=orig_image_href,
+                node='node',
                 vm_mode=fields_obj.VMMode.HVM)
         flavor = instance.get_flavor()
 
@@ -3229,7 +3230,7 @@ class _ComputeAPIUnitTestMixIn(object):
         _get_image.side_effect = get_image
         bdm_get_by_instance_uuid.return_value = bdms
 
-        fake_spec = objects.RequestSpec()
+        fake_spec = objects.RequestSpec(id=1)
         req_spec_get_by_inst_uuid.return_value = fake_spec
 
         with mock.patch.object(self.compute_api.compute_task_api,
@@ -3247,10 +3248,9 @@ class _ComputeAPIUnitTestMixIn(object):
             # 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)
+            self.assertIn('_nova_check_type', fake_spec.scheduler_hints)
+            self.assertEqual('rebuild',
+                             fake_spec.scheduler_hints['_nova_check_type'][0])
 
         _check_auto_disk_config.assert_called_once_with(image=new_image)
         _checks_for_create_and_rebuild.assert_called_once_with(self.context,

releasenotes/notes/bug-1664931-refine-validate-image-rebuild-6d730042438eec10.yaml

@@ -0,0 +1,20 @@
+---
+fixes:
+  - |
+    The fix for `OSSA-2017-005`_ (CVE-2017-16239) was too far-reaching in that
+    rebuilds can now fail based on scheduling filters that should not apply
+    to rebuild. For example, a rebuild of an instance on a disabled compute
+    host could fail whereas it would not before the fix for CVE-2017-16239.
+    Similarly, rebuilding an instance on a host that is at capacity for vcpu,
+    memory or disk could fail since the scheduler filters would treat it as a
+    new build request even though the rebuild is not claiming *new* resources.
+
+    Therefore this release contains a fix for those regressions in scheduling
+    behavior on rebuild while maintaining the original fix for CVE-2017-16239.
+
+    .. note:: The fix relies on a ``RUN_ON_REBUILD`` variable which is checked
+              for all scheduler filters during a rebuild. The reasoning behind
+              the value for that variable depends on each filter. If you have
+              out-of-tree scheduler filters, you will likely need to assess
+              whether or not they need to override the default value (False)
+              for the new variable.