From 590988f0f90a148d51d031f6a625f514970f7d36 Mon Sep 17 00:00:00 2001
From: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
Date: Wed, 9 Oct 2024 14:38:17 +0000
Subject: [PATCH] Improve error messaging when a connections to nodes fail
 (#47362)

Prevents duplicate information about missing tunnels preventing a
connection when attempting to connect locally and via a proxy peer.
The error from the remote dial is now omitted from the message
provided to end users. The information about how the connection
was attempted was instead updated to reflect that tunnels were
attempted via both the local proxy and a peer and they both
failed. Additionally, if a hostname was provided in the dial
request, it will be included in the error message instead of
the internal @local-node identifier for tunnels or the public
address for direct dial nodes.

Closes https://github.com/gravitational/teleport/issues/45256.
---
 lib/reversetunnel/localsite.go | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/lib/reversetunnel/localsite.go b/lib/reversetunnel/localsite.go
index 3f81379178c15..b903cba67d20b 100644
--- a/lib/reversetunnel/localsite.go
+++ b/lib/reversetunnel/localsite.go
@@ -517,6 +517,11 @@ with the cluster.`
 		toAddr = params.To.String()
 	}
 
+	// Prefer providing the hostname over an address.
+	if params.TargetServer != nil {
+		toAddr = params.TargetServer.GetHostname()
+	}
+
 	return fmt.Sprintf(errorMessageTemplate, params.ConnType, toAddr, connStr, err)
 }
 
@@ -613,7 +618,8 @@ func (s *localSite) getConn(params reversetunnelclient.DialParams) (conn net.Con
 		return newMetricConn(conn, dt, dialStart, s.srv.Clock), true, nil
 	}
 
-	if s.tryProxyPeering(params) {
+	peeringEnabled := s.tryProxyPeering(params)
+	if peeringEnabled {
 		s.log.Info("Dialing over peer proxy")
 		conn, peerErr = s.peerClient.DialNode(
 			params.ProxyIDs, params.ServerID, params.From, params.To, params.ConnType,
@@ -623,20 +629,28 @@ func (s *localSite) getConn(params reversetunnelclient.DialParams) (conn net.Con
 		}
 	}
 
-	err = trace.NewAggregate(tunnelErr, peerErr)
-	tunnelMsg := getTunnelErrorMessage(params, "reverse tunnel", err)
+	// If a connection via tunnel failed directly and via a remote peer,
+	// then update the tunnel message to indicate that tunnels were not
+	// found in either place. Avoid aggregating the local and peer errors
+	// to reduce duplicate data since this message makes its way back to
+	// users and can be confusing.
+	msg := "reverse tunnel"
+	if peeringEnabled {
+		msg = "local and peer reverse tunnels"
+	}
+	tunnelMsg := getTunnelErrorMessage(params, msg, tunnelErr)
 
 	// Skip direct dial when the tunnel error is not a not found error. This
 	// means the agent is tunneling but the connection failed for some reason.
 	if !trace.IsNotFound(tunnelErr) {
-		return nil, false, trace.ConnectionProblem(err, tunnelMsg)
+		return nil, false, trace.ConnectionProblem(tunnelErr, tunnelMsg)
 	}
 
 	skip, err := s.skipDirectDial(params)
 	if err != nil {
 		return nil, false, trace.Wrap(err)
 	} else if skip {
-		return nil, false, trace.ConnectionProblem(err, tunnelMsg)
+		return nil, false, trace.ConnectionProblem(tunnelErr, tunnelMsg)
 	}
 
 	// If no tunnel connection was found, dial to the target host.