Merge pull request #718 from pjbgf/default-managed

libgit2: enable managed transport by default

Author: Paulo Gomes

docs/spec/v1beta2/gitrepositories.md

@@ -388,16 +388,17 @@ Some Git providers like Azure DevOps _require_ the `libgit2` implementation, as
 their Git servers provide only support for the
 [v2 protocol](https://git-scm.com/docs/protocol-v2).
 
-#### Experimental managed transport for `libgit2` Git implementation
+#### Managed transport for `libgit2` Git implementation
 
-The `libgit2` Git implementation supports a new experimental transport for
+The `libgit2` Git implementation supports a new managed transport for
 improved reliability, adding timeout enforcement for Git network operations.
-Opt-in by setting the environment variable `EXPERIMENTAL_GIT_TRANSPORT` to
-`true` in the controller's Deployment. This will result in the low-level
-transport being handled by the controller, instead of `libgit2`.
 
-This may lead to an increased number of timeout messages in the logs, however
-it will fix the bug in which Git operations make the controllers hang indefinitely.
+This feature is enabled by default. It can be disabled by starting the
+controller with the argument `--feature-gates=GitManagedTransport=false`.
+
+By disabling this feature the management of the transport is passed on to
+`libgit2`, which may result in blocking Git operations leading the controllers 
+to hang indefinitely.
 
 #### Optimized Git clones
 

internal/features/features.go

@@ -30,12 +30,25 @@ const (
 	// the last revision is still the same at the target repository,
 	// and if that is so, skips the reconciliation.
 	OptimizedGitClones = "OptimizedGitClones"
+
+	// GitManagedTransport implements a managed transport for GitRepository
+	// objects that use the libgit2 implementation.
+	//
+	// When enabled, improves the reliability of libgit2 reconciliations,
+	// by enforcing timeouts and ensuring libgit2 cannot hijack the process
+	// and hang it indefinitely.
+	GitManagedTransport = "GitManagedTransport"
 )
 
 var features = map[string]bool{
 	// OptimizedGitClones
 	// opt-out from v0.25
 	OptimizedGitClones: true,
+
+	// GitManagedTransport
+	// opt-in from v0.22 (via environment variable)
+	// opt-out from v0.25
+	GitManagedTransport: true,
 }
 
 // DefaultFeatureGates contains a list of all supported feature gates and

main.go

@@ -310,7 +310,7 @@ func main() {
 		startFileServer(storage.BasePath, storageAddr, setupLog)
 	}()
 
-	if managed.Enabled() {
+	if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
 		managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
 	}
 

pkg/git/libgit2/managed/flag.go

@@ -40,8 +40,18 @@ var (
 
 	debugLog logr.Logger
 	traceLog logr.Logger
+	enabled  bool
 )
 
+// Enabled defines whether the use of Managed Transport is enabled which
+// is only true if InitManagedTransport was called successfully at least
+// once.
+//
+// This is only affects git operations that uses libgit2 implementation.
+func Enabled() bool {
+	return enabled
+}
+
 // InitManagedTransport initialises HTTP(S) and SSH managed transport
 // for git2go, and therefore only impact git operations using the
 // libgit2 implementation.
@@ -57,7 +67,7 @@ func InitManagedTransport(log logr.Logger) error {
 	var err error
 
 	once.Do(func() {
-		log.Info("Enabling experimental managed transport")
+		log.Info("Initializing managed transport")
 		debugLog = log.V(logger.DebugLevel)
 		traceLog = log.V(logger.TraceLevel)
 
@@ -66,6 +76,7 @@ func InitManagedTransport(log logr.Logger) error {
 		}
 
 		err = registerManagedSSH()
+		enabled = true
 	})
 
 	return err

pkg/git/libgit2/managed/init.go

@@ -201,32 +201,6 @@ func TestOptions(t *testing.T) {
 	}
 }
 
-func TestFlagStatus(t *testing.T) {
-	if Enabled() {
-		t.Errorf("experimental transport should not be enabled by default")
-	}
-
-	os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true")
-	if !Enabled() {
-		t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=true")
-	}
-
-	os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "1")
-	if !Enabled() {
-		t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=1")
-	}
-
-	os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "somethingelse")
-	if Enabled() {
-		t.Errorf("experimental transport should be enabled only when env EXPERIMENTAL_GIT_TRANSPORT is 1 or true but was enabled for 'somethingelse'")
-	}
-
-	os.Unsetenv("EXPERIMENTAL_GIT_TRANSPORT")
-	if Enabled() {
-		t.Errorf("experimental transport should not be enabled when env EXPERIMENTAL_GIT_TRANSPORT is not present")
-	}
-}
-
 func TestManagedTransport_E2E(t *testing.T) {
 	g := NewWithT(t)