address review feedback: SigV4 region, session lifecycle, S3+SigV4, Meson

Author: plusplusjiajia

.github/workflows/aws_test.yml

@@ -15,9 +15,9 @@
 # specific language governing permissions and limitations
 # under the License.
 
-# 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.
+# AWS-related tests. ICEBERG_S3 and ICEBERG_SIGV4 are exercised individually and
+# together; with both on, Arrow's S3 reuses SigV4's system AWS SDK
+# (AWSSDK_SOURCE=SYSTEM) so a single AWS SDK is linked (no ODR).
 name: AWS Tests
 
 on:
@@ -62,6 +62,16 @@ jobs:
             CXX: g++-14
             s3: "OFF"
             sigv4: "ON"
+            aws-sdk-features: core
+          - title: AMD64 Ubuntu 24.04, S3 + SigV4
+            runs-on: ubuntu-24.04
+            CC: gcc-14
+            CXX: g++-14
+            s3: "ON"
+            sigv4: "ON"
+            # Arrow's S3 filesystem consumes this same AWS SDK, so it needs the
+            # S3-related components in addition to core.
+            aws-sdk-features: core,s3,identity-management,sts,transfer
           - title: AArch64 macOS 26, S3
             runs-on: macos-26
             s3: "ON"
@@ -88,11 +98,11 @@ jobs:
         id: vcpkg-cache
         with:
           path: /usr/local/share/vcpkg/installed
-          key: vcpkg-x64-linux-aws-sdk-cpp-core-${{ hashFiles('.github/workflows/aws_test.yml') }}
+          key: vcpkg-x64-linux-aws-sdk-cpp-s3-${{ matrix.s3 }}-sigv4-${{ matrix.sigv4 }}-${{ 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
+        run: vcpkg install "aws-sdk-cpp[${{ matrix.aws-sdk-features }}]:x64-linux"
       - name: Set Ubuntu Compilers
         if: ${{ startsWith(matrix.runs-on, 'ubuntu') }}
         run: |
@@ -113,3 +123,47 @@ jobs:
         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 }}
+
+  # Exercise the Meson build with SigV4 enabled (resolves aws-cpp-sdk-core via
+  # its CMake config, not pkg-config whose Cflags force -std=c++11).
+  meson-sigv4:
+    if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }}
+    name: Meson SigV4 (AMD64 Ubuntu 24.04)
+    runs-on: ubuntu-24.04
+    timeout-minutes: 45
+    steps:
+      - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0
+        with:
+          python-version: '3.x'
+      - name: Checkout iceberg-cpp
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+        with:
+          persist-credentials: false
+      - name: Install build dependencies
+        shell: bash
+        run: |
+          sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
+          python3 -m pip install --upgrade pip
+          python3 -m pip install -r requirements.txt
+      - 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/aws_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: Set Ubuntu Compilers
+        run: |
+          echo "CC=gcc-14" >> $GITHUB_ENV
+          echo "CXX=g++-14" >> $GITHUB_ENV
+      - name: Build and test Iceberg
+        shell: bash
+        env:
+          CMAKE_PREFIX_PATH: /usr/local/share/vcpkg/installed/x64-linux
+        run: |
+          meson setup builddir -Dsigv4=enabled
+          meson compile -C builddir
+          meson test -C builddir --timeout-multiplier 0 --print-errorlogs

cmake_modules/IcebergThirdpartyToolchain.cmake

@@ -110,6 +110,11 @@ function(resolve_arrow_dependency)
   set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
   set(ARROW_POSITION_INDEPENDENT_CODE ON)
   set(ARROW_DEPENDENCY_SOURCE "BUNDLED")
+  # With SigV4 also on, make Arrow's S3 reuse the system AWS SDK (the one SigV4
+  # finds) instead of bundling its own, so only one AWS SDK is linked (no ODR).
+  if(ICEBERG_S3 AND ICEBERG_SIGV4)
+    set(AWSSDK_SOURCE "SYSTEM")
+  endif()
   set(ARROW_WITH_ZLIB ON)
   set(ZLIB_SOURCE "SYSTEM")
   set(ARROW_VERBOSE_THIRDPARTY_BUILD OFF)

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

@@ -78,12 +78,13 @@ class OAuth2AuthSession : public AuthSession,
     return session;
   }
 
-  Status Authenticate(std::unordered_map<std::string, std::string>& headers) override {
+  Result<HttpRequest> Authenticate(const HttpRequest& request) override {
+    HttpRequest authenticated = request;
     std::shared_lock lock(mutex_);
     for (const auto& [key, value] : headers_) {
-      headers.try_emplace(key, value);
+      authenticated.headers.try_emplace(key, value);
     }
-    return {};
+    return authenticated;
   }
 
   Status Close() override { return CloseImpl(); }

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

@@ -32,7 +32,9 @@
 #  include <aws/core/auth/AWSCredentialsProvider.h>
 #  include <aws/core/auth/AWSCredentialsProviderChain.h>
 #  include <aws/core/client/ClientConfiguration.h>
+#  include <aws/core/config/ConfigAndCredentialsCacheManager.h>
 #  include <aws/core/http/standard/StandardHttpRequest.h>
+#  include <aws/core/platform/Environment.h>
 #  include <aws/core/utils/HashingUtils.h>
 
 #  include "iceberg/catalog/rest/auth/auth_managers.h"
@@ -170,7 +172,11 @@ SigV4AuthSession::SigV4AuthSession(
       signer_(std::make_unique<RestSigV4Signer>(
           credentials_provider_, signing_name_.c_str(), signing_region_.c_str())) {}
 
-SigV4AuthSession::~SigV4AuthSession() { AwsSdkLifecycle::Instance().UnregisterSession(); }
+SigV4AuthSession::~SigV4AuthSession() {
+  if (owns_sdk_registration_) {
+    AwsSdkLifecycle::Instance().UnregisterSession();
+  }
+}
 
 Result<HttpRequest> SigV4AuthSession::Authenticate(const HttpRequest& request) {
   ICEBERG_ASSIGN_OR_RAISE(auto delegate_request, delegate_->Authenticate(request));
@@ -338,18 +344,36 @@ SigV4AuthManager::MakeCredentialsProvider(
   return std::make_shared<Aws::Auth::DefaultAWSCredentialsProviderChain>();
 }
 
-std::string SigV4AuthManager::ResolveSigningRegion(
+Result<std::string> SigV4AuthManager::ResolveSigningRegion(
     const std::unordered_map<std::string, std::string>& properties) {
   if (auto it = properties.find(AuthProperties::kSigV4SigningRegion);
       it != properties.end() && !it->second.empty()) {
     return it->second;
   }
-  // ClientConfiguration() walks env / profile / IMDS / us-east-1; the IMDS
-  // step can block for seconds on non-EC2 hosts. Resolve once per process
-  // (set AWS_EC2_METADATA_DISABLED=true to skip IMDS).
-  static const std::string kSdkResolvedRegion =
-      std::string(Aws::Client::ClientConfiguration().region.c_str());
-  return kSdkResolvedRegion;
+  // Resolve from env then the shared config profile (skip IMDS — it can block
+  // on non-EC2 hosts), and fail rather than silently defaulting to us-east-1.
+  // Resolved once per process.
+  static const std::string kResolvedRegion = []() -> std::string {
+    Aws::String region = Aws::Environment::GetEnv("AWS_REGION");
+    if (region.empty()) {
+      region = Aws::Environment::GetEnv("AWS_DEFAULT_REGION");
+    }
+    if (region.empty()) {
+      const auto& profiles = Aws::Config::GetCachedConfigProfiles();
+      if (auto it = profiles.find(Aws::Auth::GetConfigProfileName());
+          it != profiles.end()) {
+        region = it->second.GetRegion();
+      }
+    }
+    return std::string(region.c_str());
+  }();
+  if (kResolvedRegion.empty()) {
+    return InvalidArgument(
+        "SigV4: could not resolve a signing region; set the '{}' property or the "
+        "AWS_REGION environment variable",
+        AuthProperties::kSigV4SigningRegion);
+  }
+  return kResolvedRegion;
 }
 
 std::string SigV4AuthManager::ResolveSigningName(
@@ -392,7 +416,7 @@ Result<std::shared_ptr<AuthSession>> SigV4AuthManager::WrapSession(
     const std::unordered_map<std::string, std::string>& properties,
     std::shared_ptr<Aws::Auth::AWSCredentialsProvider> reuse_credentials) {
   ICEBERG_ASSIGN_OR_RAISE(auto slot, SessionSlot::Reserve());
-  auto region = ResolveSigningRegion(properties);
+  ICEBERG_ASSIGN_OR_RAISE(auto region, ResolveSigningRegion(properties));
   auto service = ResolveSigningName(properties);
 
   // Reuse the parent's provider unless properties override keys, avoiding a
@@ -409,6 +433,8 @@ Result<std::shared_ptr<AuthSession>> SigV4AuthManager::WrapSession(
   auto session =
       std::make_shared<SigV4AuthSession>(std::move(delegate_session), std::move(region),
                                          std::move(service), std::move(credentials));
+  // The reserved slot's unregister responsibility now belongs to the session.
+  session->owns_sdk_registration_ = true;
   slot.Release();
   return session;
 }

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

@@ -78,11 +78,17 @@ class ICEBERG_REST_EXPORT SigV4AuthSession : public AuthSession {
   }
 
  private:
+  // WrapSession() reserves an AwsSdkLifecycle slot and transfers it here.
+  friend class SigV4AuthManager;
+
   std::shared_ptr<AuthSession> delegate_;
   std::string signing_region_;
   std::string signing_name_;
   std::shared_ptr<Aws::Auth::AWSCredentialsProvider> credentials_provider_;
   std::unique_ptr<Aws::Client::AWSAuthV4Signer> signer_;
+  // Only WrapSession()-created sessions registered a slot; directly-constructed
+  // ones (e.g. tests) must not unregister and underflow the count.
+  bool owns_sdk_registration_ = false;
 };
 
 /// \brief An AuthManager that produces SigV4AuthSession instances.
@@ -114,7 +120,7 @@ class ICEBERG_REST_EXPORT SigV4AuthManager : public AuthManager {
  private:
   static Result<std::shared_ptr<Aws::Auth::AWSCredentialsProvider>>
   MakeCredentialsProvider(const std::unordered_map<std::string, std::string>& properties);
-  static std::string ResolveSigningRegion(
+  static Result<std::string> ResolveSigningRegion(
       const std::unordered_map<std::string, std::string>& properties);
   static std::string ResolveSigningName(
       const std::unordered_map<std::string, std::string>& properties);

src/iceberg/catalog/rest/meson.build

@@ -47,7 +47,14 @@ iceberg_rest_build_deps = [iceberg_dep, cpr_dep]
 iceberg_rest_compile_defs = []
 
 sigv4_opt = get_option('sigv4')
-aws_sdk_core_dep = dependency('aws-cpp-sdk-core', required: sigv4_opt)
+# Use the CMake config, not pkg-config: aws-cpp-sdk-core.pc Cflags force
+# -std=c++11 -fno-exceptions, which would override the project's C++23 build.
+aws_sdk_core_dep = dependency(
+    'aws-cpp-sdk-core',
+    method: 'cmake',
+    modules: ['aws-cpp-sdk-core'],
+    required: sigv4_opt,
+)
 if aws_sdk_core_dep.found()
     iceberg_rest_build_deps += aws_sdk_core_dep
     iceberg_rest_compile_defs += '-DICEBERG_SIGV4'

src/iceberg/test/auth_manager_test.cc

@@ -496,9 +496,9 @@ TEST(OAuth2AuthSessionTest, InitialTokenIsUsed) {
   ASSERT_THAT(session_result, IsOk());
   auto session = session_result.value();
 
-  std::unordered_map<std::string, std::string> headers;
-  ASSERT_THAT(session->Authenticate(headers), IsOk());
-  EXPECT_EQ(headers["Authorization"], "Bearer initial-token-123");
+  auto auth_result = session->Authenticate({});
+  ASSERT_THAT(auth_result, IsOk());
+  EXPECT_EQ(auth_result.value().headers.at("Authorization"), "Bearer initial-token-123");
 
   session->Close();
 }

src/iceberg/test/sigv4_auth_test.cc

@@ -72,6 +72,32 @@ TEST_F(SigV4AuthTest, LifecycleFinalizeRefusesWhileSessionsAlive) {
   EXPECT_TRUE(IsAwsSdkInitialized());
 }
 
+TEST_F(SigV4AuthTest, DirectlyConstructedSessionDoesNotCorruptLifecycleCount) {
+  // A directly-constructed session never registered with AwsSdkLifecycle, so
+  // destroying it must not decrement the count. Otherwise it underflows, and a
+  // later real session wraps it back to zero, letting FinalizeAwsSdk() shut the
+  // SDK down while a session is still alive.
+  {
+    auto delegate = AuthSession::MakeDefault({});
+    auto credentials = std::make_shared<Aws::Auth::SimpleAWSCredentialsProvider>(
+        Aws::Auth::AWSCredentials("id", "secret"));
+    auto direct = std::make_shared<SigV4AuthSession>(delegate, "us-east-1", "execute-api",
+                                                     credentials);
+  }  // destroyed here — must leave the lifecycle count untouched
+
+  auto properties = MakeSigV4Properties();
+  auto manager_result = AuthManagers::Load("test-catalog", properties);
+  ASSERT_THAT(manager_result, IsOk());
+  auto session_result = manager_result.value()->CatalogSession(client_, properties);
+  ASSERT_THAT(session_result, IsOk());
+
+  // Exactly one live (registered) session, so Finalize must refuse. With the
+  // underflow bug the count would have wrapped and Finalize could wrongly
+  // succeed and shut the SDK down.
+  EXPECT_THAT(FinalizeAwsSdk(), IsError(ErrorKind::kInvalid));
+  EXPECT_TRUE(IsAwsSdkInitialized());
+}
+
 TEST_F(SigV4AuthTest, LoadSigV4AuthManager) {
   auto properties = MakeSigV4Properties();
   auto manager_result = AuthManagers::Load("test-catalog", properties);