Copilot commented on code in PR #12868:
URL: https://github.com/apache/trafficserver/pull/12868#discussion_r2782876178
##########
src/proxy/http/remap/NextHopSelectionStrategy.cc:
##########
@@ -269,8 +269,13 @@ void
NextHopSelectionStrategy::setHostHeader(TSHttpTxn txnp, const char *hostname)
{
if (host_override && nullptr != hostname) {
- HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
-
sm->t_state.hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ HttpSM *const sm = reinterpret_cast<HttpSM *>(txnp);
+ if (!sm->t_state.hdr_info.server_request.valid()) {
+
sm->t_state.hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
Review Comment:
When overriding the client_request Host header, the header target cache is
not invalidated. Other call sites typically call mark_target_dirty() (or clear
m_target_cached) after changing Host to keep derived URL/host state coherent.
Consider marking client_request target dirty here too, for symmetry with the
server_request branch and to avoid stale host-dependent behavior (e.g.,
SNI/HostDB decisions).
```suggestion
sm->t_state.hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
sm->t_state.hdr_info.client_request.mark_target_dirty();
```
##########
tests/gold_tests/tls/tls_sni_parent_failover.test.py:
##########
@@ -0,0 +1,169 @@
+'''
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Test parent failover sni name
+'''
+
+# Verify proxy.config.http.cache.ignore_authentication behavior.
+#Test.ATSReplayTest(replay_file="replay/tls_sni_parent_failover.replay.yaml")
+
+# Define default ATS
+ts = Test.MakeATSProcess(
+ "ts",
+ enable_tls=True,
+ enable_cache=False,
+ # command="valgrind.sh",
+)
+
+server_foo = Test.MakeOriginServer(
+ "server_foo",
+ ssl=True,
+ options={
+ "--key": "{0}/server-foo.key".format(Test.RunDirectory),
+ "--cert": "{0}/server-foo.pem".format(Test.RunDirectory),
+ },
+)
+server_bar = Test.MakeOriginServer(
+ "server_bar",
+ ssl=True,
+ options={
+ "--key": "{0}/server-bar.key".format(Test.RunDirectory),
+ "--cert": "{0}/server-bar.pem".format(Test.RunDirectory),
+ },
+)
+
+# default check request/response
+request_foo_header = {"headers": "GET / HTTP/1.1\r\nHost: foo.com\r\n\r\n",
"timestamp": "1469733493.993", "body": ""}
+response_foo_header = {"headers": "HTTP/1.1 200 OK\r\nConnection:
close\r\n\r\n", "timestamp": "1469733493.993", "body": "foo ok"}
+request_bar_header = {"headers": "GET / HTTP/1.1\r\nHost: bar.com\r\n\r\n",
"timestamp": "1469733493.993", "body": ""}
+response_bar_header = {"headers": "HTTP/1.1 200 OK\r\nConnection:
close\r\n\r\n", "timestamp": "1469733493.993", "body": "bar ok"}
+
+server_foo.addResponse("sessionlog.json", request_foo_header,
response_foo_header)
+server_bar.addResponse("sessionlog.json", request_bar_header,
response_bar_header)
+
+# successful request to be served by bar.com
+request_bar_header = {"headers": "GET /path HTTP/1.1\r\nHost:
bar.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
+response_bar_header = {
+ "headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n",
+ "timestamp": "1469733493.993",
+ "body": "path bar ok"
+}
+
+server_bar.addResponse("sessionlog.json", request_bar_header,
response_bar_header)
+
+#ts.Setup.CopyAs("valgrind.sh", ts.Variables.RUNTIMEDIR + "/../bin/")
+
+ts.addSSLfile("ssl/server-foo.pem")
+ts.addSSLfile("ssl/server-foo.key")
+ts.addSSLfile("ssl/server-bar.pem")
+ts.addSSLfile("ssl/server-bar.key")
+
+#ts.addFile("valgrind.sh")
+
+dns = Test.MakeDNServer("dns")
+
+ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http|ssl|parent_select|next_hop',
+ 'proxy.config.ssl.client.verify.server.policy': 'ENFORCED',
+ #'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
+ 'proxy.config.ssl.client.verify.server.properties': 'NAME',
+ 'proxy.config.url_remap.pristine_host_hdr': 0,
+ 'proxy.config.dns.nameservers':
'127.0.0.1:{0}'.format(dns.Variables.Port),
+ 'proxy.config.dns.resolv_conf': 'NULL',
+ 'proxy.config.exec_thread.autoconfig.scale': 1.0,
+ #'proxy.config.ssl.client.sni_policy': 'host',
+ 'proxy.config.http.connect.down.policy': 1, # tls failures don't mark
down
+ })
+
+dns.addRecords(records={"foo.com.": ["127.0.0.1"]})
+dns.addRecords(records={"bar.com.": ["127.0.0.1"]})
+dns.addRecords(records={"parent.": ["127.0.0.1"]})
+dns.addRecords(records={"strategy.": ["127.0.0.1"]})
+
+ts.Disk.remap_config.AddLines([
+ "map http://parent https://parent";,
+ "map http://strategy https://strategy @strategy=strat",
+])
+
+ts.Disk.parent_config.AddLine(
+ 'dest_domain=. port=443 parent="foo.com:{0}|1;bar.com:{1}|1"
parent_retry=simple_retry parent_is_proxy=false go_direct=false
simple_server_retry_responses="404" host_override=true'
+ .format(server_foo.Variables.SSL_Port, server_bar.Variables.SSL_Port))
+
+# build strategies.yaml file
+ts.Disk.File(ts.Variables.CONFIGDIR + "/strategies.yaml", id="strategies",
typename="ats:config")
+
+s = ts.Disk.strategies
+s.AddLine("groups:")
+s.AddLines(
+ [
+ f" - &gstrat",
+ f" - host: foo.com",
+ f" protocol:",
+ f" - scheme: https",
+ f" port: {server_foo.Variables.SSL_Port}",
+ f" weight: 1.0",
+ f" - host: bar.com",
+ f" protocol:",
+ f" - scheme: https",
+ f" port: {server_bar.Variables.SSL_Port}",
+ f" weight: 1.0",
+ ])
+
+s.AddLine("strategies:")
+
+s.AddLines(
+ [
+ f" - strategy: strat",
+ f" policy: first_live",
+ f" go_direct: false",
+ f" parent_is_proxy: false",
+ f" ignore_self_detect: true",
+ f" host_override: true",
+ f" groups:",
+ f" - *gstrat",
+ f" scheme: https",
+ f" failover:",
+ f" ring_mode: exhaust_ring",
+ f" response_codes:",
+ f" - 404",
+ ])
+
+curl_args = f"-s -L -o /dev/stdout -D /dev/stderr -x
localhost:{ts.Variables.port}/"
Review Comment:
curl's -x/--proxy option expects a proxy host[:port] or URL; the trailing
'/' is unusual and differs from other gold tests, and can be parsed
inconsistently across curl versions. Drop the trailing slash (or use an
explicit scheme like http://localhost:PORT).
```suggestion
curl_args = f"-s -L -o /dev/stdout -D /dev/stderr -x
localhost:{ts.Variables.port}"
```
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -224,6 +226,20 @@ findParent(HttpTransact::State *s)
} else if (nullptr != s->parent_params) {
s->parent_params->findParent(&s->request_data, &s->parent_result,
s->txn_conf->parent_fail_threshold,
s->txn_conf->parent_retry_time);
+ if (s->parent_result.host_override()) {
+ char const *const hostname = s->parent_result.hostname;
+ if (!s->hdr_info.server_request.valid()) {
+
s->hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.client_request.mark_target_dirty();
+ } else {
+
s->hdr_info.server_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
+ s->hdr_info.server_request.mark_target_dirty();
+ }
+
+ if (nullptr != hostname) {
Review Comment:
This block calls s->parent_result.host_override() and then uses
parent_result.hostname in value_set(). If parent selection yields DIRECT /
UNDEFINED, ParentResult may have rec==nullptr and/or hostname==nullptr;
host_override() can crash (rec deref) and std::string_view(hostname) is
undefined for nullptr. Guard with parent_result.is_some() and hostname!=nullptr
(or make host_override() safe) before overriding the Host header.
```suggestion
if (s->parent_result.is_some() && s->parent_result.host_override()) {
char const *const hostname = s->parent_result.hostname;
if (nullptr != hostname) {
if (!s->hdr_info.server_request.valid()) {
s->hdr_info.client_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
s->hdr_info.client_request.mark_target_dirty();
} else {
s->hdr_info.server_request.value_set(static_cast<std::string_view>(MIME_FIELD_HOST),
hostname);
s->hdr_info.server_request.mark_target_dirty();
}
```
##########
include/proxy/ParentSelection.h:
##########
@@ -242,6 +243,12 @@ struct ParentResult {
return is_api_result() ? ParentRetry_t::NONE : rec->parent_retry;
}
+ bool
+ host_override() const
+ {
+ return is_api_result() ? false : rec->host_override;
Review Comment:
ParentResult::host_override() dereferences rec without checking for nullptr.
ParentResult::is_some() explicitly allows rec==nullptr for DIRECT / UNDEFINED
results, so calling host_override() in those states will crash. Make
host_override() return false when rec is nullptr (and keep the API-result
behavior).
```suggestion
if (is_api_result()) {
return false;
}
if (rec == nullptr) {
// If we don't have a result, we either haven't done a parent
// lookup yet (ParentResultType::UNDEFINED), or the lookup didn't match
// anything (ParentResultType::DIRECT).
ink_assert(result == ParentResultType::UNDEFINED || result ==
ParentResultType::DIRECT);
return false;
}
return rec->host_override;
```
##########
tests/gold_tests/tls/tls_sni_parent_failover.test.py:
##########
@@ -0,0 +1,169 @@
+'''
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = '''
+Test parent failover sni name
+'''
+
+# Verify proxy.config.http.cache.ignore_authentication behavior.
Review Comment:
This comment appears to be copy/pasted and unrelated to this test (it
mentions proxy.config.http.cache.ignore_authentication). Please update/remove
it to match the actual intent (SNI name handling with parent failover).
```suggestion
# Verify SNI name handling with parent failover.
```
##########
tests/gold_tests/tls/valgrind.sh:
##########
@@ -0,0 +1,24 @@
+#!/bin/bash -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+valgrind \
+ --leak-check=full \
+ --log-file=vlog.txt \
+ ts/bin/traffic_server \
+ $* \
Review Comment:
Using $* unquoted will word-split and drop empty arguments; it also
mishandles arguments containing spaces. Prefer forwarding args with "$@" to
preserve exact argument boundaries.
```suggestion
"$@" \
```
##########
tests/gold_tests/tls/ssl/gen_foobar_certs.sh:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# bash script used to generate sni parent test certs
+
+for name in foo bar
+do
+ openssl req -x509 -newkey rsa:2048 \
+ -keyout server-${name}.key -out server-${name}.pem \
+ -days 3650 -nodes \
+ -subj "/C=US/ST=CO/L=Denver/O=Comcast/OU=Edge/CN=${name}.com Root" \
Review Comment:
The subject CN in the generation script ("CN=${name}.com Root") doesn't
match the committed certificates (which have CN set to "foo.com" / "bar.com").
Update the script to reproduce the checked-in certs, or regenerate the cert/key
pair from this script to avoid future confusion.
```suggestion
-subj "/C=US/ST=CO/L=Denver/O=Comcast/OU=Edge/CN=${name}.com" \
```
--
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]
