From 18670021b16bf787d79055b4bce7db5c553686c4 Mon Sep 17 00:00:00 2001 From: Garret Alfert Date: Fri, 5 Jan 2018 06:49:42 +0100 Subject: [PATCH 1/3] Handle out of sync metadata in Redis on PUTs When the IF-MATCH comparison fails, we check the actual metadata on the Swift server to be sure. --- lib/remote_storage/swift.rb | 14 ++++++++++++-- spec/swift/app_spec.rb | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/lib/remote_storage/swift.rb b/lib/remote_storage/swift.rb index 73e05f3..dc2d326 100644 --- a/lib/remote_storage/swift.rb +++ b/lib/remote_storage/swift.rb @@ -139,8 +139,18 @@ module RemoteStorage url = url_for_key(user, directory, key) if required_match = server.env["HTTP_IF_MATCH"] - unless required_match.gsub(/^"?W\//, "") == %Q("#{existing_metadata["e"]}") - server.halt 412, "Precondition Failed" + required_match = required_match.gsub(/^"?W\//, "") + unless required_match == %Q("#{existing_metadata["e"]}") + + # get actual metadata and compare in case redis metadata became out of sync + head_res = do_head_request(url) + + if required_match == %Q("#{head_res.headers[:etag]}") + # log previous size difference that was missed ealier because of redis failure + log_size_difference(user, existing_metadata["s"], head_res.headers[:content_length]) + else + server.halt 412, "Precondition Failed" + end end end if server.env["HTTP_IF_NONE_MATCH"] == "*" diff --git a/spec/swift/app_spec.rb b/spec/swift/app_spec.rb index 60d7fda..ae29074 100644 --- a/spec/swift/app_spec.rb +++ b/spec/swift/app_spec.rb @@ -284,11 +284,44 @@ describe "App" do it "fails the request if the header does not match the current ETag" do header "If-Match", "someotheretag" - put "/phil/food/aguacate", "aye" + head_stub = OpenStruct.new(headers: { + etag: "oldetag", + last_modified: "Fri, 04 Mar 2016 12:20:18 GMT", + content_type: "text/plain", + content_length: 23 + }) + + RestClient.stub :head, head_stub do + put "/phil/food/aguacate", "aye" + end last_response.status.must_equal 412 last_response.body.must_equal "Precondition Failed" end + + it "allows the request if redis metadata became out of sync" do + header "If-Match", "\"existingetag\"" + + head_stub = OpenStruct.new(headers: { + etag: "existingetag", + last_modified: "Fri, 04 Mar 2016 12:20:18 GMT", + content_type: "text/plain", + content_length: 23 + }) + + put_stub = OpenStruct.new(headers: { + etag: "newetag", + last_modified: "Fri, 04 Mar 2016 12:20:18 GMT" + }) + + RestClient.stub :head, head_stub do + RestClient.stub :put, put_stub do + put "/phil/food/aguacate", "aye" + end + end + + last_response.status.must_equal 200 + end end describe "If-None-Match header set to '*'" do @@ -307,7 +340,7 @@ describe "App" do last_response.status.must_equal 201 end - it "fails the request if the document already exsits" do + it "fails the request if the document already exists" do put_stub = OpenStruct.new(headers: { etag: "someetag", last_modified: "Fri, 04 Mar 2016 12:20:18 GMT" From 2fac808343f6809b2d789c433908cd76a77ed2c4 Mon Sep 17 00:00:00 2001 From: Garret Alfert Date: Fri, 5 Jan 2018 06:55:46 +0100 Subject: [PATCH 2/3] Split list of ETAGs before removing Weak indicator --- lib/remote_storage/swift.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/remote_storage/swift.rb b/lib/remote_storage/swift.rb index dc2d326..ab4dc1d 100644 --- a/lib/remote_storage/swift.rb +++ b/lib/remote_storage/swift.rb @@ -52,7 +52,9 @@ module RemoteStorage set_response_headers(res) - none_match = (server.env["HTTP_IF_NONE_MATCH"] || "").gsub(/^"?W\//, "").split(",").map(&:strip) + none_match = (server.env["HTTP_IF_NONE_MATCH"] || "").split(",") + .map(&:strip) + .map { |s| s.gsub(/^"?W\//, "") } server.halt 304 if none_match.include? %Q("#{res.headers[:etag]}") return res.body @@ -71,7 +73,9 @@ module RemoteStorage server.headers["Content-Type"] = "application/ld+json" - none_match = (server.env["HTTP_IF_NONE_MATCH"] || "").gsub(/^"?W\//, "").split(",").map(&:strip) + none_match = (server.env["HTTP_IF_NONE_MATCH"] || "").split(",") + .map(&:strip) + .map { |s| s.gsub(/^"?W\//, "") } if etag server.halt 304 if none_match.include? %Q("#{etag}") From 3ddcccaee764a793023c4e33e617b1be2196b9d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Kar=C3=A9kinian?= Date: Fri, 5 Jan 2018 13:11:47 +0100 Subject: [PATCH 3/3] Handle the case of a PUT on a non-existing object The previous code was returning a 500 because the HEAD request failed --- lib/remote_storage/swift.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/remote_storage/swift.rb b/lib/remote_storage/swift.rb index ab4dc1d..9a531ff 100644 --- a/lib/remote_storage/swift.rb +++ b/lib/remote_storage/swift.rb @@ -147,7 +147,12 @@ module RemoteStorage unless required_match == %Q("#{existing_metadata["e"]}") # get actual metadata and compare in case redis metadata became out of sync - head_res = do_head_request(url) + begin + head_res = do_head_request(url) + # The file doesn't exist in Orbit, return 412 + rescue RestClient::ResourceNotFound + server.halt 412, "Precondition Failed" + end if required_match == %Q("#{head_res.headers[:etag]}") # log previous size difference that was missed ealier because of redis failure