feat: add restcatalog authentication api #479

lishuxu · 2026-01-02T10:24:36Z

feat: add restcatalog authentication api

HeartLinked · 2026-01-13T08:15:56Z

src/iceberg/catalog/rest/auth/auth_manager.h

+  /// \param properties Configuration properties.
+  /// \return A session for initialization, or the catalog session by default.
+  virtual std::shared_ptr<AuthSession> InitSession(
+      HttpClient* init_client,


why use raw pointer？🤔 use std::shared_ptr instead?

AuthManager simply borrows the HTTP client owned by the catalog, it isn’t supposed to manage that client’s lifetime, so here use raw pointer.

Perhaps it is better to use HttpClient& to save the null check?

src/iceberg/catalog/rest/auth/auth_manager.h

wgtmac · 2026-01-14T15:39:48Z

src/iceberg/catalog/rest/auth/CMakeLists.txt

+set(ICEBERG_REST_AUTH_SOURCES auth_session.cc auth_manager.cc auth_managers.cc)
+
+# Expose sources to parent scope for inclusion in iceberg_rest library
+set(ICEBERG_REST_AUTH_SOURCES
+    ${ICEBERG_REST_AUTH_SOURCES}
+    PARENT_SCOPE)


Suggested change

set(ICEBERG_REST_AUTH_SOURCES auth_session.cc auth_manager.cc auth_managers.cc)

# Expose sources to parent scope for inclusion in iceberg_rest library

set(ICEBERG_REST_AUTH_SOURCES

${ICEBERG_REST_AUTH_SOURCES}

PARENT_SCOPE)

iceberg_install_all_headers(iceberg/catalog/rest/auth)

The current pattern is inconsistent with other CMake config. We should only install headers for this subdirectory here.

wgtmac · 2026-01-14T15:40:55Z

src/iceberg/catalog/rest/CMakeLists.txt

+# Add auth sources with proper path prefix
+foreach(AUTH_SOURCE ${ICEBERG_REST_AUTH_SOURCES})
+  list(APPEND ICEBERG_REST_SOURCES "auth/${AUTH_SOURCE}")
+endforeach()
+


Suggested change

# Add auth sources with proper path prefix

foreach(AUTH_SOURCE ${ICEBERG_REST_AUTH_SOURCES})

list(APPEND ICEBERG_REST_SOURCES "auth/${AUTH_SOURCE}")

endforeach()

Let's remove these lines by directly adding those files in the above set(ICEBERG_REST_SOURCES).

wgtmac · 2026-01-14T15:41:11Z

src/iceberg/catalog/rest/CMakeLists.txt

 # specific language governing permissions and limitations
 # under the License.

+# Include auth subdirectory


Suggested change

# Include auth subdirectory

This comment is unnecessary.

wgtmac · 2026-01-14T15:41:42Z

src/iceberg/catalog/rest/type_fwd.h


 }  // namespace iceberg::rest
+
+namespace iceberg::rest::auth {


Do we really need the extra auth namespace?

Here mirrors the Java package structure (org.apache.iceberg.rest.auth), making it easier to navigate between implementations.

WZhuo · 2026-01-15T02:06:06Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+/// \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 is a StringUtils::ToLower func in util/string_util.h

WZhuo · 2026-01-15T02:16:15Z

src/iceberg/catalog/rest/auth/auth_session.cc

+  void Close() override {
+    // No resources to release
+  }


Can be removed if do nothing.

wgtmac · 2026-01-15T06:03:57Z

src/iceberg/catalog/rest/auth/auth_manager.h

+/// \brief Authentication manager interface for REST catalog.
+
+namespace iceberg::rest {
+class HttpClient;


Remove this by include type_fwd.h.

wgtmac · 2026-01-15T06:04:09Z

src/iceberg/catalog/rest/auth/auth_manager.h

+#include <string>
+#include <unordered_map>
+
+#include "iceberg/catalog/rest/auth/auth_session.h"


Remove this to use forward declaration.

wgtmac · 2026-01-15T06:04:28Z

src/iceberg/catalog/rest/auth/auth_manager.h

+///
+/// This interface is modeled after Java Iceberg's AuthManager interface.


Suggested change

///

/// This interface is modeled after Java Iceberg's AuthManager interface.

wgtmac · 2026-01-15T06:05:44Z

src/iceberg/catalog/rest/auth/auth_manager.h

+  /// \param properties Configuration properties.
+  /// \return A session for initialization, or the catalog session by default.
+  virtual std::shared_ptr<AuthSession> InitSession(
+      HttpClient* init_client,


Perhaps it is better to use HttpClient& to save the null check?

wgtmac · 2026-01-15T06:18:12Z

src/iceberg/catalog/rest/auth/auth_manager.h

+  /// \param properties Configuration properties (merged with server config).
+  /// \return A session for catalog operations, or an error if session creation fails
+  ///         (e.g., missing required credentials, network failure during token fetch).
+  virtual Result<std::shared_ptr<AuthSession>> CatalogSession(


wgtmac · 2026-01-15T06:57:22Z

src/iceberg/catalog/rest/auth/auth_session.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;


Do you think it is worth adding a class HttpRequest? We don't have it yet.

wgtmac · 2026-01-15T06:58:28Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+/// OAuth2 authentication. Otherwise, defaults to no authentication.
+///
+/// This behavior is consistent with Java Iceberg's AuthManagers.
+std::string InferAuthType(


Should we add a enum class for the auth type?

wgtmac · 2026-01-15T06:59:31Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+}  // namespace
+
+std::unordered_map<std::string, AuthManagerFactory>& AuthManagers::GetRegistry() {
+  static std::unordered_map<std::string, AuthManagerFactory> registry;


Suggested change

static std::unordered_map<std::string, AuthManagerFactory> registry;

static std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual> registry;

Let's support heterogeneous lookup

wgtmac · 2026-01-15T07:00:35Z

src/iceberg/catalog/rest/auth/auth_managers.h

+
+ private:
+  /// \brief Get the global registry of auth manager factories.
+  static std::unordered_map<std::string, AuthManagerFactory>& GetRegistry();


We don't need this function in the header file, right?

wgtmac · 2026-01-15T07:02:11Z

src/iceberg/catalog/rest/auth/auth_managers.h

+  /// other properties (e.g., presence of "credential" or "token" implies oauth2).
+  /// If no auth-related properties are found, it defaults to "none".
+  ///
+  /// \param name A name for the manager (used for logging).


Suggested change

/// \param name A name for the manager (used for logging).

/// \param name A name for the manager.

shuxu.li added 3 commits January 2, 2026 18:23

feat: add restcatalog authentication api

572d02a

feat: add restcatalog authentication api

904fdf0

feat: add restcatalog authentication api

83084ad

HeartLinked reviewed Jan 13, 2026

View reviewed changes

feat: add restcatalog authentication api

72df76d

wgtmac reviewed Jan 14, 2026

View reviewed changes

WZhuo reviewed Jan 15, 2026

View reviewed changes

wgtmac requested changes Jan 15, 2026

View reviewed changes

		///
		/// This interface is modeled after Java Iceberg's AuthManager interface.

	static std::unordered_map<std::string, AuthManagerFactory> registry;
	static std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual> registry;

	/// \param name A name for the manager (used for logging).
	/// \param name A name for the manager.

feat: add restcatalog authentication api #479

Are you sure you want to change the base?

feat: add restcatalog authentication api #479

Uh oh!

Conversation

lishuxu commented Jan 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants