feat: Implement NoopAuthManager and integrate AuthManager into RestCa… #544
Conversation
lishuxu
commented
Jan 29, 2026
lishuxu
commented
- Add NoopAuthManager for "none" authentication type
- Register "none" auth type in static registry with case-insensitive matching
- Add KnownAuthTypes() to distinguish known-but-unimplemented types (NotImplemented) from unknown types (InvalidArgument)
- Integrate AuthManager into RestCatalog
|
|
||
| add_rest_iceberg_test(rest_catalog_test | ||
| SOURCES | ||
| auth_manager_test.cc |
There was a problem hiding this comment.
Please add it to the test in the meson.build as well.
| /// \brief Fetch server configuration from the REST catalog server. | ||
| Result<CatalogConfig> FetchServerConfig( | ||
| const ResourcePaths& paths, const RestCatalogProperties& current_config, | ||
| const std::shared_ptr<auth::AuthSession>& session) { |
There was a problem hiding this comment.
| const std::shared_ptr<auth::AuthSession>& session) { | |
| const auth::AuthSession& session) { |
If the session cannot be null and it is read-only, let's use its reference directly.
| ICEBERG_ASSIGN_OR_RAISE(auto config_path, paths.Config()); | ||
| HttpClient client(current_config.ExtractHeaders()); | ||
|
|
||
| // Get authentication headers |
There was a problem hiding this comment.
| // Get authentication headers |
We don't need such comment
|
|
||
| std::string_view RestCatalog::name() const { return name_; } | ||
|
|
||
| Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const { |
There was a problem hiding this comment.
Is it better to add AuthSession* session as an optional input parameter to HttpClient::Get (and its friends)? Auth headers should be handled internally in the client instead of scattering them around here.
|
|
||
| /// \brief Known authentication types that are defined in the Iceberg spec. | ||
| const std::unordered_set<std::string, StringHash, StringEqual>& KnownAuthTypes() { | ||
| static const std::unordered_set<std::string, StringHash, StringEqual> types = { |
There was a problem hiding this comment.
| static const std::unordered_set<std::string, StringHash, StringEqual> types = { | |
| static const std::unordered_set<std::string, StringHash, StringEqual> kAuthTypes = { |
| class NoopAuthManager : public AuthManager { | ||
| public: | ||
| Result<std::shared_ptr<AuthSession>> CatalogSession( | ||
| [[maybe_unused]] HttpClient& shared_client, |
There was a problem hiding this comment.
| [[maybe_unused]] HttpClient& shared_client, | |
| [[maybe_unused]] HttpClient& client, |
Let's rename it to remove shared_ prefix as it is misleading.
| [[maybe_unused]] HttpClient& shared_client, | ||
| [[maybe_unused]] const std::unordered_map<std::string, std::string>& properties) | ||
| override { | ||
| return AuthSession::MakeDefault({}); |
There was a problem hiding this comment.
Should we check auth type from properties and return error for unimplemented auth types instead of blindly returning the noop impl?
2 participants
