wgtmac commented on code in PR #544:
URL: https://github.com/apache/iceberg-cpp/pull/544#discussion_r2781026369
##########
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:
I think it is much cleaner to hide the auth api call in the http client to
avoid the repeated `Authenticate` call. In addition, for auth type like SigV4,
the authentication process requires more than headers so it requires
involvement of the http client as well. This is the same pattern used by the
Java `RESTClient` (though it uses `withAuthSession` as a stateful api which we
don't have to mimic).
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -33,6 +33,17 @@ namespace {
using AuthManagerRegistry =
std::unordered_map<std::string, AuthManagerFactory, StringHash,
StringEqual>;
+/// \brief Known authentication types that are defined in the Iceberg spec.
Review Comment:
```suggestion
```
I'd suggest removing this comment as they are self-descriptive and actually
not defined in the iceberg spec.
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -51,9 +62,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& client,
+ [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties)
+ override {
+ return AuthSession::MakeDefault({});
+ }
+};
+
// Get the global registry of auth manager factories.
AuthManagerRegistry& GetRegistry() {
- static AuthManagerRegistry registry;
+ static AuthManagerRegistry registry = [] {
+ AuthManagerRegistry r;
+ r[AuthProperties::kAuthTypeNone] =
Review Comment:
How about using the pattern below? Then each auth manager impl is required
to add a Make function with same signature.
```cpp
template <typename T>
AuthManagerFactory MakeAuthFactory() {
return [](std::string_view name,
const std::unordered_map<std::string, std::string>& props)
-> Result<std::unique_ptr<AuthManager>> {
return T::Make(name, props);
};
}
AuthManagerRegistry CreateDefaultRegistry() {
return {
{AuthProperties::kAuthTypeNone, MakeAuthFactory<NoopAuthManager>()},
// {AuthProperties::kAuthTypeOAuth, MakeAuthFactory<OAuthManager>()},
};
}
AuthManagerRegistry& GetRegistry() {
static AuthManagerRegistry registry = CreateDefaultRegistry();
return registry;
}
```
--
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]
