Skip to content

Commit fd956fb

Browse files
v-pratapv-pratap
authored
feat(storage): Migrate async client to unified checksum options (googleapis#16261)
* feat(storage): migrate async client to unified checksum options * fix(storage): address reviewer comments and fix format * fix: resolve review comments and unit tests failures * fix(storage): clang-format style issues in connection_impl.cc * fix: address PR review comments for read hash tests and upload checksum helper * fix(storage): resolve read hash tests failing due to incorrect checksum settings * fix(storage): resolve clang-format style issues * fix(storage): resolve reviewer comments for read hash tests and upload checksum helper * fix(storage): resolve clang format * fix(storage): add missing TODO username and disable deprecation warnings for tests * fix(storage): fix alphabetical order in google_cloud_cpp_storage_grpc.bzl * test(storage): retain legacy checksum option test cases
1 parent d26d929 commit fd956fb

10 files changed

Lines changed: 217 additions & 56 deletions

‎google/cloud/storage/google_cloud_cpp_storage_grpc.bzl‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ google_cloud_cpp_storage_grpc_hdrs = [
3737
"async/writer.h",
3838
"async/writer_connection.h",
3939
"grpc_plugin.h",
40+
"internal/async/checksum_helpers.h",
4041
"internal/async/connection_fwd.h",
4142
"internal/async/connection_impl.h",
4243
"internal/async/connection_tracing.h",

‎google/cloud/storage/google_cloud_cpp_storage_grpc.cmake‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ add_library(
9999
async/writer_connection.h
100100
grpc_plugin.cc
101101
grpc_plugin.h
102+
internal/async/checksum_helpers.h
102103
internal/async/connection_fwd.h
103104
internal/async/connection_impl.cc
104105
internal/async/connection_impl.h

‎google/cloud/storage/internal/async/checksum_helpers.h‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Copyright 2024 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H
16+
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H
17+
18+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
19+
#include "google/cloud/storage/async/options.h"
20+
#include "google/cloud/storage/options.h"
21+
#include "google/cloud/options.h"
22+
23+
namespace google {
24+
namespace cloud {
25+
namespace storage_internal {
26+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
27+
28+
struct ChecksumSettings {
29+
bool enable_crc32c;
30+
bool enable_md5;
31+
};
32+
33+
inline ChecksumSettings GetDownloadChecksumSettings(Options const& options) {
34+
if (options.has<storage::DownloadChecksumValidationOption>()) {
35+
auto const algo = options.get<storage::DownloadChecksumValidationOption>();
36+
return {algo == storage::ChecksumAlgorithm::kCrc32c,
37+
algo == storage::ChecksumAlgorithm::kMD5};
38+
}
39+
return {options.get<storage::EnableCrc32cValidationOption>(),
40+
options.get<storage::EnableMD5ValidationOption>()};
41+
}
42+
43+
inline ChecksumSettings GetUploadChecksumSettings(Options const& options) {
44+
if (options.has<storage::UploadChecksumValidationOption>()) {
45+
auto const algo = options.get<storage::UploadChecksumValidationOption>();
46+
return {algo == storage::ChecksumAlgorithm::kCrc32c,
47+
algo == storage::ChecksumAlgorithm::kMD5};
48+
}
49+
return {options.get<storage::EnableCrc32cValidationOption>(),
50+
options.get<storage::EnableMD5ValidationOption>()};
51+
}
52+
53+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
54+
} // namespace storage_internal
55+
} // namespace cloud
56+
} // namespace google
57+
58+
#include "google/cloud/internal/diagnostics_pop.inc"
59+
#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H

‎google/cloud/storage/internal/async/connection_impl.cc‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "google/cloud/storage/async/reader.h"
2020
#include "google/cloud/storage/async/resume_policy.h"
2121
#include "google/cloud/storage/async/retry_policy.h"
22+
#include "google/cloud/storage/internal/async/checksum_helpers.h"
2223
#include "google/cloud/storage/internal/async/default_options.h"
2324
#include "google/cloud/storage/internal/async/handle_redirect_error.h"
2425
#include "google/cloud/storage/internal/async/insert_object.h"
@@ -80,14 +81,18 @@ inline std::unique_ptr<storage::AsyncIdempotencyPolicy> idempotency_policy(
8081
}
8182

8283
std::unique_ptr<storage::internal::HashFunction> CreateHashFunction(
83-
Options const& options) {
84+
Options const& options,
85+
storage_internal::ChecksumSettings const& settings) {
8486
auto crc32c = std::unique_ptr<storage::internal::HashFunction>();
87+
auto const enable_crc32c = settings.enable_crc32c;
88+
auto const enable_md5 = settings.enable_md5;
89+
8590
if (options.has<storage::UseCrc32cValueOption>()) {
8691
crc32c = std::make_unique<storage::internal::PrecomputedHashFunction>(
8792
storage::internal::HashValues{
8893
Crc32cFromProto(options.get<storage::UseCrc32cValueOption>()),
8994
/*.md5=*/{}});
90-
} else if (options.get<storage::EnableCrc32cValidationOption>()) {
95+
} else if (enable_crc32c) {
9196
crc32c = std::make_unique<storage::internal::Crc32cHashFunction>();
9297
}
9398

@@ -97,7 +102,7 @@ std::unique_ptr<storage::internal::HashFunction> CreateHashFunction(
97102
storage::internal::HashValues{
98103
/*.crc32=*/{},
99104
MD5FromProto(options.get<storage::UseMD5ValueOption>())});
100-
} else if (options.get<storage::EnableMD5ValidationOption>()) {
105+
} else if (enable_md5) {
101106
md5 = storage::internal::MD5HashFunction::Create();
102107
}
103108

@@ -117,9 +122,9 @@ std::unique_ptr<storage::internal::HashValidator> CreateHashValidator(
117122
request.read_limit() != 0 || request.read_offset() != 0;
118123
if (is_ranged_read) return storage::internal::CreateNullHashValidator();
119124

120-
auto const enable_crc32c =
121-
options.get<storage::EnableCrc32cValidationOption>();
122-
auto const enable_md5 = options.get<storage::EnableMD5ValidationOption>();
125+
auto const settings = GetDownloadChecksumSettings(options);
126+
auto const enable_crc32c = settings.enable_crc32c;
127+
auto const enable_md5 = settings.enable_md5;
123128

124129
if (enable_crc32c && enable_md5) {
125130
return std::make_unique<storage::internal::CompositeValidator>(
@@ -167,7 +172,8 @@ future<StatusOr<google::storage::v2::Object>> AsyncConnectionImpl::InsertObject(
167172
options->get<storage::TransferStallMinimumRateOption>(),
168173
google::storage::v2::ServiceConstants::MAX_WRITE_CHUNK_BYTES);
169174

170-
auto hash_function = CreateHashFunction(*options);
175+
auto hash_function =
176+
CreateHashFunction(*options, GetUploadChecksumSettings(*options));
171177
ApplyRoutingHeaders(*context, request.write_object_spec());
172178
AddIdempotencyToken(*context, id);
173179
auto rpc = stub->AsyncWriteObject(cq, std::move(context), options);
@@ -271,9 +277,10 @@ AsyncConnectionImpl::ReadObject(ReadObjectParams p) {
271277
// Create the hash function and validator based on the original request. Note
272278
// that p.request will be moved-from, so we have to do it relatively early in
273279
// this function.
280+
auto const settings = GetDownloadChecksumSettings(*current);
274281
auto hash_function =
275282
std::make_shared<storage::internal::Crc32cMessageHashFunction>(
276-
CreateHashFunction(*current));
283+
CreateHashFunction(*current, settings));
277284
auto hash_validator = CreateHashValidator(p.request, *current);
278285

279286
std::optional<std::int64_t> requested_length;
@@ -323,7 +330,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) {
323330
auto current = internal::MakeImmutableOptions(std::move(p.options));
324331
auto request = p.request;
325332
std::shared_ptr<storage::internal::HashFunction> hash_function =
326-
CreateHashFunction(*current);
333+
CreateHashFunction(*current, GetUploadChecksumSettings(*current));
327334
auto retry =
328335
std::shared_ptr<storage::AsyncRetryPolicy>(retry_policy(*current));
329336
auto backoff =
@@ -703,7 +710,8 @@ AsyncConnectionImpl::StartUnbufferedUploadImpl(
703710
StatusOr<std::unique_ptr<storage::AsyncWriterConnection>>(
704711
std::move(response).status()));
705712
}
706-
auto hash_function = CreateHashFunction(*current);
713+
auto hash_function =
714+
CreateHashFunction(*current, GetUploadChecksumSettings(*current));
707715
auto configure =
708716
[current, upload = response->upload_id()](grpc::ClientContext& context) {
709717
ApplyResumableUploadRoutingHeader(context, upload);
@@ -741,7 +749,8 @@ AsyncConnectionImpl::ResumeUnbufferedUploadImpl(
741749
// the upload resumes from the beginning of the file.
742750
auto hash_function = storage::internal::CreateNullHashFunction();
743751
if (response->persisted_size() == 0) {
744-
hash_function = CreateHashFunction(*current);
752+
hash_function =
753+
CreateHashFunction(*current, GetUploadChecksumSettings(*current));
745754
}
746755
auto configure =
747756
[current, upload_id = query.upload_id()](grpc::ClientContext& context) {

‎google/cloud/storage/internal/async/connection_impl_read_hash_test.cc‎

Lines changed: 72 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@
1313
// limitations under the License.
1414

1515
#include "google/cloud/storage/async/options.h"
16+
17+
// TODO(v-pratap): Remove this when EnableMD5ValidationOption and
18+
// EnableCrc32cValidationOption are removed.
19+
#include "google/cloud/internal/disable_deprecation_warnings.inc"
1620
#include "google/cloud/storage/internal/async/connection_impl.h"
1721
#include "google/cloud/storage/internal/async/default_options.h"
1822
#include "google/cloud/storage/internal/crc32c.h"
@@ -75,15 +79,21 @@ auto GeneratedObjectChecksums(HashTestCase const& tc) {
7579
std::ostream& operator<<(std::ostream& os, HashTestCase const& rhs) {
7680
os << "HashTestCase={options={";
7781
os << "expected_status_code=" << rhs.expected_status_code //
78-
<< std::boolalpha //
79-
<< ", enable_crc32c_validation="
80-
<< rhs.options.get<storage::EnableCrc32cValidationOption>();
82+
<< std::boolalpha; //
83+
if (rhs.options.has<storage::DownloadChecksumValidationOption>()) {
84+
os << ", download_checksum="
85+
<< static_cast<int>(
86+
rhs.options.get<storage::DownloadChecksumValidationOption>());
87+
} else {
88+
os << ", enable_crc32c_validation="
89+
<< rhs.options.get<storage::EnableCrc32cValidationOption>();
90+
os << ", enable_md5_validation="
91+
<< rhs.options.get<storage::EnableMD5ValidationOption>();
92+
}
8193
if (rhs.options.has<storage::UseCrc32cValueOption>()) {
8294
os << ", use_crc32_value="
8395
<< rhs.options.get<storage::UseCrc32cValueOption>();
8496
}
85-
os << ", enable_md5_validation="
86-
<< rhs.options.get<storage::EnableMD5ValidationOption>();
8797
if (rhs.options.has<storage::UseMD5ValueOption>()) {
8898
os << ", use_md5_value=" << rhs.options.get<storage::UseMD5ValueOption>();
8999
}
@@ -129,16 +139,14 @@ INSTANTIATE_TEST_SUITE_P(
129139
// This is the common case. Only CRC32C is enabled by default. The
130140
// service returns both CRC32C and MD5 values.
131141
StatusCode::kOk,
132-
Options{}
133-
.set<storage::EnableCrc32cValidationOption>(true)
134-
.set<storage::EnableMD5ValidationOption>(false),
142+
Options{}.set<storage::DownloadChecksumValidationOption>(
143+
storage::ChecksumAlgorithm::kCrc32c),
135144
kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
136145
HashTestCase{
137146
// This is also common, the service does not return a MD5 value.
138147
StatusCode::kOk,
139-
Options{}
140-
.set<storage::EnableCrc32cValidationOption>(true)
141-
.set<storage::EnableMD5ValidationOption>(false),
148+
Options{}.set<storage::DownloadChecksumValidationOption>(
149+
storage::ChecksumAlgorithm::kCrc32c),
142150
kQuickFoxCrc32cChecksum, ""},
143151
// Make sure things work when both hashes are validated too.
144152
HashTestCase{StatusCode::kOk,
@@ -148,6 +156,31 @@ INSTANTIATE_TEST_SUITE_P(
148156
kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
149157
// In the next three cases we verify that disabling some validation
150158
// works.
159+
HashTestCase{StatusCode::kOk,
160+
Options{}.set<storage::DownloadChecksumValidationOption>(
161+
storage::ChecksumAlgorithm::kCrc32c),
162+
kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad},
163+
HashTestCase{StatusCode::kOk,
164+
Options{}.set<storage::DownloadChecksumValidationOption>(
165+
storage::ChecksumAlgorithm::kMD5),
166+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash},
167+
HashTestCase{StatusCode::kOk,
168+
Options{}.set<storage::DownloadChecksumValidationOption>(
169+
storage::ChecksumAlgorithm::kNone),
170+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
171+
HashTestCase{StatusCode::kOk,
172+
Options{}.set<storage::DownloadChecksumValidationOption>(
173+
storage::ChecksumAlgorithm::kCrc32c),
174+
kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad},
175+
HashTestCase{StatusCode::kOk,
176+
Options{}.set<storage::DownloadChecksumValidationOption>(
177+
storage::ChecksumAlgorithm::kMD5),
178+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash},
179+
HashTestCase{StatusCode::kOk,
180+
Options{}.set<storage::DownloadChecksumValidationOption>(
181+
storage::ChecksumAlgorithm::kNone),
182+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
183+
// Legacy options
151184
HashTestCase{StatusCode::kOk,
152185
Options{}
153186
.set<storage::EnableCrc32cValidationOption>(true)
@@ -165,38 +198,55 @@ INSTANTIATE_TEST_SUITE_P(
165198
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
166199
// In the next three cases we verify that validation works when the
167200
// returned values are not correct.
201+
HashTestCase{StatusCode::kInvalidArgument,
202+
Options{}.set<storage::DownloadChecksumValidationOption>(
203+
storage::ChecksumAlgorithm::kMD5),
204+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
205+
HashTestCase{StatusCode::kInvalidArgument,
206+
Options{}.set<storage::DownloadChecksumValidationOption>(
207+
storage::ChecksumAlgorithm::kCrc32c),
208+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
168209
HashTestCase{StatusCode::kInvalidArgument,
169210
Options{}
170-
.set<storage::EnableCrc32cValidationOption>(false)
211+
.set<storage::EnableCrc32cValidationOption>(true)
171212
.set<storage::EnableMD5ValidationOption>(true),
172213
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
214+
HashTestCase{StatusCode::kInvalidArgument,
215+
Options{}.set<storage::DownloadChecksumValidationOption>(
216+
storage::ChecksumAlgorithm::kCrc32c),
217+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
218+
HashTestCase{StatusCode::kInvalidArgument,
219+
Options{}.set<storage::DownloadChecksumValidationOption>(
220+
storage::ChecksumAlgorithm::kMD5),
221+
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
222+
// Legacy options
173223
HashTestCase{StatusCode::kInvalidArgument,
174224
Options{}
175-
.set<storage::EnableCrc32cValidationOption>(true)
176-
.set<storage::EnableMD5ValidationOption>(false),
225+
.set<storage::EnableCrc32cValidationOption>(false)
226+
.set<storage::EnableMD5ValidationOption>(true),
177227
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
178228
HashTestCase{StatusCode::kInvalidArgument,
179229
Options{}
180230
.set<storage::EnableCrc32cValidationOption>(true)
181-
.set<storage::EnableMD5ValidationOption>(true),
231+
.set<storage::EnableMD5ValidationOption>(false),
182232
kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
183233
// The application may know what the values should be. Verify the
184234
// validation works correctly when the application provides correct
185235
// values.
186236
HashTestCase{
187237
StatusCode::kOk,
188238
Options{}
189-
.set<storage::EnableCrc32cValidationOption>(true)
190-
.set<storage::EnableMD5ValidationOption>(true)
239+
.set<storage::DownloadChecksumValidationOption>(
240+
storage::ChecksumAlgorithm::kCrc32c)
191241
.set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksum)
192242
.set<storage::UseMD5ValueOption>(BinaryMD5(kQuickFoxMD5Hash)),
193243
kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
194244
// Verify bad values are detected
195245
HashTestCase{
196246
StatusCode::kInvalidArgument,
197247
Options{}
198-
.set<storage::EnableCrc32cValidationOption>(true)
199-
.set<storage::EnableMD5ValidationOption>(true)
248+
.set<storage::DownloadChecksumValidationOption>(
249+
storage::ChecksumAlgorithm::kCrc32c)
200250
.set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksumBad)
201251
.set<storage::UseMD5ValueOption>(BinaryMD5(kQuickFoxMD5Hash)),
202252
kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
@@ -212,8 +262,8 @@ INSTANTIATE_TEST_SUITE_P(
212262
HashTestCase{
213263
StatusCode::kInvalidArgument,
214264
Options{}
215-
.set<storage::EnableCrc32cValidationOption>(true)
216-
.set<storage::EnableMD5ValidationOption>(true)
265+
.set<storage::DownloadChecksumValidationOption>(
266+
storage::ChecksumAlgorithm::kCrc32c)
217267
.set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksumBad)
218268
.set<storage::UseMD5ValueOption>(
219269
BinaryMD5(kQuickFoxMD5HashBad)),
@@ -477,3 +527,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
477527
} // namespace storage_internal
478528
} // namespace cloud
479529
} // namespace google
530+
#include "google/cloud/internal/diagnostics_pop.inc"

0 commit comments

Comments
 (0)