Merge pull request #819 from aryan9600/remove-unmanaged-transport

libgit2: decommission unmanaged transport

Author: darkowlzz

controllers/gitrepository_controller.go

@@ -56,7 +56,6 @@ import (
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
 	"github.com/fluxcd/source-controller/internal/util"
 	"github.com/fluxcd/source-controller/pkg/git"
-	"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
 	"github.com/fluxcd/source-controller/pkg/git/strategy"
 	"github.com/fluxcd/source-controller/pkg/sourceignore"
 )
@@ -116,6 +115,9 @@ type GitRepositoryReconciler struct {
 
 	Storage        *Storage
 	ControllerName string
+	// Libgit2TransportInitialized lets the reconciler know whether
+	// libgit2 transport was intialized successfully.
+	Libgit2TransportInitialized func() bool
 
 	requeueDependency time.Duration
 	features          map[string]bool
@@ -428,6 +430,12 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context,
 // change, it short-circuits the whole reconciliation with an early return.
 func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 	obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) {
+	// Exit early, if we need to use libgit2 AND managed transport hasn't been intialized.
+	if !r.Libgit2TransportInitialized() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
+		return sreconcile.ResultEmpty, serror.NewStalling(
+			errors.New("libgit2 managed transport not initialized"), "Libgit2TransportNotEnabled",
+		)
+	}
 	// Configure authentication strategy to access the source
 	var authOpts *git.AuthOptions
 	var err error
@@ -745,8 +753,8 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 		return nil, e
 	}
 
-	// managed GIT transport only affects the libgit2 implementation
-	if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
+	// this is needed only for libgit2, due to managed transport.
+	if obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
 		// We set the TransportOptionsURL of this set of authentication options here by constructing
 		// a unique URL that won't clash in a multi tenant environment. This unique URL is used by
 		// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in

controllers/gitrepository_controller_test.go

@@ -62,6 +62,7 @@ import (
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
 	"github.com/fluxcd/source-controller/pkg/git"
+	"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
 )
 
 const (
@@ -149,6 +150,10 @@ var (
 	testGitImplementations = []string{sourcev1.GoGitImplementation, sourcev1.LibGit2Implementation}
 )
 
+func mockTransportNotInitialized() bool {
+	return false
+}
+
 func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	g := NewWithT(t)
 
@@ -504,10 +509,11 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 			}
 
 			r := &GitRepositoryReconciler{
-				Client:        builder.Build(),
-				EventRecorder: record.NewFakeRecorder(32),
-				Storage:       testStorage,
-				features:      features.FeatureGates(),
+				Client:                      builder.Build(),
+				EventRecorder:               record.NewFakeRecorder(32),
+				Storage:                     testStorage,
+				features:                    features.FeatureGates(),
+				Libgit2TransportInitialized: managed.Enabled,
 			}
 
 			for _, i := range testGitImplementations {
@@ -544,6 +550,40 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 	}
 }
 
+func TestGitRepositoryReconciler_reconcileSource_libgit2TransportUninitialized(t *testing.T) {
+	g := NewWithT(t)
+
+	r := &GitRepositoryReconciler{
+		Client:                      fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(),
+		EventRecorder:               record.NewFakeRecorder(32),
+		Storage:                     testStorage,
+		features:                    features.FeatureGates(),
+		Libgit2TransportInitialized: mockTransportNotInitialized,
+	}
+
+	obj := &sourcev1.GitRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: "libgit2-transport",
+		},
+		Spec: sourcev1.GitRepositorySpec{
+			Interval: metav1.Duration{Duration: interval},
+			Timeout:  &metav1.Duration{Duration: timeout},
+			Reference: &sourcev1.GitRepositoryRef{
+				Branch: git.DefaultBranch,
+			},
+			GitImplementation: sourcev1.LibGit2Implementation,
+		},
+	}
+
+	tmpDir := t.TempDir()
+	var commit git.Commit
+	var includes artifactSet
+	_, err := r.reconcileSource(ctx, obj, &commit, &includes, tmpDir)
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err).To(BeAssignableToTypeOf(&serror.Stalling{}))
+	g.Expect(err.Error()).To(Equal("libgit2 managed transport not initialized"))
+}
+
 func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) {
 	g := NewWithT(t)
 
@@ -702,10 +742,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 	}
 
 	r := &GitRepositoryReconciler{
-		Client:        fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(),
-		EventRecorder: record.NewFakeRecorder(32),
-		Storage:       testStorage,
-		features:      features.FeatureGates(),
+		Client:                      fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(),
+		EventRecorder:               record.NewFakeRecorder(32),
+		Storage:                     testStorage,
+		features:                    features.FeatureGates(),
+		Libgit2TransportInitialized: managed.Enabled,
 	}
 
 	for _, tt := range tests {
@@ -1563,10 +1604,11 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) {
 			builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).WithObjects(obj)
 
 			r := &GitRepositoryReconciler{
-				Client:        builder.Build(),
-				EventRecorder: record.NewFakeRecorder(32),
-				Storage:       testStorage,
-				features:      features.FeatureGates(),
+				Client:                      builder.Build(),
+				EventRecorder:               record.NewFakeRecorder(32),
+				Storage:                     testStorage,
+				features:                    features.FeatureGates(),
+				Libgit2TransportInitialized: managed.Enabled,
 			}
 
 			key := client.ObjectKeyFromObject(obj)

controllers/suite_test.go

@@ -37,7 +37,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	"github.com/fluxcd/pkg/runtime/controller"
-	feathelper "github.com/fluxcd/pkg/runtime/features"
 	"github.com/fluxcd/pkg/runtime/testenv"
 	"github.com/fluxcd/pkg/testserver"
 	"github.com/phayes/freeport"
@@ -206,16 +205,17 @@ func TestMain(m *testing.M) {
 		panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
 	}
 
-	fg := feathelper.FeatureGates{}
-	fg.SupportedFeatures(features.FeatureGates())
-	managed.InitManagedTransport()
+	if err = managed.InitManagedTransport(); err != nil {
+		panic(fmt.Sprintf("Failed to initialize libgit2 managed transport: %v", err))
+	}
 
 	if err := (&GitRepositoryReconciler{
-		Client:        testEnv,
-		EventRecorder: record.NewFakeRecorder(32),
-		Metrics:       testMetricsH,
-		Storage:       testStorage,
-		features:      features.FeatureGates(),
+		Client:                      testEnv,
+		EventRecorder:               record.NewFakeRecorder(32),
+		Metrics:                     testMetricsH,
+		Storage:                     testStorage,
+		features:                    features.FeatureGates(),
+		Libgit2TransportInitialized: managed.Enabled,
 	}).SetupWithManager(testEnv); err != nil {
 		panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err))
 	}

docs/spec/v1beta2/gitrepositories.md

@@ -405,18 +405,6 @@ 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).
 
-#### Managed transport for `libgit2` Git implementation
-
-The `libgit2` Git implementation supports a new managed transport for
-improved reliability, adding timeout enforcement for Git network operations.
-
-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
 
 Optimized Git clones decreases resource utilization for GitRepository
@@ -432,9 +420,6 @@ usual.
 
 This feature is enabled by default. It can be disabled by starting the
 controller with the argument `--feature-gates=OptimizedGitClones=false`.
-Please note that this feature is only active when managed transport for
-`libgit2` is active. Disabling managed transport for `libgit2` automatically
-disables this feature.
 
 NB: GitRepository objects configured for SemVer or Commit clones are
 not affected by this functionality.

internal/features/features.go

@@ -30,25 +30,12 @@ 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

@@ -204,12 +204,18 @@ func main() {
 	}
 	storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, setupLog)
 
+	if err = managed.InitManagedTransport(); err != nil {
+		// Log the error, but don't exit so as to not block reconcilers that are healthy.
+		setupLog.Error(err, "unable to initialize libgit2 managed transport")
+	}
+
 	if err = (&controllers.GitRepositoryReconciler{
-		Client:         mgr.GetClient(),
-		EventRecorder:  eventRecorder,
-		Metrics:        metricsH,
-		Storage:        storage,
-		ControllerName: controllerName,
+		Client:                      mgr.GetClient(),
+		EventRecorder:               eventRecorder,
+		Metrics:                     metricsH,
+		Storage:                     storage,
+		ControllerName:              controllerName,
+		Libgit2TransportInitialized: managed.Enabled,
 	}).SetupWithManagerAndOptions(mgr, controllers.GitRepositoryReconcilerOptions{
 		MaxConcurrentReconciles:   concurrent,
 		DependencyRequeueInterval: requeueDependency,
@@ -310,17 +316,6 @@ func main() {
 		startFileServer(storage.BasePath, storageAddr, setupLog)
 	}()
 
-	if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
-		managed.InitManagedTransport()
-	} else {
-		if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize {
-			features.Disable(features.OptimizedGitClones)
-			setupLog.Info(
-				"disabling optimized git clones; git clones can only be optimized when using managed transport",
-			)
-		}
-	}
-
 	setupLog.Info("starting manager")
 	if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
 		setupLog.Error(err, "problem running manager")