|
1 commit 1f4ec235cdfd8c868f2d6458532f9dc32c00b8ca |
|
2 Author: Peter Portante <[email protected]> |
|
3 Date: Fri Jul 26 15:03:34 2013 -0400 |
|
4 |
|
5 Fix handling of DELETE obj reqs with old timestamp |
|
6 |
|
7 The DELETE object REST API was creating tombstone files with old |
|
8 timestamps, potentially filling up the disk, as well as sending |
|
9 container updates. |
|
10 |
|
11 Here we now make DELETEs with a request timestamp return a 409 (HTTP |
|
12 Conflict) if a data file exists with a newer timestamp, only creating |
|
13 tombstones if they have a newer timestamp. |
|
14 |
|
15 The key fix is to actually read the timestamp metadata from an |
|
16 existing tombstone file (thanks to Pete Zaitcev for catching this), |
|
17 and then only create tombstone files with newer timestamps. |
|
18 |
|
19 We also prevent PUT and POST operations using old timestamps as well. |
|
20 |
|
21 Change-Id: I631957029d17c6578bca5779367df5144ba01fc9 |
|
22 Signed-off-by: Peter Portante <[email protected]> |
|
23 |
|
24 --- a/swift/obj/server.py |
|
25 +++ b/swift/obj/server.py |
|
26 @@ -46,7 +46,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPCreated, \ |
|
27 HTTPInternalServerError, HTTPNoContent, HTTPNotFound, HTTPNotModified, \ |
|
28 HTTPPreconditionFailed, HTTPRequestTimeout, HTTPUnprocessableEntity, \ |
|
29 HTTPClientDisconnect, HTTPMethodNotAllowed, Request, Response, UTC, \ |
|
30 - HTTPInsufficientStorage, multi_range_iterator |
|
31 + HTTPInsufficientStorage, multi_range_iterator, HTTPConflict |
|
32 |
|
33 |
|
34 DATADIR = 'objects' |
|
35 @@ -121,7 +121,6 @@ class DiskFile(object): |
|
36 self.tmppath = None |
|
37 self.logger = logger |
|
38 self.metadata = {} |
|
39 - self.meta_file = None |
|
40 self.data_file = None |
|
41 self.fp = None |
|
42 self.iter_etag = None |
|
43 @@ -133,15 +132,18 @@ class DiskFile(object): |
|
44 if not os.path.exists(self.datadir): |
|
45 return |
|
46 files = sorted(os.listdir(self.datadir), reverse=True) |
|
47 - for file in files: |
|
48 - if file.endswith('.ts'): |
|
49 - self.data_file = self.meta_file = None |
|
50 - self.metadata = {'deleted': True} |
|
51 - return |
|
52 - if file.endswith('.meta') and not self.meta_file: |
|
53 - self.meta_file = os.path.join(self.datadir, file) |
|
54 - if file.endswith('.data') and not self.data_file: |
|
55 - self.data_file = os.path.join(self.datadir, file) |
|
56 + meta_file = None |
|
57 + for afile in files: |
|
58 + if afile.endswith('.ts'): |
|
59 + self.data_file = None |
|
60 + with open(os.path.join(self.datadir, afile)) as mfp: |
|
61 + self.metadata = read_metadata(mfp) |
|
62 + self.metadata['deleted'] = True |
|
63 + break |
|
64 + if afile.endswith('.meta') and not meta_file: |
|
65 + meta_file = os.path.join(self.datadir, afile) |
|
66 + if afile.endswith('.data') and not self.data_file: |
|
67 + self.data_file = os.path.join(self.datadir, afile) |
|
68 break |
|
69 if not self.data_file: |
|
70 return |
|
71 @@ -149,8 +151,8 @@ class DiskFile(object): |
|
72 self.metadata = read_metadata(self.fp) |
|
73 if not keep_data_fp: |
|
74 self.close(verify_file=False) |
|
75 - if self.meta_file: |
|
76 - with open(self.meta_file) as mfp: |
|
77 + if meta_file: |
|
78 + with open(meta_file) as mfp: |
|
79 for key in self.metadata.keys(): |
|
80 if key.lower() not in DISALLOWED_HEADERS: |
|
81 del self.metadata[key] |
|
82 @@ -594,6 +596,9 @@ class ObjectController(object): |
|
83 except (DiskFileError, DiskFileNotExist): |
|
84 file.quarantine() |
|
85 return HTTPNotFound(request=request) |
|
86 + orig_timestamp = file.metadata.get('X-Timestamp', '0') |
|
87 + if orig_timestamp >= request.headers['x-timestamp']: |
|
88 + return HTTPConflict(request=request) |
|
89 metadata = {'X-Timestamp': request.headers['x-timestamp']} |
|
90 metadata.update(val for val in request.headers.iteritems() |
|
91 if val[0].lower().startswith('x-object-meta-')) |
|
92 @@ -639,6 +644,8 @@ class ObjectController(object): |
|
93 file = DiskFile(self.devices, device, partition, account, container, |
|
94 obj, self.logger, disk_chunk_size=self.disk_chunk_size) |
|
95 orig_timestamp = file.metadata.get('X-Timestamp') |
|
96 + if orig_timestamp and orig_timestamp >= request.headers['x-timestamp']: |
|
97 + return HTTPConflict(request=request) |
|
98 upload_expiration = time.time() + self.max_upload_time |
|
99 etag = md5() |
|
100 upload_size = 0 |
|
101 @@ -863,23 +870,26 @@ class ObjectController(object): |
|
102 return HTTPPreconditionFailed( |
|
103 request=request, |
|
104 body='X-If-Delete-At and X-Delete-At do not match') |
|
105 - orig_timestamp = file.metadata.get('X-Timestamp') |
|
106 - if file.is_deleted() or file.is_expired(): |
|
107 - response_class = HTTPNotFound |
|
108 - metadata = { |
|
109 - 'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True, |
|
110 - } |
|
111 old_delete_at = int(file.metadata.get('X-Delete-At') or 0) |
|
112 if old_delete_at: |
|
113 self.delete_at_update('DELETE', old_delete_at, account, |
|
114 container, obj, request.headers, device) |
|
115 - file.put_metadata(metadata, tombstone=True) |
|
116 - file.unlinkold(metadata['X-Timestamp']) |
|
117 - if not orig_timestamp or \ |
|
118 - orig_timestamp < request.headers['x-timestamp']: |
|
119 + orig_timestamp = file.metadata.get('X-Timestamp', 0) |
|
120 + req_timestamp = request.headers['X-Timestamp'] |
|
121 + if file.is_deleted() or file.is_expired(): |
|
122 + response_class = HTTPNotFound |
|
123 + else: |
|
124 + if orig_timestamp < req_timestamp: |
|
125 + response_class = HTTPNoContent |
|
126 + else: |
|
127 + response_class = HTTPConflict |
|
128 + if orig_timestamp < req_timestamp: |
|
129 + file.put_metadata({'X-Timestamp': req_timestamp}, |
|
130 + tombstone=True) |
|
131 + file.unlinkold(req_timestamp) |
|
132 self.container_update( |
|
133 'DELETE', account, container, obj, request.headers, |
|
134 - {'x-timestamp': metadata['X-Timestamp'], |
|
135 + {'x-timestamp': req_timestamp, |
|
136 'x-trans-id': request.headers.get('x-trans-id', '-')}, |
|
137 device) |
|
138 resp = response_class(request=request) |
|
139 |
|
140 |
|
141 diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py |
|
142 |
|
143 index 8ee266b..b354b97 100755 (executable) |
|
144 |
|
145 |
|
146 --- a/test/unit/obj/test_server.py |
|
147 +++ b/test/unit/obj/test_server.py |
|
148 @@ -509,6 +509,41 @@ class TestObjectController(unittest.TestCase): |
|
149 "X-Object-Meta-3" in resp.headers) |
|
150 self.assertEquals(resp.headers['Content-Type'], 'application/x-test') |
|
151 |
|
152 + def test_POST_old_timestamp(self): |
|
153 + ts = time() |
|
154 + timestamp = normalize_timestamp(ts) |
|
155 + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
156 + headers={'X-Timestamp': timestamp, |
|
157 + 'Content-Type': 'application/x-test', |
|
158 + 'X-Object-Meta-1': 'One', |
|
159 + 'X-Object-Meta-Two': 'Two'}) |
|
160 + req.body = 'VERIFY' |
|
161 + resp = self.object_controller.PUT(req) |
|
162 + self.assertEquals(resp.status_int, 201) |
|
163 + |
|
164 + # Same timestamp should result in 409 |
|
165 + req = Request.blank('/sda1/p/a/c/o', |
|
166 + environ={'REQUEST_METHOD': 'POST'}, |
|
167 + headers={'X-Timestamp': timestamp, |
|
168 + 'X-Object-Meta-3': 'Three', |
|
169 + 'X-Object-Meta-4': 'Four', |
|
170 + 'Content-Encoding': 'gzip', |
|
171 + 'Content-Type': 'application/x-test'}) |
|
172 + resp = self.object_controller.POST(req) |
|
173 + self.assertEquals(resp.status_int, 409) |
|
174 + |
|
175 + # Earlier timestamp should result in 409 |
|
176 + timestamp = normalize_timestamp(ts - 1) |
|
177 + req = Request.blank('/sda1/p/a/c/o', |
|
178 + environ={'REQUEST_METHOD': 'POST'}, |
|
179 + headers={'X-Timestamp': timestamp, |
|
180 + 'X-Object-Meta-5': 'Five', |
|
181 + 'X-Object-Meta-6': 'Six', |
|
182 + 'Content-Encoding': 'gzip', |
|
183 + 'Content-Type': 'application/x-test'}) |
|
184 + resp = self.object_controller.POST(req) |
|
185 + self.assertEquals(resp.status_int, 409) |
|
186 + |
|
187 def test_POST_not_exist(self): |
|
188 timestamp = normalize_timestamp(time()) |
|
189 req = Request.blank('/sda1/p/a/c/fail', |
|
190 @@ -555,11 +590,15 @@ class TestObjectController(unittest.TestCase): |
|
191 |
|
192 old_http_connect = object_server.http_connect |
|
193 try: |
|
194 - timestamp = normalize_timestamp(time()) |
|
195 + ts = time() |
|
196 + timestamp = normalize_timestamp(ts) |
|
197 req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': |
|
198 'POST'}, headers={'X-Timestamp': timestamp, 'Content-Type': |
|
199 'text/plain', 'Content-Length': '0'}) |
|
200 resp = self.object_controller.PUT(req) |
|
201 + self.assertEquals(resp.status_int, 201) |
|
202 + |
|
203 + timestamp = normalize_timestamp(ts + 1) |
|
204 req = Request.blank('/sda1/p/a/c/o', |
|
205 environ={'REQUEST_METHOD': 'POST'}, |
|
206 headers={'X-Timestamp': timestamp, |
|
207 @@ -571,6 +610,8 @@ class TestObjectController(unittest.TestCase): |
|
208 object_server.http_connect = mock_http_connect(202) |
|
209 resp = self.object_controller.POST(req) |
|
210 self.assertEquals(resp.status_int, 202) |
|
211 + |
|
212 + timestamp = normalize_timestamp(ts + 2) |
|
213 req = Request.blank('/sda1/p/a/c/o', |
|
214 environ={'REQUEST_METHOD': 'POST'}, |
|
215 headers={'X-Timestamp': timestamp, |
|
216 @@ -582,6 +623,8 @@ class TestObjectController(unittest.TestCase): |
|
217 object_server.http_connect = mock_http_connect(202, with_exc=True) |
|
218 resp = self.object_controller.POST(req) |
|
219 self.assertEquals(resp.status_int, 202) |
|
220 + |
|
221 + timestamp = normalize_timestamp(ts + 3) |
|
222 req = Request.blank('/sda1/p/a/c/o', |
|
223 environ={'REQUEST_METHOD': 'POST'}, |
|
224 headers={'X-Timestamp': timestamp, |
|
225 @@ -718,6 +761,32 @@ class TestObjectController(unittest.TestCase): |
|
226 'name': '/a/c/o', |
|
227 'Content-Encoding': 'gzip'}) |
|
228 |
|
229 + def test_PUT_old_timestamp(self): |
|
230 + ts = time() |
|
231 + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
232 + headers={'X-Timestamp': normalize_timestamp(ts), |
|
233 + 'Content-Length': '6', |
|
234 + 'Content-Type': 'application/octet-stream'}) |
|
235 + req.body = 'VERIFY' |
|
236 + resp = self.object_controller.PUT(req) |
|
237 + self.assertEquals(resp.status_int, 201) |
|
238 + |
|
239 + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
240 + headers={'X-Timestamp': normalize_timestamp(ts), |
|
241 + 'Content-Type': 'text/plain', |
|
242 + 'Content-Encoding': 'gzip'}) |
|
243 + req.body = 'VERIFY TWO' |
|
244 + resp = self.object_controller.PUT(req) |
|
245 + self.assertEquals(resp.status_int, 409) |
|
246 + |
|
247 + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
248 + headers={'X-Timestamp': normalize_timestamp(ts - 1), |
|
249 + 'Content-Type': 'text/plain', |
|
250 + 'Content-Encoding': 'gzip'}) |
|
251 + req.body = 'VERIFY THREE' |
|
252 + resp = self.object_controller.PUT(req) |
|
253 + self.assertEquals(resp.status_int, 409) |
|
254 + |
|
255 def test_PUT_no_etag(self): |
|
256 req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
257 headers={'X-Timestamp': normalize_timestamp(time()), |
|
258 @@ -1306,12 +1375,32 @@ class TestObjectController(unittest.TestCase): |
|
259 self.assertEquals(resp.status_int, 400) |
|
260 # self.assertRaises(KeyError, self.object_controller.DELETE, req) |
|
261 |
|
262 + # The following should have created a tombstone file |
|
263 timestamp = normalize_timestamp(time()) |
|
264 req = Request.blank('/sda1/p/a/c/o', |
|
265 environ={'REQUEST_METHOD': 'DELETE'}, |
|
266 headers={'X-Timestamp': timestamp}) |
|
267 resp = self.object_controller.DELETE(req) |
|
268 self.assertEquals(resp.status_int, 404) |
|
269 + objfile = os.path.join(self.testdir, 'sda1', |
|
270 + storage_directory(object_server.DATADIR, 'p', |
|
271 + hash_path('a', 'c', 'o')), |
|
272 + timestamp + '.ts') |
|
273 + self.assert_(os.path.isfile(objfile)) |
|
274 + |
|
275 + # The following should *not* have created a tombstone file. |
|
276 + timestamp = normalize_timestamp(float(timestamp) - 1) |
|
277 + req = Request.blank('/sda1/p/a/c/o', |
|
278 + environ={'REQUEST_METHOD': 'DELETE'}, |
|
279 + headers={'X-Timestamp': timestamp}) |
|
280 + resp = self.object_controller.DELETE(req) |
|
281 + self.assertEquals(resp.status_int, 404) |
|
282 + objfile = os.path.join(self.testdir, 'sda1', |
|
283 + storage_directory(object_server.DATADIR, 'p', |
|
284 + hash_path('a', 'c', 'o')), |
|
285 + timestamp + '.ts') |
|
286 + self.assertFalse(os.path.exists(objfile)) |
|
287 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
288 |
|
289 sleep(.00001) |
|
290 timestamp = normalize_timestamp(time()) |
|
291 @@ -1325,17 +1414,19 @@ class TestObjectController(unittest.TestCase): |
|
292 resp = self.object_controller.PUT(req) |
|
293 self.assertEquals(resp.status_int, 201) |
|
294 |
|
295 + # The following should *not* have created a tombstone file. |
|
296 timestamp = normalize_timestamp(float(timestamp) - 1) |
|
297 req = Request.blank('/sda1/p/a/c/o', |
|
298 environ={'REQUEST_METHOD': 'DELETE'}, |
|
299 headers={'X-Timestamp': timestamp}) |
|
300 resp = self.object_controller.DELETE(req) |
|
301 - self.assertEquals(resp.status_int, 204) |
|
302 + self.assertEquals(resp.status_int, 409) |
|
303 objfile = os.path.join(self.testdir, 'sda1', |
|
304 storage_directory(object_server.DATADIR, 'p', |
|
305 hash_path('a', 'c', 'o')), |
|
306 timestamp + '.ts') |
|
307 - self.assert_(os.path.isfile(objfile)) |
|
308 + self.assertFalse(os.path.exists(objfile)) |
|
309 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
310 |
|
311 sleep(.00001) |
|
312 timestamp = normalize_timestamp(time()) |
|
313 @@ -1350,6 +1441,103 @@ class TestObjectController(unittest.TestCase): |
|
314 timestamp + '.ts') |
|
315 self.assert_(os.path.isfile(objfile)) |
|
316 |
|
317 + def test_DELETE_container_updates(self): |
|
318 + # Test swift.object_server.ObjectController.DELETE and container |
|
319 + # updates, making sure container update is called in the correct |
|
320 + # state. |
|
321 + timestamp = normalize_timestamp(time()) |
|
322 + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, |
|
323 + headers={ |
|
324 + 'X-Timestamp': timestamp, |
|
325 + 'Content-Type': 'application/octet-stream', |
|
326 + 'Content-Length': '4', |
|
327 + }) |
|
328 + req.body = 'test' |
|
329 + resp = self.object_controller.PUT(req) |
|
330 + self.assertEquals(resp.status_int, 201) |
|
331 + |
|
332 + calls_made = [0] |
|
333 + |
|
334 + def our_container_update(*args, **kwargs): |
|
335 + calls_made[0] += 1 |
|
336 + |
|
337 + orig_cu = self.object_controller.container_update |
|
338 + self.object_controller.container_update = our_container_update |
|
339 + try: |
|
340 + # The following request should return 409 (HTTP Conflict). A |
|
341 + # tombstone file should not have been created with this timestamp. |
|
342 + timestamp = normalize_timestamp(float(timestamp) - 1) |
|
343 + req = Request.blank('/sda1/p/a/c/o', |
|
344 + environ={'REQUEST_METHOD': 'DELETE'}, |
|
345 + headers={'X-Timestamp': timestamp}) |
|
346 + resp = self.object_controller.DELETE(req) |
|
347 + self.assertEquals(resp.status_int, 409) |
|
348 + objfile = os.path.join(self.testdir, 'sda1', |
|
349 + storage_directory(object_server.DATADIR, 'p', |
|
350 + hash_path('a', 'c', 'o')), |
|
351 + timestamp + '.ts') |
|
352 + self.assertFalse(os.path.isfile(objfile)) |
|
353 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
354 + self.assertEquals(0, calls_made[0]) |
|
355 + |
|
356 + # The following request should return 204, and the object should |
|
357 + # be truly deleted (container update is performed) because this |
|
358 + # timestamp is newer. A tombstone file should have been created |
|
359 + # with this timestamp. |
|
360 + sleep(.00001) |
|
361 + timestamp = normalize_timestamp(time()) |
|
362 + req = Request.blank('/sda1/p/a/c/o', |
|
363 + environ={'REQUEST_METHOD': 'DELETE'}, |
|
364 + headers={'X-Timestamp': timestamp}) |
|
365 + resp = self.object_controller.DELETE(req) |
|
366 + self.assertEquals(resp.status_int, 204) |
|
367 + objfile = os.path.join(self.testdir, 'sda1', |
|
368 + storage_directory(object_server.DATADIR, 'p', |
|
369 + hash_path('a', 'c', 'o')), |
|
370 + timestamp + '.ts') |
|
371 + self.assert_(os.path.isfile(objfile)) |
|
372 + self.assertEquals(1, calls_made[0]) |
|
373 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
374 + |
|
375 + # The following request should return a 404, as the object should |
|
376 + # already have been deleted, but it should have also performed a |
|
377 + # container update because the timestamp is newer, and a tombstone |
|
378 + # file should also exist with this timestamp. |
|
379 + sleep(.00001) |
|
380 + timestamp = normalize_timestamp(time()) |
|
381 + req = Request.blank('/sda1/p/a/c/o', |
|
382 + environ={'REQUEST_METHOD': 'DELETE'}, |
|
383 + headers={'X-Timestamp': timestamp}) |
|
384 + resp = self.object_controller.DELETE(req) |
|
385 + self.assertEquals(resp.status_int, 404) |
|
386 + objfile = os.path.join(self.testdir, 'sda1', |
|
387 + storage_directory(object_server.DATADIR, 'p', |
|
388 + hash_path('a', 'c', 'o')), |
|
389 + timestamp + '.ts') |
|
390 + self.assert_(os.path.isfile(objfile)) |
|
391 + self.assertEquals(2, calls_made[0]) |
|
392 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
393 + |
|
394 + # The following request should return a 404, as the object should |
|
395 + # already have been deleted, and it should not have performed a |
|
396 + # container update because the timestamp is older, or created a |
|
397 + # tombstone file with this timestamp. |
|
398 + timestamp = normalize_timestamp(float(timestamp) - 1) |
|
399 + req = Request.blank('/sda1/p/a/c/o', |
|
400 + environ={'REQUEST_METHOD': 'DELETE'}, |
|
401 + headers={'X-Timestamp': timestamp}) |
|
402 + resp = self.object_controller.DELETE(req) |
|
403 + self.assertEquals(resp.status_int, 404) |
|
404 + objfile = os.path.join(self.testdir, 'sda1', |
|
405 + storage_directory(object_server.DATADIR, 'p', |
|
406 + hash_path('a', 'c', 'o')), |
|
407 + timestamp + '.ts') |
|
408 + self.assertFalse(os.path.isfile(objfile)) |
|
409 + self.assertEquals(2, calls_made[0]) |
|
410 + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) |
|
411 + finally: |
|
412 + self.object_controller.container_update = orig_cu |
|
413 + |
|
414 def test_call(self): |
|
415 """ Test swift.object_server.ObjectController.__call__ """ |
|
416 inbuf = StringIO() |