wgtmac commented on code in PR #892:
URL: https://github.com/apache/iceberg-cpp/pull/892#discussion_r4296275841
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -314,6 +325,262 @@ std::string CanonicalizeS3Scheme(std::string_view
location) {
return std::string(location);
}
+// Lead time before expiry, matching Java's VendedCredentialsProvider.
+constexpr auto kRefreshLeadTime = std::chrono::minutes(5);
+
+// How long to wait after a failed refresh before asking the catalog again.
+constexpr auto kRefreshRetryBackoff = std::chrono::seconds(30);
+
+// Floor on a backoff shortened to land on the expiry.
+constexpr auto kMinRefreshRetryBackoff = std::chrono::seconds(1);
+
+// How long a caller with expired credentials waits for a refresh under way.
+constexpr auto kExpiredCredentialWait = std::chrono::seconds(10);
+
+// Whether `config` carries an access key pair.
+bool HasKeys(const std::unordered_map<std::string, std::string>& config) {
+ return FindProperty(config, S3Properties::kAccessKeyId) != nullptr &&
+ FindProperty(config, S3Properties::kSecretAccessKey) != nullptr;
+}
+
+// The properties an S3 client for `credential` is built from.
+std::unordered_map<std::string, std::string> PropertiesFor(
+ const std::unordered_map<std::string, std::string>& default_properties,
+ const StorageCredential& credential) {
+ auto properties = default_properties;
+ for (const auto& [key, value] : credential.config) {
+ properties[key] = value;
+ }
+ return properties;
+}
+
+// The client settings in `properties`: everything but the keys.
+std::unordered_map<std::string, std::string> SettingsOf(
+ std::unordered_map<std::string, std::string> properties) {
+ for (const auto key :
+ {S3Properties::kAccessKeyId, S3Properties::kSecretAccessKey,
+ S3Properties::kSessionToken, S3Properties::kSessionTokenExpiresAtMs}) {
+ properties.erase(std::string(key));
+ }
+ return properties;
+}
+
+// When the session token that `properties` sign with expires: nullopt if they
+// sign without one, or an error if the expiry is missing or invalid.
+Result<std::optional<std::chrono::system_clock::time_point>> ExpiryOf(
+ const std::unordered_map<std::string, std::string>& properties,
+ std::string_view prefix) {
+ // Only a nonempty token alongside an access key pair signs requests.
+ const auto* token = FindProperty(properties, S3Properties::kSessionToken);
+ if (!HasKeys(properties) || token == nullptr || token->empty()) {
+ return std::nullopt;
+ }
+ const auto* value = FindProperty(properties,
S3Properties::kSessionTokenExpiresAtMs);
+ if (value == nullptr) {
+ return InvalidArgument("Credential \"{}\" has a session token but no
\"{}\"", prefix,
+ S3Properties::kSessionTokenExpiresAtMs);
+ }
+ auto millis = StringUtils::ParseNumber<int64_t>(*value);
+ if (!millis.has_value()) {
+ return InvalidArgument(
+ "Credential \"{}\" has a session token but an unparseable \"{}\" value
\"{}\"",
+ prefix, S3Properties::kSessionTokenExpiresAtMs, *value);
+ }
+ // Beyond what the clock can hold, converting would overflow it.
+ constexpr auto kMaxMillis =
std::chrono::duration_cast<std::chrono::milliseconds>(
+ std::chrono::system_clock::duration::max())
+ .count();
+ constexpr auto kMinMillis =
std::chrono::duration_cast<std::chrono::milliseconds>(
+ std::chrono::system_clock::duration::min())
+ .count();
+ if (*millis > kMaxMillis || *millis < kMinMillis) {
+ return InvalidArgument("Credential \"{}\" has an out-of-range \"{}\" value
\"{}\"",
+ prefix, S3Properties::kSessionTokenExpiresAtMs,
*value);
+ }
+ return
std::chrono::system_clock::time_point(std::chrono::milliseconds(*millis));
+}
+
+// The S3 credentials in effect: the first for each canonical prefix, which is
+// the one its locations match.
+std::vector<const StorageCredential*> S3CredentialsOf(
+ const std::vector<StorageCredential>& credentials) {
+ std::vector<const StorageCredential*> s3_credentials;
+ std::unordered_set<std::string> prefixes;
+ for (const auto& credential : credentials) {
+ if (IsS3CredentialPrefix(credential.prefix) &&
+ prefixes.insert(CanonicalizeS3Scheme(credential.prefix)).second) {
Review Comment:
`CanonicalizeS3Scheme` merges `s3://` and `oss://` into the same routing key
here. If a REST response contains both prefixes for the same bucket/path but
different endpoints, this drops one credential set and both locations use the
first one. Can we preserve the scheme in routing keys, or reject this ambiguity
explicitly? Please add a test for simultaneous `s3://` and `oss://` prefixes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
