Deuxfleurs/diplonat
23
4
Fork 1
Code Issues 8 Pull requests Releases Wiki Activity

Modular Diplonat (with the option to disable useless modules) #8

Closed
adrien wants to merge 12 commits from adrien/diplonat:feature/modular-config into main
pull from: adrien/diplonat:feature/modular-config
merge into: Deuxfleurs:main
Conversation 4 Commits 12 Files changed 11 +32 -35
3 changed files with 32 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 006f84e3b0 - Show all commits

2
src/diplonat.rs
View file

@ -19,7 +19,7 @@ impl Diplonat {
let consul_actor = ConsulActor::new(config.consul); let consul_actor = ConsulActor::new(config.consul);
quentin commented 2021-09-20 16:06:44 +00:00
Review
Copy link

Now that our actors are modular, I think that this function fails to convey the real meaning of what we want to do.

IMHO, I think we want to:

  1. Instantiate actors chosen by the user
  2. Run them

With the current code, I see many problems.

((1)) I have a feeling that we are duplicating logic between the "runtime config" and our actor initialization. Currently, the constructor of the actor is only:

  • some boilerplate
  • a duplicated check found in runtime

To fix this problem, I see 2 options:

  • Delete your runtime configuration file and let actors initialize themselves from their constructor
  • Remove actors' constructor and instantiate them directly from this function only if they are required

((2)) The current try_join! fails to convey the information that we run only the initialized actors. I think that, independently of the choice you made for 1, you should create a vector of initialized actors that you will populate and then run. You will replace try_join! by try_join_all

Tip: start by solving problem number 2, it will help you solve problem number 1

Now that our actors are modular, I think that this function fails to convey the real meaning of what we want to do. IMHO, I think we want to: 1. Instantiate actors chosen by the user 2. Run them --- With the current code, I see many problems. ((1)) I have a feeling that we are duplicating logic between the "runtime config" and our actor initialization. Currently, the constructor of the actor is only: - some boilerplate - a duplicated check found in runtime To fix this problem, I see 2 options: - Delete your runtime configuration file and let actors initialize themselves from their constructor - Remove actors' constructor and instantiate them directly from this function only if they are required ((2)) The current `try_join!` fails to convey the information that we run only the initialized actors. I think that, independently of the choice you made for 1, you should create a vector of initialized actors that you will populate and then run. You will replace `try_join!` by [`try_join_all`](https://docs.rs/futures/0.3.17/futures/future/fn.try_join_all.html) *Tip: start by solving problem number 2, it will help you solve problem number 1*
let firewall_actor = FirewallActor::new(config.firewall, &consul_actor.rx_open_ports).await?; let firewall_actor = FirewallActor::new(config.firewall, &consul_actor.rx_open_ports)?;
let igd_actor = IgdActor::new(config.igd, &consul_actor.rx_open_ports).await?; let igd_actor = IgdActor::new(config.igd, &consul_actor.rx_open_ports).await?;

29
src/fw_actor.rs
View file

@ -21,25 +21,24 @@ pub struct FirewallActor {
} }
impl FirewallActor { impl FirewallActor {
pub async fn new( pub fn new(
config: Option<RuntimeConfigFirewall>, config: Option<RuntimeConfigFirewall>,
rxp: &watch::Receiver<messages::PublicExposedPorts>, rxp: &watch::Receiver<messages::PublicExposedPorts>,
) -> Result<Option<Self>> { ) -> Result<Option<Self>> {
if config.is_none() { match config {
return Ok(None) None => Ok(None),
Some(c) => {
let ctx = Self {
quentin commented 2021-09-20 16:12:14 +00:00
Outdated
Review
Copy link

No. Never use unwrap.

Your function should be written with a match:

impl FirewallActor {
  pub async fn new(
    _refresh: Duration,
    config: Option<RuntimeConfigFirewall>,
    rxp: &watch::Receiver<messages::PublicExposedPorts>,
  ) -> Result<Option<Self>> {
  match config {
    None => Ok(None)
    Some(c) => {
      let ctx = Self { ... }
      fw::setup(&ctx.ipt)?;
      return Ok(Some(ctx))
    }
  }
}

As a general rule, in many places in your code, you could better communicate your intent with a match

No. Never use unwrap. Your function should be written with a match: ```rust impl FirewallActor { pub async fn new( _refresh: Duration, config: Option<RuntimeConfigFirewall>, rxp: &watch::Receiver<messages::PublicExposedPorts>, ) -> Result<Option<Self>> { match config { None => Ok(None) Some(c) => { let ctx = Self { ... } fw::setup(&ctx.ipt)?; return Ok(Some(ctx)) } } } ``` As a general rule, in many places in your code, you could better communicate your intent with a `match`
ipt: iptables::new(false)?,
last_ports: messages::PublicExposedPorts::new(),
refresh: c.refresh_time,
rx_ports: rxp.clone(),
};
fw::setup(&ctx.ipt)?;
Ok(Some(ctx))
}
} }
let config = config.unwrap();
let ctx = Self {
ipt: iptables::new(false)?,
last_ports: messages::PublicExposedPorts::new(),
refresh: config.refresh_time,
rx_ports: rxp.clone(),
};
fw::setup(&ctx.ipt)?;
return Ok(Some(ctx))
} }
pub async fn listen(&mut self) -> Result<()> { pub async fn listen(&mut self) -> Result<()> {

36
src/igd_actor.rs
View file

@ -26,26 +26,24 @@ impl IgdActor {
config: Option<RuntimeConfigIgd>, config: Option<RuntimeConfigIgd>,
rxp: &watch::Receiver<messages::PublicExposedPorts>, rxp: &watch::Receiver<messages::PublicExposedPorts>,
) -> Result<Option<Self>> { ) -> Result<Option<Self>> {
if config.is_none() { match config {
quentin commented 2021-09-20 16:12:30 +00:00
Outdated
Review
Copy link

Same as my previous comment

Same as my previous comment
return Ok(None) None => Ok(None),
Some(c) => {
let gw = search_gateway(Default::default())
.await
.context("Failed to find IGD gateway")?;
info!("IGD gateway: {}", gw);
Ok(Some(Self {
expire: c.expiration_time,
gateway: gw,
last_ports: messages::PublicExposedPorts::new(),
private_ip: c.private_ip,
refresh: c.refresh_time,
rx_ports: rxp.clone(),
}))
}
} }
let config = config.unwrap();
let gw = search_gateway(Default::default())
.await
.context("Failed to find IGD gateway")?;
info!("IGD gateway: {}", gw);
let ctx = Self {
expire: config.expiration_time,
gateway: gw,
last_ports: messages::PublicExposedPorts::new(),
private_ip: config.private_ip,
refresh: config.refresh_time,
rx_ports: rxp.clone(),
};
return Ok(Some(ctx))
} }
pub async fn listen(&mut self) -> Result<()> { pub async fn listen(&mut self) -> Result<()> {