Skip to content

feat: make ServerTailnet set peers lost when it reconnects to the coordinator #11682

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 23, 2024
Merged

feat: make ServerTailnet set peers lost when it reconnects to the coordinator #11682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,9 @@ gen: \
site/e2e/provisionerGenerated.ts \
site/src/theme/icons.json \
examples/examples.gen.json \
tailnet/tailnettest/coordinatormock.go
tailnet/tailnettest/coordinatormock.go \
tailnet/tailnettest/coordinateemock.go \
tailnet/tailnettest/multiagentmock.go
.PHONY: gen

# Mark all generated files as fresh so make thinks they're up-to-date. This is
Expand Down Expand Up @@ -504,6 +506,8 @@ gen/mark-fresh:
site/src/theme/icons.json \
examples/examples.gen.json \
tailnet/tailnettest/coordinatormock.go \
tailnet/tailnettest/coordinateemock.go \
tailnet/tailnettest/multiagentmock.go \
"
for file in $$files; do
echo "$$file"
Expand Down Expand Up @@ -531,7 +535,7 @@ coderd/database/querier.go: coderd/database/sqlc.yaml coderd/database/dump.sql $
coderd/database/dbmock/dbmock.go: coderd/database/db.go coderd/database/querier.go
go generate ./coderd/database/dbmock/

tailnet/tailnettest/coordinatormock.go: tailnet/coordinator.go
tailnet/tailnettest/coordinatormock.go tailnet/tailnettest/multiagentmock.go tailnet/tailnettest/coordinateemock.go: tailnet/coordinator.go tailnet/multiagent.go
go generate ./tailnet/tailnettest/

tailnet/proto/tailnet.pb.go: tailnet/proto/tailnet.proto
Expand Down
15 changes: 11 additions & 4 deletions coderd/tailnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ func NewServerTailnet(
logger: logger,
tracer: traceProvider.Tracer(tracing.TracerName),
conn: conn,
coordinatee: conn,
getMultiAgent: getMultiAgent,
cache: cache,
agentConnectionTimes: map[uuid.UUID]time.Time{},
Expand Down Expand Up @@ -224,13 +225,14 @@ func (s *ServerTailnet) watchAgentUpdates() {
if !ok {
if conn.IsClosed() && s.ctx.Err() == nil {
s.logger.Warn(s.ctx, "multiagent closed, reinitializing")
s.coordinatee.SetAllPeersLost()
s.reinitCoordinator()
continue
}
return
}

err := s.conn.UpdatePeers(resp.GetPeerUpdates())
err := s.coordinatee.UpdatePeers(resp.GetPeerUpdates())
if err != nil {
if xerrors.Is(err, tailnet.ErrConnClosed) {
s.logger.Warn(context.Background(), "tailnet conn closed, exiting watchAgentUpdates", slog.Error(err))
Expand Down Expand Up @@ -280,9 +282,14 @@ type ServerTailnet struct {
cancel func()
derpMapUpdaterClosed chan struct{}

logger slog.Logger
tracer trace.Tracer
conn *tailnet.Conn
logger slog.Logger
tracer trace.Tracer

// in prod, these are the same, but coordinatee is a subset of Conn's
// methods which makes some tests easier.
conn *tailnet.Conn
coordinatee tailnet.Coordinatee

getMultiAgent func(context.Context) (tailnet.MultiAgentConn, error)
agentConn atomic.Pointer[tailnet.MultiAgentConn]
cache *wsconncache.Cache
Expand Down
75 changes: 75 additions & 0 deletions coderd/tailnet_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package coderd

import (
"context"
"sync/atomic"
"testing"
"time"

"github.com/google/uuid"
"go.uber.org/mock/gomock"

"cdr.dev/slog"
"cdr.dev/slog/sloggers/slogtest"
"github.com/coder/coder/v2/tailnet"
"github.com/coder/coder/v2/tailnet/tailnettest"
"github.com/coder/coder/v2/testutil"
)

// TestServerTailnet_Reconnect tests that ServerTailnet calls SetAllPeersLost on the Coordinatee
// (tailnet.Conn in production) when it disconnects from the Coordinator (via MultiAgentConn) and
// reconnects.
func TestServerTailnet_Reconnect(t *testing.T) {
t.Parallel()
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
ctrl := gomock.NewController(t)
ctx := testutil.Context(t, testutil.WaitShort)

mMultiAgent0 := tailnettest.NewMockMultiAgentConn(ctrl)
mMultiAgent1 := tailnettest.NewMockMultiAgentConn(ctrl)
mac := make(chan tailnet.MultiAgentConn, 2)
mac <- mMultiAgent0
mac <- mMultiAgent1
mCoord := tailnettest.NewMockCoordinatee(ctrl)

uut := &ServerTailnet{
ctx: ctx,
logger: logger,
coordinatee: mCoord,
getMultiAgent: func(ctx context.Context) (tailnet.MultiAgentConn, error) {
select {
case <-ctx.Done():
return nil, ctx.Err()
case m := <-mac:
return m, nil
}
},
agentConn: atomic.Pointer[tailnet.MultiAgentConn]{},
agentConnectionTimes: make(map[uuid.UUID]time.Time),
}
// reinit the Coordinator once, to load mMultiAgent0
uut.reinitCoordinator()

mMultiAgent0.EXPECT().NextUpdate(gomock.Any()).
Times(1).
Return(nil, false) // this indicates there are no more updates
closed0 := mMultiAgent0.EXPECT().IsClosed().
Times(1).
Return(true) // this triggers reconnect
setLost := mCoord.EXPECT().SetAllPeersLost().Times(1).After(closed0)
mMultiAgent1.EXPECT().NextUpdate(gomock.Any()).
Times(1).
After(setLost).
Return(nil, false)
mMultiAgent1.EXPECT().IsClosed().
Times(1).
Return(false) // this causes us to exit and not reconnect

done := make(chan struct{})
go func() {
uut.watchAgentUpdates()
close(done)
}()

testutil.RequireRecvCtx(ctx, t, done)
}
2 changes: 1 addition & 1 deletion tailnet/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func TestClientService_ServeClient_V2(t *testing.T) {
err = c.Close()
require.NoError(t, err)
err = testutil.RequireRecvCtx(ctx, t, errCh)
require.ErrorIs(t, err, io.EOF)
require.True(t, xerrors.Is(err, io.EOF) || xerrors.Is(err, io.ErrClosedPipe))
}

func TestClientService_ServeClient_V1(t *testing.T) {
Expand Down
79 changes: 79 additions & 0 deletions tailnet/tailnettest/coordinateemock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

141 changes: 141 additions & 0 deletions tailnet/tailnettest/multiagentmock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions tailnet/tailnettest/tailnettest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import (
"github.com/coder/coder/v2/tailnet"
)

//go:generate mockgen -destination ./multiagentmock.go -package tailnettest github.com/coder/coder/v2/tailnet MultiAgentConn
//go:generate mockgen -destination ./coordinatormock.go -package tailnettest github.com/coder/coder/v2/tailnet Coordinator
//go:generate mockgen -destination ./coordinateemock.go -package tailnettest github.com/coder/coder/v2/tailnet Coordinatee

// RunDERPAndSTUN creates a DERP mapping for tests.
func RunDERPAndSTUN(t *testing.T) (*tailcfg.DERPMap, *derp.Server) {
Expand Down