address review feedback

Author: plusplusjiajia

.github/workflows/cpp-linter.yml

@@ -40,25 +40,13 @@ jobs:
       - name: Install dependencies
         shell: bash
         run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
-      - name: Cache vcpkg packages
-        uses: actions/cache@v4
-        id: vcpkg-cache
-        with:
-          path: /usr/local/share/vcpkg/installed
-          key: vcpkg-x64-linux-aws-sdk-cpp-core-${{ hashFiles('.github/workflows/cpp-linter.yml') }}
-      - name: Install AWS SDK via vcpkg
-        if: steps.vcpkg-cache.outputs.cache-hit != 'true'
-        shell: bash
-        run: vcpkg install aws-sdk-cpp[core]:x64-linux
       - name: Run build
         env:
           CC: gcc-14
           CXX: g++-14
         run: |
           mkdir build && cd build
-          cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-            -DICEBERG_BUILD_SIGV4=ON \
-            -DCMAKE_TOOLCHAIN_FILE=/usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake
+          cmake .. -G Ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
           cmake --build .
       - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
         id: linter

.github/workflows/sigv4_test.yml

@@ -0,0 +1,71 @@
+# 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.
+
+# SigV4 build + unit tests (Linux only; aws-cpp-sdk-core via vcpkg).
+name: SigV4 Tests
+
+on:
+  push:
+    branches:
+      - '**'
+      - '!dependabot/**'
+    tags:
+      - '**'
+  pull_request:
+
+concurrency:
+  group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+env:
+  ICEBERG_HOME: /tmp/iceberg
+
+jobs:
+  sigv4:
+    name: SigV4 (AMD64 Ubuntu 24.04)
+    runs-on: ubuntu-24.04
+    timeout-minutes: 35
+    env:
+      CC: gcc-14
+      CXX: g++-14
+      AWS_EC2_METADATA_DISABLED: "TRUE"
+    steps:
+      - name: Checkout iceberg-cpp
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+        with:
+          persist-credentials: false
+      - name: Install dependencies
+        shell: bash
+        run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
+      - name: Cache vcpkg packages
+        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/sigv4_test.yml') }}
+      - name: Install AWS SDK via vcpkg
+        if: steps.vcpkg-cache.outputs.cache-hit != 'true'
+        shell: bash
+        run: vcpkg install aws-sdk-cpp[core]:x64-linux
+      - name: Build and test Iceberg with SigV4
+        shell: bash
+        env:
+          CMAKE_TOOLCHAIN_FILE: /usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake
+        run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF OFF ON

CMakeLists.txt

@@ -46,7 +46,7 @@ option(ICEBERG_BUILD_BUNDLE "Build the battery included library" ON)
 option(ICEBERG_BUILD_REST "Build rest catalog client" ON)
 option(ICEBERG_BUILD_REST_INTEGRATION_TESTS "Build rest catalog integration tests" OFF)
 option(ICEBERG_S3 "Build with S3 support" OFF)
-option(ICEBERG_BUILD_SIGV4 "Build SigV4 authentication support (requires AWS SDK)" OFF)
+option(ICEBERG_SIGV4 "Build SigV4 authentication support (requires AWS SDK)" OFF)
 option(ICEBERG_ENABLE_ASAN "Enable Address Sanitizer" OFF)
 option(ICEBERG_ENABLE_UBSAN "Enable Undefined Behavior Sanitizer" OFF)
 

ci/scripts/build_iceberg.sh

@@ -17,7 +17,7 @@
 # specific language governing permissions and limitations
 # under the License.
 #
-# Usage: build_iceberg.sh <source_dir> [rest_integration_tests=OFF] [sccache=OFF] [s3=OFF]
+# Usage: build_iceberg.sh <source_dir> [rest_integration_tests=OFF] [sccache=OFF] [s3=OFF] [sigv4=OFF]
 
 set -eux
 
@@ -26,6 +26,7 @@ build_dir=${1}/build
 build_rest_integration_test=${2:-OFF}
 build_enable_sccache=${3:-OFF}
 build_enable_s3=${4:-OFF}
+build_enable_sigv4=${5:-OFF}
 
 mkdir ${build_dir}
 pushd ${build_dir}
@@ -48,11 +49,20 @@ else
     CMAKE_ARGS+=("-DICEBERG_S3=OFF")
 fi
 
+if [[ "${build_enable_sigv4}" == "ON" ]]; then
+    CMAKE_ARGS+=("-DICEBERG_SIGV4=ON")
+else
+    CMAKE_ARGS+=("-DICEBERG_SIGV4=OFF")
+fi
+
 if is_windows; then
     CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake")
     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

cmake_modules/IcebergThirdpartyToolchain.cmake

@@ -551,9 +551,9 @@ function(resolve_aws_sdk_dependency)
       PARENT_SCOPE)
 endfunction()
 
-if(ICEBERG_BUILD_SIGV4)
+if(ICEBERG_SIGV4)
   if(NOT ICEBERG_BUILD_REST)
-    message(FATAL_ERROR "ICEBERG_BUILD_SIGV4 requires ICEBERG_BUILD_REST to be ON")
+    message(FATAL_ERROR "ICEBERG_SIGV4 requires ICEBERG_BUILD_REST to be ON")
   endif()
   resolve_aws_sdk_dependency()
 endif()

src/iceberg/catalog/rest/CMakeLists.txt

@@ -34,9 +34,7 @@ set(ICEBERG_REST_SOURCES
     rest_util.cc
     types.cc)
 
-if(ICEBERG_BUILD_SIGV4)
-  list(APPEND ICEBERG_REST_SOURCES auth/sigv4_auth_manager.cc)
-endif()
+list(APPEND ICEBERG_REST_SOURCES auth/sigv4_auth_manager.cc)
 
 set(ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS)
 set(ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS)
@@ -56,7 +54,7 @@ list(APPEND
      "$<IF:$<TARGET_EXISTS:iceberg::iceberg_shared>,iceberg::iceberg_shared,iceberg::iceberg_static>"
      "$<IF:$<BOOL:${CPR_VENDORED}>,iceberg::cpr,cpr::cpr>")
 
-if(ICEBERG_BUILD_SIGV4)
+if(ICEBERG_SIGV4)
   list(APPEND ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS aws-cpp-sdk-core)
   list(APPEND ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS aws-cpp-sdk-core)
   list(APPEND ICEBERG_REST_STATIC_INSTALL_INTERFACE_LIBS aws-cpp-sdk-core)
@@ -75,10 +73,10 @@ add_iceberg_lib(iceberg_rest
                 SHARED_INSTALL_INTERFACE_LIBS
                 ${ICEBERG_REST_SHARED_INSTALL_INTERFACE_LIBS})
 
-if(ICEBERG_BUILD_SIGV4)
+if(ICEBERG_SIGV4)
   foreach(LIB iceberg_rest_static iceberg_rest_shared)
     if(TARGET ${LIB})
-      target_compile_definitions(${LIB} PUBLIC ICEBERG_BUILD_SIGV4)
+      target_compile_definitions(${LIB} PUBLIC ICEBERG_SIGV4)
     endif()
   endforeach()
 endif()

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

@@ -47,11 +47,10 @@ Result<std::unique_ptr<AuthManager>> MakeOAuth2Manager(
     std::string_view name,
     const std::unordered_map<std::string, std::string>& properties);
 
-#ifdef ICEBERG_BUILD_SIGV4
-/// \brief Create a SigV4 authentication manager with a delegate.
+/// \brief Create a SigV4 authentication manager with a delegate. Returns
+/// NotSupported when the library was built without ICEBERG_SIGV4.
 Result<std::unique_ptr<AuthManager>> MakeSigV4AuthManager(
     std::string_view name,
     const std::unordered_map<std::string, std::string>& properties);
-#endif
 
 }  // namespace iceberg::rest::auth

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

@@ -66,10 +66,8 @@ AuthManagerRegistry CreateDefaultRegistry() {
       {AuthProperties::kAuthTypeNone, MakeNoopAuthManager},
       {AuthProperties::kAuthTypeBasic, MakeBasicAuthManager},
       {AuthProperties::kAuthTypeOAuth2, MakeOAuth2Manager},
+      {AuthProperties::kAuthTypeSigV4, MakeSigV4AuthManager},
   };
-#ifdef ICEBERG_BUILD_SIGV4
-  registry[AuthProperties::kAuthTypeSigV4] = MakeSigV4AuthManager;
-#endif
   return registry;
 }
 

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

@@ -33,8 +33,8 @@ class DefaultAuthSession : public AuthSession {
   explicit DefaultAuthSession(std::unordered_map<std::string, std::string> headers)
       : headers_(std::move(headers)) {}
 
-  Result<HTTPRequest> Authenticate(const HTTPRequest& request) override {
-    HTTPRequest authenticated = request;
+  Result<HttpRequest> Authenticate(const HttpRequest& request) override {
+    HttpRequest authenticated = request;
     for (const auto& [key, value] : headers_) {
       authenticated.headers.try_emplace(key, value);
     }

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

@@ -23,7 +23,7 @@
 #include <string>
 #include <unordered_map>
 
-#include "iceberg/catalog/rest/endpoint.h"
+#include "iceberg/catalog/rest/http_request.h"
 #include "iceberg/catalog/rest/iceberg_rest_export.h"
 #include "iceberg/catalog/rest/type_fwd.h"
 #include "iceberg/result.h"
@@ -33,17 +33,6 @@
 
 namespace iceberg::rest::auth {
 
-/// \brief An outgoing HTTP request passed through an AuthSession. Mirrors the
-/// HTTPRequest type used by the Java reference implementation so signing
-/// implementations like SigV4 can operate on method, url, headers, and body
-/// as a single value.
-struct ICEBERG_REST_EXPORT HTTPRequest {
-  HttpMethod method = HttpMethod::kGet;
-  std::string url;
-  std::unordered_map<std::string, std::string> headers;
-  std::string body;
-};
-
 /// \brief An authentication session that can authenticate outgoing HTTP requests.
 class ICEBERG_REST_EXPORT AuthSession {
  public:
@@ -63,7 +52,7 @@ class ICEBERG_REST_EXPORT AuthSession {
   ///         - NotAuthorized: Not authenticated (401)
   ///         - IOError: Network or connection errors when reaching auth server
   ///         - RestError: HTTP errors from authentication service
-  virtual Result<HTTPRequest> Authenticate(const HTTPRequest& request) = 0;
+  virtual Result<HttpRequest> Authenticate(const HttpRequest& request) = 0;
 
   /// \brief Close the session and release any resources.
   ///