diff --git a/agent/agent.go b/agent/agent.go index 75117769d8e2d..63db87f2d9e4a 100644 --- a/agent/agent.go +++ b/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 { diff --git a/coderd/tailnet.go b/coderd/tailnet.go index cfdc667f4da0f..172edea95a586 100644 --- a/coderd/tailnet.go +++ b/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 diff --git a/codersdk/agentsdk/agentsdk.go b/codersdk/agentsdk/agentsdk.go index a78ee3c5608dd..5bd0030456757 100644 --- a/codersdk/agentsdk/agentsdk.go +++ b/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 diff --git a/codersdk/agentsdk/agentsdk_test.go b/codersdk/agentsdk/agentsdk_test.go index 8ad2d69be0b98..e6ea6838dd9b2 100644 --- a/codersdk/agentsdk/agentsdk_test.go +++ b/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) +} diff --git a/codersdk/workspacesdk/workspacesdk.go b/codersdk/workspacesdk/workspacesdk.go index 83f236a215b56..9f587cf5267a8 100644 --- a/codersdk/workspacesdk/workspacesdk.go +++ b/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") diff --git a/codersdk/workspacesdk/workspacesdk_test.go b/codersdk/workspacesdk/workspacesdk_test.go index 16a523b2d4d53..f1158cf9034aa 100644 --- a/codersdk/workspacesdk/workspacesdk_test.go +++ b/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) diff --git a/tailnet/controllers.go b/tailnet/controllers.go index b7d4e246a4bee..f2fd12946c8c0 100644 --- a/tailnet/controllers.go +++ b/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) } } diff --git a/tailnet/controllers_test.go b/tailnet/controllers_test.go index ed83d3e086be1..63602ff4e8867 100644 --- a/tailnet/controllers_test.go +++ b/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: + } +} diff --git a/tailnet/derp.go b/tailnet/derp.go index 41106474c9cd6..293bafcfe3a7d 100644 --- a/tailnet/derp.go +++ b/tailnet/derp.go @@ -5,10 +5,15 @@ import ( "context" "log" "net/http" + "net/url" + "strconv" "strings" "sync" "tailscale.com/derp" + "tailscale.com/tailcfg" + + "cdr.dev/slog" "github.com/coder/websocket" ) @@ -70,3 +75,54 @@ func WithWebsocketSupport(s *derp.Server, base http.Handler) (http.Handler, func mu.Unlock() } } + +type DERPMapRewriter interface { + RewriteDERPMap(derpMap *tailcfg.DERPMap) +} + +// RewriteDERPMapDefaultRelay rewrites the DERP map to use the given access URL +// as the "embedded relay" access URL. The passed derp map is modified in place. +// +// This is used by clients and agents to rewrite the default DERP relay to use +// their preferred access URL. Both of these clients can use a different access +// URL than the deployment has configured (with `--access-url`), so we need to +// accommodate that and respect the locally configured access URL. +// +// Note: passed context is only used for logging. +func RewriteDERPMapDefaultRelay(ctx context.Context, logger slog.Logger, derpMap *tailcfg.DERPMap, accessURL *url.URL) { + if derpMap == nil { + return + } + + accessPort := 80 + if accessURL.Scheme == "https" { + accessPort = 443 + } + if accessURL.Port() != "" { + parsedAccessPort, err := strconv.Atoi(accessURL.Port()) + if err != nil { + // This should never happen because URL.Port() returns the empty string + // if the port is not valid. + logger.Critical(ctx, "failed to parse URL port, using default port", + slog.F("port", accessURL.Port()), + slog.F("access_url", accessURL)) + } else { + accessPort = parsedAccessPort + } + } + + for _, region := range derpMap.Regions { + if !region.EmbeddedRelay { + continue + } + + for _, node := range region.Nodes { + if node.STUNOnly { + continue + } + node.HostName = accessURL.Hostname() + node.DERPPort = accessPort + node.ForceHTTP = accessURL.Scheme == "http" + } + } +} diff --git a/vpn/client.go b/vpn/client.go index d52718e7fa7ab..0411b209c24a8 100644 --- a/vpn/client.go +++ b/vpn/client.go @@ -78,6 +78,19 @@ type Options struct { UpdateHandler tailnet.UpdatesHandler } +type derpMapRewriter struct { + logger slog.Logger + serverURL *url.URL +} + +var _ tailnet.DERPMapRewriter = &derpMapRewriter{} + +// RewriteDERPMap implements tailnet.DERPMapRewriter. See +// tailnet.RewriteDERPMapDefaultRelay for more details on why this is necessary. +func (d *derpMapRewriter) RewriteDERPMap(derpMap *tailcfg.DERPMap) { + tailnet.RewriteDERPMapDefaultRelay(context.Background(), d.logger, derpMap, d.serverURL) +} + func (*client) NewConn(initCtx context.Context, serverURL *url.URL, token string, options *Options) (vpnC Conn, err error) { if options == nil { options = &Options{} @@ -135,6 +148,12 @@ func (*client) NewConn(initCtx context.Context, serverURL *url.URL, token string WorkspaceOwnerId: tailnet.UUIDToByteSlice(me.ID), })) + derpMapRewriter := &derpMapRewriter{ + logger: options.Logger, + serverURL: serverURL, + } + derpMapRewriter.RewriteDERPMap(connInfo.DERPMap) + clonedHeaders := headers.Clone() ip := tailnet.CoderServicePrefix.RandomAddr() conn, err := tailnet.NewConn(&tailnet.Options{ @@ -164,7 +183,7 @@ func (*client) NewConn(initCtx context.Context, serverURL *url.URL, token string coordCtrl := tailnet.NewTunnelSrcCoordController(options.Logger, conn) controller.ResumeTokenCtrl = tailnet.NewBasicResumeTokenController(options.Logger, clk) controller.CoordCtrl = coordCtrl - controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, conn) + controller.DERPCtrl = tailnet.NewBasicDERPController(options.Logger, derpMapRewriter, conn) updatesCtrl := tailnet.NewTunnelAllWorkspaceUpdatesController( options.Logger, coordCtrl, diff --git a/vpn/client_test.go b/vpn/client_test.go index de13b2349d5d4..dd359afd0a44b 100644 --- a/vpn/client_test.go +++ b/vpn/client_test.go @@ -43,14 +43,19 @@ func TestClient_WorkspaceUpdates(t *testing.T) { hostnames []string }{ { - name: "empty", - agentConnectionInfo: workspacesdk.AgentConnectionInfo{}, - hostnames: []string{"wrk.coder.", "agnt.wrk.me.coder.", "agnt.wrk.rootbeer.coder."}, + name: "empty", + agentConnectionInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, + hostnames: []string{"wrk.coder.", "agnt.wrk.me.coder.", "agnt.wrk.rootbeer.coder."}, }, { - name: "suffix", - agentConnectionInfo: workspacesdk.AgentConnectionInfo{HostnameSuffix: "float"}, - hostnames: []string{"wrk.float.", "agnt.wrk.me.float.", "agnt.wrk.rootbeer.float."}, + name: "suffix", + agentConnectionInfo: workspacesdk.AgentConnectionInfo{ + HostnameSuffix: "float", + DERPMap: &tailcfg.DERPMap{}, + }, + hostnames: []string{"wrk.float.", "agnt.wrk.me.float.", "agnt.wrk.rootbeer.float."}, }, } for _, tc := range testCases {