Improving how headscale handles the client startup process #31
Conversation
There was a problem hiding this comment.
I tried the code, and it seems to work, so that's good! With the tweaks below it should be good to merge.
Judging by the tailscale source, we should be getting three requests in quick succession on startup:
- ReadOnly=true, OmitPeers=true, Stream=false (aka "give me the DERP map, ignore my endpoint update, it is empty/meaningless")
- ReadOnly=false, OmitPeers=true, Stream=false (aka "here's my endpoint update, don't stream to me")
- ReadOnly=false, OmitPeers=false, Stream=true (aka the HTTP Long Poll connection)
It seems that the client sometimes remembers the DERP map from a previous invocation, which means it skips the first request.
api.go
Outdated
| return | ||
| } | ||
| if req.OmitPeers { | ||
| log.Printf("[%s] Client is starting up. Ready to receive the peers", m.Name) |
There was a problem hiding this comment.
Based on the comments from the tailscale source, this log line isn't quite right. How about something like this instead:
log.Printf("[%s] Client sent endpoint update and is ok with a response without peer list.", m.Name)
Arguably this log messing is kind of meaningless in day to day operation; this would be a good candidate for a debug level log message I think.
There was a problem hiding this comment.
Fully agree. This messages should be disabled once we are confident with our interpretation of the protocol.
There was a problem hiding this comment.
Fully agree. This messages should be disabled once we are confident with our interpretation of the protocol.
api.go
Outdated
| c.Data(200, "application/json; charset=utf-8", *data) | ||
| return | ||
| } | ||
| if req.OmitPeers { |
There was a problem hiding this comment.
I think that the tailscale client currently only uses the combination of OmitPeers == true and Stream == false, and logically that seems to be the intention, but we should probably be explicit about that expectation How about we change this if to
if req.OmitPeers && ! req.Stream {
...
} else if req.OmitPeers && req.Stream {
log.Printf("[%s] Warning, ignoring request, don't know how to handle it", m.Name)
// send 400 error code back with empty json response
return
}
|
Let's merge then! |
As discussed in Gitter (https://gitter.im/headscale-dev/community?at=60a9618c801b07264e631b7f), there is some weirdness in how the tailscale clients first interact with the control server in the
/mapendpoint.The client uses
tailcfg.MapRequestto indicate the different stages of its startup process. At the beginning of the interaction, the client just expects an updatedtailcfg.DERPMap- not the full list of its peers. It will close the connection as soon as it receives the firsttailcfg.MapResponse.With this PR headscale closes the connection when the client is expecting so, in these initial interactions to
/map. In these stages it does not start the keepAlives either.I don't think this is completely solved, so I added a few more logging messages to track this.