Skip to content

Conversation

@lishuxu
Copy link
Contributor

@lishuxu lishuxu commented Jan 29, 2026

  • 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

/// \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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the session argument type to auth::AuthSession&. Authenticate() is non-const to support OAuth2 token refresh.


std::string_view RestCatalog::name() const { return name_; }

Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current design keeps HttpClient focused on HTTP transport concerns, while RestCatalog handles authentication as a business-layer responsibility. So I think current design may be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


std::string_view RestCatalog::name() const { return name_; }

Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines 83 to 84
return []([[maybe_unused]] std::string_view name,
[[maybe_unused]] const std::unordered_map<std::string, std::string>& props)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return []([[maybe_unused]] std::string_view name,
[[maybe_unused]] const std::unordered_map<std::string, std::string>& props)
return [](std::string_view name,
const std::unordered_map<std::string, std::string>& props)

HttpClient& operator=(HttpClient&&) = delete;

/// \brief Set the authentication session
void SetAuthSession(auth::AuthSession* session);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right approach to introduce a stateful http client. Let's remove this function and extend below functions to accept a session like:

  Result<HttpResponse> Get(
    const std::string& path,
    const std::unordered_map<std::string, std::string>& params,
    const std::unordered_map<std::string, std::string>& headers,
    const ErrorHandler& error_handler,
    AuthSession& session);

The input session is a reference so it is required and a no-op session should be used if authentication is not enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Removed SetAuthSession and changed all HTTP methods to accept AuthSession& as a required reference parameter. When authentication is not enabled, NoopAuthManager produces a noop session via AuthSession::MakeDefault({})

ICEBERG_ASSIGN_OR_RAISE(
const auto response,
client_->Get(path, params, /*headers=*/{}, *NamespaceErrorHandler::Instance()));
client_->Get(path, params, *NamespaceErrorHandler::Instance()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use contextual session as the Java impl does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a session catalog yet, so there's no session context to derive a contextual session from. I think using catalog_session_ directly is equivalent to what Java's RESTCatalog does. Will add contextual session support when session-aware catalog is supported.

@wgtmac wgtmac changed the title feat: Implement NoopAuthManager and integrate AuthManager into RestCa… feat: Implement NoopAuthManager and integrate it with RestCatalog Feb 11, 2026
@wgtmac wgtmac merged commit 721e529 into apache:main Feb 11, 2026
10 checks passed
@lishuxu lishuxu deleted the feature/auth2 branch February 12, 2026 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants