Merge branch 'config-partials-transparent' into 'main'

Absolish builders for CircMgrConfig and DirMgrConfig

See merge request tpo/core/arti!417
This commit is contained in:
Ian Jackson 2022-03-17 12:30:46 +00:00
commit 40bede587c
13 changed files with 207 additions and 153 deletions

2
Cargo.lock generated
View File

@ -145,6 +145,7 @@ dependencies = [
"tor-config",
"tor-dirmgr",
"tor-error",
"tor-netdoc",
"tor-persist",
"tor-proto",
"tor-rtcompat",
@ -3123,6 +3124,7 @@ dependencies = [
name = "tor-basic-utils"
version = "0.1.0"
dependencies = [
"derive_more",
"educe",
"rand 0.8.5",
]

View File

@ -34,6 +34,7 @@ tor-config = { path="../tor-config", version = "0.1.0"}
tor-chanmgr = { path="../tor-chanmgr", version = "0.1.0"}
tor-dirmgr = { path="../tor-dirmgr", version = "0.1.0"}
tor-error = { path="../tor-error", version = "0.1.0"}
tor-netdoc = { path="../tor-netdoc", version = "0.1.0"}
tor-persist = { path="../tor-persist", version = "0.1.0"}
tor-proto = { path="../tor-proto", version = "0.1.0"}
tor-rtcompat = { path="../tor-rtcompat", version = "0.1.0"}

View File

@ -332,11 +332,9 @@ impl<R: Runtime> TorClient<R> {
autobootstrap: BootstrapBehavior,
dirmgr_builder: &dyn crate::builder::DirProviderBuilder<R>,
) -> StdResult<Self, ErrorDetail> {
let circ_cfg = config.get_circmgr_config()?;
let dir_cfg = config.get_dirmgr_config()?;
let dir_cfg = (&config).try_into()?;
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<R: Runtime> TorClient<R> {
};
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,8 +540,7 @@ impl<R: Runtime> TorClient<R> {
_ => {}
}
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 dir_cfg = new_config.try_into().map_err(wrap_err)?;
let state_cfg = new_config.storage.expand_state_dir().map_err(wrap_err)?;
let addr_cfg = &new_config.address_filter;
let timeout_cfg = &new_config.stream_timeouts;
@ -551,7 +549,9 @@ impl<R: Runtime> TorClient<R> {
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 {

View File

@ -12,8 +12,10 @@
//! [#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::convert::TryInto;
use std::path::Path;
use std::path::PathBuf;
use std::time::Duration;
@ -22,17 +24,17 @@ 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,
};
}
/// Types for configuring how Tor accesses its directory information.
pub mod dir {
pub use tor_dirmgr::{
Authority, AuthorityBuilder, DirMgrConfig, DirMgrConfigBuilder, DownloadSchedule,
DownloadScheduleConfig, DownloadScheduleConfigBuilder, FallbackDir, FallbackDirBuilder,
NetworkConfig, NetworkConfigBuilder,
Authority, AuthorityBuilder, DirMgrConfig, DownloadSchedule, DownloadScheduleConfig,
DownloadScheduleConfigBuilder, FallbackDir, FallbackDirBuilder, NetworkConfig,
NetworkConfigBuilder,
};
}
@ -270,7 +272,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,
@ -283,15 +285,18 @@ pub struct TorClientConfig {
/// Facility to override network parameters from the values set in the
/// consensus.
override_net_params: HashMap<String, i32>,
override_net_params: tor_netdoc::doc::netstatus::NetParams<i32>,
/// 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 +309,8 @@ pub struct TorClientConfig {
pub system: SystemConfig,
}
impl tor_circmgr::CircMgrConfig for TorClientConfig {}
impl Default for TorClientConfig {
fn default() -> Self {
Self::builder()
@ -317,27 +324,19 @@ impl TorClientConfig {
pub fn builder() -> TorClientConfigBuilder {
TorClientConfigBuilder::default()
}
/// Build a DirMgrConfig from this configuration.
pub(crate) fn get_dirmgr_config(&self) -> Result<dir::DirMgrConfig, ConfigBuildError> {
let mut dircfg = dir::DirMgrConfigBuilder::default();
dircfg.network_config(self.tor_network.clone());
dircfg.schedule_config(self.download_schedule.clone());
dircfg.cache_path(self.storage.expand_cache_dir()?);
for (k, v) in &self.override_net_params {
dircfg.override_net_param(k.clone(), *v);
}
dircfg.build()
}
/// Return a [`CircMgrConfig`](circ::CircMgrConfig) object based on the user's selected
/// configuration.
pub(crate) fn get_circmgr_config(&self) -> Result<circ::CircMgrConfig, ConfigBuildError> {
let mut builder = circ::CircMgrConfigBuilder::default();
builder
.path_rules(self.path_rules.clone())
.circuit_timing(self.circuit_timing.clone())
.build()
impl TryInto<dir::DirMgrConfig> for &TorClientConfig {
type Error = ConfigBuildError;
#[rustfmt::skip]
fn try_into(self) -> Result<dir::DirMgrConfig, ConfigBuildError> {
Ok(dir::DirMgrConfig {
network_config: self.tor_network .clone(),
schedule_config: self.download_schedule .clone(),
cache_path: self.storage.expand_cache_dir()?,
override_net_params: self.override_net_params.clone(),
})
}
}
@ -391,7 +390,11 @@ impl TorClientConfigBuilder {
.download_schedule
.build()
.map_err(|e| e.within("download_schedule"))?;
let override_net_params = self.override_net_params.clone();
let mut override_net_params = tor_netdoc::doc::netstatus::NetParams::new();
for (k, v) in &self.override_net_params {
override_net_params.set(k.clone(), *v);
}
let path_rules = self
.path_rules
.build()

View File

@ -15,4 +15,5 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/"
rand = "0.8"
[dev-dependencies]
derive_more = "0.99"
educe = "0.4.6"

View File

@ -72,3 +72,71 @@ pub fn skip_fmt<T>(_: &T, f: &mut fmt::Formatter) -> fmt::Result {
}
// ----------------------------------------------------------------------
/// Define an "accessor trait", which describes structs that have fields of certain types
///
/// This can be useful if a large struct, living high up in the dependency graph,
/// contains fields that lower-lever crates want to be able to use without having
/// to copy the data about etc.
///
/// ```
/// // imagine this in the lower-level module
/// use tor_basic_utils::define_accessor_trait;
/// define_accessor_trait! {
/// pub trait View {
/// lorem: String,
/// ipsum: usize,
/// }
/// }
///
/// fn test_view<V: View>(v: &V) {
/// assert_eq!(v.lorem(), "sit");
/// assert_eq!(v.ipsum(), &42);
/// }
///
/// // imagine this in the higher-level module
/// use derive_more::AsRef;
/// #[derive(AsRef)]
/// struct Everything {
/// #[as_ref] lorem: String,
/// #[as_ref] ipsum: usize,
/// dolor: Vec<()>,
/// }
/// impl View for Everything { }
///
/// let everything = Everything {
/// lorem: "sit".into(),
/// ipsum: 42,
/// dolor: vec![()],
/// };
///
/// test_view(&everything);
/// ```
///
/// ### Generated code
///
/// ```
/// pub trait View: AsRef<String> + AsRef<usize> {
/// fn lorem(&self) -> &String { self.as_ref() }
/// fn ipsum(&self) -> &usize { self.as_ref() }
/// }
/// ```
#[macro_export]
macro_rules! define_accessor_trait {
{
$( #[ $attr:meta ])*
$vis:vis trait $Trait:ident {
$( $accessor:ident: $type:ty, )*
}
} => {
$( #[ $attr ])*
$vis trait $Trait: $( core::convert::AsRef<$type> + )* {
$(
/// Access the field
fn $accessor(&self) -> &$type { core::convert::AsRef::as_ref(self) }
)*
}
}
}
// ----------------------------------------------------------------------

View File

@ -4,6 +4,7 @@
//!
//! Most types in this module are re-exported by `arti-client`.
use tor_basic_utils::define_accessor_trait;
use tor_config::ConfigBuildError;
use derive_builder::Builder;
@ -248,37 +249,38 @@ impl PreemptiveCircuitConfig {
}
}
/// Configuration for a circuit manager.
define_accessor_trait! {
/// 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.
//
// We do not use a builder here. Instead, additions or changes here are API breaks.
//
// Rationale:
//
// The purpose of using a builder is to allow the code to continue to
// compile when new fields are added to the built struct.
//
// However, here, the DirMgrConfig is just a subset of the fields of a
// TorClientConfig, and it is important that all its fields are
// initialised by arti-client.
//
// If it grows a field, arti-client ought not to compile any more.
//
// Indeed, we have already had a bug where a manually-written
// conversion function omitted to copy a config field from
// TorClientConfig into then-existing CircMgrConfigBuilder.
//
// We use this AsRef-based trait, so that we can pass a reference
// to the configuration when we build a new CircMgr, rather than
// cloning all the fields an extra time.
pub trait CircMgrConfig {
path_rules: PathConfig,
circuit_timing: CircuitTiming,
preemptive_circuits: PreemptiveCircuitConfig,
}
}

View File

@ -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<R: Runtime> {
impl<R: Runtime> CircMgr<R> {
/// Construct a new circuit manager.
pub fn new<SM>(
config: CircMgrConfig,
pub fn new<SM, CFG: CircMgrConfig>(
config: &CFG,
storage: SM,
runtime: &R,
chanmgr: Arc<ChanMgr<R>>,
@ -166,14 +166,8 @@ impl<R: Runtime> CircMgr<R> {
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<R: Runtime> CircMgr<R> {
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<R: Runtime> CircMgr<R> {
/// 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<CFG: CircMgrConfig>(
&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<R: Runtime> CircMgr<R> {
}
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

View File

@ -151,28 +151,36 @@ impl DownloadScheduleConfig {
/// Configuration type for network directory operations.
///
/// This type is immutable once constructed.
/// If the directory manager gains new configurabilities, this structure will gain additional
/// supertraits, as an API break.
///
/// To create an object of this type, use [`DirMgrConfigBuilder`], or
/// deserialize it from a string. (Arti generally uses Toml for configuration,
/// but you can use other formats if you prefer.)
///
/// Many members of this type can be replaced with a new configuration on a
/// running Arti client. Those that cannot are documented.
#[derive(Debug, Clone, Builder, Eq, PartialEq)]
#[builder(build_fn(error = "ConfigBuildError"))]
#[builder(derive(Deserialize))]
/// Prefer to use `TorClientConfig`, which will always be convertible to this struct
/// via `TryInto`.
//
// We do not use a builder here. Instead, additions or changes here are API breaks.
//
// Rationale:
//
// The purpose of using a builder is to allow the code to continue to
// compile when new fields are added to the built struct.
//
// However, here, the DirMgrConfig is just a subset of the fields of a
// TorClientConfig, and it is important that all its fields are
// initialised by arti-client.
//
// If it grows a field, arti-client ought not to compile any more.
#[derive(Debug, Clone)]
#[cfg_attr(test, derive(Default))]
#[allow(clippy::exhaustive_structs)]
pub struct DirMgrConfig {
/// Location to use for storing and reading current-format
/// directory information.
///
/// Cannot be changed on a running Arti client.
#[builder(setter(into))]
cache_path: PathBuf,
pub cache_path: PathBuf,
/// Configuration information about the network.
#[builder(default)]
network_config: NetworkConfig,
pub network_config: NetworkConfig,
/// Configuration information about when we download things.
///
@ -184,8 +192,7 @@ pub struct DirMgrConfig {
/// on in-progress attempts as well, at least at the top level. Users
/// should _not_ assume that the effect of changing this option will always
/// be delayed.)
#[builder(default)]
schedule_config: DownloadScheduleConfig,
pub schedule_config: DownloadScheduleConfig,
/// A map of network parameters that we're overriding from their settings in
/// the consensus.
@ -196,36 +203,10 @@ pub struct DirMgrConfig {
/// (The above is a limitation: we would like it to someday take effect
/// immediately. Users should _not_ assume that the effect of changing this
/// option will always be delayed.)
#[builder(default)]
override_net_params: netstatus::NetParams<i32>,
}
impl DirMgrConfigBuilder {
/// Overrides the network consensus parameter named `param` with a
/// new value.
///
/// If the new value is out of range, it will be clamped to the
/// acceptable range.
///
/// If the parameter is not recognized by Arti, it will be
/// ignored, and a warning will be produced when we try to apply
/// it to the consensus.
///
/// By default no parameters will be overridden.
pub fn override_net_param(&mut self, param: String, value: i32) -> &mut Self {
self.override_net_params
.get_or_insert_with(netstatus::NetParams::default)
.set(param, value);
self
}
pub override_net_params: netstatus::NetParams<i32>,
}
impl DirMgrConfig {
/// Return a new builder to construct a DirMgrConfig.
pub fn builder() -> DirMgrConfigBuilder {
DirMgrConfigBuilder::default()
}
/// Create a store from this configuration.
///
/// Note that each time this is called, a new store object will be
@ -354,10 +335,10 @@ mod test {
fn simplest_config() -> Result<()> {
let tmp = tempdir().unwrap();
let dir = DirMgrConfigBuilder::default()
.cache_path(tmp.path().to_path_buf())
.build()
.unwrap();
let dir = DirMgrConfig {
cache_path: tmp.path().into(),
..Default::default()
};
assert!(dir.authorities().len() >= 3);
assert!(dir.fallbacks().len() >= 3);
@ -434,18 +415,13 @@ mod test {
#[test]
fn build_dirmgrcfg() -> Result<()> {
let mut bld = DirMgrConfig::builder();
let mut bld = DirMgrConfig::default();
let tmp = tempdir().unwrap();
let cfg = bld
.override_net_param("circwindow".into(), 999)
.cache_path(tmp.path())
.network_config(NetworkConfig::default())
.schedule_config(DownloadScheduleConfig::default())
.build()
.unwrap();
bld.override_net_params.set("circwindow".into(), 999);
bld.cache_path = tmp.path().into();
assert_eq!(cfg.override_net_params().get("circwindow").unwrap(), &999);
assert_eq!(bld.override_net_params().get("circwindow").unwrap(), &999);
Ok(())
}

View File

@ -91,8 +91,8 @@ use std::{fmt::Debug, time::SystemTime};
pub use authority::{Authority, AuthorityBuilder};
pub use config::{
DirMgrConfig, DirMgrConfigBuilder, DownloadScheduleConfig, DownloadScheduleConfigBuilder,
NetworkConfig, NetworkConfigBuilder,
DirMgrConfig, DownloadScheduleConfig, DownloadScheduleConfigBuilder, NetworkConfig,
NetworkConfigBuilder,
};
pub use docid::DocId;
pub use err::Error;
@ -1063,10 +1063,10 @@ mod test {
pub(crate) fn new_mgr<R: Runtime>(runtime: R) -> (TempDir, DirMgr<R>) {
let dir = TempDir::new().unwrap();
let config = DirMgrConfig::builder()
.cache_path(dir.path())
.build()
.unwrap();
let config = DirMgrConfig {
cache_path: dir.path().into(),
..Default::default()
};
let dirmgr = DirMgr::from_config(config, runtime, None, false).unwrap();
(dir, dirmgr)

View File

@ -966,11 +966,11 @@ mod test {
if let Some(a) = authorities {
netcfg.authorities(a);
}
let cfg = DirMgrConfig::builder()
.cache_path("/we_will_never_use_this/")
.network_config(netcfg.build().unwrap())
.build()
.unwrap();
let cfg = DirMgrConfig {
cache_path: "/we_will_never_use_this/".into(),
network_config: netcfg.build().unwrap(),
..Default::default()
};
let cfg = Arc::new(cfg);
DirRcv {
now,

View File

@ -152,7 +152,7 @@ pub struct NetParams<T> {
impl<T> NetParams<T> {
/// Create a new empty list of NetParams.
#[allow(unused)]
pub(crate) fn new() -> Self {
pub fn new() -> Self {
NetParams {
params: HashMap::new(),
}

View File

@ -31,6 +31,9 @@ arti-client:
Replace ArtiClientBuilder's methods for individual elements of TorClientConfigBuilder
with an accessor `.tor()` to get `&mut TorClientConfigBuilder`.
Abolished `TorClientConfig::get_circmgr_config`.
Abolished `TorClientConfig::get_dirmgr_config`.
arti:
Provide library crate with unstable API.
@ -46,7 +49,10 @@ arti-client:
tor-dirmgr:
new-api: DirMgrConfig object now has accessors.
DirMgrCfg: totally changed, builder abolished.
tor-circmgr:
CircMgrCfg: totally changed, builder abolished.
tor-netdoc: