From 748b29ef805179351a1dbe150205fc53838958fb Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 23 Jun 2017 18:06:24 +0000 Subject: [PATCH] Make If-None-Match:* work properly with 0-byte PUTs When PUTting an object with `If-None-Match: *`, we rely 100-continue support: the proxy checks the responses from all object-servers, and if any of them respond 412, it closes down the connections. When there's actual data for the object, this ensures that even nodes that *don't* respond 412 will hit a ChunkReadTimeout and abort the PUT. However, if the client does a PUT with a Content-Length of 0, that would get sent all the way to the object server, which had all the information it needed to respond 201. After replication, the PUT propagates to the other nodes and the old object is lost, despite the client receiving a 412 indicating the operation failed. Now, when PUTting a zero-byte object, switch to a chunked transfer so the object-server still gets a ChunkReadTimeout. Change-Id: Ie88e41aca2d59246c3134d743c1531c8e996f9e4 --- swift/proxy/controllers/obj.py | 12 ++- .../probe/test_object_conditional_requests.py | 79 +++++++++++++++++++ test/unit/proxy/controllers/test_obj.py | 4 +- test/unit/proxy/test_server.py | 9 ++- 4 files changed, 95 insertions(+), 9 deletions(-) create mode 100644 test/probe/test_object_conditional_requests.py diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index 6bec505f32..5e0619723d 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -633,9 +633,12 @@ class BaseObjectController(Controller): pile = GreenPile(len(nodes)) for nheaders in outgoing_headers: - # RFC2616:8.2.3 disallows 100-continue without a body - if (req.content_length > 0) or req.is_chunked: - nheaders['Expect'] = '100-continue' + # RFC2616:8.2.3 disallows 100-continue without a body, + # so switch to chunked request + if nheaders.get('Content-Length') == '0': + nheaders['Transfer-Encoding'] = 'chunked' + del nheaders['Content-Length'] + nheaders['Expect'] = '100-continue' pile.spawn(self._connect_put_node, node_iter, partition, req, nheaders, self.app.logger.thread_locals) @@ -875,12 +878,13 @@ class ReplicatedObjectController(BaseObjectController): logger=self.app.logger, need_multiphase=False) else: + te = ',' + headers.get('Transfer-Encoding', '') putter = Putter.connect( node, part, req.swift_entity_path, headers, conn_timeout=self.app.conn_timeout, node_timeout=self.app.node_timeout, logger=self.app.logger, - chunked=req.is_chunked) + chunked=te.endswith(',chunked')) return putter def _transfer_data(self, req, data_source, putters, nodes): diff --git a/test/probe/test_object_conditional_requests.py b/test/probe/test_object_conditional_requests.py new file mode 100644 index 0000000000..29021ba681 --- /dev/null +++ b/test/probe/test_object_conditional_requests.py @@ -0,0 +1,79 @@ +# Copyright (c) 2017 OpenStack Foundation +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import uuid + +from swift.common.manager import Manager +from swiftclient import client + +from test.probe.brain import BrainSplitter +from test.probe.common import ReplProbeTest + + +def chunker(body): + '''Helper to ensure swiftclient sends a chunked request.''' + yield body + + +class TestPutIfNoneMatchRepl(ReplProbeTest): + def setUp(self): + super(TestPutIfNoneMatchRepl, self).setUp() + self.container_name = 'container-%s' % uuid.uuid4() + self.object_name = 'object-%s' % uuid.uuid4() + self.brain = BrainSplitter(self.url, self.token, self.container_name, + self.object_name, 'object', + policy=self.policy) + + def _do_test(self, overwrite_contents): + self.brain.put_container() + self.brain.stop_primary_half() + # put object to only 1 of 3 primaries + self.brain.put_object(contents='VERIFY') + self.brain.start_primary_half() + + # Restart services and attempt to overwrite + with self.assertRaises(client.ClientException) as exc_mgr: + self.brain.put_object(headers={'If-None-Match': '*'}, + contents=overwrite_contents) + self.assertEqual(exc_mgr.exception.http_status, 412) + + # make sure we're GETting from the servers that missed the original PUT + self.brain.stop_handoff_half() + + # verify the PUT did not complete + with self.assertRaises(client.ClientException) as exc_mgr: + client.get_object( + self.url, self.token, self.container_name, self.object_name) + self.assertEqual(exc_mgr.exception.http_status, 404) + + # for completeness, run replicators... + Manager(['object-replicator']).once() + + # ...and verify the object was not overwritten + _headers, body = client.get_object( + self.url, self.token, self.container_name, self.object_name) + self.assertEqual(body, 'VERIFY') + + def test_content_length_nonzero(self): + self._do_test('OVERWRITE') + + def test_content_length_zero(self): + self._do_test('') + + def test_chunked(self): + self._do_test(chunker('OVERWRITE')) + + def test_chunked_empty(self): + self._do_test(chunker('')) diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py index 0efca8f488..77e931670a 100644 --- a/test/unit/proxy/controllers/test_obj.py +++ b/test/unit/proxy/controllers/test_obj.py @@ -1083,13 +1083,13 @@ class TestReplicatedObjController(BaseObjectControllerMixin, for connection_id, info in put_requests.items(): body = ''.join(info['chunks']) headers = info['headers'] - if chunked: + if chunked or not test_body: body = unchunk_body(body) self.assertEqual('100-continue', headers['Expect']) self.assertEqual('chunked', headers['Transfer-Encoding']) else: self.assertNotIn('Transfer-Encoding', headers) - if body: + if body or not test_body: self.assertEqual('100-continue', headers['Expect']) else: self.assertNotIn('Expect', headers) diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index bee74c380a..3867ee2389 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -2584,9 +2584,12 @@ class TestReplicatedObjectController( def test_connect(ipaddr, port, device, partition, method, path, headers=None, query_string=None): if path == '/a/c/o.jpg': - if 'expect' in headers or 'Expect' in headers: - test_errors.append('Expect was in headers for object ' - 'server!') + if headers.get('Transfer-Encoding') != 'chunked': + test_errors.append('"Transfer-Encoding: chunked" should ' + 'be in headers for object server!') + if 'Expect' not in headers: + test_errors.append('Expect should be in headers for ' + 'object server!') with save_globals(): controller = ReplicatedObjectController(