Skip to content

Commit

Permalink
Multiple fixes for STS token refresh (#2115)
Browse files Browse the repository at this point in the history
#### Reference Issues/PRs
<!--Example: Fixes #1234. See also #3456.-->
Inherit changes from #2095

#### What does this implement or fix?
1. Write aws s3 config files to a temp directory so pytest workers and
session will not overwrite each other's and result in refresh token fail
later.
2. Patch aws s3 sdk so it would refresh the token 5 minutes before
expiry as intendend (Presumingly)
3. Fix a bad lambda capture by refereance 
#### Any other comments?
* For the changes of vcpkg overlay, they are all copied from vcpkg repo,
apart from `fix-refresh.patch`
* Non-in-CI STS-related testing on Windows requires manually setup aws
sts config (can refer to
`.github/actions/set_s3_sts_persistent_storage_env_vars/action.yml`.
It's due to the C++ binaries will hold a copy of enviornment variable
once started, which makes it impossible to update `AWS_CONFIG_FILE` once
started

---------

Co-authored-by: Georgi Rusev <Georgi Rusev>
  • Loading branch information
phoebusm authored Feb 3, 2025
1 parent 958a6ff commit 1afd029
Show file tree
Hide file tree
Showing 17 changed files with 1,585 additions and 59 deletions.
31 changes: 31 additions & 0 deletions .github/actions/set_s3_sts_persistent_storage_env_vars/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: 'Set Persistent storages env variables'
description: 'Set the necessary variables for Persistent storage tests'
inputs:
bucket: {default: 'arcticdb-ci-test-bucket-02', type: string, description: The name of the S3 bucket that we will test against}
endpoint: {default: 'http://s3.eu-west-1.amazonaws.com', type: string, description: The address of the S3 endpoint}
region: {default: 'eu-west-1', type: string, description: The S3 region of the bucket}
aws_access_key: {required: true, type: string, description: The value for the AWS Access key}
aws_secret_key: {required: true, type: string, description: The value for the AWS Secret key}
runs:
using: "composite"
steps:
# add a step to run locally define python script
- name: Set s3 sts real storage variables
shell: bash
run: |
python -c "
from arcticdb.storage_fixtures.s3 import real_s3_sts_from_environment_variables, real_s3_sts_resources_ready
from arcticdb_ext.storage import NativeVariantStorage
import os
config_file_path = os.path.expanduser(os.path.join('~', '.aws', 'config'))
f = real_s3_sts_from_environment_variables(
user_name='${ARCTICDB_REAL_S3_STS_TEST_USERNAME}',
role_name='${ARCTICDB_REAL_S3_STS_TEST_ROLE}',
policy_name='${ARCTICDB_REAL_S3_STS_TEST_POLICY_NAME}',
profile_name='sts_test_profile',
native_config=NativeVariantStorage(), # Setting here is purposely wrong to see whether it will get overridden later
config_file_path=config_file_path
)
real_s3_sts_resources_ready(f)
"
9 changes: 8 additions & 1 deletion .github/workflows/build_steps.yml
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,13 @@ jobs:
aws_access_key: "${{ secrets.AWS_S3_ACCESS_KEY }}"
aws_secret_key: "${{ secrets.AWS_S3_SECRET_KEY }}"
strategy_branch: "${{ env.distinguishing_name }}"

- name: Set s3 sts persistent storage variables for Windows
if: inputs.persistent_storage == 'true' && matrix.os == 'windows'
uses: ./.github/actions/set_s3_sts_persistent_storage_env_vars
with:
aws_access_key: "${{ secrets.AWS_S3_ACCESS_KEY }}"
aws_secret_key: "${{ secrets.AWS_S3_SECRET_KEY }}"

- name: Run test
run: |
Expand All @@ -381,7 +388,7 @@ jobs:

# Fallback if the clean up at test fixutre tear down fails due to crash or etc
- name: Remove AWS testing account and credentials
if: always() && inputs.persistent_storage == 'true'
if: always() && inputs.persistent_storage == 'true' && matrix.os == 'windows'
run: |
python -c "
from arcticdb.storage_fixtures.s3 import real_s3_sts_clean_up
Expand Down
3 changes: 2 additions & 1 deletion cpp/arcticdb/storage/s3/s3_storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,10 @@ void S3Storage::create_s3_client(const S3Settings &conf, const Aws::Auth::AWSCre
s3_client_ = std::make_unique<MockS3Client>();
}
else if (conf.aws_auth() == AWSAuthMethod::STS_PROFILE_CREDENTIALS_PROVIDER){
ARCTICDB_RUNTIME_DEBUG(log::storage(), "Load sts profile credentials provider");
Aws::Config::ReloadCachedConfigFile(); // config files loaded in Aws::InitAPI; It runs once at first S3Storage object construct; reload to get latest
auto client_config = get_s3_config(conf);
auto sts_client_factory = [&](const Aws::Auth::AWSCredentials& creds) { // Get default allocation tag
auto sts_client_factory = [conf, this](const Aws::Auth::AWSCredentials& creds) { // Get default allocation tag
auto sts_config = get_proxy_config(conf.https() ? Aws::Http::Scheme::HTTPS : Aws::Http::Scheme::HTTP);
auto allocation_tag = Aws::STS::STSClient::GetAllocationTag();
sts_client_ = std::make_unique<Aws::STS::STSClient>(creds, Aws::MakeShared<Aws::STS::Endpoint::STSEndpointProvider>(allocation_tag), sts_config);
Expand Down
20 changes: 20 additions & 0 deletions cpp/third_party/vcpkg_overlays/aws-sdk-cpp/fix-refresh.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
diff --git a/src/aws-cpp-sdk-identity-management/source/auth/STSProfileCredentialsProvider.cpp b/src/aws-cpp-sdk-identity-management/source/auth/STSProfileCredentialsProvider.cpp
index fd82b678fba..9fd0537e217 100644
--- a/src/aws-cpp-sdk-identity-management/source/auth/STSProfileCredentialsProvider.cpp
+++ b/src/aws-cpp-sdk-identity-management/source/auth/STSProfileCredentialsProvider.cpp
@@ -45,13 +45,13 @@ AWSCredentials STSProfileCredentialsProvider::GetAWSCredentials()
void STSProfileCredentialsProvider::RefreshIfExpired()
{
Utils::Threading::ReaderLockGuard guard(m_reloadLock);
- if (!IsTimeToRefresh(static_cast<long>(m_reloadFrequency.count())) || !m_credentials.IsExpiredOrEmpty())
+ if (!IsTimeToRefresh(static_cast<long>(m_reloadFrequency.count())) && !m_credentials.IsExpiredOrEmpty())
{
return;
}

guard.UpgradeToWriterLock();
- if (!IsTimeToRefresh(static_cast<long>(m_reloadFrequency.count())) || !m_credentials.IsExpiredOrEmpty()) // double-checked lock to avoid refreshing twice
+ if (!IsTimeToRefresh(static_cast<long>(m_reloadFrequency.count())) && !m_credentials.IsExpiredOrEmpty()) // double-checked lock to avoid refreshing twice
{
return;
}
103 changes: 103 additions & 0 deletions cpp/third_party/vcpkg_overlays/aws-sdk-cpp/portfile.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
vcpkg_buildpath_length_warning(37)

vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO aws/aws-sdk-cpp
REF "${VERSION}"
SHA512 8c138dfde232b364016cdfbaf13568efa330040894d1dbd648e48204828634812d95741eebe0a2795f2b08ba9edf85621aa962a8f4f696aae6930e6e1fd8dede
PATCHES
fix-aws-root.patch
lock-curl-http-and-tls-settings.patch
fix_find_curl.patch
find-dependency.patch
fix-refresh.patch
)

string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "dynamic" FORCE_SHARED_CRT)

set(EXTRA_ARGS "")
if(VCPKG_TARGET_IS_OSX OR VCPKG_TARGET_IS_IOS)
set(rpath "@loader_path")
elseif (VCPKG_TARGET_IS_ANDROID)
set(EXTRA_ARGS "-DTARGET_ARCH=ANDROID"
"-DGIT_EXECUTABLE=--invalid-git-executable--"
"-DGIT_FOUND=TRUE"
"-DNDK_DIR=$ENV{ANDROID_NDK_HOME}"
"-DANDROID_BUILD_ZLIB=FALSE"
"-DANDROID_BUILD_CURL=FALSE"
"-DANDROID_BUILD_OPENSSL=FALSE"
)
else()
set(rpath "\$ORIGIN")
endif()

string(REPLACE "awsmigrationhub" "AWSMigrationHub" targets "${FEATURES}")
vcpkg_cmake_configure(
SOURCE_PATH "${SOURCE_PATH}"
DISABLE_PARALLEL_CONFIGURE
OPTIONS
${EXTRA_ARGS}
"-DENABLE_UNITY_BUILD=ON"
"-DENABLE_TESTING=OFF"
"-DFORCE_SHARED_CRT=${FORCE_SHARED_CRT}"
"-DBUILD_ONLY=${targets}"
"-DBUILD_DEPS=OFF"
"-DBUILD_SHARED_LIBS=OFF"
"-DAWS_SDK_WARNINGS_ARE_ERRORS=OFF"
"-DCMAKE_INSTALL_RPATH=${rpath}"
"-DCMAKE_MODULE_PATH=${CURRENT_INSTALLED_DIR}/share/aws-c-common" # use extra cmake files
)
vcpkg_cmake_install()

foreach(TARGET IN LISTS targets)
string(TOLOWER "aws-cpp-sdk-${TARGET}" package)
vcpkg_cmake_config_fixup(PACKAGE_NAME "${package}" CONFIG_PATH "lib/cmake/aws-cpp-sdk-${TARGET}" DO_NOT_DELETE_PARENT_CONFIG_PATH)
endforeach()
vcpkg_cmake_config_fixup(PACKAGE_NAME "awssdk" CONFIG_PATH "lib/cmake/AWSSDK")

vcpkg_copy_pdbs()

file(GLOB_RECURSE AWS_TARGETS "${CURRENT_PACKAGES_DIR}/share/*/*-targets-*.cmake")
foreach(AWS_TARGET IN LISTS AWS_TARGETS)
file(READ ${AWS_TARGET} _contents)
string(REGEX REPLACE
"bin\\/([A-Za-z0-9_.-]+\\.lib)"
"lib/\\1"
_contents "${_contents}")
file(WRITE ${AWS_TARGET} "${_contents}")
endforeach()

file(GLOB AWS_CONFIGS "${CURRENT_PACKAGES_DIR}/share/*/aws-cpp-sdk-*-config.cmake")
list(FILTER AWS_CONFIGS EXCLUDE REGEX "aws-cpp-sdk-core-config\\.cmake\$")
foreach(AWS_CONFIG IN LISTS AWS_CONFIGS)
file(READ "${AWS_CONFIG}" _contents)
file(WRITE "${AWS_CONFIG}" "include(CMakeFindDependencyMacro)\nfind_dependency(aws-cpp-sdk-core)\n${_contents}")
endforeach()

file(REMOVE_RECURSE
"${CURRENT_PACKAGES_DIR}/debug/include"
"${CURRENT_PACKAGES_DIR}/debug/share"
"${CURRENT_PACKAGES_DIR}/lib/pkgconfig"
"${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig"
"${CURRENT_PACKAGES_DIR}/nuget"
"${CURRENT_PACKAGES_DIR}/debug/nuget"
)

if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
file(GLOB LIB_FILES ${CURRENT_PACKAGES_DIR}/bin/*.lib)
if(LIB_FILES)
file(COPY ${LIB_FILES} DESTINATION ${CURRENT_PACKAGES_DIR}/lib)
file(REMOVE ${LIB_FILES})
endif()
file(GLOB DEBUG_LIB_FILES ${CURRENT_PACKAGES_DIR}/debug/bin/*.lib)
if(DEBUG_LIB_FILES)
file(COPY ${DEBUG_LIB_FILES} DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib)
file(REMOVE ${DEBUG_LIB_FILES})
endif()

file(APPEND "${CURRENT_PACKAGES_DIR}/include/aws/core/SDKConfig.h" "#ifndef USE_IMPORT_EXPORT\n#define USE_IMPORT_EXPORT\n#endif")
endif()

configure_file("${CURRENT_PORT_DIR}/usage" "${CURRENT_PACKAGES_DIR}/share/${PORT}/usage" @ONLY)

vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE")
1 change: 1 addition & 0 deletions cpp/third_party/vcpkg_overlays/aws-sdk-cpp/usage
1 change: 1 addition & 0 deletions cpp/third_party/vcpkg_overlays/aws-sdk-cpp/vcpkg.in.json
Loading

0 comments on commit 1afd029

Please sign in to comment.