Skip to content

wgengine/magicsock: allow passing region dialer to derp client #13

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
Mar 14, 2023
Merged

wgengine/magicsock: allow passing region dialer to derp client #13

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
36 changes: 36 additions & 0 deletions derp/derphttp/derphttp_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ type Client struct {
logf logger.Logf
dialer func(ctx context.Context, network, addr string) (net.Conn, error)

// regionDialer allows the caller to override the dialer used to
// connect to DERP regions. If nil, the default dialer is used.
regionDialer func(ctx context.Context, r *tailcfg.DERPRegion) net.Conn

// Either url or getRegion is non-nil:
url *url.URL
getRegion func() *tailcfg.DERPRegion
Expand Down Expand Up @@ -313,6 +317,31 @@ func (c *Client) connect(ctx context.Context, caller string) (client *derp.Clien
}
}()

if c.regionDialer != nil {
conn := c.regionDialer(ctx, reg)
if conn != nil {
brw := bufio.NewReadWriter(bufio.NewReader(conn), bufio.NewWriter(conn))
derpClient, err := derp.NewClient(c.privateKey, conn, brw, c.logf,
derp.MeshKey(c.MeshKey),
derp.CanAckPings(c.canAckPings),
derp.IsProber(c.IsProber),
)
if err != nil {
return nil, 0, err
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, 0, err
go conn.Close()
return nil, 0, err

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't do this below, so I don't think it's necessary here.

Somewhat related tailscale#7557

Copy link
Member

@mafredri mafredri Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The reason it's OK below is that a defer func is created that checks the return error and closes tcpConn if there was an error. Here, however, regionDialer could have allocated resources/goroutines that need to be torn down or otherwise they will leak. That's why we need the close here.

Edit: Granted, I didn't verify that it's actually OK below. As long as the TLS client doesn't need to be torn down, should be fine.

As for the websocket dial, it's unclear if we should do a close there as well or if that'll be guaranteed by the context (it'd definitely be safer to close there as well).

}
if c.preferred {
// It's important that this happens in a goroutine because
// of synchronous I/O woes.
go derpClient.NotePreferred(true)
}
c.serverPubKey = derpClient.ServerPublicKey()
c.client = derpClient
c.netConn = conn
c.connGen++
return c.client, c.connGen, nil
}
}

var node *tailcfg.DERPNode // nil when using c.url to dial
switch {
case c.useWebsockets():
Expand Down Expand Up @@ -513,6 +542,13 @@ func (c *Client) SetURLDialer(dialer func(ctx context.Context, network, addr str
c.dialer = dialer
}

// SetRegionDialer sets the dialer to use for dialing DERP regions.
func (c *Client) SetRegionDialer(dialer func(ctx context.Context, region *tailcfg.DERPRegion) net.Conn) {
c.mu.Lock()
c.regionDialer = dialer
c.mu.Unlock()
}

// SetForcedWebsocketCallback is a callback that is called when the client
// decides to force WebSockets on the next connection attempt.
func (c *Client) SetForcedWebsocketCallback(callback func(region int, reason string)) {
Expand Down
16 changes: 16 additions & 0 deletions wgengine/magicsock/magicsock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,9 @@ type Conn struct {
// headers that are passed to the DERP HTTP client
derpHeader atomic.Pointer[http.Header]

// derpRegionDialer is passed to the DERP client
derpRegionDialer atomic.Pointer[func(ctx context.Context, region *tailcfg.DERPRegion) net.Conn]

// stats maintains per-connection counters.
stats atomic.Pointer[connstats.Statistics]

Expand Down Expand Up @@ -1514,6 +1517,10 @@ func (c *Conn) derpWriteChanOfAddr(addr netip.AddrPort, peer key.NodePublic) cha
if header != nil {
dc.Header = header.Clone()
}
dialer := c.derpRegionDialer.Load()
if dialer != nil {
dc.SetRegionDialer(*dialer)
}

ctx, cancel := context.WithCancel(c.connCtx)
ch := make(chan derpWriteRequest, bufferedDerpWritesBeforeDrop)
Expand Down Expand Up @@ -2367,6 +2374,15 @@ func (c *Conn) SetDERPHeader(header http.Header) {
c.derpHeader.Store(&header)
}

func (c *Conn) SetDERPRegionDialer(dialer func(ctx context.Context, region *tailcfg.DERPRegion) net.Conn) {
c.derpRegionDialer.Store(&dialer)
c.mu.Lock()
defer c.mu.Unlock()
for _, d := range c.activeDerp {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance c.activeDerp could contain a result where d.c is the c *Conn itself? Thus leading to a mutex deadlock?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe d.c refers to client, in which case there should be no issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

d.c refers to client, so should be good I believe!

d.c.SetRegionDialer(dialer)
}
}

func (c *Conn) SetNetworkUp(up bool) {
c.mu.Lock()
defer c.mu.Unlock()
Expand Down