-
Notifications
You must be signed in to change notification settings - Fork 0
feat: credential vending for REST catalog #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
620041b
ef849e7
f705f8a
5ce2cb1
25055c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,9 @@ | |
| #include "iceberg/catalog/rest/rest_file_io.h" | ||
|
|
||
| #include <string> | ||
| #include <unordered_map> | ||
|
|
||
| #include "iceberg/catalog/rest/catalog_properties.h" | ||
| #include "iceberg/file_io_registry.h" | ||
| #include "iceberg/util/macros.h" | ||
|
|
||
|
|
@@ -69,12 +71,13 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const RestCatalogProperties& c | |
|
|
||
| if (io_impl.empty()) { | ||
| if (warehouse.empty()) { | ||
| return InvalidArgument(R"("{}" or "{}" property is required to create FileIO)", | ||
| RestCatalogProperties::kIOImpl.key(), | ||
| RestCatalogProperties::kWarehouse.key()); | ||
| // No io-impl or warehouse configured. Fall back to a local FileIO as a | ||
| // default — enabling per-table ResolveTableFileIO (vending). | ||
| io_impl = std::string(FileIORegistry::kArrowLocalFileIO); | ||
| } else { | ||
| ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse)); | ||
| io_impl = std::string(BuiltinFileIOName(detected_kind)); | ||
| } | ||
| ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse)); | ||
| io_impl = std::string(BuiltinFileIOName(detected_kind)); | ||
| } | ||
|
|
||
| if (!warehouse.empty() && IsBuiltinImpl(io_impl)) { | ||
|
|
@@ -92,4 +95,71 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const RestCatalogProperties& c | |
| return FileIORegistry::Load(io_impl, config.configs()); | ||
| } | ||
|
|
||
| namespace { | ||
|
|
||
| const StorageCredential* ResolveStorageCredential( | ||
| const std::vector<StorageCredential>& credentials, std::string_view location) { | ||
| const StorageCredential* best = nullptr; | ||
| for (const auto& cred : credentials) { | ||
| if (location.starts_with(cred.prefix)) { | ||
| if (!best || cred.prefix.size() > best->prefix.size()) { | ||
| best = &cred; | ||
| } | ||
| } | ||
| } | ||
| return best; | ||
| } | ||
|
|
||
| std::unordered_map<std::string, std::string> MergeTableProperties( | ||
| const std::unordered_map<std::string, std::string>& catalog_props, | ||
| const std::unordered_map<std::string, std::string>& table_config, | ||
| const std::unordered_map<std::string, std::string>& credential_config) { | ||
| auto merged = catalog_props; | ||
| for (const auto& [k, v] : table_config) { | ||
| merged[k] = v; | ||
| } | ||
| for (const auto& [k, v] : credential_config) { | ||
| merged[k] = v; | ||
| } | ||
| return merged; | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| Result<std::shared_ptr<FileIO>> ResolveTableFileIO( | ||
| const std::shared_ptr<FileIO>& catalog_io, | ||
| const std::unordered_map<std::string, std::string>& catalog_props, | ||
| std::string_view warehouse, const LoadTableResult& result) { | ||
| if (result.config.empty() && result.storage_credentials.empty()) { | ||
| return catalog_io; | ||
| } | ||
|
|
||
| // Merge order: catalog props < table config < storage credentials (highest priority). | ||
| const StorageCredential* cred = nullptr; | ||
| if (!result.metadata_location.empty()) { | ||
| cred = ResolveStorageCredential(result.storage_credentials, result.metadata_location); | ||
| } | ||
| const std::unordered_map<std::string, std::string> kEmpty; | ||
| auto merged = | ||
| MergeTableProperties(catalog_props, result.config, cred ? cred->config : kEmpty); | ||
|
|
||
| // Detect FileIO type: explicit io-impl > warehouse scheme > metadata_location scheme. | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. matches pyiceberg? |
||
| std::string io_impl; | ||
| if (auto it = merged.find(std::string(RestCatalogProperties::kIOImpl.key())); | ||
| it != merged.end()) { | ||
| io_impl = it->second; | ||
| } else if (!warehouse.empty()) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto kind, DetectBuiltinFileIO(warehouse)); | ||
| io_impl = std::string(BuiltinFileIOName(kind)); | ||
| } else if (!result.metadata_location.empty()) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto kind, DetectBuiltinFileIO(result.metadata_location)); | ||
| io_impl = std::string(BuiltinFileIOName(kind)); | ||
| } else { | ||
| return catalog_io; | ||
| } | ||
|
|
||
| ICEBERG_ASSIGN_OR_RAISE(auto table_io, FileIORegistry::Load(io_impl, merged)); | ||
| return std::shared_ptr<FileIO>(std::move(table_io)); | ||
| } | ||
|
|
||
| } // namespace iceberg::rest | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ | |
| #include <span> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
||
| #include "iceberg/iceberg_export.h" | ||
|
|
@@ -118,6 +119,14 @@ class ICEBERG_EXPORT FileIO { | |
| FileIO() = default; | ||
| virtual ~FileIO() = default; | ||
|
|
||
| /// \brief Returns the configuration properties used to initialize this FileIO. | ||
| /// | ||
| /// Engines that need to configure their own storage access (e.g., for credential | ||
| /// vending) can read these properties to obtain the resolved credentials. | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Real-world engine validation: Trino Trino (a major Java query engine) validates this exact pattern. Its The flow: See: |
||
| const std::unordered_map<std::string, std::string>& properties() const { | ||
| return properties_; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why We initially had
No implementation exposes credentials on the Table object itself. |
||
| } | ||
|
|
||
| /// \brief Create an input file handle for the given location. | ||
| virtual Result<std::unique_ptr<InputFile>> NewInputFile(std::string file_location); | ||
|
|
||
|
|
@@ -165,6 +174,10 @@ class ICEBERG_EXPORT FileIO { | |
| /// \param file_locations The locations of the files to delete. | ||
| /// \return void if all deletes succeed, or an error code if any delete fails. | ||
| virtual Status DeleteFiles(const std::vector<std::string>& file_locations); | ||
|
|
||
| private: | ||
| friend class FileIORegistry; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rev: PR comment on this please. friend is controversial
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a second factory path emerges later (e.g., custom FileIO outside the registry), we can add a protected setter at that point. |
||
| std::unordered_map<std::string, std::string> properties_; | ||
| }; | ||
|
|
||
| } // namespace iceberg | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |
| #include <mutex> | ||
| #include <utility> | ||
|
|
||
| #include "iceberg/util/macros.h" | ||
|
|
||
| namespace iceberg { | ||
|
|
||
| namespace { | ||
|
|
@@ -55,7 +57,9 @@ Result<std::unique_ptr<FileIO>> FileIORegistry::Load( | |
| } | ||
| factory = it->second; | ||
| } | ||
| return factory(properties); | ||
| ICEBERG_ASSIGN_OR_RAISE(auto io, factory(properties)); | ||
| io->properties_ = properties; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return io; | ||
| } | ||
|
|
||
| } // namespace iceberg | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is change from previously returning
InvalidArgument, is this approach to credential vending fine?