From 6c93ce76bf26eaef2f66bdc19119acc9818c6c22 Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Tue, 11 Feb 2025 05:58:32 +0000
Subject: [PATCH] chore(phoenix-channel): log all errors when connection fails
 (#8089)

Currently, we are only logging the last error when we fail to connect to
any of the addresses from the portal. This is often not useful because
the last one is likely to be an IPv6 address which may not be supported
on the system so all we learn is "The requested address is not valid in
its context.".
---
 rust/Cargo.lock                 |  1 +
 rust/phoenix-channel/Cargo.toml |  1 +
 rust/phoenix-channel/src/lib.rs | 27 +++++++++++++++++++--------
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index d6e173319..010f55309 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -4661,6 +4661,7 @@ dependencies = [
  "futures",
  "hex",
  "hostname",
+ "itertools 0.13.0",
  "libc",
  "os_info",
  "rand_core 0.6.4",
diff --git a/rust/phoenix-channel/Cargo.toml b/rust/phoenix-channel/Cargo.toml
index 66397cec9..24cf49bc9 100644
--- a/rust/phoenix-channel/Cargo.toml
+++ b/rust/phoenix-channel/Cargo.toml
@@ -11,6 +11,7 @@ base64 = { workspace = true }
 firezone-logging = { workspace = true }
 futures = { workspace = true }
 hex = { workspace = true }
+itertools = { workspace = true }
 libc = { workspace = true }
 os_info = { workspace = true }
 rand_core = { workspace = true }
diff --git a/rust/phoenix-channel/src/lib.rs b/rust/phoenix-channel/src/lib.rs
index 7adda5e2c..64c5810ac 100644
--- a/rust/phoenix-channel/src/lib.rs
+++ b/rust/phoenix-channel/src/lib.rs
@@ -19,6 +19,7 @@ use firezone_logging::{err_with_src, telemetry_span};
 use futures::future::BoxFuture;
 use futures::{FutureExt, SinkExt, StreamExt};
 use heartbeat::{Heartbeat, MissedLastHeartbeat};
+use itertools::Itertools as _;
 use rand_core::{OsRng, RngCore};
 use secrecy::{ExposeSecret, Secret};
 use serde::{de::DeserializeOwned, Deserialize, Serialize};
@@ -109,7 +110,8 @@ async fn connect(
     addresses: Vec<SocketAddr>,
     socket_factory: &dyn SocketFactory<TcpSocket>,
 ) -> Result<TcpStream, InternalError> {
-    let mut last_error = None;
+    let mut errors = Vec::with_capacity(addresses.len());
+
     for addr in addresses {
         let Ok(socket) = socket_factory(&addr) else {
             continue;
@@ -118,16 +120,16 @@ async fn connect(
         match socket.connect(addr).await {
             Ok(socket) => return Ok(socket),
             Err(e) => {
-                last_error = Some(e);
+                errors.push((addr, e));
             }
         }
     }
 
-    let Some(err) = last_error else {
+    if errors.is_empty() {
         return Err(InternalError::InvalidUrl);
-    };
+    }
 
-    Err(InternalError::SocketConnection(err))
+    Err(InternalError::SocketConnection(errors))
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -162,7 +164,7 @@ enum InternalError {
     StreamClosed,
     InvalidUrl,
     FailedToBuildRequest(tokio_tungstenite::tungstenite::http::Error),
-    SocketConnection(std::io::Error),
+    SocketConnection(Vec<(SocketAddr, std::io::Error)>),
     Timeout { duration: Duration },
 }
 
@@ -185,7 +187,16 @@ impl fmt::Display for InternalError {
             InternalError::CloseMessage => write!(f, "portal closed the websocket connection"),
             InternalError::StreamClosed => write!(f, "websocket stream was closed"),
             InternalError::InvalidUrl => write!(f, "failed to resolve url"),
-            InternalError::SocketConnection(_) => write!(f, "failed to connect socket"),
+            InternalError::SocketConnection(errors) => {
+                write!(
+                    f,
+                    "failed to connect socket: [{}]",
+                    errors
+                        .iter()
+                        .map(|(addr, e)| format!("{addr}: {e}"))
+                        .join(", ")
+                )
+            }
             InternalError::Timeout { duration, .. } => {
                 write!(f, "operation timed out after {duration:?}")
             }
@@ -202,8 +213,8 @@ impl std::error::Error for InternalError {
             InternalError::WebSocket(tokio_tungstenite::tungstenite::Error::Http(_)) => None,
             InternalError::WebSocket(e) => Some(e),
             InternalError::Serde(e) => Some(e),
-            InternalError::SocketConnection(e) => Some(e),
             InternalError::FailedToBuildRequest(e) => Some(e),
+            InternalError::SocketConnection(_) => None,
             InternalError::MissedHeartbeat => None,
             InternalError::CloseMessage => None,
             InternalError::StreamClosed => None,