fix: avoid connection logging crashes in agent (#20307)

- Ignore errors when reporting a connection from the server, just log
them instead
- Translate connection log IP `localhost` to `127.0.0.1` on both the
server and the agent

Note that the temporary fix for converting invalid IPs to localhost is
not required in main since the database no longer forbids NULL for the
IP column since https://github.com/coder/coder/pull/19788

Relates to #20194

agent/agent.go

@@ -781,11 +781,15 @@ func (a *agent) reportConnectionsLoop(ctx context.Context, aAPI proto.DRPCAgentC
 			logger.Debug(ctx, "reporting connection")
 			_, err := aAPI.ReportConnection(ctx, payload)
 			if err != nil {
-				return xerrors.Errorf("failed to report connection: %w", err)
+				// Do not fail the loop if we fail to report a connection, just
+				// log a warning.
+				// Related to https://github.com/coder/coder/issues/20194
+				logger.Warn(ctx, "failed to report connection to server", slog.Error(err))
+				// keep going, we still need to remove it from the slice
+			} else {
+				logger.Debug(ctx, "successfully reported connection")
 			}
 
-			logger.Debug(ctx, "successfully reported connection")
-
 			// Remove the payload we sent.
 			a.reportConnectionsMu.Lock()
 			a.reportConnections[0] = nil // Release the pointer from the underlying array.
@@ -816,6 +820,13 @@ func (a *agent) reportConnection(id uuid.UUID, connectionType proto.Connection_T
 		ip = host
 	}
 
+	// If the IP is "localhost" (which it can be in some cases), set it to
+	// 127.0.0.1 instead.
+	// Related to https://github.com/coder/coder/issues/20194
+	if ip == "localhost" {
+		ip = "127.0.0.1"
+	}
+
 	a.reportConnectionsMu.Lock()
 	defer a.reportConnectionsMu.Unlock()
 

coderd/agentapi/connectionlog.go

@@ -61,6 +61,14 @@ func (a *ConnLogAPI) ReportConnection(ctx context.Context, req *agentproto.Repor
 		return nil, xerrors.Errorf("get workspace by agent id: %w", err)
 	}
 
+	// Some older clients may incorrectly report "localhost" as the IP address.
+	// Related to https://github.com/coder/coder/issues/20194
+	logIPRaw := req.GetConnection().GetIp()
+	if logIPRaw == "localhost" {
+		logIPRaw = "127.0.0.1"
+	}
+	logIP := database.ParseIP(logIPRaw) // will return null if invalid
+
 	reason := req.GetConnection().GetReason()
 	connLogger := *a.ConnectionLogger.Load()
 	err = connLogger.Upsert(ctx, database.UpsertConnectionLogParams{
@@ -73,7 +81,7 @@ func (a *ConnLogAPI) ReportConnection(ctx context.Context, req *agentproto.Repor
 		AgentName:        workspaceAgent.Name,
 		Type:             connectionType,
 		Code:             code,
-		Ip:               database.ParseIP(req.GetConnection().GetIp()),
+		Ip:               logIP,
 		ConnectionID: uuid.NullUUID{
 			UUID:  connectionID,
 			Valid: true,

coderd/agentapi/connectionlog_test.go

@@ -3,13 +3,11 @@ package agentapi_test
 import (
 	"context"
 	"database/sql"
-	"net"
 	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
-	"github.com/sqlc-dev/pqtype"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
 	"google.golang.org/protobuf/types/known/timestamppb"
@@ -75,6 +73,9 @@ func TestConnectionLog(t *testing.T) {
 			action: agentproto.Connection_CONNECT.Enum(),
 			typ:    agentproto.Connection_JETBRAINS.Enum(),
 			time:   dbtime.Now(),
+			// Sometimes, JetBrains clients report as localhost, see
+			// https://github.com/coder/coder/issues/20194
+			ip: "localhost",
 		},
 		{
 			name:   "Reconnecting PTY Connect",
@@ -129,6 +130,12 @@ func TestConnectionLog(t *testing.T) {
 				},
 			})
 
+			expectedIPRaw := tt.ip
+			if expectedIPRaw == "localhost" {
+				expectedIPRaw = "127.0.0.1"
+			}
+			expectedIP := database.ParseIP(expectedIPRaw)
+
 			require.True(t, connLogger.Contains(t, database.UpsertConnectionLogParams{
 				Time:             dbtime.Time(tt.time).In(time.UTC),
 				OrganizationID:   workspace.OrganizationID,
@@ -146,7 +153,7 @@ func TestConnectionLog(t *testing.T) {
 					Int32: tt.status,
 					Valid: *tt.action == agentproto.Connection_DISCONNECT,
 				},
-				Ip:   pqtype.Inet{Valid: true, IPNet: net.IPNet{IP: net.ParseIP(tt.ip), Mask: net.CIDRMask(32, 32)}},
+				Ip:   expectedIP,
 				Type: agentProtoConnectionTypeToConnectionLog(t, *tt.typ),
 				DisconnectReason: sql.NullString{
 					String: tt.reason,

coderd/connectionlog/connectionlog.go

@@ -62,10 +62,6 @@ func (m *FakeConnectionLogger) Contains(t testing.TB, expected database.UpsertCo
 			t.Logf("connection log %d: expected ID %s, got %s", idx+1, expected.ID, cl.ID)
 			continue
 		}
-		if !expected.Time.IsZero() && expected.Time != cl.Time {
-			t.Logf("connection log %d: expected Time %s, got %s", idx+1, expected.Time, cl.Time)
-			continue
-		}
 		if expected.OrganizationID != uuid.Nil && cl.OrganizationID != expected.OrganizationID {
 			t.Logf("connection log %d: expected OrganizationID %s, got %s", idx+1, expected.OrganizationID, cl.OrganizationID)
 			continue
@@ -114,6 +110,18 @@ func (m *FakeConnectionLogger) Contains(t testing.TB, expected database.UpsertCo
 			t.Logf("connection log %d: expected ConnectionID %s, got %s", idx+1, expected.ConnectionID.UUID, cl.ConnectionID.UUID)
 			continue
 		}
+		if expected.DisconnectReason.Valid && cl.DisconnectReason.String != expected.DisconnectReason.String {
+			t.Logf("connection log %d: expected DisconnectReason %s, got %s", idx+1, expected.DisconnectReason.String, cl.DisconnectReason.String)
+			continue
+		}
+		if !expected.Time.IsZero() && expected.Time != cl.Time {
+			t.Logf("connection log %d: expected Time %s, got %s", idx+1, expected.Time, cl.Time)
+			continue
+		}
+		if expected.ConnectionStatus != "" && expected.ConnectionStatus != cl.ConnectionStatus {
+			t.Logf("connection log %d: expected ConnectionStatus %s, got %s", idx+1, expected.ConnectionStatus, cl.ConnectionStatus)
+			continue
+		}
 		return true
 	}