guardmgr config: Introduce and require new GuardMgrConfig trait

It doesn't seem to me like it makes sense to provide the backward
compatibility here.

crates/arti-client/src/config.rs

@@ -398,6 +398,7 @@ impl AsRef<tor_guardmgr::fallback::FallbackList> for TorClientConfig {
         self.tor_network.fallback_caches()
     }
 }
+impl tor_guardmgr::GuardMgrConfig for TorClientConfig {}
 
 impl TorClientConfig {
     /// Try to create a DirMgrConfig corresponding to this object.

crates/tor-circmgr/src/config.rs

@@ -7,8 +7,7 @@
 use tor_basic_utils::define_accessor_trait;
 use tor_config::impl_standard_builder;
 use tor_config::{define_list_builder_accessors, define_list_builder_helper, ConfigBuildError};
-use tor_guardmgr::fallback::FallbackList;
-use tor_guardmgr::GuardFilter;
+use tor_guardmgr::{GuardFilter, GuardMgrConfig};
 
 use derive_builder::Builder;
 use serde::{Deserialize, Serialize};
@@ -286,11 +285,10 @@ define_accessor_trait! {
     // 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 {
+    pub trait CircMgrConfig: GuardMgrConfig {
         path_rules: PathConfig,
         circuit_timing: CircuitTiming,
         preemptive_circuits: PreemptiveCircuitConfig,
-        fallbacks: FallbackList,
     }
 }
 

crates/tor-circmgr/src/lib.rs

@@ -67,7 +67,7 @@ mod usage;
 pub use err::Error;
 pub use isolation::IsolationToken;
 use tor_guardmgr::fallback::FallbackList;
-pub use tor_guardmgr::{ClockSkewEvents, SkewEstimate};
+pub use tor_guardmgr::{ClockSkewEvents, GuardMgrConfig, SkewEstimate};
 pub use usage::{TargetPort, TargetPorts};
 
 pub use config::{
@@ -181,11 +181,7 @@ impl<R: Runtime> CircMgr<R> {
             config.preemptive_circuits().clone(),
         )));
 
-        let guardmgr = tor_guardmgr::GuardMgr::new(
-            runtime.clone(),
-            storage.clone(),
-            config.fallbacks().clone(),
-        )?;
+        let guardmgr = tor_guardmgr::GuardMgr::new(runtime.clone(), storage.clone(), config)?;
         guardmgr.set_filter(config.path_rules().build_guard_filter(), None);
 
         let storage_handle = storage.create_handle(PARETO_TIMEOUT_DATA_KEY);
@@ -294,10 +290,7 @@ impl<R: Runtime> CircMgr<R> {
             return Ok(());
         }
 
-        self.mgr
-            .peek_builder()
-            .guardmgr()
-            .replace_fallback_list(new_config.fallbacks());
+        self.mgr.peek_builder().guardmgr().reconfigure(new_config)?;
 
         let new_reachable = &new_config.path_rules().reachable_addrs;
         if new_reachable != &old_path_rules.reachable_addrs {

crates/tor-circmgr/src/path/dirpath.rs

@@ -101,6 +101,7 @@ mod test {
     use std::collections::HashSet;
     use tor_basic_utils::test_rng::testing_rng;
     use tor_guardmgr::fallback::{FallbackDir, FallbackList};
+    use tor_guardmgr::TestConfig;
     use tor_linkspec::RelayIds;
     use tor_netdir::testnet;
 
@@ -189,7 +190,8 @@ mod test {
             let mut rng = testing_rng();
             let dirinfo = (&netdir).into();
             let statemgr = tor_persist::TestingStateMgr::new();
-            let guards = tor_guardmgr::GuardMgr::new(rt.clone(), statemgr, [].into()).unwrap();
+            let guards =
+                tor_guardmgr::GuardMgr::new(rt.clone(), statemgr, &TestConfig::default()).unwrap();
             guards.update_network(&netdir);
 
             let mut distinct_guards = HashSet::new();

crates/tor-circmgr/src/path/exitpath.rs

@@ -299,6 +299,7 @@ mod test {
     use crate::test::OptDummyGuardMgr;
     use std::collections::HashSet;
     use tor_basic_utils::test_rng::testing_rng;
+    use tor_guardmgr::TestConfig;
     use tor_linkspec::{HasRelayIds, RelayIds};
     use tor_llcrypto::pk::ed25519::Ed25519Identity;
     use tor_netdir::testnet;
@@ -470,7 +471,8 @@ mod test {
             let mut rng = testing_rng();
             let dirinfo = (&netdir).into();
             let statemgr = tor_persist::TestingStateMgr::new();
-            let guards = tor_guardmgr::GuardMgr::new(rt.clone(), statemgr, [].into()).unwrap();
+            let guards =
+                tor_guardmgr::GuardMgr::new(rt.clone(), statemgr, &TestConfig::default()).unwrap();
             let config = PathConfig::default();
             guards.update_network(&netdir);
             let port443 = TargetPort::ipv4(443);

crates/tor-guardmgr/semver.md

@@ -0,0 +1,2 @@
+BREAKING: GuardMgr::new takes &impl GuardMgrConfig
+BREAKING: GuardMgr::reconfigure replaces replace_fallback_list

crates/tor-guardmgr/src/config.rs

@@ -0,0 +1,44 @@
+//! Configuration elements for the gaurd manager
+
+use tor_basic_utils::define_accessor_trait;
+
+use crate::fallback::FallbackList;
+
+define_accessor_trait! {
+    /// Configuration for a guard manager
+    ///
+    /// If the guard manager gains new configurabilities, this trait will gain additional
+    /// supertraits, as an API break.
+    ///
+    /// Prefer to use `TorClientConfig`, which will always implement this trait.
+    pub trait GuardMgrConfig {
+        fallbacks: FallbackList,
+    }
+}
+
+/// Helpers for testing configuration
+#[cfg(feature = "testing")]
+pub(crate) mod testing {
+    // @@ begin test lint list maintained by maint/add_warning @@
+    #![allow(clippy::bool_assert_comparison)]
+    #![allow(clippy::clone_on_copy)]
+    #![allow(clippy::dbg_macro)]
+    #![allow(clippy::print_stderr)]
+    #![allow(clippy::print_stdout)]
+    #![allow(clippy::single_char_pattern)]
+    #![allow(clippy::unwrap_used)]
+    //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
+
+    use super::*;
+    use derive_more::AsRef;
+
+    /// A dummy test copnfiguration, with transparent fields for testing
+    #[derive(Default, Debug, AsRef)]
+    #[allow(clippy::exhaustive_structs)]
+    pub struct TestConfig {
+        ///
+        #[as_ref]
+        pub fallbacks: FallbackList,
+    }
+    impl GuardMgrConfig for TestConfig {}
+}

crates/tor-guardmgr/src/lib.rs

@@ -57,6 +57,7 @@ use tor_proto::ClockSkew;
 use tracing::{debug, info, trace, warn};
 
 use tor_config::impl_standard_builder;
+use tor_config::ReconfigureError;
 use tor_config::{define_list_builder_accessors, define_list_builder_helper};
 use tor_netdir::{params::NetParameters, NetDir, Relay};
 use tor_persist::{DynStorageHandle, StateMgr};
@@ -64,6 +65,7 @@ use tor_rtcompat::Runtime;
 
 #[cfg(feature = "bridge-client")]
 pub mod bridge;
+mod config;
 mod daemon;
 mod dirstatus;
 mod err;
@@ -77,6 +79,10 @@ mod sample;
 mod skew;
 mod util;
 
+#[cfg(feature = "testing")]
+pub use config::testing::TestConfig;
+
+pub use config::GuardMgrConfig;
 pub use err::{GuardMgrError, PickGuardError};
 pub use events::ClockSkewEvents;
 pub use filter::GuardFilter;
@@ -170,7 +176,6 @@ struct GuardMgrInner {
 
     /// A list of fallback directories used to access the directory system
     /// when no other directory information is yet known.
-    // TODO: reconfigure when the configuration changes.
     fallbacks: fallback::FallbackState,
 
     /// Location in which to store persistent state.
@@ -247,10 +252,6 @@ struct GuardSets {
 const STORAGE_KEY: &str = "guards";
 
 impl<R: Runtime> GuardMgr<R> {
-    // TODO pt-client: We need a configuration argument on construction, and a
-    // reconfigure function. They need to take a configuration object including
-    // a BridgeList, and the "are bridges on or off" object.
-
     /// Create a new "empty" guard manager and launch its background tasks.
     ///
     /// It won't be able to hand out any guards until
@@ -258,7 +259,7 @@ impl<R: Runtime> GuardMgr<R> {
     pub fn new<S>(
         runtime: R,
         state_mgr: S,
-        fallbacks: fallback::FallbackList,
+        config: &impl GuardMgrConfig,
     ) -> Result<Self, GuardMgrError>
     where
         S: StateMgr + Send + Sync + 'static,
@@ -282,7 +283,7 @@ impl<R: Runtime> GuardMgr<R> {
             ctrl,
             pending: HashMap::new(),
             waiting: Vec::new(),
-            fallbacks: (&fallbacks).into(),
+            fallbacks: config.fallbacks().into(),
             storage,
             send_skew,
             recv_skew,
@@ -414,15 +415,22 @@ impl<R: Runtime> GuardMgr<R> {
         inner.update(now, Some(netdir));
     }
 
-    /// Replace the fallback list held by this GuardMgr with `new_list`.
-    pub fn replace_fallback_list(&self, list: &fallback::FallbackList) {
+    /// Replace the fallback list held by this GuardMgr with `new_list`
+    fn replace_fallback_list(&self, list: &fallback::FallbackList) {
         let mut fallbacks: fallback::FallbackState = list.into();
         let mut inner = self.inner.lock().expect("Poisoned lock");
         std::mem::swap(&mut inner.fallbacks, &mut fallbacks);
         inner.fallbacks.take_status_from(fallbacks);
     }
 
+    /// Reconfigure
+    pub fn reconfigure(&self, config: &impl GuardMgrConfig) -> Result<(), ReconfigureError> {
+        self.replace_fallback_list(config.fallbacks());
+        Ok(())
+    }
+
     /// Replace the current [`GuardFilter`] used by this `GuardMgr`.
+    // TODO should this be part of the config?
     pub fn set_filter(&self, filter: GuardFilter, netdir: Option<&NetDir>) {
         let now = self.runtime.wallclock();
         let mut inner = self.inner.lock().expect("Poisoned lock");
@@ -1483,7 +1491,7 @@ mod test {
         let statemgr = TestingStateMgr::new();
         let have_lock = statemgr.try_lock().unwrap();
         assert!(have_lock.held());
-        let guardmgr = GuardMgr::new(rt, statemgr.clone(), [].into()).unwrap();
+        let guardmgr = GuardMgr::new(rt, statemgr.clone(), &TestConfig::default()).unwrap();
         let (con, mds) = testnet::construct_network().unwrap();
         let param_overrides = vec![
             // We make the sample size smaller than usual to compensate for the
@@ -1531,7 +1539,8 @@ mod test {
             drop(guardmgr);
 
             // Try reloading from the state...
-            let guardmgr2 = GuardMgr::new(rt.clone(), statemgr.clone(), [].into()).unwrap();
+            let guardmgr2 =
+                GuardMgr::new(rt.clone(), statemgr.clone(), &TestConfig::default()).unwrap();
             guardmgr2.update_network(&netdir);
 
             // Since the guard was confirmed, we should get the same one this time!