-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Add support for configurable keep-alives #2349
Conversation
20cbeb4
to
2dbaa01
Compare
@@ -38,13 +38,13 @@ import ( | |||
"github.com/gravitational/teleport/lib/utils" | |||
|
|||
"github.com/gravitational/trace" | |||
log "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's up with this refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to our new structured logging everywhere in lib/srv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does renaming to logrus help?
closeContext, closeCancel := context.WithCancel(context.Background()) | ||
defer closeCancel() | ||
|
||
clusterConfig, err := s.GetAccessPoint().GetClusterConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for the error handling logic and cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything should be cleaned up in a defer here.
// The keep-alive loop will keep pinging the remote server and after it has | ||
// missed a certain number of keep-alive requests it will cancel the | ||
// closeContext which signals the server to shutdown. | ||
go srv.StartKeepAliveLoop(srv.KeepAliveParams{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you pass a context to keepalive loop as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am passing a context to the keep-alive loop?
2dbaa01
to
337dc02
Compare
control how often the server sends keep-alive messages to clients and after how many missed keep-alive replies the server tears down the connection to the client.
337dc02
to
175ad7d
Compare
Purpose
Allow configurable keep-alives to be sent from Teleport servers to clients. This is to prevent firewalls, load balancers, and VPNs from tearing down idle SSH session.
Implementation
keep_alive_interval
to Teleport configuration. This sets the interval at which Teleport will send keep-alive messages. The default value mirrorssshd
at 15 minutes.keep_alive_count_max
, the number of messed keep-alive messages before the server tears down the connection to the client. The default value mirrorssshd
at 3.Related Issues
Fixes #2334