wgengine/magicsock: allow passing region dialer to derp client #13
Conversation
derp/derphttp/derphttp_client.go
Outdated
| if c.nodeDialer != nil { | ||
| conn, err := c.nodeDialer(ctx, n) | ||
| // If both conn and err are nil, fallback to default behavior. | ||
| if conn != nil || err != nil { |
There was a problem hiding this comment.
Doesn't this mean we never return conn since we return nil, err below?
There was a problem hiding this comment.
Ahh, oops! I intended to return both.
derp/derphttp/derphttp_client.go
Outdated
| @@ -513,6 +517,11 @@ func (c *Client) SetURLDialer(dialer func(ctx context.Context, network, addr str | |||
| c.dialer = dialer | |||
| } | |||
|
|
|||
| // SetNodeDialer sets the dialer to use for dialing DERP nodes. | |||
| func (c *Client) SetNodeDialer(dialer func(ctx context.Context, node *tailcfg.DERPNode) (net.Conn, error)) { | |||
| c.nodeDialer = dialer | |||
There was a problem hiding this comment.
Do we need to protect via mutex or is this invoked synchronously at a safe time?
There was a problem hiding this comment.
It's invoked synchronously, but I'll add a mutex to be safe.
kylecarbs
force-pushed
the
derpurldialer
branch
4 times, most recently
from
e281f64
to
c653484
Compare
| c.derpRegionDialer.Store(&dialer) | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| for _, d := range c.activeDerp { |
There was a problem hiding this comment.
Is there any chance c.activeDerp could contain a result where d.c is the c *Conn itself? Thus leading to a mutex deadlock?
There was a problem hiding this comment.
Or maybe d.c refers to client, in which case there should be no issue.
There was a problem hiding this comment.
d.c refers to client, so should be good I believe!
| derp.IsProber(c.IsProber), | ||
| ) | ||
| if err != nil { | ||
| return nil, 0, err |
There was a problem hiding this comment.
| return nil, 0, err | |
| go conn.Close() | |
| return nil, 0, err |
There was a problem hiding this comment.
It doesn't do this below, so I don't think it's necessary here.
Somewhat related tailscale#7557
There was a problem hiding this comment.
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).
Coder pods using the embedded relay would dial the access URL to dial DERP. This is unnecessary and led to oddities in deployments with restricted networking. This will allow us to directly dial the DERP server, eliminating the external network path entirely.
kylecarbs
force-pushed
the
derpurldialer
branch
from
c653484
to
0cda9db
Compare
kylecarbs
changed the title
Coder pods using the embedded relay would dial the access URL to dial DERP. This is unnecessary and led to oddities in deployments with restricted networking.
This will allow us to directly dial the DERP server, eliminating the external network path entirely.
See tailscale#7557