Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: avoid connection logging crashes in agent#20307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
deansheather merged 3 commits intomainfromdean/connlog-fix-main
Oct 15, 2025
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletionsagent/agent.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

what is the consequence for a failed report? I see one action isDisconnect, so do we have hanging connection in our reports?

This would fundamentally change that behavior, which is ok if we can document it. Or have some other kind of timeout/heartbeat to resolve the hanging "connection"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah I see,Connection Logs is a new page to me. TIL

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's fine if connection logs get dropped. We document them as best effort

// 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.
Expand DownExpand Up@@ -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" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do we need to handle this in both agent and coderd? I'd imagine just coderd would suffice but then again doesn't hurt either.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah it probably doesn't need to be fixed here, but I think it's fine. It'd be great to fix the localhost issue entirely cuz it does seem like it shouldn't be happening anyway (should probably be a tailnet IP?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, I think tailnet IP would be more appropriate 👍🏻

ip = "127.0.0.1"
}
Comment on lines +823 to +828
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Technicallylocalhost is not case sensitive. We should cover all bases and handle differentlocalhost casing

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I thought about this, but in the end these will just show up asNULL in the DB which I think is OK.localhost (or any variant) isn't very useful anyway


a.reportConnectionsMu.Lock()
defer a.reportConnectionsMu.Unlock()

Expand Down
10 changes: 9 additions & 1 deletioncoderd/agentapi/connectionlog.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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{
Expand All@@ -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,
Expand Down
13 changes: 10 additions & 3 deletionscoderd/agentapi/connectionlog_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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"
Expand DownExpand Up@@ -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",
Expand DownExpand Up@@ -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,
Expand All@@ -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,
Expand Down
16 changes: 12 additions & 4 deletionscoderd/connectionlog/connectionlog.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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
}

Expand Down
Loading
[8]ページ先頭
©2009-2025 Movatter.jp