Add Error Message when `wireguard-go` not found. #232

noyez · 2022-08-08T15:32:36Z

Previously when trying to add a peer, i for the following output, which
was not helpful since i wasn't sure what file could not be found.

bkn@mbp16 bkn % sudo /Users/bkn/.cargo/bin/innernet  --verbose  install mbp16.toml
✔ Interface name · chester
[*] bringing up interface chester.
[E] failed to start the interface: chester - No such file or directory (os error 2).
[*] bringing down the interface.
[!] failed to bring down interface: chester - WireGuard name file can't be read.
[E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible!

[E] chester - No such file or directory (os error 2)

After poking through the code I finally figured out the No such file or directory error was due to no wireguard implementation could be found.
Now when running the above command i get the following output.

bkn@mbp16 bkn % sudo /Users/bkn/source/rust/innernet/target/debug/innernet  --verbose  install mbp16.toml
✔ Interface name · chester
[*] bringing up interface chester.
[E] failed to start the interface: chester - Cannot find wireguard implementation 'wireguard-go', to specificy custom wireguard implementation set $WG_USERSPACE_IMPLEMENTATION to wiregaurd binary.
[*] bringing down the interface.
[!] failed to bring down interface: chester - WireGuard name file can't be read.
[E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible!

[E] chester - Cannot find wireguard implementation 'wireguard-go', to specificy custom wireguard implementation set $WG_USERSPACE_IMPLEMENTATION to wiregaurd binary

I find this error message to be more explicit and if a user encounters a
similar problem its more clear how to remidy this specific error.

Previously when trying to add a peer, i for the following output, which was not helpful since i wasn't sure what file could not be found. bkn@mbp16 bkn % sudo /Users/bkn/.cargo/bin/innernet --verbose install mbp16.toml ✔ Interface name · chester [*] bringing up interface chester. [E] failed to start the interface: chester - No such file or directory (os error 2). [*] bringing down the interface. [!] failed to bring down interface: chester - WireGuard name file can't be read. [E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible! [E] chester - No such file or directory (os error 2) After poking through the code I finally figured out the `No such file or directory` error was due to no wireguard implementation could be found. Now when running the above command i get the following output. bkn@mbp16 bkn % sudo /Users/bkn/source/rust/innernet/target/debug/innernet --verbose install mbp16.toml ✔ Interface name · chester [*] bringing up interface chester. [E] failed to start the interface: chester - Cannot find wireguard implementation 'wireguard-go', to specificy custom wireguard implementation set $WG_USERSPACE_IMPLEMENTATION to wiregaurd binary. [*] bringing down the interface. [!] failed to bring down interface: chester - WireGuard name file can't be read. [E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible! [E] chester - Cannot find wireguard implementation 'wireguard-go', to specificy custom wireguard implementation set $WG_USERSPACE_IMPLEMENTATION to wiregaurd binary I find this error message to be more explicit and if a user encounters a similar problem its more clear how to remidy this specific error.

mcginty

Thanks! This should be helpful to folks. Just a few formatting/nitpick comments.

wireguard-control/src/backends/userspace.rs

noyez · 2022-08-12T12:06:16Z

No worries. I made those changes. I'm on the road -- hence my interest in this project -- so my PR was kind of quick and dirty. let me know if you want any more changes.

mcginty

Looks good - sorry to have left this hanging.

wireguard-control/src/backends/userspace.rs

bschwind · 2023-06-13T02:43:46Z

@noyez if we get the merge conflict resolved and apply @mcginty 's suggestion, we can get this merged and release the next version of innernet :)

Co-authored-by: Jake McGinty <[email protected]>

noyez · 2023-06-13T14:05:16Z

@noyez if we get the merge conflict resolved and apply @mcginty 's suggestion, we can get this merged and release the next version of innernet :)

next release? ... let's get this merged then. I'm looking forward to the new release! thanks!

mcginty

Looks good to me! Thanks for updating it!

bschwind · 2023-06-14T02:18:18Z

next release? ... let's get this merged then. I'm looking forward to the new release! thanks!

Yep next release. It seems the merge conflict is still there though so we'll have to resolve that first.

bschwind · 2023-06-14T02:21:43Z

For the conflicting line, I think you just want it to be this:

let mut command = Command::new(get_userspace_implementation()?);

bschwind · 2023-06-14T07:36:12Z

Ah, it seems I can resolve the merge conflict myself, didn't know I could push to branches on forks.

bschwind · 2023-06-14T08:04:07Z

wireguard-control/src/backends/userspace.rs

+            .or_else(|_| std::env::var("WG_QUICK_USERSPACE_IMPLEMENTATION"))
+            .unwrap_or_else(|_| "wireguard-go".to_string()),
+    );
+    if !wg_path.exists() {


Ah, the docker test is failing correctly.

We can find wireguard-go directly in the previous code because it's in the PATH. But now if we treat it as a PathBuf and call .exists() on it, we're asking if there's a wireguard-go file in the default working directory of the container. So this function is returning an Error, the container exits, and our docker test fails to find the peer container because it exited and disappeared.

We'll have to re-think how we detect if it's there or not. Maybe we just run the command and detect if we encounter an error, or maybe we use the which crate or something else to find the full path of the binary, or something else entirely.

Ah yeah, maybe we want to rethink this as simply providing better output when we run the command in start_userspace_wireguard(). Personally I don't think the benefits of adding a dependency are worth it compared to adding a better "wireguard-go failed to execute - is it installed?" type message to the user.

i think there is a switch for --version in wireguard-go. We could try to execute wireguard-go --version. If that succeeds, then wireguard-go exists in the path.

» /usr/bin/wireguard --version wireguard-go v0.0.20220117 Userspace WireGuard daemon for linux-amd64. Information available at https://www.wireguard.com. Copyright (C) Jason A. Donenfeld <[email protected]>. » echo $? 0

@noyez I don't think that's necessary: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d93bf64dbf49743403648a4dfebf8b46

We can just check the result of output() and the result will be:

Err(Os { code: 2, kind: NotFound, message: "No such file or directory" })

when the binary is missing :).

So I think this PR is more about modifying start_userspace_wireguard() to provide more informative error messages up the chain for the user.

So if the output() fails it could mean wireguard-go doesn't exists, or perhaps there's another error with the configurations. In start_userspace_wireguard() you already check if output() returns an error status, if so it returns Err(io::ErrorKind::AddrNotAvailable.int()).

Do you know to differentiate the different error codes?

@noyez at first glance this looks reasonable, we can simplify it a bit and I think we don't want to return io::ErrorKind::AddrNotAvailable if we get an error running the command (no output was produced)

fn start_userspace_wireguard(iface: &InterfaceName) -> io::Result<Output> { let mut command = Command::new(get_userspace_implementation()); let output_res = if cfg!(target_os = "linux") { command.args(&[iface.to_string()]).output() } else { command .env("WG_TUN_NAME_FILE", &format!("{VAR_RUN_PATH}/{iface}.name")) .args(["utun"]) .output() }; match output_res { Ok(output) => { if output.status.success() { Ok(output) } else { Err(io::ErrorKind::AddrNotAvailable.into()) } }, Err(e) if e.kind() == io::ErrorKind::NotFound => Err(io::Error::new( io::ErrorKind::NotFound, "Cannot find \"wireguard-go\". Specify a custom path with WG_USERSPACE_IMPLEMENTATION.", )), Err(e) => Err(e), } }

Yeah, I actually also don't get why we're returning io::ErrorKind::AddrNotAvailable for all errors, but I can fix that up in another PR since it's not related to wireguard-go being missing altogether and was already in the code.

yeah, i didn't know what else to return, that's what was getting returned previously in the start_userspace_wireguard function so i kept it, but can def be changed.

Yeah I think that one is my bad ;). It'd make this PR less "short and sweet" but I feel like maybe the right thing to do is to pack into the error the return code as well as possibly the stderr output so that we can output it to the user.

Feel free to take that on if you wish, otherwise I can address it in a later PR since it's my oversight.

I actually also don't get why we're returning io::ErrorKind::AddrNotAvailable for all errors

The previous code had a question mark operator:

innernet/wireguard-control/src/backends/userspace.rs

Line 278 in 376ab64

command.args(&[iface.to_string()]).output()?

So ErrorKind::AddrNotAvailable is only returned when there's not a success status code after running the command. But you're right that we should return all the details to the caller.

bschwind

Requesting changes to fix the failing docker test, see my comment for details.

noyez · 2023-08-26T12:59:09Z

Closing this PR in favor of : #284

mcginty requested changes Aug 10, 2022

View reviewed changes

wireguard-control/src/backends/userspace.rs Outdated Show resolved Hide resolved

wireguard-control/src/backends/userspace.rs Outdated Show resolved Hide resolved

wireguard-control/src/backends/userspace.rs Outdated Show resolved Hide resolved

Fixing up formatting

2b42d3f

mcginty approved these changes May 30, 2023

View reviewed changes

wireguard-control/src/backends/userspace.rs Outdated Show resolved Hide resolved

Update wireguard-control/src/backends/userspace.rs

fc2dd9f

Co-authored-by: Jake McGinty <[email protected]>

mcginty approved these changes Jun 13, 2023

View reviewed changes

Merge branch 'main' into noyez_add_error_msg_when_wg_not_found

ef30254

bschwind reviewed Jun 14, 2023

View reviewed changes

bschwind requested changes Jun 14, 2023

View reviewed changes

noyez mentioned this pull request Aug 26, 2023

Add Error Message when wireguard-go not found. replacement for #232 #284

Open

noyez closed this Aug 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Error Message when `wireguard-go` not found. #232

Add Error Message when `wireguard-go` not found. #232

noyez commented Aug 8, 2022

mcginty left a comment

noyez commented Aug 12, 2022

mcginty left a comment

bschwind commented Jun 13, 2023

noyez commented Jun 13, 2023

mcginty left a comment

bschwind commented Jun 14, 2023

bschwind commented Jun 14, 2023

bschwind commented Jun 14, 2023

bschwind Jun 14, 2023

mcginty Aug 23, 2023 •

edited

Loading

noyez Aug 23, 2023

mcginty Aug 23, 2023

noyez Aug 23, 2023

bschwind Aug 24, 2023

mcginty Aug 24, 2023 •

edited

Loading

noyez Aug 24, 2023

mcginty Aug 24, 2023 •

edited

Loading

bschwind Aug 25, 2023

bschwind left a comment

noyez commented Aug 26, 2023

Add Error Message when wireguard-go not found. #232

Add Error Message when wireguard-go not found. #232

Conversation

noyez commented Aug 8, 2022

mcginty left a comment

Choose a reason for hiding this comment

noyez commented Aug 12, 2022

mcginty left a comment

Choose a reason for hiding this comment

bschwind commented Jun 13, 2023

noyez commented Jun 13, 2023

mcginty left a comment

Choose a reason for hiding this comment

bschwind commented Jun 14, 2023

bschwind commented Jun 14, 2023

bschwind commented Jun 14, 2023

bschwind Jun 14, 2023

Choose a reason for hiding this comment

mcginty Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

noyez Aug 23, 2023

Choose a reason for hiding this comment

mcginty Aug 23, 2023

Choose a reason for hiding this comment

noyez Aug 23, 2023

Choose a reason for hiding this comment

bschwind Aug 24, 2023

Choose a reason for hiding this comment

mcginty Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

noyez Aug 24, 2023

Choose a reason for hiding this comment

mcginty Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

bschwind Aug 25, 2023

Choose a reason for hiding this comment

bschwind left a comment

Choose a reason for hiding this comment

noyez commented Aug 26, 2023

Add Error Message when `wireguard-go` not found. #232

Add Error Message when `wireguard-go` not found. #232

mcginty Aug 23, 2023 •

edited

Loading

mcginty Aug 24, 2023 •

edited

Loading

mcginty Aug 24, 2023 •

edited

Loading