SSH connection hand-over #38545
SSH connection hand-over #38545
Conversation
lib/resumption/handover_unix.go
Outdated
| } | ||
| } | ||
|
|
||
| paths, firstErr := filterNonConnectableSockets(ctx, paths) |
There was a problem hiding this comment.
Got tripped up by the function naming here, which to me sounds like you're filtering out non-connectable sockets, consider chnaging to retainNonConnectableSockets or filterOutConnectableSockets
There was a problem hiding this comment.
Shouldn't we check firstErr for the case where len(paths) >= 1 but we got an error (i.e. we got an error after having already added some non-connectable sockets to paths)?
There was a problem hiding this comment.
We return (and then log into the void) an aggregate including firstErr, but we might as well clean up the paths that we got that we know are from sockets that exist and returned ECONNREFUSED before returning - a partial cleanup is better than no cleanup.
lib/resumption/handover_unix.go
Outdated
| lastErr = trace.ConvertSystemError(err) | ||
| } | ||
|
|
||
| return filtered, lastErr |
There was a problem hiding this comment.
It looks like we only return lastErr = trace.ConvertSystemError(err) for errors that aren't os.ErrNotExist or syscall.ECONNREFUSED if they occurred on the last item in paths, which seems like a bug since there's nothing significant about the last item.
There was a problem hiding this comment.
I don't think it'd be super useful to collect all the errors, this is basically just to log the error to identify potential issues during cleanup (say, after changing the user that runs Teleport or something like that). That's why I'm only keeping the last one that occurred.
There was a problem hiding this comment.
That reasoning doesn't really make sense to me, if we want to identify potential issues during cleanup then we'd want to see all the errors in case there were multiple different ones.
If you're adamant that this is the correct approach then I won't fight you further, but you should at least add a comment explaining why you're doing something non-standard.
There was a problem hiding this comment.
I'm just not sure of what'll happen when we aggregate a potentially unbound amount of errors; I'm already kind of spooked by the potential for weirdness as we aggregate errors for all the Remove()s at the end of the cleanup function, and there's going to be a lot more existing sockets than sockets we decide to remove.
There was a problem hiding this comment.
I've capped the amount of errors when aggregating in f434358, in both retainNonConnectableSockets and handoverCleanup. WDYT?
|
|
||
| func (r *SSHServerWrapper) resumeConnection(entry *connEntry, conn net.Conn, remoteIP netip.Addr) { | ||
| if entry.remoteIP != remoteIP { | ||
| r.log.Warn("Resumable connection attempted resumption from a different remote address.") |
There was a problem hiding this comment.
nit: isn't this an r.log.Error?
There was a problem hiding this comment.
I'm a bit reluctant to run it as an error because it can kind of happen naturally even if there's no bad actors involved, just as a result of changing network conditions.
| var remoteIP netip.Addr | ||
| if t, _ := conn.RemoteAddr().(*net.TCPAddr); t != nil { | ||
| remoteIP, _ = netip.AddrFromSlice(t.IP) | ||
| } | ||
| remoteIP16 := remoteIP.As16() |
There was a problem hiding this comment.
| var remoteIP netip.Addr | |
| if t, _ := conn.RemoteAddr().(*net.TCPAddr); t != nil { | |
| remoteIP, _ = netip.AddrFromSlice(t.IP) | |
| } | |
| remoteIP16 := remoteIP.As16() | |
| var remoteIP netip.Addr | |
| if t, ok := conn.RemoteAddr().(*net.TCPAddr); ok { | |
| remoteIP, ok = netip.AddrFromSlice(t.IP) | |
| if !ok { | |
| r.log.Error("Failed to convert TCPAddr IP to netip.Addr") | |
| return | |
| } | |
| } else { | |
| r.log.Warn("Remote address is not a *net.TCPAddr, handover failed") | |
| return | |
| } | |
| remoteIP16 := remoteIP.As16() |
There was a problem hiding this comment.
I'm comfortable leaving the remote IP slice as all zeroes and let the server decide if that's good enough or not, rather than prohibit it in the client side.
public-teleport-github-review-bot
bot
removed the request for review
from kimlisa
|
@espadolini See the table below for backport results.
|
This PR implements SSH connection hand-over, allowing SSH connections to be resumed across different Teleport processes during a graceful shutdown procedure.
Implements part of #38183
Part of #30415
changelog: SSH connection resumption will now work during graceful upgrades of the Teleport agent