fix panics on unmanaged http and proxy on managed http

Signed-off-by: Sanskar Jaiswal <[email protected]>

Author: aryan9600

controllers/gitrepository_controller.go

@@ -455,21 +455,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 		return sreconcile.ResultEmpty, e
 	}
 
-	repositoryURL := obj.Spec.URL
-	// managed GIT transport only affects the libgit2 implementation
-	if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
-		// We set the TransportAuthID of this set of authentication options here by constructing
-		// a unique ID that won't clash in a multi tenant environment. This unique ID is used by
-		// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in
-		// libgit2, which is inflexible and unstable.
-		if strings.HasPrefix(repositoryURL, "http") {
-			authOpts.TransportAuthID = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
-		}
-		if strings.HasPrefix(repositoryURL, "ssh") {
-			authOpts.TransportAuthID = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
-		}
-	}
-
 	// Fetch the included artifact metadata.
 	artifacts, err := r.fetchIncludes(ctx, obj)
 	if err != nil {
@@ -491,7 +476,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 		optimizedClone = true
 	}
 
-	c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, optimizedClone)
+	c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone)
 	if err != nil {
 		return sreconcile.ResultEmpty, err
 	}
@@ -525,7 +510,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 
 		// If we can't skip the reconciliation, checkout again without any
 		// optimization.
-		c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, false)
+		c, err := r.gitCheckout(ctx, obj, authOpts, dir, false)
 		if err != nil {
 			return sreconcile.ResultEmpty, err
 		}
@@ -717,7 +702,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
 // gitCheckout builds checkout options with the given configurations and
 // performs a git checkout.
 func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
-	obj *sourcev1.GitRepository, repoURL string, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
+	obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
 	// Configure checkout strategy.
 	checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules}
 	if ref := obj.Spec.Reference; ref != nil {
@@ -743,15 +728,34 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 			Err:    fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
 			Reason: sourcev1.GitOperationFailedReason,
 		}
-		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		// Do not return err as recovery without changes is impossible.
 		return nil, e
 	}
 
+	// managed GIT transport only affects the libgit2 implementation
+	if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
+		// We set the TransportOptionsURL of this set of authentication options here by constructing
+		// a unique ID that won't clash in a multi tenant environment. This unique ID is used by
+		// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in
+		// libgit2, which is inflexible and unstable.
+		if strings.HasPrefix(obj.Spec.URL, "http") {
+			authOpts.TransportOptionsURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
+		} else if strings.HasPrefix(obj.Spec.URL, "ssh") {
+			authOpts.TransportOptionsURL = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
+		} else {
+			e := &serror.Stalling{
+				Err:    fmt.Errorf("git repository URL has invalid transport type: '%s'", obj.Spec.URL),
+				Reason: sourcev1.GitOperationFailedReason,
+			}
+			return nil, e
+		}
+	}
+
 	// Checkout HEAD of reference in object
 	gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
-	commit, err := checkoutStrategy.Checkout(gitCtx, dir, repoURL, authOpts)
+
+	commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
 	if err != nil {
 		e := serror.NewGeneric(
 			fmt.Errorf("failed to checkout and determine revision: %w", err),

controllers/gitrepository_controller_test.go

@@ -362,7 +362,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 			},
 			wantErr: true,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to fetch-connect to remote '<url>': PEM CA bundle could not be appended to x509 certificate pool"),
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to clone '<url>': PEM CA bundle could not be appended to x509 certificate pool"),
 			},
 		},
 		{
@@ -645,10 +645,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 				}
 				conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo")
 			},
-			want:                 sreconcile.ResultEmpty,
-			wantErr:              true,
-			wantRevision:         "staging/<commit>",
-			wantArtifactOutdated: false,
+			want:                  sreconcile.ResultEmpty,
+			wantErr:               true,
+			wantRevision:          "staging/<commit>",
+			wantArtifactOutdated:  false,
+			skipForImplementation: "libgit2",
 		},
 		{
 			name: "Optimized clone different ignore",
@@ -669,9 +670,10 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 				}
 				conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo")
 			},
-			want:                 sreconcile.ResultSuccess,
-			wantRevision:         "staging/<commit>",
-			wantArtifactOutdated: false,
+			want:                  sreconcile.ResultSuccess,
+			wantRevision:          "staging/<commit>",
+			wantArtifactOutdated:  false,
+			skipForImplementation: "libgit2",
 		},
 	}
 

go.mod

@@ -28,7 +28,7 @@ require (
 	github.com/fluxcd/pkg/helmtestserver v0.5.0
 	github.com/fluxcd/pkg/lockedfile v0.1.0
 	github.com/fluxcd/pkg/runtime v0.16.0
-	github.com/fluxcd/pkg/ssh v0.3.4
+	github.com/fluxcd/pkg/ssh v0.4.0
 	github.com/fluxcd/pkg/testserver v0.2.0
 	github.com/fluxcd/pkg/untar v0.1.0
 	github.com/fluxcd/pkg/version v0.1.0

go.sum

@@ -350,8 +350,8 @@ github.com/fluxcd/pkg/lockedfile v0.1.0 h1:YsYFAkd6wawMCcD74ikadAKXA4s2sukdxrn7w
 github.com/fluxcd/pkg/lockedfile v0.1.0/go.mod h1:EJLan8t9MiOcgTs8+puDjbE6I/KAfHbdvIy9VUgIjm8=
 github.com/fluxcd/pkg/runtime v0.16.0 h1:ynzvkOedFFZHlsa47EE7XtxZe8qs8edhtmjVZBEWi1Y=
 github.com/fluxcd/pkg/runtime v0.16.0/go.mod h1:Iklg+r/Jnqc9cNf2NK+iaosvw49CxX07Pyn0r3zSg/o=
-github.com/fluxcd/pkg/ssh v0.3.4 h1:Ko+MUNiiQG3evyoMO19iRk7d4X0VJ6w6+GEeVQ1jLC0=
-github.com/fluxcd/pkg/ssh v0.3.4/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE=
+github.com/fluxcd/pkg/ssh v0.4.0 h1:2HY88irZ5BCSMlzZExR6cnhRkjxCDsK/lTHHQqCJDJQ=
+github.com/fluxcd/pkg/ssh v0.4.0/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE=
 github.com/fluxcd/pkg/testserver v0.2.0 h1:Mj0TapmKaywI6Fi5wvt1LAZpakUHmtzWQpJNKQ0Krt4=
 github.com/fluxcd/pkg/testserver v0.2.0/go.mod h1:bgjjydkXsZTeFzjz9Cr4heGANr41uTB1Aj1Q5qzuYVk=
 github.com/fluxcd/pkg/untar v0.1.0 h1:k97V/xV5hFrAkIkVPuv5AVhyxh1ZzzAKba/lbDfGo6o=

main.go

@@ -312,6 +312,14 @@ func main() {
 
 	if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
 		managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
+	} else {
+		if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize {
+			setupLog.Error(
+				fmt.Errorf("OptimizedGitClones=true but GitManagedTransport=false"),
+				"git clones can only be optimized when using managed transort",
+			)
+			os.Exit(1)
+		}
 	}
 
 	setupLog.Info("starting manager")