Skip to content

Commit

Permalink
fix: use independent deadline for sending error (#79)
Browse files Browse the repository at this point in the history
* fix: use independent deadline for sending error
* tests: adapt unit tests to cover this case

The other side of the connection relies on this error message being
received in order to close the connection, indefinitely keeping it open
otherwise. Taking this into account, this should be considered a control
message, not part of the connection but the session instead.
However, since c.writeDeadline is being used (which is indeed controlled
via net.Conn interface), the upper crypto/tls.Conn is setting this
deadline to an invalid value in order to prevent further writes.
This causes the error messages to never be sent.
  • Loading branch information
aruiz14 committed Jul 1, 2024
1 parent 8acbd20 commit 40afe79
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 3 deletions.
3 changes: 2 additions & 1 deletion connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ func (c *connection) writeErr(err error) {
if err != nil {
msg := newErrorMessage(c.connID, err)
metrics.AddSMTotalTransmitErrorBytesOnWS(c.session.clientKey, float64(len(msg.Bytes())))
if _, err2 := c.session.writeMessage(c.writeDeadline, msg); err2 != nil {
deadline := time.Now().Add(SendErrorTimeout)
if _, err2 := c.session.writeMessage(deadline, msg); err2 != nil {
logrus.Warnf("[%d] encountered error %q while writing error %q to close remotedialer", c.connID, err2, err)
}
}
Expand Down
10 changes: 8 additions & 2 deletions session_serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ func TestSession_closeConnection(t *testing.T) {
s := setupDummySession(t, 0)
var msg *message
s.conn = &fakeWSConn{
writeMessageCallback: func(msgType int, _ time.Time, data []byte) (err error) {
writeMessageCallback: func(msgType int, deadline time.Time, data []byte) (err error) {
if !deadline.IsZero() && deadline.Before(time.Now()) {
return errors.New("deadline exceeded")
}
msg, err = newServerMessage(bytes.NewReader(data))
return
},
Expand All @@ -118,13 +121,16 @@ func TestSession_closeConnection(t *testing.T) {
conn := newConnection(connID, s, "test", "test")
s.addConnection(connID, conn)

// Ensure Error message is sent regardless of the WriteDeadline value, see https://github.com/rancher/remotedialer/pull/79
_ = conn.SetWriteDeadline(time.Now())

expectedErr := errors.New("connection closed")
s.closeConnection(connID, expectedErr)

if s.getConnection(connID) != nil {
t.Errorf("connection was not closed correctly")
}
if conn.err == nil {
if conn.err == nil || msg == nil {
t.Fatal("message not sent on closed connection")
} else if msg.messageType != Error {
t.Errorf("incorrect message type sent")
Expand Down
2 changes: 2 additions & 0 deletions types.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,6 @@ const (
SyncConnectionsTimeout = 60 * time.Second
MaxRead = 8192
HandshakeTimeOut = 10 * time.Second
// SendErrorTimeout sets the maximum duration for sending an error message to close a single connection
SendErrorTimeout = 5 * time.Second
)

0 comments on commit 40afe79

Please sign in to comment.