Merge pull request #718 from pjbgf/default-managed

libgit2: enable managed transport by default

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
+This feature is enabled by default. It can be disabled by starting the
-it will fix the bug in which Git operations make the controllers hang indefinitely.
+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

@@ -1,34 +0,0 @@
-/*
-Copyright 2022 The Flux authors
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package managed
-
-import (
-	"os"
-	"strings"
-)
-
-// Enabled defines whether the use of Managed Transport should be enabled.
-// This is only affects git operations that uses libgit2 implementation.
-//
-// True is returned when the environment variable `EXPERIMENTAL_GIT_TRANSPORT`
-// is detected with the value of `true` or `1`.
-func Enabled() bool {
-	if v, ok := os.LookupEnv("EXPERIMENTAL_GIT_TRANSPORT"); ok {
-		return strings.ToLower(v) == "true" || v == "1"
-	}
-	return false
-}

pkg/git/libgit2/managed/init.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/managed_test.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)