HwangDongJun commented on issue #12711:
URL: https://github.com/apache/gluten/issues/12711#issuecomment-5275623199
Hi, @zhouyuan
As suggested, I tried a code-level fix that doesn't require the CA-bundle
symlink, and verified it with an A/B test under a reproducible hang condition.
**TL;DR**: Using Gluten's existing mechanism for patching the Velox source
at build time (`get-velox.sh` + a patch file, same pattern as
`modify_arrow.patch`), I made Velox's S3 client read the CA bundle path from
the `SSL_CERT_FILE` environment variable instead of relying solely on the
hardcoded compile-time default.
## How we fixed it (patch ready, happy to upstream)
Added a step to `ep/build-velox/src/get-velox.sh` to apply the patch below
(right next to the existing step that applies `modify_arrow.patch`):
```diff
diff --git a/ep/build-velox/src/get-velox.sh
b/ep/build-velox/src/get-velox.sh
index 5df0b0c28..d90d640ce 100755
--- a/ep/build-velox/src/get-velox.sh
+++ b/ep/build-velox/src/get-velox.sh
@@ -162,6 +162,23 @@ function apply_compilation_fixes {
git add
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/modify_arrow.patch # to
avoid the file from being deleted by git clean -dffx :/
}
+function apply_s3_ssl_ca_override_patch {
+ pushd $VELOX_HOME
+ (git apply --check ${CURRENT_DIR}/s3-ssl-ca-override.patch && \
+ git apply ${CURRENT_DIR}/s3-ssl-ca-override.patch) || {
+ echo "Failed to apply s3-ssl-ca-override.patch"
+ exit 1
+ }
+ popd
+}
+
function setup_linux {
local LINUX_DISTRIBUTION=$(. /etc/os-release && echo ${ID})
local LINUX_VERSION_ID=$(. /etc/os-release && echo ${VERSION_ID})
@@ -242,6 +259,8 @@ fi
apply_provided_velox_patch
+apply_s3_ssl_ca_override_patch
+
apply_compilation_fixes
echo "Finished getting Velox code"
```
New `ep/build-velox/src/s3-ssl-ca-override.patch` (applied to two files
under Velox's `velox/connectors/hive/storage_adapters/s3fs/`):
```diff
--- a/velox/connectors/hive/storage_adapters/s3fs/S3Config.h
+++ b/velox/connectors/hive/storage_adapters/s3fs/S3Config.h
@@ -78,6 +78,7 @@
kCredentialsProvider,
kIMDSEnabled,
kMultipartMinPartSize,
+ kSSLCAFile,
kEnd
};
@@ -119,6 +120,7 @@
{Keys::kIMDSEnabled, std::make_pair("aws-imds-enabled",
"true")},
{Keys::kMultipartMinPartSize,
std::make_pair("min-part-size", "10MB")},
+ {Keys::kSSLCAFile, std::make_pair("ssl.ca-file", std::nullopt)},
};
return config;
}
@@ -254,6 +256,15 @@
return folly::to<bool>(value);
}
+ /// Custom CA bundle file path used to verify the S3 endpoint's TLS
+ /// certificate, overriding the HTTP client's compiled-in default trust
+ /// store location. If not set, S3FileSystem falls back to the
+ /// SSL_CERT_FILE environment variable before using the compiled-in
+ /// default.
+ std::optional<std::string> sslCAFile() const {
+ return config_.find(Keys::kSSLCAFile)->second;
+ }
+
size_t minPartSize() const;
private:
--- a/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp
+++ b/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp
@@ -27,6 +27,7 @@
#include <fmt/format.h>
#include <glog/logging.h>
+#include <cstdlib>
#include <memory>
#include <stdexcept>
@@ -239,6 +240,25 @@
Aws::Client::RequestChecksumCalculation::WHEN_REQUIRED;
clientConfig.checksumConfig.responseChecksumValidation =
Aws::Client::ResponseChecksumValidation::WHEN_REQUIRED;
+
+ // Allow overriding the TLS CA bundle used to verify S3 endpoints.
+ // HTTP client libraries built via static toolchains (e.g. vcpkg) bake
+ // in a compile-time default CA bundle path (for example the RHEL-family
+ // path when built on Rocky/CentOS), which may not exist on a different
+ // runtime OS (e.g. a Debian/Ubuntu container), causing TLS handshake
+ // failures despite the system otherwise having a valid, working CA
+ // bundle elsewhere. Prefer an explicit 'hive.s3.ssl.ca-file' config;
+ // otherwise fall back to the widely-used SSL_CERT_FILE environment
+ // variable convention (also respected by OpenSSL, the curl CLI,
+ // Python, Node.js, etc.), so a runtime image only needs to set one
+ // environment variable to point at its actual CA bundle location,
+ // without requiring a Velox/Spark config change.
+ if (s3Config_->sslCAFile().has_value()) {
+ clientConfig.caFile = awsString(s3Config_->sslCAFile().value());
+ } else if (const char* envCAFile = std::getenv("SSL_CERT_FILE")) {
+ clientConfig.caFile = envCAFile;
+ }
+
if (s3Config_->endpoint().has_value()) {
clientConfig.endpointOverride = s3Config_->endpoint().value();
}
```
`caFile` is the officially supported AWS SDK C++ mechanism for this (it
flows through to libcurl's `CURLOPT_CAINFO`). Users who don't touch either
setting see no behavior change (`caFile` stays unset, same compiled-in default
path as before).
## Verified
Tested on a base image with no CA-bundle symlink at all, running a query
that reads an Iceberg table on S3-compatible object storage with
`batchscan=true`. Confirmed `numFallbackNodes == 0` (fully native execution) on
every run, to make sure the comparison was actually exercising the native S3
path (the code path touched by the patch above).
With the identical patched build, identical query, and identical native
execution plan, I toggled only the `SSL_CERT_FILE` environment variable and
repeated each condition twice:
- `SSL_CERT_FILE` set: completed normally within tens of seconds, every time
- `SSL_CERT_FILE` unset (reproducing the original issue): hung for 240+
seconds with no error/exception, every time — matching the symptom described in
this issue and #10670 exactly
With `SSL_CERT_FILE` set, it was also noticeably faster than the non-native
fallback (`batchscan=false`), comparable to or better than the speedup observed
with the symlink workaround.
## Known limitations
`hive.s3.ssl.ca-file` isn't wired into the `spark.hadoop.fs.s3a.*` →
`hive.s3.*` passthrough yet (`cpp/velox/utils/ConfigExtractor.cc` on the Gluten
side), so only the `SSL_CERT_FILE` environment variable path currently works
(not a Spark config yet). Happy to add that if useful.
Also, `SSL_CERT_FILE` was chosen because it's a widely-used convention in
the OpenSSL/curl ecosystem, but that also means any deployment that already
sets this variable for unrelated reasons could see a behavior change. Open to
discussing whether a Gluten-specific name would be preferable.
Happy to open a PR if there's interest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
