Minor fixes and polishing for TypeDB extension #114
Conversation
|
|
||
| def received_from_http2_client(self, data, default_handler): | ||
| def received_from_http2_client(self, data, default_handler: Callable): | ||
| if self.proxying is False: |
There was a problem hiding this comment.
Note: Had to slightly adjust the logic in here, to match the logic of what we had prior to the last refactoring round. We need to ensure that non-TypeDB requests sent via HTTP2 are not proxied and forwarded to the target properly.
The challenge is that this function may be called multiple times (for multiple HTTP2 frames) until the headers are fully received, hence we distinguish between the value of proxying being None (connection not fully initialized yet, still waiting for headers), and being False (connection initialized, headers indicate that the request is not in scope for the proxy).
This behavior can be tested by using the resource browser in the LocalStack Web application in Chrome browser, which sends connections via HTTP2 by default. (This logic is now also covered by a new integration test as part of this PR.)
There was a problem hiding this comment.
Right, that was the challenge — it was hard to follow the rationale originally. Thanks for fixing it here and adding the failing test! When extracting the logic it seemed there was ternary logic involved, and I don't like bool | None for that, so I started to use an Enum initially with match, to make this state machine more explicit. Then at some point I wrongly concluded that there were only two possible states, and reverted to a bool.
|
|
||
| def received_from_http2_client(self, data, default_handler): | ||
| def received_from_http2_client(self, data, default_handler: Callable): | ||
| if self.proxying is False: |
There was a problem hiding this comment.
Right, that was the challenge — it was hard to follow the rationale originally. Thanks for fixing it here and adding the failing test! When extracting the logic it seemed there was ternary logic involved, and I don't like bool | None for that, so I started to use an Enum initially with match, to make this state machine more explicit. Then at some point I wrongly concluded that there were only two possible states, and reverted to a bool.
| pip install localstack | ||
| make install | ||
| make lint |
There was a problem hiding this comment.
🙏 Thanks! I missed linting/formatting manually, doh!
| def _dataReceived(fn, self, data, *args, **kwargs): | ||
| self._ls_forwarding_buffer.received_from_http2_client(data, lambda d: fn(d, *args, **kwargs)) | ||
| self._ls_forwarding_buffer.received_from_http2_client( | ||
| data, lambda d: fn(self, d, *args, **kwargs) |
| """Parse the data from an HTTP2 stream into an iterable of frames""" | ||
| buffer = FrameBuffer(server=True) | ||
| buffer.max_frame_size = 16384 | ||
| buffer.add_data(data) |
There was a problem hiding this comment.
Curious about this one — did it fail?
There was a problem hiding this comment.
Yes, great catch - it does actually fail for certain HTTP bytes that are not representing a proper HTTP/2 preamble:
...
File "/opt/code/extensions/typedb/localstack_typedb/utils/h2_proxy.py", line 128, in get_headers_from_data_stream
return get_headers_from_frames(get_frames_from_http2_stream(stream))
File "/opt/code/extensions/typedb/localstack_typedb/utils/h2_proxy.py", line 135, in get_headers_from_frames
for frame in frames:
File "/opt/code/extensions/typedb/localstack_typedb/utils/h2_proxy.py", line 149, in get_frames_from_http2_stream
buffer.add_data(data)
File "/opt/code/localstack/.venv/lib/python3.13/site-packages/h2/frame_buffer.py", line 51, in add_data
raise ProtocolError(msg)
h2.exceptions.ProtocolError: Invalid HTTP/2 preamble.
Don't have a reproducible test case for this one (will try to pull something together!), but seen this in the logs a couple of times, hence adding the additional guard here..
| # make an HTTP/2 request to the LocalStack health endpoint | ||
| with httpx.Client(http2=True, verify=False, trust_env=False) as client: | ||
| health_url = f"{url}/_localstack/health" | ||
| response = client.get(health_url) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response.http_version == "HTTP/2" | ||
| assert '"services":' in response.text | ||
|
|
||
| # make an HTTP/2 request to a LocalStack endpoint outside the extension (S3 list buckets) | ||
| headers = { | ||
| "Authorization": "AWS4-HMAC-SHA256 Credential=000000000000/20250101/us-east-1/s3/aws4_request, ..." | ||
| } | ||
| with httpx.Client(http2=True, verify=False, trust_env=False) as client: | ||
| response = client.get(url, headers=headers) |
There was a problem hiding this comment.
Yes! If I had poked around a bit more with LS alongside the extension, the need for this would quickly have become more obvious.
Minor fixes and polishing for TypeDB extension. Follow-up from #108
selfparameter tolambdaexpression forreceived_from_http2_clientcall (was causing errors when running and testing the extension locally)ForwardingBuffer.received_from_http2_client(..)to fix connection handling for non-TypeDB HTTP2 requests - was previously getting connection timeouts, this can be tested by interacting with the emulator from the Web app resource browser using Chrome browser (which sends HTTP2 requests by default, at least for me under macOS)make linttarget, and add to CI build configArgumentErrorwithValueError0.1.1