-
Notifications
You must be signed in to change notification settings - Fork 18
feat: add internal listener and upstream transport support #87
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
feat: add internal listener and upstream transport support #87
Conversation
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.
Summary of Changes
Hello @Eeshu-Yadav, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant new capability by adding support for internal listeners and internal upstream transport. This feature allows for efficient, in-process communication between different components or services within the proxy, bypassing traditional network overhead. It lays the groundwork for more advanced internal routing and service mesh functionalities, improving performance and enabling new architectural patterns for inter-service communication.
Highlights
- Internal Listener Implementation: Introduced the
InternalListenerwithListenerAddressenum and buffer configuration, enabling listeners to operate without binding to a network socket. - Internal Endpoint and Transport Support: Added
EndpointAddressandInternalEndpointAddressfor internal routing within the proxy. ImplementedTransportSocketwithInternalUpstreamandRawBuffervariants to facilitate internal communication channels. - Metadata Passthrough: Enabled metadata passthrough via
InternalUpstreamTransport,MetadataValueSource, andMetadataKind, allowing rich context to be carried across internal connections. - Bootstrap and Core Component Integration: Integrated
BootstrapExtensionandInternalListenerBootstrapfor factory registration and updated bootstrap parsing. Modified load balancer and listener manager to support internal addresses and connections. - Envoy Protobuf Conversion Updates: Extended Envoy protobuf conversion logic to correctly handle the new internal listener, endpoint, and transport socket configurations.
- String Type Refactoring: Refactored various
listener_nameandTracingKeyfields from&'static strtoStringacrossorion-libandorion-configurationto allow for more flexible ownership and usage.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces significant new functionality by adding support for internal listeners and upstream transports. This is a great feature for improving performance of inter-service communication on the same host. The changes are extensive, touching configuration, the core listener and cluster logic, and the load balancing implementation. The introduction of new enums like ListenerAddress and EndpointAddress to handle both socket and internal addresses is well-designed. The addition of tests for the new configuration serialization is also a great practice.
I have identified a few areas for improvement:
- A potential panic in the load balancer when using certain policies with internal endpoints.
- Performance concerns related to string allocations for metrics on the hot path.
- Some complex and potentially buggy logic in the Envoy configuration conversion for transport sockets.
Detailed comments are provided below. Overall, this is a solid contribution.
| let upstream_transport_socket_config = if let Some(ts) = transport_socket.as_ref() { | ||
| Some(UpstreamTransportSocketConfig::try_from(ts.clone()).with_node("transport_socket")?) | ||
| } else { | ||
| None | ||
| }; | ||
| let (_tls_config, transport_socket_config) = if let Some(ts) = transport_socket { | ||
| match TransportSocket::try_from(ts.clone()) { | ||
| Ok(transport_socket) => (None, Some(transport_socket)), | ||
| Err(_) => { | ||
| // Fall back to TLS config conversion - extract UpstreamTlsContext from TransportSocket | ||
| if let Some(orion_data_plane_api::envoy_data_plane_api::envoy::config::core::v3::transport_socket::ConfigType::TypedConfig(typed_config)) = &ts.config_type { | ||
| if typed_config.type_url == "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext" { | ||
| let upstream_tls_context = UpstreamTlsContext::decode(typed_config.value.as_slice()) | ||
| .map_err(|e| GenericError::from_msg_with_cause("Failed to decode UpstreamTlsContext", e))?; | ||
| let tls_config = TlsConfig::try_from(upstream_tls_context).with_node("transport_socket")?; | ||
| (Some(tls_config), None) | ||
| } else { | ||
| return Err(GenericError::from_msg(format!("Unsupported transport socket type: {}", typed_config.type_url))); | ||
| } | ||
| } else { | ||
| (None, None) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| (None, None) | ||
| }; |
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 logic for parsing the transport socket appears to have a bug and is also confusing.
- The first block calculating
upstream_transport_socket_config(lines 591-595) will likely fail the entire cluster conversion if thetransport_socketis an internal transport type, asUpstreamTransportSocketConfig::try_fromwould return an error. - The second block (lines 596-617) calculates
_tls_configwhich is then discarded. This is dead code and makes the logic hard to follow.
The intention seems to be to populate either transport_socket (for TLS) or internal_transport_socket (for internal listeners), but not both. The current implementation is incorrect and inefficient.
I suggest refactoring this to conditionally parse the transport socket.
let (upstream_transport_socket_config, transport_socket_config) = if let Some(ts) = transport_socket {
if let Ok(internal_socket) = TransportSocket::try_from(ts.clone()) {
// It's an internal transport socket.
(None, Some(internal_socket))
} else {
// It's not an internal transport socket, so try to parse it as a regular upstream transport socket (e.g., TLS).
let upstream_socket = UpstreamTransportSocketConfig::try_from(ts).with_node("transport_socket")?;
(Some(upstream_socket), None)
}
} else {
(None, None)
};| with_metric!( | ||
| http::DOWNSTREAM_CX_TOTAL, | ||
| add, | ||
| 1, | ||
| shard_id, | ||
| &[KeyValue::new("listener", listener_name.to_string())] | ||
| ); | ||
| with_metric!( | ||
| http::DOWNSTREAM_CX_ACTIVE, | ||
| add, | ||
| 1, | ||
| shard_id, | ||
| &[KeyValue::new("listener", listener_name.to_string())] | ||
| ); | ||
| defer! { | ||
| with_metric!(http::DOWNSTREAM_CX_DESTROY, add, 1, shard_id, &[KeyValue::new("listener", listener_name)]); | ||
| with_metric!(http::DOWNSTREAM_CX_ACTIVE, sub, 1, shard_id, &[KeyValue::new("listener", listener_name)]); | ||
| with_metric!(http::DOWNSTREAM_CX_DESTROY, add, 1, shard_id, &[KeyValue::new("listener", listener_name.to_string())]); | ||
| with_metric!(http::DOWNSTREAM_CX_ACTIVE, sub, 1, shard_id, &[KeyValue::new("listener", listener_name.to_string())]); | ||
| let ms = u64::try_from(start_instant.elapsed().as_millis()).unwrap_or(u64::MAX); | ||
| with_histogram!(http::DOWNSTREAM_CX_LENGTH_MS, record, ms, &[KeyValue::new("listener", listener_name)]); | ||
| with_histogram!(http::DOWNSTREAM_CX_LENGTH_MS, record, ms, &[KeyValue::new("listener", listener_name.to_string())]); | ||
| } |
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.
In this block, and in several other places in the listener and HTTP connection manager code, listener_name.to_string() is called within the with_metric! and with_histogram! macros. Since these metrics are on the hot path (per-connection), creating a new String for each metric can introduce performance overhead due to repeated memory allocations.
This pattern is also present in http_connection_manager.rs. Consider using an Arc<String> or Arc<CompactString> for the listener name throughout the listener stack. This would allow sharing the name string across threads and components without cloning the string data itself, only incrementing the reference count.
e2d4464 to
19d2b11
Compare
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.
Please provide a sample of Envoy/Orion configuration. Examples can be found in orion-proxy/conf so this can be tested with Envoy and Orion.
An example is here. Envoy bootstrap section should be indentical (but stripped down) with your envoy configuration files.
Otherwise you could provide just an envoy bootstrap file and use this to start Orion
orion --config orion-runtime.yaml --with-envoy-bootstrap envoy-bootstrap.yaml
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum EndpointAddressType { |
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.
Minor thing, but could we collapse EndpointAddressType and EndpointConnection enums into one.. maybe something like this:
enum EndpointAddressType{
Socket(Authority, HttpChannel, TcpChannel),
Internal(InternalEndpointAddress, InternalConnection)
}
|
Two small asks... |
Thanks @dawid-nowak, noted! I’ll make the changes you suggested. |
| @@ -0,0 +1,160 @@ | |||
| use orion_configuration::config::{ | |||
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.
Should have an example as well, like what you do in #83
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.
Okk , will add that
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.
@YaoZengzeng kindly review
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.
Pull Request Overview
This PR adds comprehensive support for internal listeners and internal transport sockets to the Orion proxy, enabling internal routing and communication between components without external network interfaces.
- Adds
InternalListener,ListenerAddress, andInternalEndpointAddresstypes for internal routing - Implements
TransportSocketwithInternalUpstreamand metadata passthrough capabilities - Updates configuration parsing, load balancing, and connection handling throughout the codebase
Reviewed Changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| orion-tracing/src/lib.rs | Updates closure pattern matching for TracingKey |
| orion-tracing/src/http_tracer.rs | Converts string literals to String type for TracingKey |
| orion-proxy/src/admin/config_dump.rs | Updates imports and test data for new address types |
| orion-lib/src/listeners/tcp_proxy.rs | Changes listener_name from static str to String |
| orion-lib/src/listeners/listeners_manager.rs | Adds support for internal listeners and updates listener handling |
| orion-lib/src/listeners/listener.rs | Implements internal listener support with new address types |
| orion-lib/src/listeners/http_connection_manager.rs | Updates listener_name handling throughout |
| orion-lib/src/listeners/filterchain.rs | Updates metric handling for new string-based listener names |
| orion-lib/src/clusters/load_assignment.rs | Adds support for internal endpoints and connection types |
| orion-lib/src/clusters/cluster/original_dst.rs | Adds internal_transport_socket field |
| orion-lib/src/clusters/cluster/dynamic.rs | Updates endpoint address handling |
| orion-lib/src/clusters/balancers/default_balancer.rs | Updates tests for new endpoint address types |
| orion-data-plane-api/src/xds/model.rs | Boxes large enum variants to reduce memory usage |
| orion-data-plane-api/src/xds/client.rs | Updates error handling for boxed Status |
| orion-data-plane-api/src/decode.rs | Adds clippy allow annotation |
| orion-data-plane-api/src/bootstrap_loader/bootstrap.rs | Adds clippy allow annotation |
| orion-configuration/tests/test_internal_listener.rs | New test file for internal listener functionality |
| orion-configuration/src/config/network_filters/tracing.rs | Updates TracingKey to use String instead of static str |
| orion-configuration/src/config/network_filters/http_connection_manager.rs | Removes comment formatting |
| orion-configuration/src/config/listener.rs | Implements ListenerAddress enum with internal listener support |
| orion-configuration/src/config/core.rs | Adds InternalAddress type and Address enum variants |
| orion-configuration/src/config/cluster.rs | Adds TransportSocket, InternalEndpointAddress, and metadata types |
| orion-configuration/src/config/bootstrap.rs | Adds BootstrapExtension and InternalListenerBootstrap |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| pub fn authority(&self) -> &Authority { | ||
| match &self.address { | ||
| EndpointAddressType::Socket(authority) => authority, | ||
| EndpointAddressType::Internal(_) => panic!("Internal endpoints don't have authorities"), |
Copilot
AI
Sep 9, 2025
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.
Using panic! for handling internal endpoints is not robust. Consider returning a Result or Option instead of panicking, which could crash the application in production.
| // For internal endpoints, we'll use a placeholder authority | ||
| // This might need to be handled differently based on the balancer requirements | ||
| panic!("Internal endpoints don't have authorities") |
Copilot
AI
Sep 9, 2025
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.
Another panic! for internal endpoints. This should be handled gracefully with proper error handling rather than crashing the application.
| // For internal endpoints, we'll use a placeholder authority | |
| // This might need to be handled differently based on the balancer requirements | |
| panic!("Internal endpoints don't have authorities") | |
| // For internal endpoints, return a static dummy authority and log a warning | |
| static DUMMY_AUTHORITY: Authority = Authority::from_static("internal.invalid"); | |
| tracing::warn!("Internal endpoints don't have authorities, returning dummy authority"); | |
| &DUMMY_AUTHORITY |
orion-lib/src/listeners/listener.rs
Outdated
| name, | ||
| socket_address: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| name: name.into(), | ||
| address: ListenerAddress::Socket(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 0)), |
Copilot
AI
Sep 9, 2025
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.
[nitpick] Using Ipv4Addr::new(127, 0, 0, 1) is more verbose than Ipv4Addr::LOCALHOST. Consider using the constant for better readability.
| address: ListenerAddress::Socket(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 0)), | |
| address: ListenerAddress::Socket(SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0)), |
| let _start = std::time::Instant::now(); | ||
|
|
Copilot
AI
Sep 9, 2025
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.
The variable _start is assigned but not used. If this was meant to replace the start variable for timing, it should be named start instead.
| let _start = std::time::Instant::now(); |
8a99be0 to
3553029
Compare
kindly review |
|
@Eeshu-Yadav , thanks.. I should have a look at it early next week |
dawid-nowak
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.
In a some places, &'static str has been changed to String, which then results in having to clone those strings. And cloning apprently affects performance.
Ideally, all those changes should be reverted.
| @@ -0,0 +1,74 @@ | |||
| runtime: | |||
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.
Please copy this file to orion-proxy/conf/
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.
Done - copied orion-config.yam to internal-listener-demo.yaml
| downstream_metadata: Arc<DownstreamConnectionMetadata>, | ||
| shard_id: ThreadId, | ||
| listener_name: &'static str, | ||
| listener_name: &str, |
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.
Why has this changed ? Because of this change you need to clone listener_str everywhere ..
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.
reverted listener_name back to &'static str in start_filterchain() method. Removed all string cloning in metrics and logging
b62c51a to
c95fd70
Compare
dawid-nowak
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.
Generally looks good, some minor changes /questions.
dawid-nowak
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.
@Eeshu-Yadav , so if I get this right... at the moment the internal communication is still not working. And in subsequent PRs we will will need to :
- Add of the internal connection factory
- Make changes changes in to
run_internal_lister - Make changes in clusters to so we can create outgoing connections ?
a4f8831 to
1e0048a
Compare
yes in the subsquent pr's , i will work on these functionalties |
1e0048a to
f0d913c
Compare
Sub-Issues for completing this |
d7cc9bf to
25aa4ec
Compare
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.
@Eeshu-Yadav Looks good... There is one small nit to fix (apologies for being pedantic here) and then we should be good to merge this one. Thanks again.
| pub struct LbEndpoint { | ||
| pub name: &'static str, | ||
| pub authority: http::uri::Authority, | ||
| pub name: CompactString, |
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.
nit: Is this change needed ?
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.
removed autority and use pub name: CompactString,
- Implement InternalListener with ListenerAddress enum and buffer configuration - Support internal listener in Envoy protobuf conversions and orion-lib - Add EndpointAddress and InternalEndpointAddress for internal routing - Implement TransportSocket with InternalUpstream and RawBuffer variants - Enable metadata passthrough via InternalUpstreamTransport, MetadataValueSource, and MetadataKind - Add BootstrapExtension and InternalListenerBootstrap for factory registration - Update bootstrap parsing and connection handling for internal endpoints - Integrate internal addresses in load balancer and listener manager Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
…dd TLV listener filter examples - Refactor EndpointAddressType and EndpointConnection into single enum - Combine socket and internal endpoint data into unified structure - Update all pattern matching and method implementations - Add sample TLV listener filter configurations for testing - Support both integrated and separate bootstrap configuration approaches Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
Add example for internal listeners and upstream transport. Includes configuration, test client, and validation script. Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Revert listener_name types from String/&str back to &'static str in: * filterchain.rs: FilterchainBuilder, start_filterchain, start_tls * http_connection_manager.rs: HttpConnectionManagerBuilder, HttpConnectionManager * listener.rs: Listener, PartialListener with proper interning - Remove unnecessary .clone() calls on &'static str references - Copy and rename internal listener demo config to orion-proxy/conf/ Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
8c3c1c0 to
2ac5c31
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dawid-nowak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
d7a5d24 to
8dfda50
Compare
- Add socket_authority() method to EndpointAddress for type-safe authority access - Implement Ord and Eq traits for EndpointAddressType for proper ordering - Refactor PartialLbEndpoint to use EndpointAddress directly instead of EndpointAddressType - Update cluster configuration to support internal endpoint addresses and upstream transport - Improve type safety and eliminate dummy authority usage Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
8dfda50 to
24e988f
Compare
|
@dawid-nowak Kindly merge this if no further changes required . |
fixes : #59