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 }