From 978148ea7139cde9358f1d6fd0025241e937e6f1 Mon Sep 17 00:00:00 2001
From: Paulo Gomes <paulo.gomes@weave.works>
Date: Fri, 27 May 2022 13:44:59 +0100
Subject: [PATCH] libgit2: enforce context timeout Some scenarios could lead a
 goroutine to be running indefinetely within managed ssh. Previously between
 the two git operations, the reconciliation could take twice the timeout set
 for the Flux object.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
---
 controllers/gitrepository_controller.go       |  9 +++---
 pkg/git/libgit2/checkout.go                   |  1 +
 pkg/git/libgit2/managed/options.go            |  2 ++
 pkg/git/libgit2/managed/ssh.go                | 32 +++++++++++++++----
 pkg/git/strategy/proxy/strategy_proxy_test.go |  2 +-
 5 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go
index 64426676c..531983b20 100644
--- a/controllers/gitrepository_controller.go
+++ b/controllers/gitrepository_controller.go
@@ -721,7 +721,10 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 		}
 	}
 
-	checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(ctx,
+	gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
+	defer cancel()
+
+	checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(gitCtx,
 		git.Implementation(obj.Spec.GitImplementation), checkoutOpts)
 	if err != nil {
 		// Do not return err as recovery without changes is impossible.
@@ -753,10 +756,6 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 		}
 	}
 
-	// Checkout HEAD of reference in object
-	gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
-	defer cancel()
-
 	commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
 	if err != nil {
 		e := serror.NewGeneric(
diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go
index 3c49633bd..3f58e2397 100644
--- a/pkg/git/libgit2/checkout.go
+++ b/pkg/git/libgit2/checkout.go
@@ -91,6 +91,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
 			TargetURL:    url,
 			AuthOpts:     opts,
 			ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
+			Context:      ctx,
 		})
 		url = opts.TransportOptionsURL
 		remoteCallBacks := managed.RemoteCallbacks()
diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go
index 3af0d914b..faa1f07b9 100644
--- a/pkg/git/libgit2/managed/options.go
+++ b/pkg/git/libgit2/managed/options.go
@@ -17,6 +17,7 @@ limitations under the License.
 package managed
 
 import (
+	"context"
 	"sync"
 
 	"github.com/fluxcd/source-controller/pkg/git"
@@ -29,6 +30,7 @@ type TransportOptions struct {
 	TargetURL    string
 	AuthOpts     *git.AuthOptions
 	ProxyOptions *git2go.ProxyOptions
+	Context      context.Context
 }
 
 var (
diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go
index ca0e02e3e..543d3ceb3 100644
--- a/pkg/git/libgit2/managed/ssh.go
+++ b/pkg/git/libgit2/managed/ssh.go
@@ -92,6 +92,7 @@ type sshSmartSubtransport struct {
 	currentStream *sshSmartSubtransportStream
 	addr          string
 	connected     bool
+	ctx           context.Context
 }
 
 func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) {
@@ -103,6 +104,8 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 		return nil, fmt.Errorf("could not find transport options for object: %s", transportOptionsURL)
 	}
 
+	t.ctx = opts.Context
+
 	u, err := url.Parse(opts.TargetURL)
 	if err != nil {
 		return nil, err
@@ -206,16 +209,33 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	// xref: https://github.com/golang/crypto/blob/eb4f295cb31f7fb5d52810411604a2638c9b19a2/ssh/session.go#L553-L558
 	go func() error {
 		defer w.Close()
+
+		var cancel context.CancelFunc
+		ctx := t.ctx
+
+		// When context is nil, creates a new with internal SSH connection timeout.
+		if ctx == nil {
+			ctx, cancel = context.WithTimeout(context.Background(), sshConnectionTimeOut)
+			defer cancel()
+		}
+
 		for {
-			if !t.connected {
+			select {
+			case <-ctx.Done():
+				t.Close()
 				return nil
-			}
 
-			_, err := io.Copy(w, reader)
-			if err != nil {
-				return err
+			default:
+				if !t.connected {
+					return nil
+				}
+
+				_, err := io.Copy(w, reader)
+				if err != nil {
+					return err
+				}
+				time.Sleep(5 * time.Millisecond)
 			}
-			time.Sleep(5 * time.Millisecond)
 		}
 	}()
 
diff --git a/pkg/git/strategy/proxy/strategy_proxy_test.go b/pkg/git/strategy/proxy/strategy_proxy_test.go
index e575cd37e..5f9573793 100644
--- a/pkg/git/strategy/proxy/strategy_proxy_test.go
+++ b/pkg/git/strategy/proxy/strategy_proxy_test.go
@@ -292,7 +292,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) {
 
 				return nil, func() {}
 			},
-			shortTimeout:  false,
+			shortTimeout:  true,
 			wantUsedProxy: false,
 			wantError:     true,
 		},