Skip to content

Commit a1b87a6

Browse files
deansheatherdeansheather
authored
fix: use client preferred URL for the default DERP (#18911)
The agentsdk currently does a remap of the DERP map to change the EmbeddedRelay node's URL to match the agent's access URL. This PR makes changes to the `workspacesdk` (used by clients like the CLI) and `vpn` (used by Coder Desktop) to match this behavior. This enables us the ability to try Coder clients in dogfood over a VPN without changing the global access URL.
1 parent fb00cd2 commit a1b87a6

File tree

11 files changed

+248
-56
lines changed

11 files changed

+248
-56
lines changed

‎agent/agent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ type Client interface {
9898
ConnectRPC26(ctx context.Context) (
9999
proto.DRPCAgentClient26, tailnetproto.DRPCTailnetClient26, error,
100100
)
101-
RewriteDERPMap(derpMap *tailcfg.DERPMap)
101+
tailnet.DERPMapRewriter
102102
}
103103

104104
type Agent interface {

‎coderd/tailnet.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func NewServerTailnet(
9898
controller := tailnet.NewController(logger, dialer)
9999
// it's important to set the DERPRegionDialer above _before_ we set the DERP map so that if
100100
// there is an embedded relay, we use the local in-memory dialer.
101-
controller.DERPCtrl = tailnet.NewBasicDERPController(logger, conn)
101+
controller.DERPCtrl = tailnet.NewBasicDERPController(logger, nil, conn)
102102
coordCtrl := NewMultiAgentController(serverCtx, logger, tracer, conn)
103103
controller.CoordCtrl = coordCtrl
104104
// TODO: support controller.TelemetryCtrl

‎codersdk/agentsdk/agentsdk.go

Lines changed: 6 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"net/http"
99
"net/http/cookiejar"
1010
"net/url"
11-
"strconv"
1211
"time"
1312

1413
"cloud.google.com/go/compute/metadata"
@@ -27,6 +26,7 @@ import (
2726
"github.com/coder/coder/v2/coderd/httpapi"
2827
"github.com/coder/coder/v2/codersdk"
2928
"github.com/coder/coder/v2/codersdk/drpcsdk"
29+
"github.com/coder/coder/v2/tailnet"
3030
tailnetproto "github.com/coder/coder/v2/tailnet/proto"
3131
)
3232

@@ -126,40 +126,13 @@ type Script struct {
126126
Script string `json:"script"`
127127
}
128128

129-
// RewriteDERPMap rewrites the DERP map to use the access URL of the SDK as the
130-
// "embedded relay" access URL. The passed derp map is modified in place.
129+
// RewriteDERPMap rewrites the DERP map to use the configured access URL of the
130+
// agent as the "embedded relay" access URL.
131131
//
132-
// Agents can provide an arbitrary access URL that may be different that the
133-
// globally configured one. This breaks the built-in DERP, which would continue
134-
// to reference the global access URL.
132+
// See tailnet.RewriteDERPMapDefaultRelay for more details on why this is
133+
// necessary.
135134
func (c *Client) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
136-
accessingPort := c.SDK.URL.Port()
137-
if accessingPort == "" {
138-
accessingPort = "80"
139-
if c.SDK.URL.Scheme == "https" {
140-
accessingPort = "443"
141-
}
142-
}
143-
accessPort, err := strconv.Atoi(accessingPort)
144-
if err != nil {
145-
// this should never happen because URL.Port() returns the empty string if the port is not
146-
// valid.
147-
c.SDK.Logger().Critical(context.Background(), "failed to parse URL port", slog.F("port", accessingPort))
148-
}
149-
for _, region := range derpMap.Regions {
150-
if !region.EmbeddedRelay {
151-
continue
152-
}
153-
154-
for _, node := range region.Nodes {
155-
if node.STUNOnly {
156-
continue
157-
}
158-
node.HostName = c.SDK.URL.Hostname()
159-
node.DERPPort = accessPort
160-
node.ForceHTTP = c.SDK.URL.Scheme == "http"
161-
}
162-
}
135+
tailnet.RewriteDERPMapDefaultRelay(context.Background(), c.SDK.Logger(), derpMap, c.SDK.URL)
163136
}
164137

165138
// ConnectRPC20 returns a dRPC client to the Agent API v2.0. Notably, it is missing

‎codersdk/agentsdk/agentsdk_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@ import (
55
"io"
66
"net/http"
77
"net/http/httptest"
8+
"net/url"
89
"testing"
910

1011
"github.com/google/uuid"
1112
"github.com/stretchr/testify/require"
13+
"tailscale.com/tailcfg"
1214

1315
"cdr.dev/slog/sloggers/slogtest"
1416
"github.com/coder/coder/v2/codersdk/agentsdk"
@@ -120,3 +122,31 @@ func TestStreamAgentReinitEvents(t *testing.T) {
120122
require.ErrorIs(t, receiveErr, context.Canceled)
121123
})
122124
}
125+
126+
func TestRewriteDERPMap(t *testing.T) {
127+
t.Parallel()
128+
// This test ensures that RewriteDERPMap mutates built-in DERPs with the
129+
// client access URL.
130+
dm := &tailcfg.DERPMap{
131+
Regions: map[int]*tailcfg.DERPRegion{
132+
1: {
133+
EmbeddedRelay: true,
134+
RegionID: 1,
135+
Nodes: []*tailcfg.DERPNode{{
136+
HostName: "bananas.org",
137+
DERPPort: 1,
138+
}},
139+
},
140+
},
141+
}
142+
parsed, err := url.Parse("https://coconuts.org:44558")
143+
require.NoError(t, err)
144+
client := agentsdk.New(parsed)
145+
client.RewriteDERPMap(dm)
146+
region := dm.Regions[1]
147+
require.True(t, region.EmbeddedRelay)
148+
require.Len(t, region.Nodes, 1)
149+
node := region.Nodes[0]
150+
require.Equal(t, "coconuts.org", node.HostName)
151+
require.Equal(t, 44558, node.DERPPort)
152+
}

‎codersdk/workspacesdk/workspacesdk.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,15 @@ type DialAgentOptions struct {
193193
EnableTelemetry bool
194194
}
195195

196+
// RewriteDERPMap rewrites the DERP map to use the configured access URL of the
197+
// client as the "embedded relay" access URL.
198+
//
199+
// See tailnet.RewriteDERPMapDefaultRelay for more details on why this is
200+
// necessary.
201+
func (c *Client) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
202+
tailnet.RewriteDERPMapDefaultRelay(context.Background(), c.client.Logger(), derpMap, c.client.URL)
203+
}
204+
196205
func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *DialAgentOptions) (agentConn *AgentConn, err error) {
197206
if options == nil {
198207
options = &DialAgentOptions{}
@@ -248,6 +257,8 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
248257
telemetrySink = basicTel
249258
controller.TelemetryCtrl = basicTel
250259
}
260+
261+
c.RewriteDERPMap(connInfo.DERPMap)
251262
conn, err := tailnet.NewConn(&tailnet.Options{
252263
Addresses: []netip.Prefix{netip.PrefixFrom(ip, 128)},
253264
DERPMap: connInfo.DERPMap,
@@ -270,7 +281,7 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
270281
coordCtrl := tailnet.NewTunnelSrcCoordController(options.Logger, conn)
271282
coordCtrl.AddDestination(agentID)
272283
controller.CoordCtrl = coordCtrl
273-
controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, conn)
284+
controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, c, conn)
274285
controller.Run(ctx)
275286

276287
options.Logger.Debug(ctx, "running tailnet API v2+ connector")

‎codersdk/workspacesdk/workspacesdk_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919

2020
"github.com/coder/coder/v2/coderd/httpapi"
2121
"github.com/coder/coder/v2/codersdk"
22-
"github.com/coder/coder/v2/codersdk/agentsdk"
2322
"github.com/coder/coder/v2/codersdk/workspacesdk"
2423
"github.com/coder/coder/v2/tailnet"
2524
"github.com/coder/coder/v2/testutil"
@@ -43,7 +42,7 @@ func TestWorkspaceRewriteDERPMap(t *testing.T) {
4342
}
4443
parsed, err := url.Parse("https://coconuts.org:44558")
4544
require.NoError(t, err)
46-
client := agentsdk.New(parsed)
45+
client := workspacesdk.New(codersdk.New(parsed))
4746
client.RewriteDERPMap(dm)
4847
region := dm.Regions[1]
4948
require.True(t, region.EmbeddedRelay)

‎tailnet/controllers.go

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -568,13 +568,15 @@ type DERPMapSetter interface {
568568
}
569569

570570
type basicDERPController struct {
571-
logger slog.Logger
572-
setter DERPMapSetter
571+
logger slog.Logger
572+
rewriter DERPMapRewriter // optional
573+
setter DERPMapSetter
573574
}
574575

575576
func (b *basicDERPController) New(client DERPClient) CloserWaiter {
576577
l := &derpSetLoop{
577578
logger: b.logger,
579+
rewriter: b.rewriter,
578580
setter: b.setter,
579581
client: client,
580582
errChan: make(chan error, 1),
@@ -584,17 +586,23 @@ func (b *basicDERPController) New(client DERPClient) CloserWaiter {
584586
return l
585587
}
586588

587-
func NewBasicDERPController(logger slog.Logger, setter DERPMapSetter) DERPController {
589+
// NewBasicDERPController creates a DERP controller that rewrites the DERP map
590+
// with the provided rewriter before setting it on the provided setter.
591+
//
592+
// The rewriter is optional and can be nil.
593+
func NewBasicDERPController(logger slog.Logger, rewriter DERPMapRewriter, setter DERPMapSetter) DERPController {
588594
return &basicDERPController{
589-
logger: logger,
590-
setter: setter,
595+
logger: logger,
596+
rewriter: rewriter,
597+
setter: setter,
591598
}
592599
}
593600

594601
type derpSetLoop struct {
595-
logger slog.Logger
596-
setter DERPMapSetter
597-
client DERPClient
602+
logger slog.Logger
603+
rewriter DERPMapRewriter // optional
604+
setter DERPMapSetter
605+
client DERPClient
598606

599607
sync.Mutex
600608
closed bool
@@ -640,6 +648,9 @@ func (l *derpSetLoop) recvLoop() {
640648
return
641649
}
642650
l.logger.Debug(context.Background(), "got new DERP Map", slog.F("derp_map", dm))
651+
if l.rewriter != nil {
652+
l.rewriter.RewriteDERPMap(dm)
653+
}
643654
l.setter.SetDERPMap(dm)
644655
}
645656
}

‎tailnet/controllers_test.go

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -586,7 +586,7 @@ func TestNewBasicDERPController_Mainline(t *testing.T) {
586586
t.Parallel()
587587
fs := make(chan *tailcfg.DERPMap)
588588
logger := testutil.Logger(t)
589-
uut := tailnet.NewBasicDERPController(logger, fakeSetter(fs))
589+
uut := tailnet.NewBasicDERPController(logger, nil, fakeSetter(fs))
590590
fc := fakeDERPClient{
591591
ch: make(chan *tailcfg.DERPMap),
592592
}
@@ -609,7 +609,7 @@ func TestNewBasicDERPController_RecvErr(t *testing.T) {
609609
t.Parallel()
610610
fs := make(chan *tailcfg.DERPMap)
611611
logger := testutil.Logger(t)
612-
uut := tailnet.NewBasicDERPController(logger, fakeSetter(fs))
612+
uut := tailnet.NewBasicDERPController(logger, nil, fakeSetter(fs))
613613
expectedErr := xerrors.New("a bad thing happened")
614614
fc := fakeDERPClient{
615615
ch: make(chan *tailcfg.DERPMap),
@@ -1041,7 +1041,7 @@ func TestController_Disconnects(t *testing.T) {
10411041
// darwin can be slow sometimes.
10421042
tailnet.WithGracefulTimeout(5*time.Second))
10431043
uut.CoordCtrl = tailnet.NewAgentCoordinationController(logger.Named("coord_ctrl"), fConn)
1044-
uut.DERPCtrl = tailnet.NewBasicDERPController(logger.Named("derp_ctrl"), fConn)
1044+
uut.DERPCtrl = tailnet.NewBasicDERPController(logger.Named("derp_ctrl"), nil, fConn)
10451045
uut.Run(ctx)
10461046

10471047
call := testutil.TryReceive(testCtx, t, fCoord.CoordinateCalls)
@@ -1945,6 +1945,52 @@ func TestTunnelAllWorkspaceUpdatesController_HandleErrors(t *testing.T) {
19451945
}
19461946
}
19471947

1948+
func TestBasicDERPController_RewriteDERPMap(t *testing.T) {
1949+
t.Parallel()
1950+
ctx := testutil.Context(t, testutil.WaitShort)
1951+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
1952+
1953+
testDERPMap := &tailcfg.DERPMap{
1954+
Regions: map[int]*tailcfg.DERPRegion{
1955+
1: {
1956+
RegionID: 1,
1957+
},
1958+
},
1959+
}
1960+
1961+
// Ensure the fake rewriter works as expected.
1962+
rewriter := &fakeDERPMapRewriter{
1963+
ctx: ctx,
1964+
calls: make(chan rewriteDERPMapCall, 16),
1965+
}
1966+
rewriter.RewriteDERPMap(testDERPMap)
1967+
rewriteCall := testutil.TryReceive(ctx, t, rewriter.calls)
1968+
require.Same(t, testDERPMap, rewriteCall.derpMap)
1969+
1970+
derpClient := &fakeDERPClient{
1971+
ch: make(chan *tailcfg.DERPMap),
1972+
err: nil,
1973+
}
1974+
defer derpClient.Close()
1975+
1976+
derpSetter := &fakeDERPMapSetter{
1977+
ctx: ctx,
1978+
calls: make(chan *setDERPMapCall, 16),
1979+
}
1980+
1981+
derpCtrl := tailnet.NewBasicDERPController(logger, rewriter, derpSetter)
1982+
derpCtrl.New(derpClient)
1983+
1984+
// Simulate receiving a new DERP map from the server, which should be passed
1985+
// to the rewriter and setter.
1986+
testDERPMap = testDERPMap.Clone() // make a new pointer
1987+
derpClient.ch <- testDERPMap
1988+
rewriteCall = testutil.TryReceive(ctx, t, rewriter.calls)
1989+
require.Same(t, testDERPMap, rewriteCall.derpMap)
1990+
setterCall := testutil.TryReceive(ctx, t, derpSetter.calls)
1991+
require.Same(t, testDERPMap, setterCall.derpMap)
1992+
}
1993+
19481994
type fakeWorkspaceUpdatesController struct {
19491995
ctx context.Context
19501996
t testing.TB
@@ -2040,3 +2086,45 @@ type fakeCloser struct{}
20402086
func (fakeCloser) Close() error {
20412087
return nil
20422088
}
2089+
2090+
type fakeDERPMapRewriter struct {
2091+
ctx context.Context
2092+
calls chan rewriteDERPMapCall
2093+
}
2094+
2095+
var _ tailnet.DERPMapRewriter = &fakeDERPMapRewriter{}
2096+
2097+
type rewriteDERPMapCall struct {
2098+
derpMap *tailcfg.DERPMap
2099+
}
2100+
2101+
func (f *fakeDERPMapRewriter) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
2102+
call := rewriteDERPMapCall{
2103+
derpMap: derpMap,
2104+
}
2105+
select {
2106+
case f.calls <- call:
2107+
case <-f.ctx.Done():
2108+
}
2109+
}
2110+
2111+
type fakeDERPMapSetter struct {
2112+
ctx context.Context
2113+
calls chan *setDERPMapCall
2114+
}
2115+
2116+
var _ tailnet.DERPMapSetter = &fakeDERPMapSetter{}
2117+
2118+
type setDERPMapCall struct {
2119+
derpMap *tailcfg.DERPMap
2120+
}
2121+
2122+
func (f *fakeDERPMapSetter) SetDERPMap(derpMap *tailcfg.DERPMap) {
2123+
call := &setDERPMapCall{
2124+
derpMap: derpMap,
2125+
}
2126+
select {
2127+
case <-f.ctx.Done():
2128+
case f.calls <- call:
2129+
}
2130+
}

0 commit comments

Comments
 (0)