bneradt commented on code in PR #12257: URL: https://github.com/apache/trafficserver/pull/12257#discussion_r2165091279
########## tests/gold_tests/h2/clients/h2empty_data_frame.py: ########## @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +''' +HTTP/2 client that sends empty DATA frame with END_STREAM flag +''' +# 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. + +import socket +import ssl + +import h2.connection +import h2.events + +import argparse + + +# TODO: cleanup with other HTTP/2 clients (h2active_timeout.py and h2client.py) +def get_socket(port: int) -> socket.socket: + """Create a TLS-wrapped socket. + + :param port: The port to connect to. + + :returns: A TLS-wrapped socket. + """ + + SERVER_NAME = 'localhost' + SERVER_PORT = port + + # generic socket and ssl configuration + socket.setdefaulttimeout(15) + + # Configure an ssl client side context which will not check the server's certificate. + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + ctx.set_alpn_protocols(['h2']) + + # open a socket to the server and initiate TLS/SSL + tls_socket = socket.create_connection((SERVER_NAME, SERVER_PORT)) + tls_socket = ctx.wrap_socket(tls_socket, server_hostname=SERVER_NAME) + return tls_socket + + +def makerequest(port: int, path: str, n: int) -> None: + """Establish an HTTP/2 connection and send a request. + + :param port: The port to connect to. + :param path: The path to request. + :param n: Number of streams to open. + """ + + tls_socket = get_socket(port) + + h2_connection = h2.connection.H2Connection() + h2_connection.initiate_connection() + tls_socket.sendall(h2_connection.data_to_send()) + + headers = [ + (':method', 'GET'), + (':path', path), + (':authority', 'localhost'), + (':scheme', 'https'), + ] + + for stream_id in range(1, n * 2, 2): + h2_connection.send_headers(stream_id, headers, end_stream=False) + h2_connection.send_data(stream_id, b'', end_stream=True) + + tls_socket.sendall(h2_connection.data_to_send()) + + # keep reading from server + terminated = False + error_code = 0 + while not terminated: + # read raw data from the socket + data = tls_socket.recv(65536 * 1024) + if not data: + break + + # feed raw data into h2, and process resulting events + events = h2_connection.receive_data(data) + for event in events: + print(event) + if isinstance(event, h2.events.ConnectionTerminated): + if not event.error_code == 0: + error_code = event.error_code + terminated = True + + # send any pending data to the server + tls_socket.sendall(h2_connection.data_to_send()) + + # tell the server we are closing the h2 connection + h2_connection.close_connection() + tls_socket.sendall(h2_connection.data_to_send()) + + # close the socket + tls_socket.close() + + if not error_code == 0: Review Comment: ```python if error_code != 0: ``` ########## tests/gold_tests/h2/clients/h2empty_data_frame.py: ########## @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +''' +HTTP/2 client that sends empty DATA frame with END_STREAM flag +''' +# 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. + +import socket +import ssl + +import h2.connection +import h2.events + +import argparse + + +# TODO: cleanup with other HTTP/2 clients (h2active_timeout.py and h2client.py) +def get_socket(port: int) -> socket.socket: + """Create a TLS-wrapped socket. + + :param port: The port to connect to. + + :returns: A TLS-wrapped socket. + """ + + SERVER_NAME = 'localhost' + SERVER_PORT = port + + # generic socket and ssl configuration + socket.setdefaulttimeout(15) + + # Configure an ssl client side context which will not check the server's certificate. + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + ctx.set_alpn_protocols(['h2']) + + # open a socket to the server and initiate TLS/SSL + tls_socket = socket.create_connection((SERVER_NAME, SERVER_PORT)) + tls_socket = ctx.wrap_socket(tls_socket, server_hostname=SERVER_NAME) + return tls_socket + + +def makerequest(port: int, path: str, n: int) -> None: Review Comment: nit: `make_request` ########## src/proxy/http2/Http2Stream.cc: ########## @@ -1081,11 +1085,20 @@ Http2Stream::clear_io_events() } } -// release and do_io_close are the same for the HTTP/2 protocol +/** + Callback from HttpSM + + release and do_io_close are the same for the HTTP/2 protocol + */ void Http2Stream::release() { - this->do_io_close(); + if (_state == Http2StreamState::HTTP2_STREAM_STATE_CLOSED) { + this->do_io_close(); + return; + } + + Http2StreamDebug("release is called, but this stream is not ready"); Review Comment: Maybe: ``` Delaying do_io_close() until stream is in the closed state. ``` ########## tests/gold_tests/h2/http2_empty_data_frame.test.py: ########## @@ -0,0 +1,71 @@ +# 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. + +import sys + +Test.Sumary = ''' +Verify Empty DATA Frame Handling +''' + + +class Http2EmptyDataFrameTest: + + def __init__(self): + self.__setupOriginServer() + self.__setupTS() + self.__setupClient() + + def __setupOriginServer(self): + self._server = Test.MakeHttpBinServer("httpbin") + + def __setupTS(self): + self._ts = Test.MakeATSProcess(f"ts", enable_tls=True, enable_cache=True) + self._ts.addDefaultSSLFiles() + self._ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http2', + 'proxy.config.ssl.server.cert.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(self._ts.Variables.SSLDir), Review Comment: ``` 'proxy.config.ssl.server.cert.path': 'self._ts.Variables.SSLDir, 'proxy.config.ssl.server.private_key.path': self._ts.Variables.SSLDir, ``` ########## tests/gold_tests/h2/http2_empty_data_frame.test.py: ########## @@ -0,0 +1,71 @@ +# 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. + +import sys + +Test.Sumary = ''' +Verify Empty DATA Frame Handling +''' + + +class Http2EmptyDataFrameTest: + + def __init__(self): + self.__setupOriginServer() + self.__setupTS() + self.__setupClient() + + def __setupOriginServer(self): + self._server = Test.MakeHttpBinServer("httpbin") + + def __setupTS(self): + self._ts = Test.MakeATSProcess(f"ts", enable_tls=True, enable_cache=True) + self._ts.addDefaultSSLFiles() + self._ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http2', + 'proxy.config.ssl.server.cert.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.http.insert_response_via_str': 2, + 'proxy.config.http2.active_timeout_in': 3, + 'proxy.config.http2.stream_error_rate_threshold': 0.1 # default + }) + self._ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}/'.format(self._server.Variables.Port)) + self._ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key') + + def __setupClient(self): + self._ts.Setup.CopyAs("clients/h2empty_data_frame.py", Test.RunDirectory) + + def run(self): + tr = Test.AddTestRun() Review Comment: Adding descriptions to `AddTestRun()` is helpful when reading failed tests. ########## tests/gold_tests/h2/http2_empty_data_frame.test.py: ########## @@ -0,0 +1,71 @@ +# 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. + +import sys + +Test.Sumary = ''' +Verify Empty DATA Frame Handling +''' + + +class Http2EmptyDataFrameTest: + + def __init__(self): + self.__setupOriginServer() + self.__setupTS() + self.__setupClient() + + def __setupOriginServer(self): + self._server = Test.MakeHttpBinServer("httpbin") + + def __setupTS(self): + self._ts = Test.MakeATSProcess(f"ts", enable_tls=True, enable_cache=True) + self._ts.addDefaultSSLFiles() + self._ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http2', + 'proxy.config.ssl.server.cert.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.http.insert_response_via_str': 2, + 'proxy.config.http2.active_timeout_in': 3, + 'proxy.config.http2.stream_error_rate_threshold': 0.1 # default + }) + self._ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}/'.format(self._server.Variables.Port)) + self._ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key') + + def __setupClient(self): + self._ts.Setup.CopyAs("clients/h2empty_data_frame.py", Test.RunDirectory) + + def run(self): + tr = Test.AddTestRun() + + tr.Processes.Default.StartBefore(self._ts) + tr.Processes.Default.StartBefore(self._server, ready=When.PortOpen(self._server.Variables.Port)) Review Comment: Is this `ready=` condition not the default for `MakeHttpBinServer`? ########## tests/gold_tests/h2/http2_empty_data_frame.test.py: ########## @@ -0,0 +1,71 @@ +# 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. + +import sys + +Test.Sumary = ''' +Verify Empty DATA Frame Handling +''' + + +class Http2EmptyDataFrameTest: + + def __init__(self): + self.__setupOriginServer() + self.__setupTS() + self.__setupClient() + + def __setupOriginServer(self): + self._server = Test.MakeHttpBinServer("httpbin") + + def __setupTS(self): + self._ts = Test.MakeATSProcess(f"ts", enable_tls=True, enable_cache=True) + self._ts.addDefaultSSLFiles() + self._ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http2', + 'proxy.config.ssl.server.cert.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(self._ts.Variables.SSLDir), + 'proxy.config.http.insert_response_via_str': 2, + 'proxy.config.http2.active_timeout_in': 3, + 'proxy.config.http2.stream_error_rate_threshold': 0.1 # default + }) + self._ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}/'.format(self._server.Variables.Port)) Review Comment: `f'...'` rather than `.format()` (for new code anyway). -- 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: github-unsubscr...@trafficserver.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
