Skip to content

Commit 3441ee4

Browse files
phoebusmphoebusm
committed
Address PR comments
1 parent da10b5e commit 3441ee4

File tree

5 files changed

+16
-16
lines changed

5 files changed

+16
-16
lines changed

‎cpp/arcticdb/storage/s3/nfs_backed_storage.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ NfsBackedStorage::NfsBackedStorage(const LibraryPath &library_path, OpenMode mod
128128
log::storage().warn("Using Mock S3 storage for NfsBackedStorage");
129129
s3_client_ = std::make_unique<s3::MockS3Client>();
130130
} else {
131-
s3_client_ = std::make_unique<s3::S3ClientImpl>(s3::get_aws_credentials(conf), s3::get_s3_config(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, false);
131+
s3_client_ = std::make_unique<s3::S3ClientImpl>(s3::get_aws_credentials(conf), s3::get_s3_config_and_set_env_var(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, false);
132132
}
133133

134134
if (conf.prefix().empty()) {

‎cpp/arcticdb/storage/s3/s3_storage.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ void S3Storage::create_s3_client(const S3Settings &conf, const Aws::Auth::AWSCre
134134
else if (conf.aws_auth() == AWSAuthMethod::STS_PROFILE_CREDENTIALS_PROVIDER){
135135
ARCTICDB_RUNTIME_DEBUG(log::storage(), "Load sts profile credentials provider");
136136
Aws::Config::ReloadCachedConfigFile(); // config files loaded in Aws::InitAPI; It runs once at first S3Storage object construct; reload to get latest
137-
auto client_config = get_s3_config(conf);
137+
auto client_config = get_s3_config_and_set_env_var(conf);
138138
auto sts_client_factory = [conf, this](const Aws::Auth::AWSCredentials& creds) { // Get default allocation tag
139139
auto sts_config = get_proxy_config(conf.https() ? Aws::Http::Scheme::HTTPS : Aws::Http::Scheme::HTTP);
140140
auto allocation_tag = Aws::STS::STSClient::GetAllocationTag();
@@ -151,10 +151,10 @@ void S3Storage::create_s3_client(const S3Settings &conf, const Aws::Auth::AWSCre
151151
}
152152
else if (creds.GetAWSAccessKeyId() == USE_AWS_CRED_PROVIDERS_TOKEN && creds.GetAWSSecretKey() == USE_AWS_CRED_PROVIDERS_TOKEN){
153153
ARCTICDB_RUNTIME_DEBUG(log::storage(), "Using AWS auth mechanisms");
154-
s3_client_ = std::make_unique<S3ClientImpl>(get_s3_config(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, conf.use_virtual_addressing());
154+
s3_client_ = std::make_unique<S3ClientImpl>(get_s3_config_and_set_env_var(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, conf.use_virtual_addressing());
155155
} else {
156156
ARCTICDB_RUNTIME_DEBUG(log::storage(), "Using provided auth credentials");
157-
s3_client_ = std::make_unique<S3ClientImpl>(creds, get_s3_config(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, conf.use_virtual_addressing());
157+
s3_client_ = std::make_unique<S3ClientImpl>(creds, get_s3_config_and_set_env_var(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, conf.use_virtual_addressing());
158158
}
159159

160160
if (conf.use_internal_client_wrapper_for_testing()){

‎cpp/arcticdb/storage/s3/s3_storage.hpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -259,9 +259,9 @@ inline void configure_s3_checksum_validation() {
259259

260260
if ((response_checksum && std::string(response_checksum) == "when_supported") ||
261261
(request_checksum && std::string(request_checksum) == "when_supported")) {
262-
log::storage().warn("S3 Checksum validation has been specifically enabled by user"
263-
"If endpoint doesn't support it, its response will not contain checksum"
264-
"which will be rejected by SDK and lead to storage exception in arcticdb");
262+
log::storage().warn("S3 Checksum validation has been specifically enabled by user. "
263+
"If endpoint doesn't support it, 1. garbage could be siliently written "
264+
"2. Endpoint response will be rejected by SDK and lead to storage exception in arcticdb");
265265
}
266266
else {
267267
#ifdef _WIN32
@@ -275,7 +275,7 @@ inline void configure_s3_checksum_validation() {
275275
}
276276

277277
template<typename ConfigType>
278-
auto get_s3_config(const ConfigType& conf) {
278+
auto get_s3_config_and_set_env_var(const ConfigType& conf) {
279279
configure_s3_checksum_validation();
280280

281281
auto endpoint_scheme = conf.https() ? Aws::Http::Scheme::HTTPS : Aws::Http::Scheme::HTTP;

‎cpp/arcticdb/storage/s3/s3_storage_tool.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ void S3StorageTool::delete_bucket(const std::string& prefix) {
193193

194194
S3StorageTool::S3StorageTool(const Config &conf) :
195195
s3_api_(S3ApiInstance::instance()),
196-
s3_client_(get_aws_credentials(conf), get_s3_config(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, false),
196+
s3_client_(get_aws_credentials(conf), get_s3_config_and_set_env_var(conf), Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, false),
197197
bucket_name_(conf.bucket_name()) {
198198
std::locale locale{ std::locale::classic(), new std::num_put<char>()};
199199
(void)std::locale::global(locale);

‎cpp/arcticdb/storage/test/test_s3_storage.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ TEST_F(ProxyEnvVarSetHttpProxyForHttpsEndpointFixture, test_config_resolution_pr
150150
arcticdb::proto::s3_storage::Config s3_config;
151151
s3_config.set_endpoint("https://test.endpoint.com");
152152
s3_config.set_https(true);
153-
auto ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config);
153+
auto ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config);
154154
ASSERT_EQ(ret_cfg.proxyHost, "http-proxy.com");
155155
ASSERT_EQ(ret_cfg.proxyPort, 443);
156156
ASSERT_EQ(ret_cfg.proxyScheme, Aws::Http::Scheme::HTTP);
@@ -159,15 +159,15 @@ TEST_F(ProxyEnvVarSetHttpProxyForHttpsEndpointFixture, test_config_resolution_pr
159159
TEST_F(ProxyEnvVarUpperCaseFixture, test_config_resolution_proxy) {
160160
arcticdb::proto::s3_storage::Config s3_config_http;
161161
s3_config_http.set_endpoint("http://test.endpoint.com");
162-
auto ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config_http);
162+
auto ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config_http);
163163
ASSERT_EQ(ret_cfg.proxyHost, "http-proxy-2.com");
164164
ASSERT_EQ(ret_cfg.proxyPort, 2222);
165165
ASSERT_EQ(ret_cfg.proxyScheme, Aws::Http::Scheme::HTTP);
166166

167167
arcticdb::proto::s3_storage::Config s3_config_https;
168168
s3_config_https.set_endpoint("https://test.endpoint.com");
169169
s3_config_https.set_https(true);
170-
ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config_https);
170+
ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config_https);
171171
ASSERT_EQ(ret_cfg.proxyHost, "https-proxy-2.com");
172172
ASSERT_EQ(ret_cfg.proxyPort, 2222);
173173
ASSERT_EQ(ret_cfg.proxyScheme, Aws::Http::Scheme::HTTPS);
@@ -177,15 +177,15 @@ TEST_F(ProxyEnvVarLowerCasePrecedenceFixture, test_config_resolution_proxy) {
177177
SKIP_WIN("Env vars are not case-sensitive on Windows");
178178
arcticdb::proto::s3_storage::Config s3_config_http;
179179
s3_config_http.set_endpoint("http://test.endpoint.com");
180-
auto ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config_http);
180+
auto ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config_http);
181181
ASSERT_EQ(ret_cfg.proxyHost, "http-proxy-1.com");
182182
ASSERT_EQ(ret_cfg.proxyPort, 2222);
183183
ASSERT_EQ(ret_cfg.proxyScheme, Aws::Http::Scheme::HTTP);
184184

185185
arcticdb::proto::s3_storage::Config s3_config_https;
186186
s3_config_https.set_endpoint("https://test.endpoint.com");
187187
s3_config_https.set_https(true);
188-
ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config_https);
188+
ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config_https);
189189
ASSERT_EQ(ret_cfg.proxyHost, "https-proxy-1.com");
190190
ASSERT_EQ(ret_cfg.proxyPort, 2222);
191191
ASSERT_EQ(ret_cfg.proxyScheme, Aws::Http::Scheme::HTTPS);
@@ -194,7 +194,7 @@ TEST_F(ProxyEnvVarLowerCasePrecedenceFixture, test_config_resolution_proxy) {
194194
TEST_F(NoProxyEnvVarUpperCaseFixture, test_config_resolution_proxy) {
195195
arcticdb::proto::s3_storage::Config s3_config;
196196
s3_config.set_endpoint("http://test.endpoint.com");
197-
auto ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config);
197+
auto ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config);
198198

199199
Aws::Utils::Array<Aws::String> expected_non_proxy_hosts{1};
200200
expected_non_proxy_hosts[0] = "http://test-1.endpoint.com";
@@ -205,7 +205,7 @@ TEST_F(NoProxyEnvVarLowerCasePrecedenceFixture, test_config_resolution_proxy) {
205205
SKIP_WIN("Env vars are not case-sensitive on Windows");
206206
arcticdb::proto::s3_storage::Config s3_config;
207207
s3_config.set_endpoint("http://test.endpoint.com");
208-
auto ret_cfg = arcticdb::storage::s3::get_s3_config(s3_config);
208+
auto ret_cfg = arcticdb::storage::s3::get_s3_config_and_set_env_var(s3_config);
209209

210210
Aws::Utils::Array<Aws::String> expected_non_proxy_hosts{2};
211211
expected_non_proxy_hosts[0] = "http://test-1.endpoint.com";

0 commit comments

Comments
 (0)