OpenStack Compute (nova)

Merge lp:~rackspace-titan/nova/servers-metadata-404-lp795169 into lp:~hudson-openstack/nova/trunk

  1. servers-metadata-404-lp795169
  2. Merge into trunk
Proposed by Brian Waldon
Status: Merged
Approved by: Dan Prince
Approved revision: 1187
Merged at revision: 1199
Proposed branch: lp:~rackspace-titan/nova/servers-metadata-404-lp795169
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 277 lines (+99/-10)
3 files modified
nova/api/openstack/server_metadata.py (+31/-7)
nova/db/sqlalchemy/api.py (+15/-1)
nova/tests/api/openstack/test_server_metadata.py (+53/-2)
To merge this branch: bzr merge lp:~rackspace-titan/nova/servers-metadata-404-lp795169
Reviewer Review Type Date Requested Status
Dan Prince (community) Approve
Matt Dietz (community) Approve
Devin Carlen (community) Approve
Review via email: mp+64740@code.launchpad.net

Description of the change

Check that server exists when interacting with /v1.1/servers/<id>/meta resource

To post a comment you must log in.
Revision history for this message
Devin Carlen (devcamcar) wrote :

lgtm

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

lgtm

review: Approve
Revision history for this message
Dan Prince (dan-prince) wrote :

Good catch Waldon. So what are your thoughts about raising an InstanceNotFound exception up from the nova/db/sqlalchemy/apy.py module? It seems like there is some precedence for that as being the place for this type of check.

review: Needs Information
Revision history for this message
Brian Waldon (bcwaldon) wrote :

I'll take a look at it, Dan. I had that same thought earlier.

Revision history for this message
Brian Waldon (bcwaldon) wrote :

I thought about it a bit more and came to the conclusion that the requirement for an instance to exist is defined in the API spec. I can think of a few cases where we may want to use the db-level functions and not worry about instance existence. Thoughts?

Revision history for this message
Matt Dietz (cerberus) wrote :

Intent looks good

I think I agree with Dan in this scenario: the precedent is for the check to fall to the database. I don't know that being defined in the API means that we need a check in the API, because I don't think there will ever be a case where the API would just "know" an instance does or does not exist. It will always be delegating to another entity. I think this really just introduces another method that might have to be maintained.

review: Needs Fixing
lp:~rackspace-titan/nova/servers-metadata-404-lp795169 updated
1187. By Brian Waldon

moving instance existance logic down to api layer

Revision history for this message
Brian Waldon (bcwaldon) wrote :

Okay, check it out now. I made a handy little decorator in db/sqlalchemy/api.py

Revision history for this message
Matt Dietz (cerberus) wrote :

I like it. Will wait for Dan before attempting to merge

review: Approve
Revision history for this message
Dan Prince (dan-prince) 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/openstack/server_metadata.py'
2--- nova/api/openstack/server_metadata.py 2011-06-13 12:53:34 +0000
3+++ nova/api/openstack/server_metadata.py 2011-06-17 21:43:23 +0000
4@@ -18,9 +18,10 @@
5 from webob import exc
6
7 from nova import compute
8-from nova import quota
9 from nova.api.openstack import faults
10 from nova.api.openstack import wsgi
11+from nova import exception
12+from nova import quota
13
14
15 class Controller(object):
16@@ -45,7 +46,11 @@
17 def index(self, req, server_id):
18 """ Returns the list of metadata for a given instance """
19 context = req.environ['nova.context']
20- return self._get_metadata(context, server_id)
21+ try:
22+ return self._get_metadata(context, server_id)
23+ except exception.InstanceNotFound:
24+ msg = _('Server %(server_id)s does not exist') % locals()
25+ raise exc.HTTPNotFound(explanation=msg)
26
27 def create(self, req, server_id, body):
28 self._check_body(body)
29@@ -55,8 +60,13 @@
30 self.compute_api.update_or_create_instance_metadata(context,
31 server_id,
32 metadata)
33+ except exception.InstanceNotFound:
34+ msg = _('Server %(server_id)s does not exist') % locals()
35+ raise exc.HTTPNotFound(explanation=msg)
36+
37 except quota.QuotaError as error:
38 self._handle_quota_error(error)
39+
40 return body
41
42 def update(self, req, server_id, id, body):
43@@ -72,6 +82,10 @@
44 self.compute_api.update_or_create_instance_metadata(context,
45 server_id,
46 body)
47+ except exception.InstanceNotFound:
48+ msg = _('Server %(server_id)s does not exist') % locals()
49+ raise exc.HTTPNotFound(explanation=msg)
50+
51 except quota.QuotaError as error:
52 self._handle_quota_error(error)
53
54@@ -80,16 +94,26 @@
55 def show(self, req, server_id, id):
56 """ Return a single metadata item """
57 context = req.environ['nova.context']
58- data = self._get_metadata(context, server_id)
59- if id in data['metadata']:
60+ try:
61+ data = self._get_metadata(context, server_id)
62+ except exception.InstanceNotFound:
63+ msg = _('Server %(server_id)s does not exist') % locals()
64+ raise exc.HTTPNotFound(explanation=msg)
65+
66+ try:
67 return {id: data['metadata'][id]}
68- else:
69- return faults.Fault(exc.HTTPNotFound())
70+ except KeyError:
71+ msg = _("metadata item %s was not found" % (id))
72+ raise exc.HTTPNotFound(explanation=msg)
73
74 def delete(self, req, server_id, id):
75 """ Deletes an existing metadata """
76 context = req.environ['nova.context']
77- self.compute_api.delete_instance_metadata(context, server_id, id)
78+ try:
79+ self.compute_api.delete_instance_metadata(context, server_id, id)
80+ except exception.InstanceNotFound:
81+ msg = _('Server %(server_id)s does not exist') % locals()
82+ raise exc.HTTPNotFound(explanation=msg)
83
84 def _handle_quota_error(self, error):
85 """Reraise quota errors as api-specific http exceptions."""
86
87=== modified file 'nova/db/sqlalchemy/api.py'
88--- nova/db/sqlalchemy/api.py 2011-06-15 21:35:31 +0000
89+++ nova/db/sqlalchemy/api.py 2011-06-17 21:43:23 +0000
90@@ -18,7 +18,7 @@
91 """
92 Implementation of SQLAlchemy backend.
93 """
94-
95+import traceback
96 import warnings
97
98 from nova import db
99@@ -2615,7 +2615,17 @@
100
101 ####################
102
103+
104+def require_instance_exists(func):
105+ def new_func(context, instance_id, *args, **kwargs):
106+ db.api.instance_get(context, instance_id)
107+ return func(context, instance_id, *args, **kwargs)
108+ new_func.__name__ = func.__name__
109+ return new_func
110+
111+
112 @require_context
113+@require_instance_exists
114 def instance_metadata_get(context, instance_id):
115 session = get_session()
116
117@@ -2631,6 +2641,7 @@
118
119
120 @require_context
121+@require_instance_exists
122 def instance_metadata_delete(context, instance_id, key):
123 session = get_session()
124 session.query(models.InstanceMetadata).\
125@@ -2643,6 +2654,7 @@
126
127
128 @require_context
129+@require_instance_exists
130 def instance_metadata_delete_all(context, instance_id):
131 session = get_session()
132 session.query(models.InstanceMetadata).\
133@@ -2654,6 +2666,7 @@
134
135
136 @require_context
137+@require_instance_exists
138 def instance_metadata_get_item(context, instance_id, key):
139 session = get_session()
140
141@@ -2670,6 +2683,7 @@
142
143
144 @require_context
145+@require_instance_exists
146 def instance_metadata_update_or_create(context, instance_id, metadata):
147 session = get_session()
148
149
150=== modified file 'nova/tests/api/openstack/test_server_metadata.py'
151--- nova/tests/api/openstack/test_server_metadata.py 2011-06-10 20:41:14 +0000
152+++ nova/tests/api/openstack/test_server_metadata.py 2011-06-17 21:43:23 +0000
153@@ -21,6 +21,7 @@
154 import webob
155
156
157+from nova import exception
158 from nova import flags
159 from nova.api import openstack
160 from nova.tests.api.openstack import fakes
161@@ -67,6 +68,14 @@
162 return metadata
163
164
165+def return_server(context, server_id):
166+ return {'id': server_id}
167+
168+
169+def return_server_nonexistant(context, server_id):
170+ raise exception.InstanceNotFound()
171+
172+
173 class ServerMetaDataTest(unittest.TestCase):
174
175 def setUp(self):
176@@ -76,6 +85,7 @@
177 fakes.FakeAuthDatabase.data = {}
178 fakes.stub_out_auth(self.stubs)
179 fakes.stub_out_key_pair_funcs(self.stubs)
180+ self.stubs.Set(nova.db.api, 'instance_get', return_server)
181
182 def tearDown(self):
183 self.stubs.UnsetAll()
184@@ -92,6 +102,13 @@
185 self.assertEqual('application/json', res.headers['Content-Type'])
186 self.assertEqual('value1', res_dict['metadata']['key1'])
187
188+ def test_index_nonexistant_server(self):
189+ self.stubs.Set(nova.db.api, 'instance_get', return_server_nonexistant)
190+ req = webob.Request.blank('/v1.1/servers/1/meta')
191+ req.environ['api.version'] = '1.1'
192+ res = req.get_response(fakes.wsgi_app())
193+ self.assertEqual(404, res.status_int)
194+
195 def test_index_no_data(self):
196 self.stubs.Set(nova.db.api, 'instance_metadata_get',
197 return_empty_server_metadata)
198@@ -114,13 +131,19 @@
199 self.assertEqual('application/json', res.headers['Content-Type'])
200 self.assertEqual('value5', res_dict['key5'])
201
202+ def test_show_nonexistant_server(self):
203+ self.stubs.Set(nova.db.api, 'instance_get', return_server_nonexistant)
204+ req = webob.Request.blank('/v1.1/servers/1/meta/key5')
205+ req.environ['api.version'] = '1.1'
206+ res = req.get_response(fakes.wsgi_app())
207+ self.assertEqual(404, res.status_int)
208+
209 def test_show_meta_not_found(self):
210 self.stubs.Set(nova.db.api, 'instance_metadata_get',
211 return_empty_server_metadata)
212 req = webob.Request.blank('/v1.1/servers/1/meta/key6')
213 req.environ['api.version'] = '1.1'
214 res = req.get_response(fakes.wsgi_app())
215- res_dict = json.loads(res.body)
216 self.assertEqual(404, res.status_int)
217
218 def test_delete(self):
219@@ -132,6 +155,14 @@
220 res = req.get_response(fakes.wsgi_app())
221 self.assertEqual(200, res.status_int)
222
223+ def test_delete_nonexistant_server(self):
224+ self.stubs.Set(nova.db.api, 'instance_get', return_server_nonexistant)
225+ req = webob.Request.blank('/v1.1/servers/1/meta/key5')
226+ req.environ['api.version'] = '1.1'
227+ req.method = 'DELETE'
228+ res = req.get_response(fakes.wsgi_app())
229+ self.assertEqual(404, res.status_int)
230+
231 def test_create(self):
232 self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
233 return_create_instance_metadata)
234@@ -141,8 +172,8 @@
235 req.body = '{"metadata": {"key1": "value1"}}'
236 req.headers["content-type"] = "application/json"
237 res = req.get_response(fakes.wsgi_app())
238+ self.assertEqual(200, res.status_int)
239 res_dict = json.loads(res.body)
240- self.assertEqual(200, res.status_int)
241 self.assertEqual('application/json', res.headers['Content-Type'])
242 self.assertEqual('value1', res_dict['metadata']['key1'])
243
244@@ -156,6 +187,16 @@
245 res = req.get_response(fakes.wsgi_app())
246 self.assertEqual(400, res.status_int)
247
248+ def test_create_nonexistant_server(self):
249+ self.stubs.Set(nova.db.api, 'instance_get', return_server_nonexistant)
250+ req = webob.Request.blank('/v1.1/servers/100/meta')
251+ req.environ['api.version'] = '1.1'
252+ req.method = 'POST'
253+ req.body = '{"metadata": {"key1": "value1"}}'
254+ req.headers["content-type"] = "application/json"
255+ res = req.get_response(fakes.wsgi_app())
256+ self.assertEqual(404, res.status_int)
257+
258 def test_update_item(self):
259 self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
260 return_create_instance_metadata)
261@@ -170,6 +211,16 @@
262 res_dict = json.loads(res.body)
263 self.assertEqual('value1', res_dict['key1'])
264
265+ def test_update_item_nonexistant_server(self):
266+ self.stubs.Set(nova.db.api, 'instance_get', return_server_nonexistant)
267+ req = webob.Request.blank('/v1.1/servers/asdf/100/key1')
268+ req.environ['api.version'] = '1.1'
269+ req.method = 'PUT'
270+ req.body = '{"key1": "value1"}'
271+ req.headers["content-type"] = "application/json"
272+ res = req.get_response(fakes.wsgi_app())
273+ self.assertEqual(404, res.status_int)
274+
275 def test_update_item_empty_body(self):
276 self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
277 return_create_instance_metadata)