Copilot commented on code in PR #12998:
URL: https://github.com/apache/trafficserver/pull/12998#discussion_r2962510178
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -123,3 +123,45 @@
ts2.Disk.traffic_out.Content = Testers.ExcludesExpression(
'Traffic Server is fully initialized', 'process should fail when invalid
certificate specified')
ts2.Disk.diags_log.Content = Testers.IncludesExpression('EMERGENCY: failed to
load SSL certificate file', 'check diags.log"')
+
+##########################################################################
+# Verify parallel cert loading with configurable concurrency
+
+ts3 = Test.MakeATSProcess("ts3", enable_tls=True)
+server3 = Test.MakeOriginServer("server3")
+server3.addResponse("sessionlog.json", request_header, response_header)
+
+ts3.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts3.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts3.Variables.SSLDir}',
+ 'proxy.config.ssl.server.multicert.concurrency': 4,
+ })
Review Comment:
The test section claims to verify configurable concurrency, but
`SSLMultiCertConfigLoader::load(..., firstLoad=true)` ignores
`proxy.config.ssl.server.multicert.concurrency` and uses
`hardware_concurrency()` instead. This makes the assertion on the "loading 2
certs with" log line dependent on the CI host having >1 hardware thread (it
will fail on single-core runners) and it doesn't actually validate the reload
behavior that the config controls. Consider restructuring this test to trigger
a `traffic_ctl config reload` and validate the threaded log line during reload
(where the configured concurrency is applied), or otherwise gate/skip when only
1 core is available.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1735,10 +1745,69 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup)
}
swoc::Errata errata(ERRATA_NOTE);
- int item_num = 0;
- for (const auto &item : parse_result.value) {
+ int num_threads = params->configLoadConcurrency;
+ if (num_threads == 0 || firstLoad) {
+ num_threads = std::thread::hardware_concurrency();
+ }
Review Comment:
`std::thread::hardware_concurrency()` can return very large values on big
systems. Since the new record validation caps user input to 256 (`[0-256]`),
consider clamping the computed thread count to the same maximum (both here and
for the `firstLoad` path) so "auto" doesn't spawn hundreds of threads
unexpectedly.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1528,11 +1529,20 @@ SSLMultiCertConfigLoader::_store_ssl_ctx(SSLCertLookup
*lookup, const shared_SSL
SSLMultiCertConfigLoader::CertLoadData data;
if (!this->_prep_ssl_ctx(sslMultCertSettings, data, common_names,
unique_names)) {
- lookup->is_valid = false;
+ {
+ std::lock_guard<std::mutex> lock(_loader_mutex);
+ lookup->is_valid = false;
+ }
return false;
}
std::vector<SSLLoadingContext> ctxs = this->init_server_ssl_ctx(data,
sslMultCertSettings.get());
+
+ // Serialize all mutations to the shared SSLCertLookup.
+ // The expensive work above (_prep_ssl_ctx + init_server_ssl_ctx) runs
+ // without the lock, allowing parallel cert loading across threads.
+ std::lock_guard<std::mutex> lock(_loader_mutex);
+
Review Comment:
The mutex scope in `_store_ssl_ctx` currently covers more than just
`SSLCertLookup` mutations. In particular, the lock is held across the later
`unique_names` processing (which calls `init_server_ssl_ctx()` again), so some
expensive SSL_CTX creation/cert loading work will still run under the lock,
limiting parallelism and contradicting the comment here. Consider narrowing the
lock to only the sections that mutate `lookup` (e.g., lock around
`_store_single_ssl_ctx()` / `register_cert_secrets()`), and do any
`init_server_ssl_ctx()` work outside the critical section.
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -49,6 +49,7 @@
#include <array>
#include <cstring>
#include <cmath>
+#include <thread>
#include <unordered_map>
Review Comment:
`SSLConfigParams::initialize()` now uses `std::max` with
`std::thread::hardware_concurrency()`. Please ensure `<algorithm>` is included
in this translation unit (to avoid relying on transitive headers) since
`std::max` is defined there.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1735,10 +1745,69 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup)
}
swoc::Errata errata(ERRATA_NOTE);
- int item_num = 0;
- for (const auto &item : parse_result.value) {
+ int num_threads = params->configLoadConcurrency;
+ if (num_threads == 0 || firstLoad) {
+ num_threads = std::thread::hardware_concurrency();
+ }
+ if (num_threads < 1) {
+ num_threads = 1;
+ }
+ num_threads = std::min(num_threads,
static_cast<int>(parse_result.value.size()));
+
Review Comment:
This new code uses `std::min` but the file does not include `<algorithm>`,
which makes the build depend on transitive includes. Please add the proper
header so this compiles reliably across toolchains/stdlibs.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1735,10 +1745,69 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup)
}
swoc::Errata errata(ERRATA_NOTE);
- int item_num = 0;
- for (const auto &item : parse_result.value) {
+ int num_threads = params->configLoadConcurrency;
+ if (num_threads == 0 || firstLoad) {
+ num_threads = std::thread::hardware_concurrency();
+ }
+ if (num_threads < 1) {
+ num_threads = 1;
+ }
+ num_threads = std::min(num_threads,
static_cast<int>(parse_result.value.size()));
+
+ if (num_threads > 1 && parse_result.value.size() > 1) {
+ std::size_t bucket_size = parse_result.value.size() / num_threads;
+ std::size_t remainder = parse_result.value.size() % num_threads;
+ auto current = parse_result.value.cbegin();
+
+ std::vector<std::thread> threads;
+ Note("(%s) loading %zu certs with %d threads", this->_debug_tag(),
parse_result.value.size(), num_threads);
+
+ for (int t = 0; t < num_threads; ++t) {
+ std::size_t this_bucket = bucket_size + (static_cast<std::size_t>(t) <
remainder ? 1 : 0);
+ auto end = current + this_bucket;
+ threads.emplace_back(&SSLMultiCertConfigLoader::_load_items, this,
lookup, current, end, std::ref(errata));
+ current = end;
+ }
+
+ for (auto &th : threads) {
+ th.join();
+ }
+
+ Note("(%s) loaded %zu certs in %d threads", this->_debug_tag(),
parse_result.value.size(), num_threads);
+ } else {
+ _load_items(lookup, parse_result.value.cbegin(),
parse_result.value.cend(), errata);
+ Note("(%s) loaded %zu certs (single-threaded)", this->_debug_tag(),
parse_result.value.size());
+ }
+
+ // We *must* have a default context even if it can't possibly work. The
default context is used to
+ // bootstrap the SSL handshake so that we can subsequently do the SNI lookup
to switch to the real
+ // context.
+ if (lookup->ssl_default == nullptr) {
+ shared_SSLMultiCertConfigParams sslMultiCertSettings(new
SSLMultiCertConfigParams);
+ sslMultiCertSettings->addr = ats_strdup("*");
+ if (!this->_store_ssl_ctx(lookup, sslMultiCertSettings)) {
+ errata.note(ERRATA_ERROR, "failed set default context");
+ }
+ }
+
+ return errata;
+}
+
+void
+SSLMultiCertConfigLoader::_load_items(SSLCertLookup *lookup,
config::SSLMultiCertConfig::const_iterator begin,
+
config::SSLMultiCertConfig::const_iterator end, swoc::Errata &errata)
+{
+ // Each thread needs its own elevated privileges since POSIX capabilities
are per-thread
+ uint32_t elevate_setting = 0;
+ elevate_setting =
RecGetRecordInt("proxy.config.ssl.cert.load_elevated").value_or(0);
+ ElevateAccess elevate_access(elevate_setting ? ElevateAccess::FILE_PRIVILEGE
: 0);
+
+ int item_num = 0;
+ for (auto it = begin; it != end; ++it) {
item_num++;
+ const auto &item = *it;
Review Comment:
`_load_items()` resets `item_num` to 0 for each thread bucket, so the errata
messages that reference "item {}" no longer correspond to the actual (global)
item index in `ssl_multicert.yaml` when loading in parallel (duplicate item
numbers across threads). Consider computing the base index from `begin` (e.g.,
distance from `parse_result.value.cbegin()`) and reporting a stable/global item
number.
--
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]
