From 02e25c43207829587eb7f5128a54bf98dc7f1bf3 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 10 Mar 2022 12:41:15 +0100 Subject: [PATCH 1/3] konn-client: Simplify DIAL_RSP case The else is pointless because the true branch has a return. Let's just remove the else and identation for better readability. There is no behavior change, this change is cosmetical. --- konnectivity-client/pkg/client/client.go | 38 ++++++++++++------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/konnectivity-client/pkg/client/client.go b/konnectivity-client/pkg/client/client.go index f901941b3..f3dc1b6d5 100644 --- a/konnectivity-client/pkg/client/client.go +++ b/konnectivity-client/pkg/client/client.go @@ -224,25 +224,25 @@ func (t *grpcTunnel) serve(tunnelCtx context.Context) { // In either scenario, we should return here and close the tunnel as it is no longer needed. klog.V(1).InfoS("DialResp not recognized; dropped", "connectionID", resp.ConnectID, "dialID", resp.Random) return - } else { - result := dialResult{connid: resp.ConnectID} - if resp.Error != "" { - result.err = &dialFailure{resp.Error, DialFailureEndpoint} - } - select { - // try to send to the result channel - case pendingDial.resultCh <- result: - // unblock if the cancel channel is closed - case <-pendingDial.cancelCh: - // Note: this condition can only be hit by a race condition where the - // DialContext() returns early (timeout) after the pendingDial is already - // fetched here, but before the result is sent. - klog.V(1).InfoS("Pending dial has been cancelled; dropped", "connectionID", resp.ConnectID, "dialID", resp.Random) - return - case <-tunnelCtx.Done(): - klog.V(1).InfoS("Tunnel has been closed; dropped", "connectionID", resp.ConnectID, "dialID", resp.Random) - return - } + } + + result := dialResult{connid: resp.ConnectID} + if resp.Error != "" { + result.err = &dialFailure{resp.Error, DialFailureEndpoint} + } + select { + // try to send to the result channel + case pendingDial.resultCh <- result: + // unblock if the cancel channel is closed + case <-pendingDial.cancelCh: + // Note: this condition can only be hit by a race condition where the + // DialContext() returns early (timeout) after the pendingDial is already + // fetched here, but before the result is sent. + klog.V(1).InfoS("Pending dial has been cancelled; dropped", "connectionID", resp.ConnectID, "dialID", resp.Random) + return + case <-tunnelCtx.Done(): + klog.V(1).InfoS("Tunnel has been closed; dropped", "connectionID", resp.ConnectID, "dialID", resp.Random) + return } if resp.Error != "" { From 37c6957754c31cb06abdbfc812dd31912ccd674f Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 10 Mar 2022 12:43:10 +0100 Subject: [PATCH 2/3] konn-client: Simplify DATA case Instead of having all the code inside an if, and outside just a log for the "uninteresting" case, let's just check if the conection is not recognize and remove the identation for the rest. There is no behavior change, this change is cosmetical. Signed-off-by: Rodrigo Campos --- konnectivity-client/pkg/client/client.go | 26 ++++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/konnectivity-client/pkg/client/client.go b/konnectivity-client/pkg/client/client.go index f3dc1b6d5..bdaa75a81 100644 --- a/konnectivity-client/pkg/client/client.go +++ b/konnectivity-client/pkg/client/client.go @@ -281,19 +281,19 @@ func (t *grpcTunnel) serve(tunnelCtx context.Context) { // TODO: flow control conn, ok := t.conns.get(resp.ConnectID) - if ok { - timer := time.NewTimer((time.Duration)(t.readTimeoutSeconds) * time.Second) - select { - case conn.readCh <- resp.Data: - timer.Stop() - case <-timer.C: - klog.ErrorS(fmt.Errorf("timeout"), "readTimeout has been reached, the grpc connection to the proxy server will be closed", "connectionID", conn.connID, "readTimeoutSeconds", t.readTimeoutSeconds) - return - case <-tunnelCtx.Done(): - klog.V(1).InfoS("Tunnel has been closed, the grpc connection to the proxy server will be closed", "connectionID", conn.connID) - } - } else { - klog.V(1).InfoS("connection not recognized", "connectionID", resp.ConnectID) + if !ok { + klog.V(1).InfoS("Connection not recognized", "connectionID", resp.ConnectID) + continue + } + timer := time.NewTimer((time.Duration)(t.readTimeoutSeconds) * time.Second) + select { + case conn.readCh <- resp.Data: + timer.Stop() + case <-timer.C: + klog.ErrorS(fmt.Errorf("timeout"), "readTimeout has been reached, the grpc connection to the proxy server will be closed", "connectionID", conn.connID, "readTimeoutSeconds", t.readTimeoutSeconds) + return + case <-tunnelCtx.Done(): + klog.V(1).InfoS("Tunnel has been closed, the grpc connection to the proxy server will be closed", "connectionID", conn.connID) } case client.PacketType_CLOSE_RSP: From 9f2918c2977a90027a3836c9f4ecbbb8595b941b Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 10 Mar 2022 12:45:49 +0100 Subject: [PATCH 3/3] konn-client: Simplify CLOSE_RSP case Again, let's move the code out of the identation and just filter error cases out first. There is no behavior change, this change is cosmetical. Signed-off-by: Rodrigo Campos --- konnectivity-client/pkg/client/client.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/konnectivity-client/pkg/client/client.go b/konnectivity-client/pkg/client/client.go index bdaa75a81..c55070ae6 100644 --- a/konnectivity-client/pkg/client/client.go +++ b/konnectivity-client/pkg/client/client.go @@ -300,14 +300,15 @@ func (t *grpcTunnel) serve(tunnelCtx context.Context) { resp := pkt.GetCloseResponse() conn, ok := t.conns.get(resp.ConnectID) - if ok { - close(conn.readCh) - conn.closeCh <- resp.Error - close(conn.closeCh) - t.conns.remove(resp.ConnectID) - return + if !ok { + klog.V(1).InfoS("Connection not recognized", "connectionID", resp.ConnectID) + continue } - klog.V(1).InfoS("connection not recognized", "connectionID", resp.ConnectID) + close(conn.readCh) + conn.closeCh <- resp.Error + close(conn.closeCh) + t.conns.remove(resp.ConnectID) + return } } }