-
Notifications
You must be signed in to change notification settings - Fork 66
feat: Add HTTP proxy support for OTLP/OTAP gRPC exporters #1679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1679 +/- ##
========================================
Coverage 84.08% 84.09%
========================================
Files 469 470 +1
Lines 137651 138441 +790
========================================
+ Hits 115746 116422 +676
- Misses 21371 21485 +114
Partials 534 534
🚀 New features to boost your workflow:
|
- Limit HTTP headers in proxy response to prevent DoS (100 headers, 8KB max) - Fix IPv6 address parsing in NO_PROXY (handle bracketed literals) - Redact credentials from proxy URLs in debug logs - Clarify performance documentation for has_proxy/effective_proxy_config - Restrict visibility of internal proxy helper methods - Fix test compilation error in otlp_exporter_proxy_tls.rs
- Make effective_proxy_config private as it's only used internally - Remove unused has_proxy method from GrpcClientSettings
Remove unused field 'rusage_thread_supported' from struct.
| let tcp_keepalive_retries = self.tcp_keepalive_retries; | ||
|
|
||
| service_fn(move |uri: http::Uri| { | ||
| let proxy = Arc::clone(&proxy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use Arc because tonic/tower requires Send + 'static for connectors.
| let tcp_keepalive_interval = self.tcp_keepalive_interval; | ||
| let tcp_keepalive_retries = self.tcp_keepalive_retries; | ||
|
|
||
| service_fn(move |uri: http::Uri| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This closure runs per TCP connection, not per RPC. With HTTP/2 multiplexing, connections are long-lived (startup + reconnects only), so the cost here is negligible. The cost here involved are:
Arc::clonefor proxy object,get_proxy_for_urilookup- string operations in
should_bypass(have TODO in code to optimize this later).
Also this code path is only invoked when a proxy is configured. Without a proxy, tonic uses its default connector directly, so above overhead is not there.
|
this is ready for review now. |
lquerel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really great to add this support to both our OTLP and OTAP exporters. I made a few suggestions, mainly to make this functionality more secure and more robust with certain proxies.
I think it would be a good idea to add complete documentation in a markdown file describing this tunneling capability. For now, we have a very detailed PR description, but unfortunately it won't be seen by future users.
| /// This function performs a series of conversions (tokio -> std -> socket2 -> std -> tokio) | ||
| /// to apply socket options that are not directly exposed by tokio's TcpStream. | ||
| /// Specifically, `socket2` is required to set detailed keepalive parameters (interval, retries). | ||
| fn apply_socket_options( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace this chain of conversions tokio -> std -> socket2 -> std -> tokio by creating a socket2 first, configuring it, and then converting it to a Tokio TcpStream. This would simplify the code and would most likely be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! You're right that creating the socket2 first, configuring it, and then converting to tokio would eliminate the conversion chain.
The main reason I didn't go that route is the async connect handling - since socket2 is synchronous, we'd need to properly handle non-blocking connect (dealing with EINPROGRESS/WouldBlock, waiting for writability, checking SO_ERROR, etc.). The current approach lets Tokio handle the async connect, and we just use socket2 to apply the keepalive options on an already-established socket.
Since this only runs on connection establishment (not per-RPC), I felt the simpler approach is fine for now. Happy to track it as a follow-up issue if you think it's worth revisiting.
Thanks for the thorough review and great suggestions! I have tried incorporating them in code and with tracking issues. Also copied the architecture details from PR desc to |
lquerel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes: #1678
Summary
Implements HTTP CONNECT proxy support for OTLP/OTAP gRPC exporters:
HTTP/1.1 CONNECTtunneling to the proxy for bothhttp://targets (gRPC over h2c inside the tunnel) andhttps://targets (TLS inside the tunnel).HTTP_PROXY,HTTPS_PROXY,NO_PROXY,ALL_PROXY)192.168.0.0/16,10.0.0.0/8)TCP_NODELAY+keepalive). When using a proxy, these are applied to the TCP connection to the proxy (the same connection that carries the CONNECT tunnel).https://proxy URLs with helpful error messagesWhy OTLP/OTAP exporters need custom proxy implementation
Azure Monitor and Geneva exporters use the
reqwestHTTP client, which provides built-in proxy support viareqwest::Proxy::all()- no custom code needed.OTLP/OTAP exporters use gRPC (
tonic), which:tower::service_fnThis PR implements the missing proxy infrastructure for gRPC-based exporters.
Changes
proxy.rs: HTTP/1.1 CONNECT tunnel implementation with socket option handlingclient_settings.rs: Integration with tonic via custom connectorsocket2andipnetcratesHow HTTP CONNECT Tunneling Works
Step 1: THE HANDSHAKE
The Exporter creates a TCP connection to the Proxy and sends a plaintext request. The Proxy establishes a TCP leg to the Backend.
Step 2: THE DATA TUNNEL
Once the 200 is received, the Exporter uses the socket for the actual OTLP data. The Proxy merely moves bytes back and forth.
Test Setup:
The implementation was verified via a manual end-to-end test setup (not included in the PR). The architecture below was used to validate proxy traversal for both h2c and TLS traffic: