feat: add restcatalog authentication api #479
Conversation
|
|
||
| } // namespace iceberg::rest | ||
|
|
||
| namespace iceberg::rest::auth { |
There was a problem hiding this comment.
Do we really need the extra auth namespace?
There was a problem hiding this comment.
Here mirrors the Java package structure (org.apache.iceberg.rest.auth), making it easier to navigate between implementations.
| /// \brief Convert a string to lowercase for case-insensitive comparison. | ||
| std::string ToLower(std::string_view str) { | ||
| std::string result(str); | ||
| std::ranges::transform(result, result.begin(), | ||
| [](unsigned char c) { return std::tolower(c); }); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
There is a StringUtils::ToLower func in util/string_util.h
| /// (i.e., request headers take precedence). | ||
| /// | ||
| /// \param headers The headers map to add authentication information to. | ||
| void Authenticate(std::unordered_map<std::string, std::string>& headers) override; |
There was a problem hiding this comment.
Do you think it is worth adding a class HttpRequest? We don't have it yet.
There was a problem hiding this comment.
I don't think we need an HttpRequest class at this stage. The current design is sufficient for most auth schemes (Basic, Bearer, OAuth2, API keys), but we can add can add it later if needed.
| /// OAuth2 authentication. Otherwise, defaults to no authentication. | ||
| /// | ||
| /// This behavior is consistent with Java Iceberg's AuthManagers. | ||
| std::string InferAuthType( |
There was a problem hiding this comment.
Should we add a enum class for the auth type?
There was a problem hiding this comment.
Auth types come from external config files/properties as strings, so the string-based approach allows users to register custom auth types without modifying core code. It also matches the Iceberg REST spec.
|
|
||
| private: | ||
| /// \brief Get the global registry of auth manager factories. | ||
| static std::unordered_map<std::string, AuthManagerFactory>& GetRegistry(); |
There was a problem hiding this comment.
We don't need this function in the header file, right?
| for (const auto& [key, value] : headers_) { | ||
| headers.try_emplace(key, value); | ||
| } |
There was a problem hiding this comment.
| for (const auto& [key, value] : headers_) { | |
| headers.try_emplace(key, value); | |
| } | |
| headers.merge(headers_); |
There was a problem hiding this comment.
merge() is destructive - it moves nodes from headers_ to headers, leaving headers_ empty or partially empty.
so, I think the current implementation can be retained.
5 participants
feat: add restcatalog authentication api