zwoop commented on code in PR #12978:
URL: https://github.com/apache/trafficserver/pull/12978#discussion_r2956047963
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -109,3 +109,49 @@
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"')
+
+##########################################################################
+# Ensure parallel cert loading works correctly with multiple certs
+
+ts3 = Test.MakeATSProcess("ts3", enable_tls=True)
+server3 = Test.MakeOriginServer("server4")
Review Comment:
Is this a typo, and it should be "server3" ?
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1962,28 +2009,49 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup)
}
if (*line != '\0' && *line != '#') {
- shared_SSLMultiCertConfigParams sslMultiCertSettings =
std::make_shared<SSLMultiCertConfigParams>();
- const char *errPtr;
+ config_lines.emplace_back(line, line_num);
+ }
+ line = tokLine(nullptr, &tok_state);
+ }
- errPtr = parseConfigLine(line, &line_info, &sslCertTags);
- Dbg(dbg_ctl_ssl_load, "currently parsing %s at line %d from config file:
%s", line, line_num, params->configFilePath);
- if (errPtr != nullptr) {
- Warning("%s: discarding %s entry at line %d: %s", __func__,
params->configFilePath, line_num, errPtr);
- } else {
- if (ssl_extract_certificate(&line_info, sslMultiCertSettings.get())) {
- // There must be a certificate specified unless the tunnel action is
set
- if (sslMultiCertSettings->cert || sslMultiCertSettings->opt !=
SSLCertContextOption::OPT_TUNNEL) {
- if (!this->_store_ssl_ctx(lookup, sslMultiCertSettings)) {
- errata.note(ERRATA_ERROR, "Failed to load certificate on line
{}", line_num);
- }
- } else {
- errata.note(ERRATA_WARN, "No ssl_cert_name specified and no tunnel
action set on line {}", line_num);
- }
- }
- }
+ swoc::Errata errata(ERRATA_NOTE);
+
+ if (params->configLoadConcurrency > 1 && config_lines.size() > 1) {
Review Comment:
Probably my bug, I think this should be
`if ((params->configLoadConcurrency > 1 || firstLoad) && config_lines.size()
> 1)`
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -109,3 +109,49 @@
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"')
+
+##########################################################################
+# Ensure parallel cert loading works correctly with multiple certs
+
+ts3 = Test.MakeATSProcess("ts3", enable_tls=True)
+server3 = Test.MakeOriginServer("server4")
+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,
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'ssl_load',
+ })
+
+ts3.addDefaultSSLFiles()
+
+ts3.Disk.remap_config.AddLine(f'map /
http://127.0.0.1:{server3.Variables.Port}')
+
+# Add enough cert lines to exercise multiple threads (need > 1 for parallel
path,
+# and ideally >= concurrency to actually use all threads)
+ts3.Disk.ssl_multicert_config.AddLines(
+ [
+ 'dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key',
+ 'ssl_cert_name=server.pem ssl_key_name=server.key',
+ 'ssl_cert_name=server.pem ssl_key_name=server.key',
+ 'ssl_cert_name=server.pem ssl_key_name=server.key',
+ 'ssl_cert_name=server.pem ssl_key_name=server.key',
+ ])
+
+tr5 = Test.AddTestRun("Verify parallel cert loading works")
+tr5.Processes.Default.StartBefore(ts3)
Review Comment:
This looks weird, I can see the StartServer's being allowed to run like
this, but then you do
tr5.StillRunningAfter = ts3
tr5.StillRunningAfter = server3
That seems like the second just overwrites the first?
--
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]
