From 6c99d5eca29dd6d2e44c080c5a557b26f02ec3ad Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 16 Oct 2025 01:56:43 +1100 Subject: [PATCH] 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 | 17 ++++++++++++++--- coderd/agentapi/connectionlog.go | 10 +++++++++- coderd/agentapi/connectionlog_test.go | 13 ++++++++++--- coderd/connectionlog/connectionlog.go | 16 ++++++++++++---- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index aed6652de6..ab882a80ef 100644 --- a/agent/agent.go +++ b/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() diff --git a/coderd/agentapi/connectionlog.go b/coderd/agentapi/connectionlog.go index f26f835746..bd11f9e726 100644 --- a/coderd/agentapi/connectionlog.go +++ b/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, diff --git a/coderd/agentapi/connectionlog_test.go b/coderd/agentapi/connectionlog_test.go index 4a060b8f16..81d969e5ba 100644 --- a/coderd/agentapi/connectionlog_test.go +++ b/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, diff --git a/coderd/connectionlog/connectionlog.go b/coderd/connectionlog/connectionlog.go index 1b56ffc288..b3d9e9115f 100644 --- a/coderd/connectionlog/connectionlog.go +++ b/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 }