diff --git a/ci/cloudbuild/builds/lib/integration.sh b/ci/cloudbuild/builds/lib/integration.sh index d6fba90073441..3506c661c860c 100644 --- a/ci/cloudbuild/builds/lib/integration.sh +++ b/ci/cloudbuild/builds/lib/integration.sh @@ -31,7 +31,7 @@ source module ci/lib/io.sh export PATH="${HOME}/.local/bin:${PATH}" python3 -m pip uninstall -y --quiet googleapis-storage-testbench python3 -m pip install --upgrade --user --quiet --disable-pip-version-check \ - "git+https://github.com/googleapis/storage-testbench@v0.59.0" + "git+https://github.com/googleapis/storage-testbench@v0.60.0" # Some of the tests will need a valid roots.pem file. rm -f /dev/shm/roots.pem diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc index 1ff0bc6ed1279..40ac5ce2e813f 100644 --- a/google/cloud/storage/internal/grpc/object_request_parser.cc +++ b/google/cloud/storage/internal/grpc/object_request_parser.cc @@ -851,6 +851,15 @@ Status Finalize(google::storage::v2::WriteObjectRequest& write_request, Merge(std::move(hashes), hash_function.Finish())); } +Status Finalize(google::storage::v2::WriteObjectRequest& write_request, + grpc::WriteOptions& options, + storage::internal::HashValues hashes) { + write_request.set_finish_write(true); + options.set_last_message(); + return FinalizeChecksums(*write_request.mutable_object_checksums(), + std::move(hashes)); +} + Status Finalize(google::storage::v2::BidiWriteObjectRequest& write_request, grpc::WriteOptions& options, storage::internal::HashFunction& hash_function, @@ -879,8 +888,11 @@ Status MaybeFinalize(google::storage::v2::WriteObjectRequest& write_request, bool chunk_has_more) { if (!chunk_has_more) options.set_last_message(); if (!request.last_chunk() || chunk_has_more) return {}; - return Finalize(write_request, options, request.hash_function(), - request.known_object_hashes()); + if (request.hash_function_ptr()) { + return Finalize(write_request, options, request.hash_function(), + request.known_object_hashes()); + } + return Finalize(write_request, options, request.known_object_hashes()); } GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/grpc/object_request_parser.h b/google/cloud/storage/internal/grpc/object_request_parser.h index 6e90bcb942e45..b1cdee22fcc14 100644 --- a/google/cloud/storage/internal/grpc/object_request_parser.h +++ b/google/cloud/storage/internal/grpc/object_request_parser.h @@ -87,6 +87,10 @@ Status Finalize(google::storage::v2::WriteObjectRequest& write_request, storage::internal::HashFunction& hash_function, storage::internal::HashValues = {}); +Status Finalize(google::storage::v2::WriteObjectRequest& write_request, + grpc::WriteOptions& options, + storage::internal::HashValues hashes); + Status Finalize(google::storage::v2::BidiWriteObjectRequest& write_request, grpc::WriteOptions& options, storage::internal::HashFunction& hash_function, diff --git a/google/cloud/storage/internal/grpc/stub.cc b/google/cloud/storage/internal/grpc/stub.cc index 9a7120c6ebae8..733b42bf4f94d 100644 --- a/google/cloud/storage/internal/grpc/stub.cc +++ b/google/cloud/storage/internal/grpc/stub.cc @@ -630,7 +630,9 @@ StatusOr GrpcStub::UploadChunk( auto& data = *proto_request.mutable_checksummed_data(); SetMutableContent(data, splitter.Next()); data.set_crc32c(Crc32c(GetContent(data))); - request.hash_function().Update(offset, GetContent(data), data.crc32c()); + if (request.hash_function_ptr()) { + request.hash_function().Update(offset, GetContent(data), data.crc32c()); + } offset += GetContent(data).size(); auto wopts = grpc::WriteOptions(); diff --git a/google/cloud/storage/internal/object_requests.cc b/google/cloud/storage/internal/object_requests.cc index 1a61dc38d19af..37857aff7bfdc 100644 --- a/google/cloud/storage/internal/object_requests.cc +++ b/google/cloud/storage/internal/object_requests.cc @@ -533,7 +533,10 @@ UploadChunkRequest UploadChunkRequest::RemainingChunk( HashValues FinishHashes(UploadChunkRequest const& request) { // Prefer the hashes provided via *Value options in the request. If those // are not set, use the computed hashes from the data. - return Merge(request.known_object_hashes(), request.hash_function().Finish()); + if (auto hf = request.hash_function_ptr()) { + return Merge(request.known_object_hashes(), hf->Finish()); + } + return request.known_object_hashes(); } std::ostream& operator<<(std::ostream& os, UploadChunkRequest const& r) { diff --git a/google/cloud/storage/internal/object_requests.h b/google/cloud/storage/internal/object_requests.h index 4e18ef4bc190d..6329b76eda038 100644 --- a/google/cloud/storage/internal/object_requests.h +++ b/google/cloud/storage/internal/object_requests.h @@ -534,6 +534,9 @@ class UploadChunkRequest HashValues const& known_object_hashes() const { return known_object_hashes_; } HashFunction& hash_function() const { return *hash_function_; } + std::shared_ptr hash_function_ptr() const { + return hash_function_; + } bool last_chunk() const { return upload_size_.has_value(); } std::size_t payload_size() const { return TotalBytes(payload_); } diff --git a/google/cloud/storage/internal/object_write_streambuf.cc b/google/cloud/storage/internal/object_write_streambuf.cc index 486b4e15b2b03..3d4238fe37f77 100644 --- a/google/cloud/storage/internal/object_write_streambuf.cc +++ b/google/cloud/storage/internal/object_write_streambuf.cc @@ -144,12 +144,18 @@ void ObjectWriteStreambuf::FlushFinal() { // Calculate the portion of the buffer that needs to be uploaded, if any. auto const actual_size = put_area_size(); + HashValues final_hashes = known_hashes_; + if (hash_function_) { + hash_function_->Update(committed_size_, {pbase(), actual_size}); + final_hashes = hash_function_->Finish(); + hash_function_.reset(); + } // After this point the session will be closed, and no more calls to the hash // function are possible. auto upload_request = UploadChunkRequest(upload_id_, committed_size_, {ConstBuffer(pbase(), actual_size)}, - hash_function_, known_hashes_); + hash_function_, final_hashes); request_.ForEachOption(internal::CopyCommonOptions(upload_request)); OptionsSpan const span(span_options_); auto response = connection_->UploadChunk(upload_request); @@ -157,9 +163,7 @@ void ObjectWriteStreambuf::FlushFinal() { last_status_ = std::move(response).status(); return; } - - auto function = std::move(hash_function_); - hash_values_ = std::move(*function).Finish(); + hash_values_ = final_hashes; committed_size_ = response->committed_size.value_or(0); metadata_ = std::move(response->payload); diff --git a/google/cloud/storage/internal/object_write_streambuf_test.cc b/google/cloud/storage/internal/object_write_streambuf_test.cc index 9b401ab4cd750..077dc83a67e55 100644 --- a/google/cloud/storage/internal/object_write_streambuf_test.cc +++ b/google/cloud/storage/internal/object_write_streambuf_test.cc @@ -670,6 +670,37 @@ TEST(ObjectWriteStreambufTest, WriteObjectWithCustomHeader) { EXPECT_STATUS_OK(response); } +/// @test Verify that hashes are computed and passed in FlushFinal. +TEST(ObjectWriteStreambufTest, FlushFinalWithHashes) { + auto mock = std::make_unique(); + auto const quantum = UploadChunkRequest::kChunkSizeQuantum; + std::string const payload = "small test payload"; + + EXPECT_CALL(*mock, UploadChunk).WillOnce([&](UploadChunkRequest const& r) { + EXPECT_EQ(payload.size(), r.payload_size()); + EXPECT_EQ(0, r.offset()); + EXPECT_TRUE(r.last_chunk()); + EXPECT_EQ(r.hash_function_ptr(), nullptr); + EXPECT_EQ(r.known_object_hashes().crc32c, ComputeCrc32cChecksum(payload)); + EXPECT_EQ(r.known_object_hashes().md5, ComputeMD5Hash(payload)); + return QueryResumableUploadResponse{payload.size(), ObjectMetadata()}; + }); + + ResumableUploadRequest request; + request.set_option(DisableCrc32cChecksum(false)); + request.set_option(DisableMD5Hash(false)); + ObjectWriteStreambuf streambuf( + std::move(mock), request, "test-only-upload-id", + /*committed_size=*/0, absl::nullopt, /*max_buffer_size=*/quantum, + CreateHashFunction(Crc32cChecksumValue(), DisableCrc32cChecksum(false), + MD5HashValue(), DisableMD5Hash(false)), + HashValues{}, CreateHashValidator(request), AutoFinalizeConfig::kEnabled); + + streambuf.sputn(payload.data(), payload.size()); + auto response = streambuf.Close(); + EXPECT_STATUS_OK(response); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index 32c0e93542be9..4a1e338f64e7c 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -770,11 +770,22 @@ StatusOr RestStub::UploadChunk( // default (at least in this case), and that wastes bandwidth as the content // length is known. builder.AddHeader("Transfer-Encoding", {}); - auto offset = request.offset(); - for (auto const& b : request.payload()) { - request.hash_function().Update(offset, - absl::string_view{b.data(), b.size()}); - offset += b.size(); + auto hash_function = request.hash_function_ptr(); + if (hash_function) { + auto offset = request.offset(); + for (auto const& b : request.payload()) { + hash_function->Update(offset, absl::string_view{b.data(), b.size()}); + offset += b.size(); + } + } + if (request.last_chunk()) { + auto const& hashes = request.known_object_hashes(); + if (!hashes.crc32c.empty()) { + builder.AddHeader("x-goog-hash", "crc32c=" + hashes.crc32c); + } + if (!hashes.md5.empty()) { + builder.AddHeader("x-goog-hash", "md5=" + hashes.md5); + } } auto failure_predicate = [](rest::HttpStatusCode code) { diff --git a/google/cloud/storage/internal/rest/stub_test.cc b/google/cloud/storage/internal/rest/stub_test.cc index b915bb2a3b07d..05202648577d2 100644 --- a/google/cloud/storage/internal/rest/stub_test.cc +++ b/google/cloud/storage/internal/rest/stub_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/rest/stub.h" +#include "google/cloud/storage/internal/hash_function.h" #include "google/cloud/storage/testing/canonical_errors.h" #include "google/cloud/internal/api_client_header.h" #include "google/cloud/testing_util/mock_rest_client.h" @@ -45,6 +46,29 @@ using ::testing::Pair; using ::testing::ResultOf; using ::testing::Return; +class NoOpHashFunction : public HashFunction { + public: + std::string Name() const override { return "NoOp"; } + void Update(absl::string_view b) override { Cormorant(b); } + Status Update(std::int64_t o, absl::string_view b) override { + Cormorant(o, b); + return Status{}; + } + Status Update(std::int64_t o, absl::string_view b, std::uint32_t c) override { + Cormorant(o, b, c); + return Status{}; + } + Status Update(std::int64_t o, absl::Cord const& b, std::uint32_t c) override { + Cormorant(o, b, c); + return Status{}; + } + HashValues Finish() override { return {}; } + + private: + template + void Cormorant(Args const&...) {} +}; + TEST(RestStubTest, ResolveStorageAuthorityProdEndpoint) { auto options = Options{}.set("https://storage.googleapis.com"); @@ -921,6 +945,92 @@ TEST(RestStubTest, DeleteNotification) { StatusIs(PermanentError().code(), PermanentError().message())); } +TEST(RestStubTest, UploadChunkLastChunkWithCrc32c) { + auto mock = std::make_shared(); + EXPECT_CALL( + *mock, + Put(ExpectedContext(), + ResultOf( + "request headers contain x-goog-hash with crc32c", + [](RestRequest const& r) { return r.headers(); }, + Contains(Pair("x-goog-hash", ElementsAre("crc32c=test-crc32c")))), + ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = std::make_unique(Options{}, mock, mock); + auto context = TestContext(); + auto status = tested->UploadChunk( + context, TestOptions(), + UploadChunkRequest("test-url", 0, {}, + std::make_shared(), + {"test-crc32c", ""})); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestStubTest, UploadChunkLastChunkWithMd5) { + auto mock = std::make_shared(); + EXPECT_CALL( + *mock, + Put(ExpectedContext(), + ResultOf( + "request headers contain x-goog-hash with md5", + [](RestRequest const& r) { return r.headers(); }, + Contains(Pair("x-goog-hash", ElementsAre("md5=test-md5")))), + ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = std::make_unique(Options{}, mock, mock); + auto context = TestContext(); + auto status = tested->UploadChunk( + context, TestOptions(), + UploadChunkRequest("test-url", 0, {}, + std::make_shared(), + {"", "test-md5"})); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestStubTest, UploadChunkLastChunkWithBoth) { + auto mock = std::make_shared(); + EXPECT_CALL( + *mock, + Put(ExpectedContext(), + ResultOf( + "request headers contain x-goog-hash with crc32c and md5", + [](RestRequest const& r) { return r.headers(); }, + Contains(Pair("x-goog-hash", ElementsAre("crc32c=test-crc32c", + "md5=test-md5")))), + ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = std::make_unique(Options{}, mock, mock); + auto context = TestContext(); + auto status = tested->UploadChunk( + context, TestOptions(), + UploadChunkRequest("test-url", 0, {}, + std::make_shared(), + {"test-crc32c", "test-md5"})); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + +TEST(RestStubTest, UploadChunkIntermediate) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, Put(ExpectedContext(), + ResultOf( + "request headers do not contain x-goog-hash", + [](RestRequest const& r) { return r.headers(); }, + Not(Contains(Pair("x-goog-hash", _)))), + ExpectedPayload())) + .WillOnce(Return(PermanentError())); + auto tested = std::make_unique(Options{}, mock, mock); + auto context = TestContext(); + auto status = tested->UploadChunk( + context, TestOptions(), + UploadChunkRequest("test-url", 0, {}, + std::make_shared())); + EXPECT_THAT(status, + StatusIs(PermanentError().code(), PermanentError().message())); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/tests/object_checksum_integration_test.cc b/google/cloud/storage/tests/object_checksum_integration_test.cc index c5f88f529f862..2247a3c26871b 100644 --- a/google/cloud/storage/tests/object_checksum_integration_test.cc +++ b/google/cloud/storage/tests/object_checksum_integration_test.cc @@ -145,14 +145,8 @@ TEST_F(ObjectChecksumIntegrationTest, WriteObjectDefault) { EXPECT_THAT(os.computed_hash(), HasSubstr(ComputeCrc32cChecksum(LoremIpsum()))); if (meta->has_metadata("x_emulator_upload")) { - if (UsingGrpc()) { - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); - } else { - // Streaming uploads over REST cannot include checksums - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); - } + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_crc32c", _))); + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); } } @@ -200,14 +194,8 @@ TEST_F(ObjectChecksumIntegrationTest, WriteObjectExplicitEnable) { EXPECT_THAT(os.computed_hash(), HasSubstr(ComputeCrc32cChecksum(LoremIpsum()))); if (meta->has_metadata("x_emulator_upload")) { - if (UsingGrpc()) { - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); - } else { - // Streaming uploads over REST cannot include checksums - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); - } + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_crc32c", _))); + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); } } @@ -289,6 +277,54 @@ TEST_F(ObjectChecksumIntegrationTest, WriteObjectUploadBadChecksum) { ASSERT_THAT(stream.metadata(), Not(IsOk())); } +/// @test Verify that full object checksums are sent in the final chunk. +TEST_F(ObjectChecksumIntegrationTest, WriteObjectWithFullChecksumValidation) { + auto client = MakeIntegrationTestClient(); + auto object_name = MakeRandomObjectName(); + auto content = LoremIpsum(); + auto expected_crc32c = ComputeCrc32cChecksum(content); + + auto os = client.WriteObject(bucket_name_, object_name, + DisableCrc32cChecksum(false), + DisableMD5Hash(true), IfGenerationMatch(0)); + os << content; + os.Close(); + auto meta = os.metadata(); + ASSERT_STATUS_OK(meta); + ScheduleForDelete(*meta); + + EXPECT_EQ(os.computed_hash(), expected_crc32c); + + if (meta->has_metadata("x_emulator_upload")) { + EXPECT_THAT(meta->metadata(), + Contains(Pair("x_emulator_crc32c", expected_crc32c))); + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", "true"))); + } +} + +/// @test Verify that the upload fails when the provided CRC32C checksum does +/// not match the data. +TEST_F(ObjectChecksumIntegrationTest, WriteObjectWithIncorrectChecksumValue) { + // TODO(#14385) - the emulator does not support this feature for gRPC. + if (UsingEmulator() && UsingGrpc()) GTEST_SKIP(); + auto client = MakeIntegrationTestClient(); + auto object_name = MakeRandomObjectName(); + auto content = LoremIpsum(); + + auto bad_crc32c = + ComputeCrc32cChecksum("this is not the data being uploaded"); + + auto os = client.WriteObject(bucket_name_, object_name, + Crc32cChecksumValue(bad_crc32c), + DisableMD5Hash(true), IfGenerationMatch(0)); + + os << content; + os.Close(); + EXPECT_TRUE(os.bad()); + auto meta = os.metadata(); + EXPECT_THAT(meta, Not(IsOk())); +} + /// @test Verify that CRC32C checksums are computed by default on downloads. TEST_F(ObjectChecksumIntegrationTest, ReadObjectDefault) { // TODO(#14385) - the emulator does not support this feature for gRPC. diff --git a/google/cloud/storage/tests/object_hash_integration_test.cc b/google/cloud/storage/tests/object_hash_integration_test.cc index d8c9325a78121..d4d6a1f060a31 100644 --- a/google/cloud/storage/tests/object_hash_integration_test.cc +++ b/google/cloud/storage/tests/object_hash_integration_test.cc @@ -192,14 +192,8 @@ TEST_F(ObjectHashIntegrationTest, WriteObjectExplicitEnable) { EXPECT_THAT(os.computed_hash(), HasSubstr(ComputeMD5Hash(LoremIpsum()))); EXPECT_THAT(os.received_hash(), HasSubstr(ComputeMD5Hash(LoremIpsum()))); if (meta->has_metadata("x_emulator_upload")) { - if (UsingGrpc()) { - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_md5", _))); - } else { - // REST cannot send the checksums computed at the end of the upload. - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_crc32c", _))); - EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_md5", _))); - } + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_no_crc32c", _))); + EXPECT_THAT(meta->metadata(), Contains(Pair("x_emulator_md5", _))); } }