Copilot commented on code in PR #12868: URL: https://github.com/apache/trafficserver/pull/12868#discussion_r2813382541
########## tests/gold_tests/tls/tls_sni_parent_failover.test.py: ########## @@ -0,0 +1,164 @@ +''' +''' +# 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 +''' + +# SNI name handling with parent failover. Review Comment: This commented-out line appears to be leftover test code. If it's not needed, consider removing it. If it's intended for future use or reference, add a comment explaining why it's kept. ```suggestion # SNI name handling with parent failover. # The following replay-based test is intentionally disabled; the explicit # origin/ATS configuration below provides the current coverage and this # line is kept only as a reference for manual/debug use. ``` ########## tests/gold_tests/tls/tls_sni_parent_failover.test.py: ########## @@ -0,0 +1,164 @@ +''' +''' +# 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 Review Comment: The module docstring is empty. Consider adding a meaningful description of what this test module does, such as "Test parent failover with SNI name handling when using another CDN as parent." ```suggestion '''Test parent failover with SNI name handling, validating that Traffic Server correctly fails over between HTTPS parents (including strategy-based parents) while enforcing TLS server name verification.''' # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file ``` ########## tests/gold_tests/tls/tls_sni_parent_failover.test.py: ########## @@ -0,0 +1,164 @@ +''' +''' +# 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 +''' + +# SNI name handling with parent failover. +#Test.ATSReplayTest(replay_file="replay/tls_sni_parent_failover.replay.yaml") + +# Define default ATS +ts = Test.MakeATSProcess( + "ts", + enable_tls=True, + enable_cache=False, +) + +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.addSSLfile("ssl/server-foo.pem") +ts.addSSLfile("ssl/server-foo.key") +ts.addSSLfile("ssl/server-bar.pem") +ts.addSSLfile("ssl/server-bar.key") + +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', Review Comment: This commented-out configuration line should either be removed or documented with a comment explaining why it's kept. Leaving alternative configuration options without explanation can cause confusion. ```suggestion ``` ########## tests/gold_tests/tls/tls_sni_parent_failover.test.py: ########## @@ -0,0 +1,164 @@ +''' +''' +# 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 +''' + +# SNI name handling with parent failover. +#Test.ATSReplayTest(replay_file="replay/tls_sni_parent_failover.replay.yaml") + +# Define default ATS +ts = Test.MakeATSProcess( + "ts", + enable_tls=True, + enable_cache=False, +) + +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.addSSLfile("ssl/server-foo.pem") +ts.addSSLfile("ssl/server-foo.key") +ts.addSSLfile("ssl/server-bar.pem") +ts.addSSLfile("ssl/server-bar.key") + +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', Review Comment: This commented-out configuration line should either be removed or documented with a comment explaining why it's kept. Leaving alternative configuration options without explanation can cause confusion during maintenance. -- 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]
