OpenStack Compute (nova)

Merge lp:~jtran/nova/lp744519 into lp:~hudson-openstack/nova/trunk

  1. lp744519
  2. Merge into trunk
Proposed by John Tran
Status: Merged
Approved by: Devin Carlen
Approved revision: 1252
Merged at revision: 1255
Proposed branch: lp:~jtran/nova/lp744519
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 111 lines (+28/-8)
2 files modified
nova/api/ec2/cloud.py (+5/-1)
nova/tests/test_cloud.py (+23/-7)
To merge this branch: bzr merge lp:~jtran/nova/lp744519
Reviewer Review Type Date Requested Status
Devin Carlen (community) Approve
Vish Ishaya (community) Approve
Brian Waldon (community) Approve
Review via email: mp+67273@code.launchpad.net

Commit message

flaw in ec2 cloud api, _get_image method , if doing a search for aki-0000009, yet that image name doesn't exist, it strips off aki- and looks for any image_id 0000009 and if there was an image match that happens to be an ami instead of aki, it will go ahead and deregister the ami instead. That behavior is unintended, so added logic to ensure that the original request image_id matches the type of image being returned from database by matching against container_format attr

Description of the change

flaw in ec2 cloud api, _get_image method , if doing a search for aki-0000009, yet that image name doesn't exist, it strips off aki- and looks for any image_id 0000009 and if there was an image match that happens to be an ami instead of aki, it will go ahead and deregister the ami instead. That behavior is unintended, so added logic to ensure that the original request image_id matches the type of image being returned from database by matching against container_format attr

To post a comment you must log in.
Revision history for this message
Brian Waldon (bcwaldon) wrote :

Looks good, John.

review: Approve
Revision history for this message
Vish Ishaya (vishvananda) wrote :

Nice Fix.

Can you change:

6 + if image.get('container_format') != image_type:

to

6 + if self._image_type(image.get('container_format')) != image_type:

This method automatically converts unknown formats to ami, which is the same logic used to display unknown images in the ec2 api. This will allow you to properly deregister raw images, etc.

Vish

review: Needs Fixing
lp:~jtran/nova/lp744519 updated
1252. By John Tran

peer review fix - per vish: 'This method automatically converts unknown formats to ami, which is the same logic used to display unknown images in the ec2 api. This will allow you to properly deregister raw images, etc.'

Revision history for this message
Vish Ishaya (vishvananda) wrote :

great. LGTM

review: Approve
Revision history for this message
Devin Carlen (devcamcar) wrote :

lgtm

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/ec2/cloud.py'
2--- nova/api/ec2/cloud.py 2011-07-01 15:47:33 +0000
3+++ nova/api/ec2/cloud.py 2011-07-08 21:21:32 +0000
4@@ -1088,12 +1088,16 @@
5 def _get_image(self, context, ec2_id):
6 try:
7 internal_id = ec2utils.ec2_id_to_id(ec2_id)
8- return self.image_service.show(context, internal_id)
9+ image = self.image_service.show(context, internal_id)
10 except (exception.InvalidEc2Id, exception.ImageNotFound):
11 try:
12 return self.image_service.show_by_name(context, ec2_id)
13 except exception.NotFound:
14 raise exception.ImageNotFound(image_id=ec2_id)
15+ image_type = ec2_id.split('-')[0]
16+ if self._image_type(image.get('container_format')) != image_type:
17+ raise exception.ImageNotFound(image_id=ec2_id)
18+ return image
19
20 def _format_image(self, image):
21 """Convert from format defined by BaseImageService to S3 format."""
22
23=== modified file 'nova/tests/test_cloud.py'
24--- nova/tests/test_cloud.py 2011-07-01 15:47:33 +0000
25+++ nova/tests/test_cloud.py 2011-07-08 21:21:32 +0000
26@@ -67,7 +67,8 @@
27 host = self.network.host
28
29 def fake_show(meh, context, id):
30- return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
31+ return {'id': 1, 'container_format': 'ami',
32+ 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
33 'type': 'machine', 'image_state': 'available'}}
34
35 self.stubs.Set(fake._FakeImageService, 'show', fake_show)
36@@ -418,7 +419,8 @@
37 describe_images = self.cloud.describe_images
38
39 def fake_detail(meh, context):
40- return [{'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
41+ return [{'id': 1, 'container_format': 'ami',
42+ 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
43 'type': 'machine'}}]
44
45 def fake_show_none(meh, context, id):
46@@ -448,7 +450,8 @@
47
48 def fake_show(meh, context, id):
49 return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
50- 'type': 'machine'}, 'is_public': True}
51+ 'type': 'machine'}, 'container_format': 'ami',
52+ 'is_public': True}
53
54 self.stubs.Set(fake._FakeImageService, 'show', fake_show)
55 self.stubs.Set(fake._FakeImageService, 'show_by_name', fake_show)
56@@ -460,7 +463,8 @@
57 modify_image_attribute = self.cloud.modify_image_attribute
58
59 def fake_show(meh, context, id):
60- return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
61+ return {'id': 1, 'container_format': 'ami',
62+ 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
63 'type': 'machine'}, 'is_public': False}
64
65 def fake_update(meh, context, image_id, metadata, data=None):
66@@ -494,6 +498,16 @@
67 self.assertRaises(exception.ImageNotFound, deregister_image,
68 self.context, 'ami-bad001')
69
70+ def test_deregister_image_wrong_container_type(self):
71+ deregister_image = self.cloud.deregister_image
72+
73+ def fake_delete(self, context, id):
74+ return None
75+
76+ self.stubs.Set(fake._FakeImageService, 'delete', fake_delete)
77+ self.assertRaises(exception.NotFound, deregister_image, self.context,
78+ 'aki-00000001')
79+
80 def _run_instance(self, **kwargs):
81 rv = self.cloud.run_instances(self.context, **kwargs)
82 instance_id = rv['instancesSet'][0]['instanceId']
83@@ -609,7 +623,7 @@
84
85 def fake_show_no_state(self, context, id):
86 return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
87- 'type': 'machine'}}
88+ 'type': 'machine'}, 'container_format': 'ami'}
89
90 self.stubs.UnsetAll()
91 self.stubs.Set(fake._FakeImageService, 'show', fake_show_no_state)
92@@ -623,7 +637,8 @@
93 run_instances = self.cloud.run_instances
94
95 def fake_show_decrypt(self, context, id):
96- return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
97+ return {'id': 1, 'container_format': 'ami',
98+ 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
99 'type': 'machine', 'image_state': 'decrypting'}}
100
101 self.stubs.UnsetAll()
102@@ -638,7 +653,8 @@
103 run_instances = self.cloud.run_instances
104
105 def fake_show_stat_active(self, context, id):
106- return {'id': 1, 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
107+ return {'id': 1, 'container_format': 'ami',
108+ 'properties': {'kernel_id': 1, 'ramdisk_id': 1,
109 'type': 'machine'}, 'status': 'active'}
110
111 self.stubs.Set(fake._FakeImageService, 'show', fake_show_stat_active)