From 0cf3243260a0282ffc5a2fb6c683bd9404c8af2a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 23 Aug 2023 12:45:16 +0100 Subject: [PATCH] ipt establisher API: pass a struct to new() Now new() only has a reasonable number of arguments and removes some repetition in the mocking arrangements in the IPT Manager. This is the minimum amount that needs to be done in the commit that touches both the IPT Establisher and the Manager. --- crates/tor-hsservice/src/ipt_mgr.rs | 26 +++++++----- crates/tor-hsservice/src/svc/ipt_establish.rs | 41 +++++++++++++++---- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/crates/tor-hsservice/src/ipt_mgr.rs b/crates/tor-hsservice/src/ipt_mgr.rs index d956671dd..f856905cb 100644 --- a/crates/tor-hsservice/src/ipt_mgr.rs +++ b/crates/tor-hsservice/src/ipt_mgr.rs @@ -32,7 +32,7 @@ use tor_rtcompat::Runtime; use crate::svc::ipt_establish; use crate::timeout_track::{TrackingInstantOffsetNow, TrackingNow}; use crate::{FatalError, OnionServiceConfig, RendRequest, StartupError}; -use ipt_establish::{IptEstablisher, IptStatus, IptStatusStatus, IptWantsToRetire}; +use ipt_establish::{IptEstablisher, IptParameters, IptStatus, IptStatusStatus, IptWantsToRetire}; use IptStatusStatus as ISS; use TrackedStatus as TS; @@ -305,12 +305,22 @@ impl IptRelay { /// Make a new introduction point at this relay /// /// It becomes the current IPT. + #[allow(unreachable_code, clippy::diverging_sub_expression)] // TODO HSS remove fn make_new_ipt>( &mut self, imm: &Immutable, mockable: &mut M, ) -> Result<(), FatalError> { - let (establisher, mut watch_rx) = mockable.make_new_ipt(imm, self.relay.clone())?; + let params = IptParameters { + netdir_provider: imm.dirprovider.clone(), + introduce_tx: imm.output_rend_reqs.clone(), + // TODO HSS IntroPointId lacks a constructor and maybe should change anyway + intro_pt_id: todo!(), + target: self.relay.clone(), + ipt_sid_keypair: todo!(), // TODO HSS + accepting_requests: todo!(), // TODO HSS + }; + let (establisher, mut watch_rx) = mockable.make_new_ipt(imm, params)?; // we'll treat it as Establishing until we find otherwise let status_last = TS::Establishing { @@ -735,7 +745,7 @@ pub(crate) trait Mockable: Debug + Send + Sync + Sized + 'static { fn make_new_ipt( &mut self, imm: &Immutable, - relay: RelayIds, + params: IptParameters, ) -> Result<(Self::IptEstablisher, watch::Receiver), FatalError>; /// Call `Publisher::new_intro_points` @@ -755,21 +765,15 @@ impl Mockable for Real { rand::thread_rng() } - #[allow(unreachable_code)] // TODO HSS remove fn make_new_ipt( &mut self, imm: &Immutable, - relay: RelayIds, + params: IptParameters, ) -> Result<(Self::IptEstablisher, watch::Receiver), FatalError> { IptEstablisher::new( imm.runtime.clone(), + params, self.circ_pool.clone(), - imm.dirprovider.clone(), - imm.output_rend_reqs.clone(), - todo!(), // TODO HSS IntroPointId lacks a constructor and maybe should change anyway - relay, - todo!(), // TODO HSS keypair ought to be provided by our caller - todo!(), // TODO HSS RequestDisposition ought to be provided by our caller ) } } diff --git a/crates/tor-hsservice/src/svc/ipt_establish.rs b/crates/tor-hsservice/src/svc/ipt_establish.rs index 7157b1ebb..1e642c344 100644 --- a/crates/tor-hsservice/src/svc/ipt_establish.rs +++ b/crates/tor-hsservice/src/svc/ipt_establish.rs @@ -151,6 +151,27 @@ impl IptError { } } +/// Parameters for an introduction point +/// +/// Consumed by `IptEstablisher::new`. +/// Primarily serves as a convenient way to bundle the many arguments required. +/// +/// Does not include: +/// * The runtime (which would force this struct to have a type parameter) +/// * The circuit builder (leaving this out makes it possible to use this +/// struct during mock execution, where we don't call `IptEstablisher::new`). +#[allow(clippy::missing_docs_in_private_items)] // TODO HSS document these and remove +pub(crate) struct IptParameters { + pub(crate) netdir_provider: Arc, + pub(crate) introduce_tx: mpsc::Sender, + pub(crate) intro_pt_id: IntroPointId, + // TODO HSS: Should this and the following elements be part of some + // configuration object? + pub(crate) target: RelayIds, + pub(crate) ipt_sid_keypair: HsIntroPtSessionIdKeypair, + pub(crate) accepting_requests: RequestDisposition, +} + impl IptEstablisher { /// Try to set up, and maintain, an IPT at `target`. /// @@ -165,19 +186,21 @@ impl IptEstablisher { /// The returned `watch::Receiver` will yield `Faulty` if the IPT /// establisher is shut down (or crashes). // TODO HSS rename to "launch" since it starts the task? - #[allow(clippy::too_many_arguments)] // TODO HSS refactor. pub(crate) fn new( runtime: R, + params: IptParameters, pool: Arc>, - netdir_provider: Arc, - introduce_tx: mpsc::Sender, - intro_pt_id: IntroPointId, - // TODO HSS: Should this and the following elements be part of some - // configuration object? - target: RelayIds, - ipt_sid_keypair: HsIntroPtSessionIdKeypair, - accepting_requests: RequestDisposition, ) -> Result<(Self, postage::watch::Receiver), FatalError> { + // This exhaustive deconstruction ensures that we don't + // accidentally forget to handle any of our inputs. + let IptParameters { + netdir_provider, + introduce_tx, + intro_pt_id, + target, + ipt_sid_keypair, + accepting_requests, + } = params; if matches!(accepting_requests, RequestDisposition::Shutdown) { return Err(bad_api_usage!( "Tried to create a IptEstablisher that that was already shutting down?"