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

adrien · 2021-08-25T15:27:28Z

adrien commented

2021-08-25 15:27:28 +00:00

Fixes #7. I just have one problem, maybe you can help:

I modified the Diplonat struct (in src/diplonat.rs) to hold firewall: Option<FirewallActor> instead of firewall: FirewallActor (same for IgdActor). Now we can't so easily do:

pub struct Diplonat {
  consul: ConsulActor,

  firewall: Option<FirewallActor>,
  igd: Option<IgdActor>,
}

impl Diplonat {
  // [...]
  pub async fn listen(&mut self) -> Result<()> {
    try_join!(
      self.consul.listen(), 
      self.firewall.listen(),
      self.igd.listen(), // Compile error: method not found in `std::option::Option<igd_actor::IgdActor>`
    )?;

    return Ok(());
  }
}

I don't understand Tokio (nor Rust) enough to find an easy fix.

Can you give it a look? The problematic function is in src/diplonat.rs:47. I added some comments.

Fixes #7. I just have one problem, maybe you can help: I modified the Diplonat struct (in `src/diplonat.rs`) to hold `firewall: Option<FirewallActor>` instead of `firewall: FirewallActor` (same for `IgdActor`). Now we can't so easily do: ```rust pub struct Diplonat { consul: ConsulActor, firewall: Option<FirewallActor>, igd: Option<IgdActor>, } impl Diplonat { // [...] pub async fn listen(&mut self) -> Result<()> { try_join!( self.consul.listen(), self.firewall.listen(), self.igd.listen(), // Compile error: method not found in `std::option::Option<igd_actor::IgdActor>` )?; return Ok(()); } } ``` I don't understand Tokio (nor Rust) enough to find an easy fix. Can you give it a look? The problematic function is in `src/diplonat.rs:47`. I added some comments.

adrien force-pushed feature/modular-config from 18122d87c9 to 76fe63791b

2021-08-26 14:08:56 +00:00

Compare

adrien commented

2021-08-26 14:11:16 +00:00

My issue with try_join! is resolved thanks to @LX :)

The PR should be good to go!
I want integration tests, though T_T

My issue with `try_join!` is resolved thanks to @LX :) The PR should be good to go! I want integration tests, though T_T

adrien changed title from ~~WIP: modular Diplonat (with the option to disable useless modules)~~ to Modular Diplonat (with the option to disable useless modules)

2021-08-26 14:11:26 +00:00

adrien force-pushed feature/modular-config from 0651a7d185 to 195aec2cfe

2021-08-26 14:17:48 +00:00

Compare

adrien added the

enhancement

label 2021-08-26 14:28:13 +00:00

adrien started working 2021-08-26 14:28:27 +00:00

adrien canceled time tracking 2021-08-26 14:28:31 +00:00

adrien added a new dependency 2021-08-26 14:29:21 +00:00

#7 Make Diplonat more modular (breaking changes)

adrien removed a dependency 2021-08-26 14:29:37 +00:00

#7 Make Diplonat more modular (breaking changes)

adrien added a new dependency 2021-08-26 14:30:31 +00:00

#7 Make Diplonat more modular (breaking changes)

quentin commented

2021-09-03 10:05:19 +00:00

Do you use some code formatting tool?
It makes harder to review the PR :s

A suggestion:

fork a branch from main
run your code formatting tool on it
let's merge it in main, then pull main in this branch
now the diff will only output your real changes and not anymore your changes + the code formatting tool changes

It could be interesting also to document this code formatting tool in the README and maybe explain how contributors can run it in a pre-commit git hook!

Thanks :)

Do you use some code formatting tool? It makes harder to review the PR :s A suggestion: 1. fork a branch from main 2. run your code formatting tool on it 3. let's merge it in main, then pull main in this branch 4. now the diff will only output your real changes and not anymore your changes + the code formatting tool changes It could be interesting also to document this code formatting tool in the README and maybe explain how contributors can run it in a pre-commit git hook! Thanks :)

👍 2

adrien commented

2021-09-11 13:02:52 +00:00

Hadn't seen the mixed indents in e.g. src/config/runtime.rs. Indeed, I need to setup some tooling to ensure good code quality---for myself and the others!

Done. See PR#10.

Hadn't seen the mixed indents in e.g. `src/config/runtime.rs`. Indeed, I need to setup some tooling to ensure good code quality---for myself and the others! Done. See [PR#10](https://git.deuxfleurs.fr/Deuxfleurs/diplonat/pulls/10).

👍 1

quentin self-assigned this 2021-09-17 13:26:32 +00:00

quentin added 2 commits 2021-09-20 09:58:20 +00:00

Local code formatting 2e797d2f62

Merge branch 'main' of git.deuxfleurs.fr:Deuxfleurs/diplonat into feature/modular-config

continuous-integration/drone/pr Build is failing

Details

f9fd13a48a

quentin added 1 commit 2021-09-20 10:08:21 +00:00

Fix code style

continuous-integration/drone/pr Build is passing

Details

ffab1f6c4b

quentin added 2 commits 2021-09-20 14:15:51 +00:00

Use the new formatting rules 900ba20fbe

Merge branch 'main' into feature/modular-config

continuous-integration/drone/pr Build is passing

Details

119b58fd79

quentin requested changes 2021-09-20 16:20:08 +00:00

quentin left a comment

Thanks for your PR, I better understand what you want to do know.
I think we could still improve your code to make it shorter and clearer,
please read the comments for the details. As a general manner:

Do not use unwrap or expect in your code if you return a Result,
there is no need to panicate if your function promises to return errors!
Pattern matching helps to convey your intent,
ask yourself if you can not rewrite your code with a match
We spread the same logic of initialization across the RuntimeConfig and the Actors'
constructors, this is bad.
Launching actors with try_join! fails to communicate that we want to launch only
the initialized modules, we have better options

Overall, I think these remarks are only minors compared to all the code you wrote, so I hope that you will not be discouraged by my requests and that we will be able to merge great code soon :)

Thanks for your PR, I better understand what you want to do know. I think we could still improve your code to make it shorter and clearer, please read the comments for the details. As a general manner: 1. Do not use `unwrap` or `expect` in your code if you return a `Result`, there is no need to panicate if your function promises to return errors! 2. Pattern matching helps to convey your intent, ask yourself if you can not rewrite your code with a `match` 3. We spread the same logic of initialization across the RuntimeConfig and the Actors' constructors, this is bad. 4. Launching actors with `try_join!` fails to communicate that we want to launch only the initialized modules, we have better options Overall, I think these remarks are only minors compared to all the code you wrote, so I hope that you will not be discouraged by my requests and that we will be able to merge great code soon :)

src/config/runtime.rs Outdated

					
				@ -46,3 +49,2 @@

				    let consul = RuntimeConfigConsul::new(opts.consul.clone())?;

				    let firewall = RuntimeConfigFirewall::new(opts.base.clone())?;

				    let igd = RuntimeConfigIgd::new(opts.base.clone())?;

				    let acme = RuntimeConfigAcme::new(opts.acme.clone())?;

quentin commented

2021-09-20 15:51:57 +00:00

Not related to this PR, but do we really need to clone the configuration here, it is strange because I do not think we modify it.

src/config/runtime.rs Outdated

					
				@ -59,0 +64,4 @@

				  pub(super) fn new(opts: ConfigOptsConsul) -> Result<Self> {

				    let node_name = opts

				      .node_name

				      .expect("'DIPLONAT_CONSUL_NODE_NAME' is required");

quentin commented

2021-09-20 15:51:11 +00:00

cf my following comment on DIPLONAT_ACME_EMAIL

cf my following comment on `DIPLONAT_ACME_EMAIL`

src/config/runtime.rs Outdated

					
				@ -67,1 +80,3 @@

				    );

				    let email = opts

				      .email

				      .expect("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled");

quentin commented

2021-09-20 15:49:07 +00:00

I know this is not linked to this specific pull request but when your function returns a Result, there is no reason to panic with expect.

Instead, replace it with:

let email = opts
  .email
  .context("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled")?;

Please do it for the whole file :)

I know this is not linked to this specific pull request but when your function returns a Result, there is no reason to panic with expect. Instead, replace it with: ```rust let email = opts .email .context("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled")?; ``` Please do it for the whole file :)

adrien commented

2021-09-22 11:13:56 +00:00

.context(...) is not a method of Option.

There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..).

I'm still modifying the calls to return errors instead of panicking straight away. Using match for better clarity ;)

`.context(...)` is not a method of `Option`. There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..). I'm still modifying the calls to return errors instead of panicking straight away. Using `match` for better clarity ;)

quentin commented

2021-09-23 05:27:22 +00:00

You can convert an Option to an Error with ok_or: https://doc.rust-lang.org/std/option/enum.Option.html#method.ok_or

But we need an Error to map to the None. It is a bit challenging as it will be our first error in Diplonat, previously we were relaying only on errors generated by our dependencies.

For now, let's keep it simple, our error library provides us a simple macro to create an error from a string. Just a simple example:

return Err(anyhow!("Missing attribute: {}", missing))

So you can write:

let email = opts
  .email
  .ok_or(anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"))?;

For your information, context allows to enrich an existing error with some context. Indeed, here it is not appropriate as we have no error at all. But in the case where your library throw a cryptic error, you can enrich this error with some context in your program and your users will thank you many times later.

You made the following point:

There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..).

First, you're right, when a parameter is invalid, we must handle this as an error. It was what I was suggesting to you in my first comment but I thought the return type of opts.email would be a Result and not an Option.

Second, we are writing Rust and we are in a library: in this situation, it is not acceptable to panic. As there is no clean way to recover from a panic, we terminate the program from a library without giving the program a chance to recover from the error. Even if we are not a "standalone library" and are writing the main program, we might change our mind the future. We should panic! only when there is no option. To illustrate my point, we could think that later, we would allow our main program to reload its configuration and retry the initialization if one actor fails.

Third, I am not sure we discussed it but ? is syntaxic sugar for:

let err = anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled")

match opts.email.ok_or(err) {
  Ok(email) => {
    // our logic
  }
  Err(e) => return Err(e)
}

So in fact, what I suggest is that we return from the function with an Error if this parameter has not be provided.

You can convert an Option to an Error with `ok_or`: https://doc.rust-lang.org/std/option/enum.Option.html#method.ok_or But we need an Error to map to the None. It is a bit challenging as it will be our first error in Diplonat, previously we were relaying only on errors generated by our dependencies. For now, let's keep it simple, our error library provides us a simple macro to create an error from a string. Just a simple example: ```rust return Err(anyhow!("Missing attribute: {}", missing)) ``` So you can write: ```rust let email = opts .email .ok_or(anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"))?; ``` For your information, `context` allows to enrich an existing error with some context. Indeed, here it is not appropriate as we have no error at all. But in the case where your library throw a cryptic error, you can enrich this error with some context in your program and your users will thank you many times later. You made the following point: > There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..). First, you're right, when a parameter is invalid, we must handle this as an error. It was what I was suggesting to you in my first comment but I thought the return type of `opts.email` would be a `Result` and not an `Option`. Second, we are writing Rust and we are in a library: in this situation, it is not acceptable to panic. As there is no clean way to recover from a panic, we terminate the program from a library without giving the program a chance to recover from the error. Even if we are not a "standalone library" and are writing the main program, we might change our mind the future. We should `panic!` only when there is no option. To illustrate my point, we could think that later, we would allow our main program to reload its configuration and retry the initialization if one actor fails. Third, I am not sure we discussed it but `?` is syntaxic sugar for: ```rust let err = anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled") match opts.email.ok_or(err) { Ok(email) => { // our logic } Err(e) => return Err(e) } ``` So in fact, what I suggest is that we return from the function with an Error if this parameter has not be provided.

adrien commented

2021-09-23 08:01:08 +00:00

Here's my new RuntimeConfigAcme implementation in full (in commit f5ac36e). Do you find it better?

impl RuntimeConfigAcme {
  pub fn new(opts: ConfigOptsAcme) -> Result<Option<Self>> {
    if !opts.enable {
      return Ok(None)
    };

    let email = match opts.email {
      Some(email) => email,
      _ => {
        return Err(anyhow!(
          "'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"
        ))
      }
    };

    Ok(Some(Self { email }))
  }
}

We do return an error instead of panicking. But I may have overdone it with using match, but I didn't find any nice shorthand in Option methods (I checked ok_or). At least the intent is clear!

PS: Quite puzzled by the fact that Rust allows putting a return inside a match arm, which should be an expression returning a String (in this case). Welp, gotta get used to it!

Here's my new RuntimeConfigAcme implementation in full (in commit `f5ac36e`). Do you find it better? ```rust impl RuntimeConfigAcme { pub fn new(opts: ConfigOptsAcme) -> Result<Option<Self>> { if !opts.enable { return Ok(None) }; let email = match opts.email { Some(email) => email, _ => { return Err(anyhow!( "'DIPLONAT_ACME_EMAIL' is required if ACME is enabled" )) } }; Ok(Some(Self { email })) } } ``` We do return an error instead of panicking. But I may have overdone it with using match, but I didn't find any nice shorthand in `Option` methods (I checked `ok_or`). At least the intent is clear! PS: Quite puzzled by the fact that Rust allows putting a `return` inside a `match` arm, which should be an expression returning a String (in this case). Welp, gotta get used to it!

quentin commented

2021-09-23 12:26:17 +00:00

Sorry, I was not clear enough ><

About handling error, I want to avoid expect and unwrap because they make our program panic, ie. the program is killed.

But unwrap_or is totally fine as it does not make the program panic.
And when the case is simple enough we can use one of these keywords: unwrap_or on Result or ok_or on Options instead of a match, it is totally ok.

So, I don't think you should replace your unwrap_or with a match in f5ac36e.

I have written a small example on how to use ok_or to convert an Option into a Result and checked that it compiles here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5bc6046485bab0cd4ae694b94d2ba1db

See the code without leaving gitea

use std::{
  fmt,
  error::Error
};

#[derive(Debug)]
struct RequiredParameterError;
impl Error for RequiredParameterError {}
impl fmt::Display for RequiredParameterError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "A required parameter is not defined")
    }
}

fn foo(param: Option<String>) -> Result<String, RequiredParameterError> {
  let email = param.ok_or(RequiredParameterError{})?;
  return Ok("maito:".to_string() + &email)
}

fn main() {
  let user_param1 = Some("hello@example.org".to_string());
  let user_param2 = None;

  assert!(foo(user_param1).is_ok());
  assert!(foo(user_param2).is_err());
}

So, you may ask why I was advocating for match?
Because in fw_actors.rs you were using again unwrap() that can cause a panic in the program and we can't replace it by these simple keywords that are ok_or, unwrap_or, etc.

To sum up, I think we can define the following rules:

Never write unwrap() or expect() (yes, never is too brutal, but it should be an exception)
Replace them, when possible, by an appropriate small function if it exists
Otherwise, if your case is too specific, use a match

I hope that this time I will be more clear.
I know that when we start coding together, it takes some times to align our minds but once it will be done, it will be easier :)

Sorry, I was not clear enough >< About handling error, I want to avoid `expect` and `unwrap` because they make our program panic, ie. the program is killed. But `unwrap_or` is totally fine as it does not make the program panic. And when the case is simple enough we can use one of these keywords: `unwrap_or` on `Result` or `ok_or` on Options instead of a `match`, it is totally ok. So, I don't think you should replace your `unwrap_or` with a `match` in `f5ac36e`. I have written a small example on how to use `ok_or` to convert an `Option` into a `Result` and checked that it compiles here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5bc6046485bab0cd4ae694b94d2ba1db <details> <summary>See the code without leaving gitea</summary> ```rust use std::{ fmt, error::Error }; #[derive(Debug)] struct RequiredParameterError; impl Error for RequiredParameterError {} impl fmt::Display for RequiredParameterError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "A required parameter is not defined") } } fn foo(param: Option<String>) -> Result<String, RequiredParameterError> { let email = param.ok_or(RequiredParameterError{})?; return Ok("maito:".to_string() + &email) } fn main() { let user_param1 = Some("hello@example.org".to_string()); let user_param2 = None; assert!(foo(user_param1).is_ok()); assert!(foo(user_param2).is_err()); } ``` </details> <br/> So, you may ask why I was advocating for `match`? Because in `fw_actors.rs` you were using again `unwrap()` that can cause a panic in the program and we can't replace it by these simple keywords that are `ok_or`, `unwrap_or`, etc. To sum up, I think we can define the following rules: 1. Never write `unwrap()` or `expect()` (yes, never is too brutal, but it should be an exception) 2. Replace them, when possible, by an appropriate small function if it exists 3. Otherwise, if your case is too specific, use a match I hope that this time I will be more clear. I know that when we start coding together, it takes some times to align our minds but once it will be done, it will be easier :)

src/diplonat.rs

					
				@ -18,0 +17,4 @@

				    let config = ConfigOpts::from_env()?;

				    println!("{:#?}", config);

				    let consul_actor = ConsulActor::new(config.consul);

quentin commented

2021-09-20 16:06:44 +00:00

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:

Instantiate actors chosen by the user
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*

src/diplonat.rs Outdated

					
				@ -42,3 +46,2 @@

				      self.consul.listen(),

				      self.igd.listen(),

				      self.firewall.listen()

				      async {

quentin commented

2021-09-20 16:07:21 +00:00

This is not very idiomatic, please refer to my previous comment to know how to improve it.

adrien commented

2021-09-29 13:09:40 +00:00

I died. I won't touch that, I don't have a good enough understanding of Rust and this goes deep into ownership, lifetimes and (async) traits.

I think we won't go far on our actor model without writing some minimal actor library, or using Actix. I did try to write a simple generic Actor trait, but did not succeed once.

Send help :O

I died. I won't touch that, I don't have a good enough understanding of Rust and this goes deep into ownership, lifetimes and (async) traits. I think we won't go far on our actor model without writing some minimal actor library, or using [Actix](https://docs.rs/actix/0.12.0/actix/index.html). I did try to write a simple generic Actor trait, but did not succeed once. Send help :O

❤️ 1

src/fw_actor.rs Outdated

					
				@ -26,0 +28,4 @@

				    if config.is_none() {

				      return Ok(None)

				    }

				    let config = config.unwrap();

quentin commented

2021-09-20 16:12:14 +00:00

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`

src/igd_actor.rs Outdated

					
				@ -28,2 +27,3 @@

				    rxp: &watch::Receiver<messages::PublicExposedPorts>,

				  ) -> Result<Self> {

				  ) -> Result<Option<Self>> {

				    if config.is_none() {