wgtmac commented on code in PR #719:
URL: https://github.com/apache/iceberg-cpp/pull/719#discussion_r3453633596
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -878,7 +892,8 @@ Result<std::shared_ptr<Table>>
RestCatalog::MakeTableFromCommitResponse(
// TODO(gangwu): If the REST commit response grows table config or
// storage credentials, derive a replacement table session/FileIO from that
// response. The current table commit response does not define config.
- ICEBERG_ASSIGN_OR_RAISE(auto table_io, TableFileIO(context, table_config));
+ ICEBERG_ASSIGN_OR_RAISE(auto table_io,
+ TableFileIO(context, table_config,
/*storage_credentials=*/{}));
Review Comment:
This rebuilds the returned table with an empty credential list after commit.
CommitTableResponse does not carry storage-credentials, so a table loaded with
vended credentials loses its FileIO credentials after the first commit.
##########
src/iceberg/catalog/rest/rest_file_io.cc:
##########
@@ -92,4 +95,30 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const
RestCatalogProperties& c
return FileIORegistry::Load(io_impl, config.configs());
}
+const StorageCredential* SelectS3StorageCredential(
+ const std::vector<StorageCredential>& credentials) {
+ const StorageCredential* best = nullptr;
+ for (const auto& cred : credentials) {
+ if (cred.prefix.starts_with("s3") &&
+ (best == nullptr || cred.prefix.size() > best->prefix.size())) {
+ best = &cred;
+ }
+ }
+ return best;
+}
+
+Result<std::unique_ptr<FileIO>> MakeS3FileIOFromCredential(
+ const std::unordered_map<std::string, std::string>& catalog_config,
+ const std::unordered_map<std::string, std::string>& table_config,
+ const StorageCredential& s3_cred) {
+ auto properties = catalog_config;
+ for (const auto& [key, value] : table_config) {
+ properties[key] = value;
+ }
+ for (const auto& [key, value] : s3_cred.config) {
Review Comment:
This makes the STS values static. Java S3FileIO keeps the credential set and
refreshes through credentials.uri when s3.session-token-expires-at-ms is
present; long scans or reused tables will fail after the vended token expires.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -516,7 +516,17 @@ Result<std::shared_ptr<auth::AuthSession>>
RestCatalog::TableAuthSession(
Result<std::shared_ptr<FileIO>> RestCatalog::TableFileIO(
const SessionContext& /*context*/,
- const std::unordered_map<std::string, std::string>& table_config) const {
+ const std::unordered_map<std::string, std::string>& table_config,
+ const std::vector<StorageCredential>& storage_credentials) const {
+ // Longest-prefix "s3" vended credential. Java's VendedCredentialsProvider
+ // resolves per path against the table location; we bind one at load time
+ // (fine for the common one-credential-per-table case).
+ if (const StorageCredential* s3_cred =
SelectS3StorageCredential(storage_credentials);
Review Comment:
storage-credentials can also carry GCS/ADLS credentials. This path silently
ignores non-S3 credentials and falls back to the catalog FileIO; if S3-only is
intentional for now, please add an explicit TODO or fail-fast note.
##########
src/iceberg/catalog/rest/rest_file_io.cc:
##########
@@ -92,4 +95,30 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const
RestCatalogProperties& c
return FileIORegistry::Load(io_impl, config.configs());
}
+const StorageCredential* SelectS3StorageCredential(
+ const std::vector<StorageCredential>& credentials) {
+ const StorageCredential* best = nullptr;
+ for (const auto& cred : credentials) {
+ if (cred.prefix.starts_with("s3") &&
Review Comment:
Prefix selection needs the actual storage path. The REST spec and Java
S3FileIO choose the longest prefix that matches each path; picking the longest
S3 prefix globally makes unmatched paths and other S3 prefixes use the wrong
credential.
--
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]
