From 66d0cb1ce5c9d716ad93685ab6e6e86ddbd0b293 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Thu, 27 Dec 2012 16:15:12 +1100 Subject: Invalid EC2 ids should make the entire request fail. Resolves bug 836978. I suspect there are many other cases where we should add this check. Change-Id: I027e44db2e27eb1ef913ddad8560cca08388906b --- nova/api/ec2/cloud.py | 44 +++++++++++++++++++++++++++++++--------- nova/tests/api/ec2/test_cloud.py | 19 +++++++++++++++++ 2 files changed, 53 insertions(+), 10 deletions(-) (limited to 'nova') diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index 156042833..d40f25c4d 100644 --- a/nova/api/ec2/cloud.py +++ b/nova/api/ec2/cloud.py @@ -219,6 +219,17 @@ class CloudController(object): def __str__(self): return 'CloudController' + def _enforce_valid_instance_ids(self, context, instance_ids): + # NOTE(mikal): Amazon's implementation of the EC2 API requires that + # _all_ instance ids passed in be valid. + instances = {} + if instance_ids: + for ec2_id in instance_ids: + instance_uuid = ec2utils.ec2_inst_id_to_uuid(context, ec2_id) + instance = self.compute_api.get(context, instance_uuid) + instances[ec2_id] = instance + return instances + def _get_image_state(self, image): # NOTE(vish): fallback status if image_state isn't set state = image.get('status') @@ -977,14 +988,19 @@ class CloudController(object): def describe_instances(self, context, **kwargs): # Optional DescribeInstances argument instance_id = kwargs.get('instance_id', None) + instances = self._enforce_valid_instance_ids(context, instance_id) return self._format_describe_instances(context, - instance_id=instance_id) + instance_id=instance_id, + instance_cache=instances) def describe_instances_v6(self, context, **kwargs): # Optional DescribeInstancesV6 argument instance_id = kwargs.get('instance_id', None) + instances = self._enforce_valid_instance_ids(context, instance_id) return self._format_describe_instances(context, - instance_id=instance_id, use_v6=True) + instance_id=instance_id, + instance_cache=instances, + use_v6=True) def _format_describe_instances(self, context, **kwargs): return {'reservationSet': self._format_instances(context, **kwargs)} @@ -1066,23 +1082,30 @@ class CloudController(object): security_group_names, 'groupId') def _format_instances(self, context, instance_id=None, use_v6=False, - **search_opts): + instances_cache=None, **search_opts): # TODO(termie): this method is poorly named as its name does not imply # that it will be making a variety of database calls # rather than simply formatting a bunch of instances that # were handed to it reservations = {} + + if not instances_cache: + instances_cache = {} + # NOTE(vish): instance_id is an optional list of ids to filter by if instance_id: instances = [] for ec2_id in instance_id: - try: - instance_uuid = ec2utils.ec2_inst_id_to_uuid(context, - ec2_id) - instance = self.compute_api.get(context, instance_uuid) - except exception.NotFound: - continue - instances.append(instance) + if ec2_id in instances_cache: + instances.append(instances_cache[ec2_id]) + else: + try: + instance_uuid = ec2utils.ec2_inst_id_to_uuid(context, + ec2_id) + instance = self.compute_api.get(context, instance_uuid) + except exception.NotFound: + continue + instances.append(instance) else: try: # always filter out deleted instances @@ -1092,6 +1115,7 @@ class CloudController(object): sort_dir='asc') except exception.NotFound: instances = [] + for instance in instances: if not context.is_admin: if instance['image_ref'] == str(CONF.vpn_image_id): diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py index 831143326..2d2ecec9e 100644 --- a/nova/tests/api/ec2/test_cloud.py +++ b/nova/tests/api/ec2/test_cloud.py @@ -781,11 +781,30 @@ class CloudTestCase(test.TestCase): self.assertEqual(instance['privateIpAddress'], '192.168.0.3') self.assertEqual(instance['dnsNameV6'], 'fe80:b33f::a8bb:ccff:fedd:eeff') + + # A filter with even one invalid id should cause an exception to be + # raised + self.assertRaises(exception.InstanceNotFound, + self.cloud.describe_instances, self.context, + instance_id=[instance_id, '435679']) + db.instance_destroy(self.context, inst1['uuid']) db.instance_destroy(self.context, inst2['uuid']) db.service_destroy(self.context, comp1['id']) db.service_destroy(self.context, comp2['id']) + def test_describe_instances_all_invalid(self): + """Makes sure describe_instances works and filters results.""" + self.flags(use_ipv6=True) + + self._stub_instance_get_with_fixed_ips('get_all') + self._stub_instance_get_with_fixed_ips('get') + + instance_id = ec2utils.id_to_ec2_inst_id('435679') + self.assertRaises(exception.InstanceNotFound, + self.cloud.describe_instances, self.context, + instance_id=[instance_id]) + def test_describe_instances_sorting(self): """Makes sure describe_instances works and is sorted as expected.""" self.flags(use_ipv6=True) -- cgit