From 8bde40fdd379118b21a98f5aaa9e856027596b74 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 17:55:40 +0000 Subject: [PATCH] Make CircMgrConfig transparent (and make it a trait) See commentary for the rationale. --- crates/arti-client/src/client.rs | 10 +++---- crates/arti-client/src/config.rs | 23 ++++++---------- crates/tor-circmgr/src/config.rs | 47 ++++++++++++-------------------- crates/tor-circmgr/src/lib.rs | 35 ++++++++++-------------- 4 files changed, 47 insertions(+), 68 deletions(-) diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs index d728ef2d2..e772125ec 100644 --- a/crates/arti-client/src/client.rs +++ b/crates/arti-client/src/client.rs @@ -332,11 +332,9 @@ impl TorClient { autobootstrap: BootstrapBehavior, dirmgr_builder: &dyn crate::builder::DirProviderBuilder, ) -> StdResult { - let circ_cfg = config.get_circmgr_config()?; let dir_cfg = config.get_dirmgr_config()?; let statemgr = FsStateMgr::from_path(config.storage.expand_state_dir()?)?; let addr_cfg = config.address_filter.clone(); - let timeout_cfg = config.stream_timeouts; let (status_sender, status_receiver) = postage::watch::channel(); let status_receiver = status::BootstrapEvents { @@ -344,9 +342,10 @@ impl TorClient { }; let chanmgr = Arc::new(tor_chanmgr::ChanMgr::new(runtime.clone())); let circmgr = - tor_circmgr::CircMgr::new(circ_cfg, statemgr.clone(), &runtime, Arc::clone(&chanmgr)) + tor_circmgr::CircMgr::new(&config, statemgr.clone(), &runtime, Arc::clone(&chanmgr)) .map_err(ErrorDetail::CircMgrSetup)?; + let timeout_cfg = config.stream_timeouts; let dirmgr = dirmgr_builder .build(runtime.clone(), Arc::clone(&circmgr), dir_cfg) .map_err(crate::Error::into_detail)?; @@ -541,7 +540,6 @@ impl TorClient { _ => {} } - let circ_cfg = new_config.get_circmgr_config().map_err(wrap_err)?; let dir_cfg = new_config.get_dirmgr_config().map_err(wrap_err)?; let state_cfg = new_config.storage.expand_state_dir().map_err(wrap_err)?; let addr_cfg = &new_config.address_filter; @@ -551,7 +549,9 @@ impl TorClient { how.cannot_change("storage.state_dir").map_err(wrap_err)?; } - self.circmgr.reconfigure(&circ_cfg, how).map_err(wrap_err)?; + self.circmgr + .reconfigure(new_config, how) + .map_err(wrap_err)?; self.dirmgr.reconfigure(&dir_cfg, how).map_err(wrap_err)?; if how == tor_config::Reconfigure::CheckAllOrNothing { diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index 1c778e55e..2a4254729 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -12,6 +12,7 @@ //! [#285]: https://gitlab.torproject.org/tpo/core/arti/-/issues/285 use derive_builder::Builder; +use derive_more::AsRef; use serde::Deserialize; use std::collections::HashMap; use std::path::Path; @@ -22,8 +23,8 @@ pub use tor_config::{CfgPath, ConfigBuildError, Reconfigure}; /// Types for configuring how Tor circuits are built. pub mod circ { pub use tor_circmgr::{ - CircMgrConfig, CircMgrConfigBuilder, CircuitTiming, CircuitTimingBuilder, PathConfig, - PathConfigBuilder, PreemptiveCircuitConfig, PreemptiveCircuitConfigBuilder, + CircMgrConfig, CircuitTiming, CircuitTimingBuilder, PathConfig, PathConfigBuilder, + PreemptiveCircuitConfig, PreemptiveCircuitConfigBuilder, }; } @@ -270,7 +271,7 @@ impl SystemConfig { /// to change. For more information see ticket [#285]. /// /// [#285]: https://gitlab.torproject.org/tpo/core/arti/-/issues/285 -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq, AsRef)] pub struct TorClientConfig { /// Information about the Tor network we want to connect to. tor_network: dir::NetworkConfig, @@ -286,12 +287,15 @@ pub struct TorClientConfig { override_net_params: tor_netdoc::doc::netstatus::NetParams, /// Information about how to build paths through the network. + #[as_ref] path_rules: circ::PathConfig, /// Information about preemptive circuits. + #[as_ref] preemptive_circuits: circ::PreemptiveCircuitConfig, /// Information about how to retry and expire circuits and request for circuits. + #[as_ref] circuit_timing: circ::CircuitTiming, /// Rules about which addresses the client is willing to connect to. @@ -304,6 +308,8 @@ pub struct TorClientConfig { pub system: SystemConfig, } +impl tor_circmgr::CircMgrConfig for TorClientConfig {} + impl Default for TorClientConfig { fn default() -> Self { Self::builder() @@ -329,17 +335,6 @@ impl TorClientConfig { } dircfg.build() } - - /// Return a [`CircMgrConfig`](circ::CircMgrConfig) object based on the user's selected - /// configuration. - pub(crate) fn get_circmgr_config(&self) -> Result { - let mut builder = circ::CircMgrConfigBuilder::default(); - builder - .path_rules(self.path_rules.clone()) - .circuit_timing(self.circuit_timing.clone()) - .preemptive_circuits(self.preemptive_circuits.clone()) - .build() - } } /// Builder object used to construct a [`TorClientConfig`]. diff --git a/crates/tor-circmgr/src/config.rs b/crates/tor-circmgr/src/config.rs index 74330dfee..6e0adecc8 100644 --- a/crates/tor-circmgr/src/config.rs +++ b/crates/tor-circmgr/src/config.rs @@ -248,37 +248,26 @@ impl PreemptiveCircuitConfig { } } -/// Configuration for a circuit manager. +/// Configuration for a circuit manager /// -/// This configuration includes information about how to build paths -/// on the Tor network, and rules for timeouts and retries on Tor -/// circuits. +/// If the circuit manager gains new configurabilities, this trait will gain additional +/// supertraits, as an API break. /// -/// This type is immutable once constructed. To create an object of -/// this type, use [`CircMgrConfigBuilder`], or deserialize it from a -/// string. (Arti generally uses Toml for configuration, but you can -/// use other formats if you prefer.) -#[derive(Debug, Clone, Builder, Default, Eq, PartialEq)] -#[builder(build_fn(error = "ConfigBuildError"))] -pub struct CircMgrConfig { - /// Override the default required distance for two relays to share - /// the same circuit. - #[builder(default)] - pub(crate) path_rules: PathConfig, - - /// Timing and retry information related to circuits themselves. - #[builder(default)] - pub(crate) circuit_timing: CircuitTiming, - - /// Information related to preemptive circuits. - #[builder(default)] - pub(crate) preemptive_circuits: PreemptiveCircuitConfig, -} - -impl CircMgrConfig { - /// Return a new [`CircMgrConfigBuilder`]. - pub fn builder() -> CircMgrConfigBuilder { - CircMgrConfigBuilder::default() +/// Prefer to use `TorClientConfig`, which will always implement this trait. +pub trait CircMgrConfig: + AsRef + AsRef + AsRef +{ + /// Get the PathConfig + fn path_rules(&self) -> &PathConfig { + self.as_ref() + } + /// Get the CircuitTiming + fn circuit_timing(&self) -> &CircuitTiming { + self.as_ref() + } + /// Get the PreemptiveCircuitConfig + fn preemptive_circuits(&self) -> &PreemptiveCircuitConfig { + self.as_ref() } } diff --git a/crates/tor-circmgr/src/lib.rs b/crates/tor-circmgr/src/lib.rs index 066367f00..cf1161760 100644 --- a/crates/tor-circmgr/src/lib.rs +++ b/crates/tor-circmgr/src/lib.rs @@ -74,8 +74,8 @@ pub use err::Error; pub use usage::{IsolationToken, StreamIsolation, StreamIsolationBuilder, TargetPort, TargetPorts}; pub use config::{ - CircMgrConfig, CircMgrConfigBuilder, CircuitTiming, CircuitTimingBuilder, PathConfig, - PathConfigBuilder, PreemptiveCircuitConfig, PreemptiveCircuitConfigBuilder, + CircMgrConfig, CircuitTiming, CircuitTimingBuilder, PathConfig, PathConfigBuilder, + PreemptiveCircuitConfig, PreemptiveCircuitConfigBuilder, }; use crate::preemptive::PreemptiveCircuitPredictor; @@ -157,8 +157,8 @@ pub struct CircMgr { impl CircMgr { /// Construct a new circuit manager. - pub fn new( - config: CircMgrConfig, + pub fn new( + config: &CFG, storage: SM, runtime: &R, chanmgr: Arc>, @@ -166,14 +166,8 @@ impl CircMgr { where SM: tor_persist::StateMgr + Send + Sync + 'static, { - let CircMgrConfig { - path_rules, - circuit_timing, - preemptive_circuits, - } = config; - let preemptive = Arc::new(Mutex::new(PreemptiveCircuitPredictor::new( - preemptive_circuits, + config.preemptive_circuits().clone(), ))); let guardmgr = tor_guardmgr::GuardMgr::new(runtime.clone(), storage.clone())?; @@ -183,11 +177,12 @@ impl CircMgr { let builder = build::CircuitBuilder::new( runtime.clone(), chanmgr, - path_rules, + config.path_rules().clone(), storage_handle, guardmgr, ); - let mgr = mgr::AbstractCircMgr::new(builder, runtime.clone(), circuit_timing); + let mgr = + mgr::AbstractCircMgr::new(builder, runtime.clone(), config.circuit_timing().clone()); let circmgr = Arc::new(CircMgr { mgr: Arc::new(mgr), predictor: preemptive, @@ -199,16 +194,16 @@ impl CircMgr { /// Try to change our configuration settings to `new_config`. /// /// The actual behavior here will depend on the value of `how`. - pub fn reconfigure( + pub fn reconfigure( &self, - new_config: &CircMgrConfig, + new_config: &CFG, how: tor_config::Reconfigure, ) -> std::result::Result<(), tor_config::ReconfigureError> { let old_path_rules = self.mgr.peek_builder().path_config(); let predictor = self.predictor.lock().expect("poisoned lock"); let preemptive_circuits = predictor.config(); if preemptive_circuits.initial_predicted_ports - != new_config.preemptive_circuits.initial_predicted_ports + != new_config.preemptive_circuits().initial_predicted_ports { // This change has no effect, since the list of ports was _initial_. how.cannot_change("preemptive_circuits.initial_predicted_ports")?; @@ -219,15 +214,15 @@ impl CircMgr { } let discard_circuits = !new_config - .path_rules + .path_rules() .at_least_as_permissive_as(&old_path_rules); self.mgr .peek_builder() - .set_path_config(new_config.path_rules.clone()); + .set_path_config(new_config.path_rules().clone()); self.mgr - .set_circuit_timing(new_config.circuit_timing.clone()); - predictor.set_config(new_config.preemptive_circuits.clone()); + .set_circuit_timing(new_config.circuit_timing().clone()); + predictor.set_config(new_config.preemptive_circuits().clone()); if discard_circuits { // TODO(nickm): Someday, we might want to take a more lenient approach, and only