Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b211485
Adding Error Message with `wireguard-go` not found.
noyez Aug 8, 2022
2b42d3f
Fixing up formatting
noyez Aug 12, 2022
fc2dd9f
Update wireguard-control/src/backends/userspace.rs
noyez Jun 13, 2023
ef30254
Merge branch 'main' into noyez_add_error_msg_when_wg_not_found
bschwind Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions wireguard-control/src/backends/userspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,27 @@ pub fn get_by_name(name: &InterfaceName) -> Result<Device, io::Error> {
/// wgctrl-rs will look for WG_USERSPACE_IMPLEMENTATION first, but will also
/// respect the WG_QUICK_USERSPACE_IMPLEMENTATION choice if the former isn't
/// available.
fn get_userspace_implementation() -> String {
std::env::var("WG_USERSPACE_IMPLEMENTATION")
.or_else(|_| std::env::var("WG_QUICK_USERSPACE_IMPLEMENTATION"))
.unwrap_or_else(|_| "wireguard-go".to_string())
fn get_userspace_implementation() -> io::Result<PathBuf> {
let wg_path: PathBuf = PathBuf::from(
std::env::var("WG_USERSPACE_IMPLEMENTATION")
.or_else(|_| std::env::var("WG_QUICK_USERSPACE_IMPLEMENTATION"))
.unwrap_or_else(|_| "wireguard-go".to_string()),
);
if !wg_path.exists() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@mcginty mcginty Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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),
    }
}

Copy link
Collaborator

@mcginty mcginty Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@mcginty mcginty Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Err(io::Error::new(
io::ErrorKind::Other,
&*format!(
"Cannot find \"{}\". Specify a custom path with WG_USERSPACE_IMPLEMENTATION.",
(*wg_path).display()
),
))
} else {
Ok(wg_path)
}
}

fn start_userspace_wireguard(iface: &InterfaceName) -> io::Result<Output> {
let mut command = Command::new(get_userspace_implementation());
let mut command = Command::new(get_userspace_implementation()?);
let output = if cfg!(target_os = "linux") {
command.args(&[iface.to_string()]).output()?
} else {
Expand Down