address review feedback

Author: plusplusjiajia

.github/workflows/s3_test.yml .github/workflows/aws_test.yml.github/workflows/s3_test.yml renamed to .github/workflows/aws_test.yml

@@ -15,8 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 
-# S3-backed tests against MinIO (Linux and macOS only).
-name: S3 Tests
+# AWS-related tests. ICEBERG_S3 (Arrow's bundled AWS SDK) and ICEBERG_SIGV4
+# (vcpkg-installed aws-cpp-sdk-core) are tested in separate jobs: linking
+# both into one binary causes ODR conflicts on the shared AWS SDK symbols.
+name: AWS Tests
 
 on:
   push:
@@ -39,21 +41,31 @@ env:
   ICEBERG_HOME: /tmp/iceberg
 
 jobs:
-  s3-minio:
+  aws:
     if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }}
-    name: S3 (${{ matrix.title }})
+    name: AWS (${{ matrix.title }})
     runs-on: ${{ matrix.runs-on }}
-    timeout-minutes: 35
+    timeout-minutes: 45
     strategy:
       fail-fast: false
       matrix:
         include:
-          - title: AMD64 Ubuntu 24.04
+          - title: AMD64 Ubuntu 24.04, S3
             runs-on: ubuntu-24.04
             CC: gcc-14
             CXX: g++-14
-          - title: AArch64 macOS 26
+            s3: "ON"
+            sigv4: "OFF"
+          - title: AMD64 Ubuntu 24.04, SigV4
+            runs-on: ubuntu-24.04
+            CC: gcc-14
+            CXX: g++-14
+            s3: "OFF"
+            sigv4: "ON"
+          - title: AArch64 macOS 26, S3
             runs-on: macos-26
+            s3: "ON"
+            sigv4: "OFF"
     env:
       ICEBERG_TEST_S3_URI: s3://iceberg-test
       AWS_ACCESS_KEY_ID: minio
@@ -70,6 +82,17 @@ jobs:
         if: ${{ startsWith(matrix.runs-on, 'ubuntu') }}
         shell: bash
         run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
+      - name: Cache vcpkg packages
+        if: ${{ matrix.sigv4 == 'ON' }}
+        uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0
+        id: vcpkg-cache
+        with:
+          path: /usr/local/share/vcpkg/installed
+          key: vcpkg-x64-linux-aws-sdk-cpp-core-${{ hashFiles('.github/workflows/aws_test.yml') }}
+      - name: Install AWS SDK via vcpkg
+        if: ${{ matrix.sigv4 == 'ON' && steps.vcpkg-cache.outputs.cache-hit != 'true' }}
+        shell: bash
+        run: vcpkg install aws-sdk-cpp[core]:x64-linux
       - name: Set Ubuntu Compilers
         if: ${{ startsWith(matrix.runs-on, 'ubuntu') }}
         run: |
@@ -82,8 +105,11 @@ jobs:
           brew install fmt
           echo "CMAKE_PREFIX_PATH=$(brew --prefix fmt):${CMAKE_PREFIX_PATH:-}" >> "${GITHUB_ENV}"
       - name: Start MinIO
+        if: ${{ matrix.s3 == 'ON' }}
         shell: bash
         run: bash ci/scripts/start_minio.sh
-      - name: Build and test Iceberg with S3
+      - name: Build and test Iceberg
         shell: bash
-        run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF ON
+        env:
+          CMAKE_TOOLCHAIN_FILE: ${{ matrix.sigv4 == 'ON' && '/usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake' || '' }}
+        run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF ${{ matrix.s3 }} ${{ matrix.sigv4 }}

.github/workflows/sigv4_test.yml

@@ -60,9 +60,6 @@ if is_windows; then
     CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Release")
 else
     CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Debug")
-    if [[ -n "${CMAKE_TOOLCHAIN_FILE:-}" ]]; then
-        CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}")
-    fi
 fi
 
 if [[ "${build_enable_sccache}" == "ON" ]]; then

ci/scripts/build_iceberg.sh

@@ -549,6 +549,11 @@ function(resolve_aws_sdk_dependency)
   set(ICEBERG_SYSTEM_DEPENDENCIES
       ${ICEBERG_SYSTEM_DEPENDENCIES}
       PARENT_SCOPE)
+  # Forwarded to find_dependency(AWSSDK ...) in iceberg-config.cmake.in so
+  # downstream installed builds load aws-cpp-sdk-core via AWSSDK_FIND_COMPONENTS.
+  set(ICEBERG_FIND_EXTRA_ARGS_AWSSDK
+      "COMPONENTS;core"
+      PARENT_SCOPE)
 endfunction()
 
 if(ICEBERG_SIGV4)

cmake_modules/IcebergThirdpartyToolchain.cmake

@@ -28,6 +28,7 @@ set(ICEBERG_REST_SOURCES
     endpoint.cc
     error_handlers.cc
     http_client.cc
+    http_request.cc
     json_serde.cc
     resource_paths.cc
     rest_catalog.cc

src/iceberg/catalog/rest/CMakeLists.txt

@@ -38,8 +38,7 @@ Result<std::shared_ptr<AuthSession>> AuthManager::InitSession(
 }
 
 Result<std::shared_ptr<AuthSession>> AuthManager::ContextualSession(
-    [[maybe_unused]] const std::unordered_map<std::string, std::string>& context,
-    std::shared_ptr<AuthSession> parent) {
+    [[maybe_unused]] const SessionContext& context, std::shared_ptr<AuthSession> parent) {
   // By default, return the parent session as-is
   return parent;
 }

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

@@ -23,6 +23,7 @@
 #include <string>
 #include <unordered_map>
 
+#include "iceberg/catalog/rest/auth/session_context.h"
 #include "iceberg/catalog/rest/iceberg_rest_export.h"
 #include "iceberg/catalog/rest/type_fwd.h"
 #include "iceberg/result.h"
@@ -70,13 +71,12 @@ class ICEBERG_REST_EXPORT AuthManager {
   /// This method is used by SessionCatalog to create sessions for different contexts
   /// (e.g., different users or tenants).
   ///
-  /// \param context Context properties (e.g., user credentials, tenant info).
+  /// \param context Per-session properties and credentials.
   /// \param parent Catalog session to inherit from or return as-is.
   /// \return A context-specific session, or the parent session if no context-specific
   /// session is needed, or an error if session creation fails.
   virtual Result<std::shared_ptr<AuthSession>> ContextualSession(
-      const std::unordered_map<std::string, std::string>& context,
-      std::shared_ptr<AuthSession> parent);
+      const SessionContext& context, std::shared_ptr<AuthSession> parent);
 
   /// \brief Create or reuse a session scoped to a single table/view.
   ///

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

@@ -46,6 +46,12 @@ const std::unordered_set<std::string, StringHash, StringEqual>& KnownAuthTypes()
 // Infer the authentication type from properties.
 std::string InferAuthType(
     const std::unordered_map<std::string, std::string>& properties) {
+  // Deprecated alias: rest.sigv4-enabled=true forces SigV4.
+  if (auto it = properties.find(AuthProperties::kSigV4Enabled);
+      it != properties.end() && StringUtils::EqualsIgnoreCase(it->second, "true")) {
+    return AuthProperties::kAuthTypeSigV4;
+  }
+
   auto it = properties.find(AuthProperties::kAuthType);
   if (it != properties.end() && !it->second.empty()) {
     return StringUtils::ToLower(it->second);
@@ -61,8 +67,8 @@ std::string InferAuthType(
   return AuthProperties::kAuthTypeNone;
 }
 
-AuthManagerRegistry CreateDefaultRegistry() {
-  AuthManagerRegistry registry = {
+AuthManagerRegistry& GetRegistry() {
+  static AuthManagerRegistry registry = {
       {AuthProperties::kAuthTypeNone, MakeNoopAuthManager},
       {AuthProperties::kAuthTypeBasic, MakeBasicAuthManager},
       {AuthProperties::kAuthTypeOAuth2, MakeOAuth2Manager},
@@ -71,12 +77,6 @@ AuthManagerRegistry CreateDefaultRegistry() {
   return registry;
 }
 
-// Get the global registry of auth manager factories.
-AuthManagerRegistry& GetRegistry() {
-  static AuthManagerRegistry registry = CreateDefaultRegistry();
-  return registry;
-}
-
 }  // namespace
 
 void AuthManagers::Register(std::string_view auth_type, AuthManagerFactory factory) {

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

@@ -54,6 +54,9 @@ class ICEBERG_REST_EXPORT AuthProperties : public ConfigBase<AuthProperties> {
 
   // ---- SigV4 entries ----
 
+  /// Deprecated: `rest.sigv4-enabled=true` selects SigV4 regardless of
+  /// `rest.auth.type`.
+  inline static const std::string kSigV4Enabled = "rest.sigv4-enabled";
   inline static const std::string kSigV4DelegateAuthType =
       "rest.auth.sigv4.delegate-auth-type";
   inline static const std::string kSigV4SigningRegion = "rest.signing-region";

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

@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+
+namespace iceberg::rest::auth {
+
+/// \brief Per-session context passed to AuthManager::ContextualSession.
+///
+/// Mirrors Java's `SessionCatalog.SessionContext`. Separate `properties` and
+/// `credentials` so per-context credential overrides don't silently collapse
+/// into properties.
+struct ICEBERG_REST_EXPORT SessionContext {
+  std::unordered_map<std::string, std::string> properties;
+  std::unordered_map<std::string, std::string> credentials;
+};
+
+}  // namespace iceberg::rest::auth