logpolicy: allow longer idle log upload connections #11167
Conversation
From a packet trace we have seen log connections being closed prematurely by the client, resulting in unnecessary extra TLS setup traffic. Updates #3363 Updates tailscale/corp#9230 Updates tailscale/corp#8564 Signed-off-by: James Tucker <james@tailscale.com>
raggi
force-pushed
the
raggi/log-idle-conn
branch
from
2a293ab
to
f5b8a25
Compare
| // We're uploading logs ideally infrequently, with specific timing that will | ||
| // change over time. Try to keep the connection open, to avoid repeatedly | ||
| // paying the cost of TLS setup. | ||
| tr.IdleConnTimeout = time.Hour |
There was a problem hiding this comment.
Presumably I need to make the equivalent change on the server side, right?
If the server closes a idle connection after the default of 90s, then this is effectively the same as before?
There was a problem hiding this comment.
Yeah, there's more considerations server side which we'll discuss over there :D
| // change over time. Try to keep the connection open, to avoid repeatedly | ||
| // paying the cost of TLS setup. | ||
| tr.IdleConnTimeout = time.Hour | ||
|
|
There was a problem hiding this comment.
Do we want to explicitly set tr.DisableKeepAlives to false with a comment that we want to make use of HTTP keep-alives, which allows us to make back-to-back requests on the same connection?
There was a problem hiding this comment.
Also, did we want to disable TCP keep alives in the same PR?
There was a problem hiding this comment.
It's set to false by default, I can't currently imagine a world where that default is going to change
There was a problem hiding this comment.
They already are (for mobile platforms), as a result of the netknob, but it happens in the dialer
There was a problem hiding this comment.
My intent wasn't so much about changing the value, but documenting that we rely on the implicit default for performance reasons.
From a packet trace we have seen log connections being closed prematurely by the client, resulting in unnecessary extra TLS setup traffic.
Updates #3363
Updates tailscale/corp#9230
Updates tailscale/corp#8564