wgtmac commented on code in PR #544:
URL: https://github.com/apache/iceberg-cpp/pull/544#discussion_r2748701707
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -215,6 +215,7 @@ if(ICEBERG_BUILD_REST)
add_rest_iceberg_test(rest_catalog_test
SOURCES
+ auth_manager_test.cc
Review Comment:
Please add it to the test in the `meson.build` as well.
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -51,9 +64,29 @@ std::string InferAuthType(
return AuthProperties::kAuthTypeNone;
}
+/// \brief Authentication manager that performs no authentication.
+class NoopAuthManager : public AuthManager {
+ public:
+ Result<std::shared_ptr<AuthSession>> CatalogSession(
+ [[maybe_unused]] HttpClient& shared_client,
Review Comment:
```suggestion
[[maybe_unused]] HttpClient& client,
```
Let's rename it to remove `shared_` prefix as it is misleading.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -65,13 +68,19 @@ std::unordered_set<Endpoint> GetDefaultEndpoints() {
};
}
-/// \brief Fetch server config and merge it with client config
-Result<CatalogConfig> FetchServerConfig(const ResourcePaths& paths,
- const RestCatalogProperties&
current_config) {
+/// \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) {
Review Comment:
```suggestion
const auth::AuthSession& session) {
```
If the session cannot be null and it is read-only, let's use its reference
directly.
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -33,6 +35,17 @@ namespace {
using AuthManagerRegistry =
std::unordered_map<std::string, AuthManagerFactory, StringHash,
StringEqual>;
+/// \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
= {
Review Comment:
```suggestion
static const std::unordered_set<std::string, StringHash, StringEqual>
kAuthTypes = {
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -65,13 +68,19 @@ std::unordered_set<Endpoint> GetDefaultEndpoints() {
};
}
-/// \brief Fetch server config and merge it with client config
-Result<CatalogConfig> FetchServerConfig(const ResourcePaths& paths,
- const RestCatalogProperties&
current_config) {
+/// \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) {
ICEBERG_ASSIGN_OR_RAISE(auto config_path, paths.Config());
HttpClient client(current_config.ExtractHeaders());
+
+ // Get authentication headers
Review Comment:
```suggestion
```
We don't need such comment
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -51,9 +64,29 @@ std::string InferAuthType(
return AuthProperties::kAuthTypeNone;
}
+/// \brief Authentication manager that performs no authentication.
+class NoopAuthManager : public AuthManager {
+ public:
+ Result<std::shared_ptr<AuthSession>> CatalogSession(
+ [[maybe_unused]] HttpClient& shared_client,
+ [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties)
+ override {
+ return AuthSession::MakeDefault({});
Review Comment:
Should we check auth type from properties and return error for unimplemented
auth types instead of blindly returning the noop impl?
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -139,27 +159,43 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
paths, ResourcePaths::Make(std::string(TrimTrailingSlash(final_uri)),
final_config->Get(RestCatalogProperties::kPrefix)));
- return std::shared_ptr<RestCatalog>(
- new RestCatalog(std::move(final_config), std::move(file_io),
std::move(paths),
- std::move(endpoints)));
+ auto client = std::make_unique<HttpClient>(final_config->ExtractHeaders());
+ ICEBERG_ASSIGN_OR_RAISE(auto catalog_session,
+ auth_manager->CatalogSession(*client,
final_config->configs()));
+ return std::shared_ptr<RestCatalog>(new RestCatalog(
+ std::move(final_config), std::move(file_io), std::move(client),
std::move(paths),
+ std::move(endpoints), std::move(auth_manager),
std::move(catalog_session)));
}
RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
std::shared_ptr<FileIO> file_io,
+ std::unique_ptr<HttpClient> client,
std::unique_ptr<ResourcePaths> paths,
- std::unordered_set<Endpoint> endpoints)
+ std::unordered_set<Endpoint> endpoints,
+ std::unique_ptr<auth::AuthManager> auth_manager,
+ std::shared_ptr<auth::AuthSession> catalog_session)
: config_(std::move(config)),
file_io_(std::move(file_io)),
- client_(std::make_unique<HttpClient>(config_->ExtractHeaders())),
+ client_(std::move(client)),
paths_(std::move(paths)),
name_(config_->Get(RestCatalogProperties::kName)),
- supported_endpoints_(std::move(endpoints)) {}
+ supported_endpoints_(std::move(endpoints)),
+ auth_manager_(std::move(auth_manager)),
+ catalog_session_(std::move(catalog_session)) {}
std::string_view RestCatalog::name() const { return name_; }
+Result<std::unordered_map<std::string, std::string>>
RestCatalog::AuthHeaders() const {
Review 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.
--
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]
