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.

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

iroh-net: enable cli pings or remove unused code #1485

Closed
divagant-martian opened this issue Sep 17, 2023 · 0 comments · Fixed by #1764
Closed

iroh-net: enable cli pings or remove unused code #1485

divagant-martian opened this issue Sep 17, 2023 · 0 comments · Fixed by #1764
Assignees
Milestone

Comments

@divagant-martian
Copy link
Contributor

divagant-martian commented Sep 17, 2023

We have a lot of machinery coming from tailscale to allow explicit pings from the cli:

I'm writting the explicit pieces of code because even removing one, rust is unable to identify the others as unused due to their pub declaration

  1. a result type

    iroh/iroh-net/src/config.rs

    Lines 125 to 151 in f2f3ead

    /// Contains response information for the "tailscale ping" subcommand,
    /// saying how Tailscale can reach a Tailscale IP or subnet-routed IP.
    /// See tailcfg.PingResponse for a related response that is sent back to control
    /// for remote diagnostic pings.
    // Based on tailscale/ipnstate
    #[derive(Debug, Clone, PartialEq, Default)]
    pub struct PingResult {
    /// ping destination
    pub ip: Option<IpAddr>,
    /// Tailscale IP of node handling IP (different for subnet routers)
    pub node_ip: Option<IpAddr>,
    /// DNS name base or (possibly not unique) hostname
    pub node_name: Option<String>,
    /// Perceived latency in seconds.
    pub latency_seconds: Option<f64>,
    /// The ip:port if direct UDP was used. It is not currently set for TSMP pings.
    pub endpoint: Option<SocketAddr>,
    /// Non-zero DERP region ID if DERP was used. It is not currently set for TSMP pings.
    pub derp_region_id: Option<u16>,
    /// The three-letter region code corresponding to derp_region_id. It is not currently set for TSMP pings.
    pub derp_region_code: Option<String>,
    /// Whether the ping request error is due to it being a ping to the local node.
    pub is_local_ip: Option<bool>,
    /// Did any error occur?
    pub err: Option<String>,
    }

  2. commented out code

    // TODO
    // /// Handles a "ping" CLI query.
    // #[instrument(skip_all, fields(self.name = %self.name))]
    // pub async fn ping<F>(&self, peer: config::Node, mut res: config::PingResult, cb: F)
    // where
    // F: Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static,
    // {
    // res.node_ip = peer.addresses.get(0).copied();
    // res.node_name = match peer.name.as_ref().and_then(|n| n.split('.').next()) {
    // Some(name) => {
    // // prefer DNS name
    // Some(name.to_string())
    // }
    // None => {
    // // else hostname
    // Some(peer.hostinfo.hostname.clone())
    // }
    // };
    // let ep = self
    // .peer_map
    // .read()
    // .await
    // .endpoint_for_node_key(&peer.key)
    // .cloned();
    // match ep {
    // Some(ep) => {
    // ep.cli_ping(res, cb).await;
    // }
    // None => {
    // res.err = Some("unknown peer".to_string());
    // cb(res);
    // }
    // }
    // }

  3. a field to handle the pings

    /// Any outstanding "tailscale ping" commands running
    pending_cli_pings: Vec<PendingCliPing>,

  4. And its type

    #[derive(derive_more::Debug)]
    pub struct PendingCliPing {
    pub res: config::PingResult,
    #[debug("cb: Box<..>")]
    pub cb: Box<dyn Fn(config::PingResult) -> BoxFuture<'static, ()> + Send + Sync + 'static>,
    }

  5. A Enum variant for the cli ping

    pub enum DiscoPingPurpose {
    /// The purpose of a ping was to see if a path was valid.
    Discovery,
    /// The user is running "tailscale ping" from the CLI. These types of pings can go over DERP.
    Cli,
    /// Ping to ensure the current route is still valid.
    StayinAlive,
    }

We should put this code to use or remove it

@b5 b5 added this to the v0.8.0 milestone Oct 2, 2023
@dignifiedquire dignifiedquire modified the milestones: v0.9.0, v0.10.0 Oct 30, 2023
@divagant-martian divagant-martian self-assigned this Nov 1, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 1, 2023
## Description

Closes #1485 

## Notes & open questions

na

## Change checklist

- [x] Self-review.
- [ ] Documentation updates if relevant.
- [ ] Tests if relevant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants