Skip to content

Commit d32ae90

Browse files
committed
fix: resolve clippy warnings and improve code quality and done some fixes
Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
1 parent e942545 commit d32ae90

File tree

12 files changed

+194
-123
lines changed

12 files changed

+194
-123
lines changed

orion-configuration/src/config/cluster.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ impl From<LbEndpointVecDeser> for Vec<LbEndpoint> {
155155
Address::Internal(internal_addr) => Some(LbEndpoint {
156156
address: EndpointAddress::Internal(InternalEndpointAddress {
157157
server_listener_name: internal_addr.server_listener_name.into(),
158-
endpoint_id: internal_addr.endpoint_id.map(|id| id.into()),
158+
endpoint_id: internal_addr.endpoint_id.map(std::convert::Into::into),
159159
}),
160160
health_status: HealthStatus::default(),
161161
load_balancing_weight: NonZeroU32::MIN,
@@ -193,7 +193,7 @@ impl EndpointAddress {
193193
pub fn into_addr(self) -> Result<SocketAddr, String> {
194194
match self {
195195
EndpointAddress::Socket(addr) => Ok(addr),
196-
EndpointAddress::Internal(_) => Err("Cannot convert internal address to socket address".to_string()),
196+
EndpointAddress::Internal(_) => Err("Cannot convert internal address to socket address".to_owned()),
197197
}
198198
}
199199
}
@@ -785,7 +785,7 @@ mod envoy_conversions {
785785
Address::Socket(socket_addr) => Ok(EndpointAddress::Socket(socket_addr)),
786786
Address::Internal(internal_addr) => Ok(EndpointAddress::Internal(InternalEndpointAddress {
787787
server_listener_name: internal_addr.server_listener_name.into(),
788-
endpoint_id: internal_addr.endpoint_id.map(|id| id.into()),
788+
endpoint_id: internal_addr.endpoint_id.map(std::convert::Into::into),
789789
})),
790790
Address::Pipe(_, _) => {
791791
Err(GenericError::unsupported_variant("Pipe addresses are not supported for endpoints"))

orion-configuration/src/config/core.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ pub mod envoy_conversions {
305305
impl std::fmt::Display for Address {
306306
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
307307
match self {
308-
Self::Socket(addr) => write!(f, "{}", addr),
308+
Self::Socket(addr) => write!(f, "{addr}"),
309309
Self::Internal(internal) => write!(f, "internal:{}", internal.server_listener_name),
310310
Self::Pipe(path, _) => f.write_str(path),
311311
}

orion-configuration/src/config/runtime.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ fn get_cgroup_v2_cpu_limit() -> crate::Result<usize> {
144144
}
145145

146146
fn parse_cgroup_v2_cpu_max(content: &str) -> crate::Result<usize> {
147-
let parts: Vec<&str> = content.trim().split_whitespace().collect();
147+
let parts: Vec<&str> = content.split_whitespace().collect();
148148
if parts.len() == 2 && parts[0] != "max" {
149149
let quota: i64 = parts[0].parse()?;
150150
let period: i64 = parts[1].parse()?;
@@ -160,8 +160,8 @@ fn parse_cgroup_v2_cpu_max(content: &str) -> crate::Result<usize> {
160160

161161
fn get_cgroup_v1_cpu_limit() -> crate::Result<usize> {
162162
let cgroup_path = get_cgroup_v1_cpu_path()?;
163-
let quota_path = format!("{}/cpu.cfs_quota_us", cgroup_path);
164-
let period_path = format!("{}/cpu.cfs_period_us", cgroup_path);
163+
let quota_path = format!("{cgroup_path}/cpu.cfs_quota_us");
164+
let period_path = format!("{cgroup_path}/cpu.cfs_period_us");
165165

166166
let quota_content = std::fs::read_to_string(&quota_path)?;
167167
let period_content = std::fs::read_to_string(&period_path)?;
@@ -193,7 +193,7 @@ fn parse_cgroup_v1_cpu_path(cgroup_content: &str) -> crate::Result<String> {
193193
let mut parts = line.split(':');
194194
if let (Some(_), Some(controllers), Some(path)) = (parts.next(), parts.next(), parts.next()) {
195195
if controllers.split(',').any(|c| c == "cpu") {
196-
return Some(format!("/sys/fs/cgroup/cpu{}", path));
196+
return Some(format!("/sys/fs/cgroup/cpu{path}"));
197197
}
198198
}
199199
None
@@ -202,7 +202,7 @@ fn parse_cgroup_v1_cpu_path(cgroup_content: &str) -> crate::Result<String> {
202202
}
203203

204204
if std::path::Path::new("/sys/fs/cgroup/cpu").exists() {
205-
Ok("/sys/fs/cgroup/cpu".to_string())
205+
Ok("/sys/fs/cgroup/cpu".to_owned())
206206
} else {
207207
Err("CPU cgroup path not found".into())
208208
}

orion-lib/src/clusters/load_assignment.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ impl EndpointAddressType {
8080
if let Ok(socket_addr) = addr_str.parse::<std::net::SocketAddr>() {
8181
EndpointAddress::Socket(socket_addr)
8282
} else {
83-
panic!("Cannot convert authority back to socket address: {}", addr_str);
83+
panic!("Cannot convert authority back to socket address: {addr_str}");
8484
}
8585
},
8686
EndpointAddressType::Internal(internal_addr, _) => EndpointAddress::Internal(internal_addr.clone()),

orion-lib/src/listeners/filter_state.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,17 @@ use compact_str::CompactString;
1919
use orion_configuration::config::common::TlvType;
2020
use std::{collections::HashMap, net::SocketAddr};
2121

22+
/// Reserved internal address used as the peer address for in-process (internal) connections.
23+
/// Chosen from the loopback range (127.255.255.254:65534) to avoid conflicts with real network traffic.
24+
/// This address clearly identifies internal connections in logs and debugging.
2225
const INTERNAL_PEER_ADDR: std::net::SocketAddr =
2326
std::net::SocketAddr::V4(std::net::SocketAddrV4::new(std::net::Ipv4Addr::new(127, 255, 255, 254), 65534));
27+
28+
/// Reserved internal address used as the local address for in-process (internal) connections.
29+
/// Chosen from the loopback range (127.255.255.255:65535) to avoid conflicts with real network traffic.
30+
/// This address clearly identifies internal connections in logs and debugging.
2431
const INTERNAL_LOCAL_ADDR: std::net::SocketAddr =
2532
std::net::SocketAddr::V4(std::net::SocketAddrV4::new(std::net::Ipv4Addr::new(127, 255, 255, 255), 65535));
26-
2733
#[derive(Debug, Clone)]
2834
pub enum DownstreamConnectionMetadata {
2935
FromSocket {
@@ -45,8 +51,8 @@ pub enum DownstreamConnectionMetadata {
4551
proxy_local_address: SocketAddr,
4652
},
4753
/// Internal connections from in-process communication (e.g., waypoint proxy)
48-
/// - listener_name: identifies which internal listener accepted this connection
49-
/// - endpoint_id: optional identifier for the specific endpoint (used for load balancing)
54+
/// - `listener_name`: identifies which internal listener accepted this connection
55+
/// - `endpoint_id`: optional identifier for the specific endpoint (used for load balancing)
5056
FromInternal {
5157
listener_name: String,
5258
endpoint_id: Option<String>,

orion-lib/src/listeners/http_connection_manager.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,7 +1024,7 @@ impl Service<ExtendedRequest<Incoming>> for HttpRequestHandler {
10241024
add,
10251025
1,
10261026
trans_handler.thread_id(),
1027-
&[KeyValue::new("listener", listener_name_for_route.to_string())]
1027+
&[KeyValue::new("listener", listener_name_for_route.to_owned())]
10281028
);
10291029

10301030
if let Some(state) = trans_handler.span_state.as_ref() {
@@ -1049,7 +1049,7 @@ impl Service<ExtendedRequest<Incoming>> for HttpRequestHandler {
10491049
add,
10501050
nbytes + resp_head_size as u64,
10511051
trans_handler.thread_id(),
1052-
&[KeyValue::new("listener", listener_name_for_response.to_string())]
1052+
&[KeyValue::new("listener", listener_name_for_response.to_owned())]
10531053
);
10541054

10551055
let is_transaction_complete = if let Some(ctx) = trans_handler.access_log_ctx.as_ref() {

orion-lib/src/listeners/http_connection_manager/route.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ impl<'a> RequestHandler<(MatchedRequest<'a>, &HttpConnectionManager)> for &Route
194194
let err = err.into_inner();
195195
let event_error = EventError::try_infer_from(&err);
196196
let flags = event_error.clone().map(ResponseFlags::from).unwrap_or_default();
197-
let event_kind = event_error.map_or(EventKind::ViaUpstream, |e| EventKind::Error(e));
197+
let event_kind = event_error.map_or(EventKind::ViaUpstream, EventKind::Error);
198198
debug!(
199199
"HttpConnectionManager Error processing response {:?}: {}({})",
200200
err,
@@ -211,7 +211,7 @@ impl<'a> RequestHandler<(MatchedRequest<'a>, &HttpConnectionManager)> for &Route
211211
let err = err.into_inner();
212212
let event_error = EventError::try_infer_from(&err);
213213
let flags = event_error.clone().map(ResponseFlags::from).unwrap_or_default();
214-
let event_kind = event_error.map_or(EventKind::ViaUpstream, |e| EventKind::Error(e));
214+
let event_kind = event_error.map_or(EventKind::ViaUpstream, EventKind::Error);
215215
debug!(
216216
"Failed to get an HTTP connection: {:?}: {}({})",
217217
err,

orion-lib/src/listeners/listener.rs

Lines changed: 76 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -57,46 +57,9 @@ use tokio::{
5757
};
5858
use tracing::{debug, info, warn};
5959

60-
async fn handle_internal_connection_static(
61-
listener_name: String,
62-
connection_pair: crate::transport::InternalConnectionPair,
63-
filter_chains: Arc<HashMap<FilterChainMatch, FilterchainType>>,
64-
) -> Result<()> {
65-
use crate::listeners::filter_state::DownstreamConnectionMetadata;
66-
67-
debug!("Handling new internal connection for listener '{}'", listener_name);
68-
69-
let downstream_metadata = DownstreamConnectionMetadata::FromInternal {
70-
listener_name: listener_name.clone(),
71-
endpoint_id: connection_pair.downstream.metadata().endpoint_id.clone(),
72-
};
73-
74-
let filter_chain = match Listener::select_filterchain(&filter_chains, &downstream_metadata, None)? {
75-
Some(fc) => fc,
76-
None => {
77-
warn!("No matching filter chain found for internal connection");
78-
return Err(crate::Error::new("No matching filter chain"));
79-
},
80-
};
81-
82-
let _downstream_stream = connection_pair.downstream;
83-
84-
match &filter_chain.handler {
85-
crate::listeners::filterchain::ConnectionHandler::Http(_http_manager) => {
86-
info!("Processing internal connection through HTTP filter chain");
87-
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
88-
Ok(())
89-
},
90-
crate::listeners::filterchain::ConnectionHandler::Tcp(_tcp_proxy) => {
91-
info!("Processing internal connection through TCP filter chain");
92-
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
93-
Ok(())
94-
},
95-
}
96-
}
97-
9860
#[derive(Debug)]
9961
struct InternalConnectionWorkerPool {
62+
workers: Vec<tokio::task::JoinHandle<()>>,
10063
senders: Vec<mpsc::UnboundedSender<InternalConnectionTask>>,
10164
next_worker: std::sync::atomic::AtomicUsize,
10265
}
@@ -111,6 +74,7 @@ struct InternalConnectionTask {
11174
impl InternalConnectionWorkerPool {
11275
fn new(num_workers: usize) -> Self {
11376
let mut senders: Vec<mpsc::UnboundedSender<InternalConnectionTask>> = Vec::with_capacity(num_workers);
77+
let mut workers = Vec::with_capacity(num_workers);
11478

11579
for _ in 0..num_workers {
11680
let (sender, mut receiver) = mpsc::unbounded_channel();
@@ -126,16 +90,24 @@ impl InternalConnectionWorkerPool {
12690
}
12791
}
12892
});
129-
drop(worker);
93+
workers.push(worker);
13094
}
13195

132-
Self { senders, next_worker: std::sync::atomic::AtomicUsize::new(0) }
96+
Self { workers, senders, next_worker: std::sync::atomic::AtomicUsize::new(0) }
13397
}
13498

13599
fn submit_task(&self, task: InternalConnectionTask) -> Result<()> {
136100
let worker_index = self.next_worker.fetch_add(1, std::sync::atomic::Ordering::Relaxed) % self.senders.len();
137101
self.senders[worker_index].send(task).map_err(|_| Error::new("Worker pool is shut down"))
138102
}
103+
104+
async fn shutdown(self) {
105+
drop(self.senders);
106+
107+
for worker in self.workers {
108+
let _ = worker.await;
109+
}
110+
}
139111
}
140112

141113
static INTERNAL_WORKER_POOL: std::sync::OnceLock<InternalConnectionWorkerPool> = std::sync::OnceLock::new();
@@ -402,7 +374,7 @@ impl Listener {
402374
maybe_route_update = route_updates_receiver.recv() => {
403375
//todo: add context to the error here once orion-error lands
404376
match maybe_route_update {
405-
Ok(route_update) => {Self::process_route_update(&name, &filter_chains, route_update)},
377+
Ok(route_update) => {Self::process_route_update(name, &filter_chains, route_update)},
406378
Err(e) => {return e.into();}
407379
}
408380
},
@@ -411,7 +383,7 @@ impl Listener {
411383
Ok(secret_update) => {
412384
// todo: possibly expensive clone - may need to rethink this structure
413385
let mut filter_chains_clone = filter_chains.as_ref().clone();
414-
Self::process_secret_update(&name, &mut filter_chains_clone, secret_update);
386+
Self::process_secret_update(name, &mut filter_chains_clone, secret_update);
415387
filter_chains = Arc::new(filter_chains_clone);
416388
}
417389
Err(e) => {return e.into();}
@@ -436,8 +408,7 @@ impl Listener {
436408
let filter_chains = Arc::new(filter_chains);
437409
let factory = global_internal_connection_factory();
438410

439-
let (_handle, mut connection_receiver, _listener_ref) = match factory.register_listener(name.to_string()).await
440-
{
411+
let (_handle, mut connection_receiver, _listener_ref) = match factory.register_listener(name.to_owned()).await {
441412
Ok(result) => result,
442413
Err(e) => {
443414
error!("Failed to register internal listener '{}': {}", name, e);
@@ -450,35 +421,32 @@ impl Listener {
450421
loop {
451422
tokio::select! {
452423
maybe_connection = connection_receiver.recv() => {
453-
match maybe_connection {
454-
Some(connection_pair) => {
455-
debug!("Internal listener '{}' received new connection", name);
456-
457-
let filter_chains_clone = filter_chains.clone();
458-
let listener_name = name.to_string();
459-
460-
// Use worker pool instead of tokio::spawn for better performance
461-
// with large numbers of short connections
462-
let task = InternalConnectionTask {
463-
listener_name,
464-
connection_pair,
465-
filter_chains: filter_chains_clone,
466-
};
467-
468-
if let Err(e) = get_internal_worker_pool().submit_task(task) {
469-
warn!("Failed to submit internal connection task: {}", e);
470-
}
471-
}
472-
None => {
473-
warn!("Internal listener '{}' connection channel closed", name);
474-
break;
424+
if let Some(connection_pair) = maybe_connection {
425+
debug!("Internal listener '{}' received new connection", name);
426+
427+
let filter_chains_clone = filter_chains.clone();
428+
let listener_name = name.to_owned();
429+
430+
// Use worker pool instead of tokio::spawn for better performance
431+
// with large numbers of short connections
432+
let task = InternalConnectionTask {
433+
listener_name,
434+
connection_pair,
435+
filter_chains: filter_chains_clone,
436+
};
437+
438+
if let Err(e) = get_internal_worker_pool().submit_task(task) {
439+
warn!("Failed to submit internal connection task: {}", e);
475440
}
441+
} else {
442+
warn!("Internal listener '{}' connection channel closed", name);
443+
break;
476444
}
477445
},
478446
maybe_route_update = route_updates_receiver.recv() => {
479447
match maybe_route_update {
480448
Ok(route_update) => {
481-
Self::process_route_update(&name, &filter_chains, route_update);
449+
Self::process_route_update(name, &filter_chains, route_update);
482450
}
483451
Err(e) => {
484452
error!("Route update error for internal listener '{}': {}", name, e);
@@ -490,7 +458,7 @@ impl Listener {
490458
match maybe_secret_update {
491459
Ok(secret_update) => {
492460
let mut filter_chains_clone = filter_chains.as_ref().clone();
493-
Self::process_secret_update(&name, &mut filter_chains_clone, secret_update);
461+
Self::process_secret_update(name, &mut filter_chains_clone, secret_update);
494462
// TODO: Update the shared filter chains state for active connections
495463
}
496464
Err(e) => {
@@ -616,8 +584,8 @@ impl Listener {
616584

617585
let ssl = AtomicBool::new(false);
618586
defer! {
619-
with_metric!(listeners::DOWNSTREAM_CX_DESTROY, add, 1, shard_id, &[KeyValue::new("listener", listener_name.to_string())]);
620-
with_metric!(listeners::DOWNSTREAM_CX_ACTIVE, sub, 1, shard_id, &[KeyValue::new("listener", listener_name.to_string())]);
587+
with_metric!(listeners::DOWNSTREAM_CX_DESTROY, add, 1, shard_id, &[KeyValue::new("listener", listener_name.to_owned())]);
588+
with_metric!(listeners::DOWNSTREAM_CX_ACTIVE, sub, 1, shard_id, &[KeyValue::new("listener", listener_name.to_owned())]);
621589
if ssl.load(Ordering::Relaxed) {
622590
with_metric!(http::DOWNSTREAM_CX_SSL_ACTIVE, add, 1, shard_id, &[KeyValue::new("listener", listener_name)]);
623591
}
@@ -656,7 +624,7 @@ impl Listener {
656624
add,
657625
1,
658626
shard_id,
659-
&[KeyValue::new("listener", listener_name.to_string())]
627+
&[KeyValue::new("listener", listener_name.to_owned())]
660628
);
661629
with_metric!(
662630
http::DOWNSTREAM_CX_SSL_ACTIVE,
@@ -1014,3 +982,40 @@ filter_chains:
1014982
assert_eq!(Listener::select_filterchain(&m, &metadata, Some("hello.world")).unwrap().copied(), Some(3));
1015983
}
1016984
}
985+
986+
async fn handle_internal_connection_static(
987+
listener_name: String,
988+
connection_pair: crate::transport::InternalConnectionPair,
989+
filter_chains: Arc<HashMap<FilterChainMatch, FilterchainType>>,
990+
) -> Result<()> {
991+
use crate::listeners::filter_state::DownstreamConnectionMetadata;
992+
993+
debug!("Handling new internal connection for listener '{}'", listener_name);
994+
995+
let downstream_metadata = DownstreamConnectionMetadata::FromInternal {
996+
listener_name: listener_name.clone(),
997+
endpoint_id: connection_pair.downstream.metadata().endpoint_id.clone(),
998+
};
999+
1000+
let filter_chain = if let Some(fc) = Listener::select_filterchain(&filter_chains, &downstream_metadata, None)? {
1001+
fc
1002+
} else {
1003+
warn!("No matching filter chain found for internal connection");
1004+
return Err(crate::Error::new("No matching filter chain"));
1005+
};
1006+
1007+
let _downstream_stream = connection_pair.downstream;
1008+
1009+
match &filter_chain.handler {
1010+
crate::listeners::filterchain::ConnectionHandler::Http(_http_manager) => {
1011+
info!("Processing internal connection through HTTP filter chain");
1012+
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
1013+
Ok(())
1014+
},
1015+
crate::listeners::filterchain::ConnectionHandler::Tcp(_tcp_proxy) => {
1016+
info!("Processing internal connection through TCP filter chain");
1017+
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
1018+
Ok(())
1019+
},
1020+
}
1021+
}

0 commit comments

Comments
 (0)