fix: use client preferred URL for the default DERP (coder#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.

Author: deansheather

agent/agent.go

@@ -98,7 +98,7 @@ type Client interface {
 	ConnectRPC26(ctx context.Context) (
 		proto.DRPCAgentClient26, tailnetproto.DRPCTailnetClient26, error,
 	)
-	RewriteDERPMap(derpMap *tailcfg.DERPMap)
+	tailnet.DERPMapRewriter
 }
 
 type Agent interface {

coderd/tailnet.go

@@ -98,7 +98,7 @@ func NewServerTailnet(
 	controller := tailnet.NewController(logger, dialer)
 	// it's important to set the DERPRegionDialer above _before_ we set the DERP map so that if
 	// there is an embedded relay, we use the local in-memory dialer.
-	controller.DERPCtrl = tailnet.NewBasicDERPController(logger, conn)
+	controller.DERPCtrl = tailnet.NewBasicDERPController(logger, nil, conn)
 	coordCtrl := NewMultiAgentController(serverCtx, logger, tracer, conn)
 	controller.CoordCtrl = coordCtrl
 	// TODO: support controller.TelemetryCtrl

codersdk/agentsdk/agentsdk.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 	"net/http/cookiejar"
 	"net/url"
-	"strconv"
 	"time"
 
 	"cloud.google.com/go/compute/metadata"
@@ -27,6 +26,7 @@ import (
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/codersdk/drpcsdk"
+	"github.com/coder/coder/v2/tailnet"
 	tailnetproto "github.com/coder/coder/v2/tailnet/proto"
 )
 
@@ -126,40 +126,13 @@ type Script struct {
 	Script string `json:"script"`
 }
 
-// RewriteDERPMap rewrites the DERP map to use the access URL of the SDK as the
-// "embedded relay" access URL. The passed derp map is modified in place.
+// RewriteDERPMap rewrites the DERP map to use the configured access URL of the
+// agent as the "embedded relay" access URL.
 //
-// Agents can provide an arbitrary access URL that may be different that the
-// globally configured one. This breaks the built-in DERP, which would continue
-// to reference the global access URL.
+// See tailnet.RewriteDERPMapDefaultRelay for more details on why this is
+// necessary.
 func (c *Client) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
-	accessingPort := c.SDK.URL.Port()
-	if accessingPort == "" {
-		accessingPort = "80"
-		if c.SDK.URL.Scheme == "https" {
-			accessingPort = "443"
-		}
-	}
-	accessPort, err := strconv.Atoi(accessingPort)
-	if err != nil {
-		// this should never happen because URL.Port() returns the empty string if the port is not
-		// valid.
-		c.SDK.Logger().Critical(context.Background(), "failed to parse URL port", slog.F("port", accessingPort))
-	}
-	for _, region := range derpMap.Regions {
-		if !region.EmbeddedRelay {
-			continue
-		}
-
-		for _, node := range region.Nodes {
-			if node.STUNOnly {
-				continue
-			}
-			node.HostName = c.SDK.URL.Hostname()
-			node.DERPPort = accessPort
-			node.ForceHTTP = c.SDK.URL.Scheme == "http"
-		}
-	}
+	tailnet.RewriteDERPMapDefaultRelay(context.Background(), c.SDK.Logger(), derpMap, c.SDK.URL)
 }
 
 // ConnectRPC20 returns a dRPC client to the Agent API v2.0.  Notably, it is missing

codersdk/agentsdk/agentsdk_test.go

@@ -5,10 +5,12 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
 
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/v2/codersdk/agentsdk"
@@ -120,3 +122,31 @@ func TestStreamAgentReinitEvents(t *testing.T) {
 		require.ErrorIs(t, receiveErr, context.Canceled)
 	})
 }
+
+func TestRewriteDERPMap(t *testing.T) {
+	t.Parallel()
+	// This test ensures that RewriteDERPMap mutates built-in DERPs with the
+	// client access URL.
+	dm := &tailcfg.DERPMap{
+		Regions: map[int]*tailcfg.DERPRegion{
+			1: {
+				EmbeddedRelay: true,
+				RegionID:      1,
+				Nodes: []*tailcfg.DERPNode{{
+					HostName: "bananas.org",
+					DERPPort: 1,
+				}},
+			},
+		},
+	}
+	parsed, err := url.Parse("https://coconuts.org:44558")
+	require.NoError(t, err)
+	client := agentsdk.New(parsed)
+	client.RewriteDERPMap(dm)
+	region := dm.Regions[1]
+	require.True(t, region.EmbeddedRelay)
+	require.Len(t, region.Nodes, 1)
+	node := region.Nodes[0]
+	require.Equal(t, "coconuts.org", node.HostName)
+	require.Equal(t, 44558, node.DERPPort)
+}

codersdk/workspacesdk/workspacesdk.go

@@ -193,6 +193,15 @@ type DialAgentOptions struct {
 	EnableTelemetry bool
 }
 
+// RewriteDERPMap rewrites the DERP map to use the configured access URL of the
+// client as the "embedded relay" access URL.
+//
+// See tailnet.RewriteDERPMapDefaultRelay for more details on why this is
+// necessary.
+func (c *Client) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
+	tailnet.RewriteDERPMapDefaultRelay(context.Background(), c.client.Logger(), derpMap, c.client.URL)
+}
+
 func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *DialAgentOptions) (agentConn *AgentConn, err error) {
 	if options == nil {
 		options = &DialAgentOptions{}
@@ -248,6 +257,8 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
 		telemetrySink = basicTel
 		controller.TelemetryCtrl = basicTel
 	}
+
+	c.RewriteDERPMap(connInfo.DERPMap)
 	conn, err := tailnet.NewConn(&tailnet.Options{
 		Addresses:           []netip.Prefix{netip.PrefixFrom(ip, 128)},
 		DERPMap:             connInfo.DERPMap,
@@ -270,7 +281,7 @@ func (c *Client) DialAgent(dialCtx context.Context, agentID uuid.UUID, options *
 	coordCtrl := tailnet.NewTunnelSrcCoordController(options.Logger, conn)
 	coordCtrl.AddDestination(agentID)
 	controller.CoordCtrl = coordCtrl
-	controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, conn)
+	controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, c, conn)
 	controller.Run(ctx)
 
 	options.Logger.Debug(ctx, "running tailnet API v2+ connector")

codersdk/workspacesdk/workspacesdk_test.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/codersdk"
-	"github.com/coder/coder/v2/codersdk/agentsdk"
 	"github.com/coder/coder/v2/codersdk/workspacesdk"
 	"github.com/coder/coder/v2/tailnet"
 	"github.com/coder/coder/v2/testutil"
@@ -43,7 +42,7 @@ func TestWorkspaceRewriteDERPMap(t *testing.T) {
 	}
 	parsed, err := url.Parse("https://coconuts.org:44558")
 	require.NoError(t, err)
-	client := agentsdk.New(parsed)
+	client := workspacesdk.New(codersdk.New(parsed))
 	client.RewriteDERPMap(dm)
 	region := dm.Regions[1]
 	require.True(t, region.EmbeddedRelay)

tailnet/controllers.go

@@ -568,13 +568,15 @@ type DERPMapSetter interface {
 }
 
 type basicDERPController struct {
-	logger slog.Logger
-	setter DERPMapSetter
+	logger   slog.Logger
+	rewriter DERPMapRewriter // optional
+	setter   DERPMapSetter
 }
 
 func (b *basicDERPController) New(client DERPClient) CloserWaiter {
 	l := &derpSetLoop{
 		logger:       b.logger,
+		rewriter:     b.rewriter,
 		setter:       b.setter,
 		client:       client,
 		errChan:      make(chan error, 1),
@@ -584,17 +586,23 @@ func (b *basicDERPController) New(client DERPClient) CloserWaiter {
 	return l
 }
 
-func NewBasicDERPController(logger slog.Logger, setter DERPMapSetter) DERPController {
+// NewBasicDERPController creates a DERP controller that rewrites the DERP map
+// with the provided rewriter before setting it on the provided setter.
+//
+// The rewriter is optional and can be nil.
+func NewBasicDERPController(logger slog.Logger, rewriter DERPMapRewriter, setter DERPMapSetter) DERPController {
 	return &basicDERPController{
-		logger: logger,
-		setter: setter,
+		logger:   logger,
+		rewriter: rewriter,
+		setter:   setter,
 	}
 }
 
 type derpSetLoop struct {
-	logger slog.Logger
-	setter DERPMapSetter
-	client DERPClient
+	logger   slog.Logger
+	rewriter DERPMapRewriter // optional
+	setter   DERPMapSetter
+	client   DERPClient
 
 	sync.Mutex
 	closed       bool
@@ -640,6 +648,9 @@ func (l *derpSetLoop) recvLoop() {
 			return
 		}
 		l.logger.Debug(context.Background(), "got new DERP Map", slog.F("derp_map", dm))
+		if l.rewriter != nil {
+			l.rewriter.RewriteDERPMap(dm)
+		}
 		l.setter.SetDERPMap(dm)
 	}
 }

tailnet/controllers_test.go

@@ -586,7 +586,7 @@ func TestNewBasicDERPController_Mainline(t *testing.T) {
 	t.Parallel()
 	fs := make(chan *tailcfg.DERPMap)
 	logger := testutil.Logger(t)
-	uut := tailnet.NewBasicDERPController(logger, fakeSetter(fs))
+	uut := tailnet.NewBasicDERPController(logger, nil, fakeSetter(fs))
 	fc := fakeDERPClient{
 		ch: make(chan *tailcfg.DERPMap),
 	}
@@ -609,7 +609,7 @@ func TestNewBasicDERPController_RecvErr(t *testing.T) {
 	t.Parallel()
 	fs := make(chan *tailcfg.DERPMap)
 	logger := testutil.Logger(t)
-	uut := tailnet.NewBasicDERPController(logger, fakeSetter(fs))
+	uut := tailnet.NewBasicDERPController(logger, nil, fakeSetter(fs))
 	expectedErr := xerrors.New("a bad thing happened")
 	fc := fakeDERPClient{
 		ch:  make(chan *tailcfg.DERPMap),
@@ -1041,7 +1041,7 @@ func TestController_Disconnects(t *testing.T) {
 		// darwin can be slow sometimes.
 		tailnet.WithGracefulTimeout(5*time.Second))
 	uut.CoordCtrl = tailnet.NewAgentCoordinationController(logger.Named("coord_ctrl"), fConn)
-	uut.DERPCtrl = tailnet.NewBasicDERPController(logger.Named("derp_ctrl"), fConn)
+	uut.DERPCtrl = tailnet.NewBasicDERPController(logger.Named("derp_ctrl"), nil, fConn)
 	uut.Run(ctx)
 
 	call := testutil.TryReceive(testCtx, t, fCoord.CoordinateCalls)
@@ -1945,6 +1945,52 @@ func TestTunnelAllWorkspaceUpdatesController_HandleErrors(t *testing.T) {
 	}
 }
 
+func TestBasicDERPController_RewriteDERPMap(t *testing.T) {
+	t.Parallel()
+	ctx := testutil.Context(t, testutil.WaitShort)
+	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
+
+	testDERPMap := &tailcfg.DERPMap{
+		Regions: map[int]*tailcfg.DERPRegion{
+			1: {
+				RegionID: 1,
+			},
+		},
+	}
+
+	// Ensure the fake rewriter works as expected.
+	rewriter := &fakeDERPMapRewriter{
+		ctx:   ctx,
+		calls: make(chan rewriteDERPMapCall, 16),
+	}
+	rewriter.RewriteDERPMap(testDERPMap)
+	rewriteCall := testutil.TryReceive(ctx, t, rewriter.calls)
+	require.Same(t, testDERPMap, rewriteCall.derpMap)
+
+	derpClient := &fakeDERPClient{
+		ch:  make(chan *tailcfg.DERPMap),
+		err: nil,
+	}
+	defer derpClient.Close()
+
+	derpSetter := &fakeDERPMapSetter{
+		ctx:   ctx,
+		calls: make(chan *setDERPMapCall, 16),
+	}
+
+	derpCtrl := tailnet.NewBasicDERPController(logger, rewriter, derpSetter)
+	derpCtrl.New(derpClient)
+
+	// Simulate receiving a new DERP map from the server, which should be passed
+	// to the rewriter and setter.
+	testDERPMap = testDERPMap.Clone() // make a new pointer
+	derpClient.ch <- testDERPMap
+	rewriteCall = testutil.TryReceive(ctx, t, rewriter.calls)
+	require.Same(t, testDERPMap, rewriteCall.derpMap)
+	setterCall := testutil.TryReceive(ctx, t, derpSetter.calls)
+	require.Same(t, testDERPMap, setterCall.derpMap)
+}
+
 type fakeWorkspaceUpdatesController struct {
 	ctx   context.Context
 	t     testing.TB
@@ -2040,3 +2086,45 @@ type fakeCloser struct{}
 func (fakeCloser) Close() error {
 	return nil
 }
+
+type fakeDERPMapRewriter struct {
+	ctx   context.Context
+	calls chan rewriteDERPMapCall
+}
+
+var _ tailnet.DERPMapRewriter = &fakeDERPMapRewriter{}
+
+type rewriteDERPMapCall struct {
+	derpMap *tailcfg.DERPMap
+}
+
+func (f *fakeDERPMapRewriter) RewriteDERPMap(derpMap *tailcfg.DERPMap) {
+	call := rewriteDERPMapCall{
+		derpMap: derpMap,
+	}
+	select {
+	case f.calls <- call:
+	case <-f.ctx.Done():
+	}
+}
+
+type fakeDERPMapSetter struct {
+	ctx   context.Context
+	calls chan *setDERPMapCall
+}
+
+var _ tailnet.DERPMapSetter = &fakeDERPMapSetter{}
+
+type setDERPMapCall struct {
+	derpMap *tailcfg.DERPMap
+}
+
+func (f *fakeDERPMapSetter) SetDERPMap(derpMap *tailcfg.DERPMap) {
+	call := &setDERPMapCall{
+		derpMap: derpMap,
+	}
+	select {
+	case <-f.ctx.Done():
+	case f.calls <- call:
+	}
+}