Merge pull request #944 from fluxcd/git-refactoring

Refactor Git operations and introduce `go-git` support for Azure DevOps and AWS CodeCommit

Author: Paulo Gomes

Dockerfile

@@ -3,7 +3,7 @@ ARG GO_VERSION=1.19
 ARG XX_VERSION=1.1.2
 
 ARG LIBGIT2_IMG=ghcr.io/fluxcd/golang-with-libgit2-only
-ARG LIBGIT2_TAG=v0.3.0
+ARG LIBGIT2_TAG=v0.4.0
 
 FROM ${LIBGIT2_IMG}:${LIBGIT2_TAG} AS libgit2-libs
 

Makefile

@@ -4,7 +4,7 @@ TAG ?= latest
 
 # Base image used to build the Go binary
 LIBGIT2_IMG ?= ghcr.io/fluxcd/golang-with-libgit2-only
-LIBGIT2_TAG ?= v0.3.0
+LIBGIT2_TAG ?= v0.4.0
 
 # Allows for defining additional Go test args, e.g. '-tags integration'.
 GO_TEST_ARGS ?= -race

controllers/gitrepository_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -41,6 +42,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/ratelimiter"
 
 	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/git"
+	"github.com/fluxcd/pkg/git/gogit"
+	"github.com/fluxcd/pkg/git/libgit2"
 	"github.com/fluxcd/pkg/runtime/conditions"
 	helper "github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/events"
@@ -54,8 +58,6 @@ import (
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"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/strategy"
 )
 
 // gitRepositoryReadyCondition contains the information required to summarize a
@@ -440,9 +442,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
 	}
 
-	// Configure authentication strategy to access the source
-	var authOpts *git.AuthOptions
-	var err error
+	var authData map[string][]byte
 	if obj.Spec.SecretRef != nil {
 		// Attempt to retrieve secret
 		name := types.NamespacedName{
@@ -459,20 +459,27 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 			// Return error as the world as observed may change
 			return sreconcile.ResultEmpty, e
 		}
+		authData = secret.Data
+	}
 
-		// Configure strategy with secret
-		authOpts, err = git.AuthOptionsFromSecret(obj.Spec.URL, &secret)
-	} else {
-		// Set the minimal auth options for valid transport.
-		authOpts, err = git.AuthOptionsWithoutSecret(obj.Spec.URL)
+	u, err := url.Parse(obj.Spec.URL)
+	if err != nil {
+		e := serror.NewStalling(
+			fmt.Errorf("failed to parse url '%s': %w", obj.Spec.URL, err),
+			sourcev1.URLInvalidReason,
+		)
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
 	}
+
+	// Configure authentication strategy to access the source
+	authOpts, err := git.NewAuthOptions(*u, authData)
 	if err != nil {
 		e := serror.NewGeneric(
-			fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
+			fmt.Errorf("failed to configure authentication options: %w", err),
 			sourcev1.AuthenticationFailedReason,
 		)
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-		// Return error as the contents of the secret may change
 		return sreconcile.ResultEmpty, e
 	}
 
@@ -725,59 +732,52 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
 func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 	obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
 	// Configure checkout strategy.
-	checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules}
+	cloneOpts := git.CloneOptions{
+		RecurseSubmodules: obj.Spec.RecurseSubmodules,
+		ShallowClone:      true,
+	}
 	if ref := obj.Spec.Reference; ref != nil {
-		checkoutOpts.Branch = ref.Branch
-		checkoutOpts.Commit = ref.Commit
-		checkoutOpts.Tag = ref.Tag
-		checkoutOpts.SemVer = ref.SemVer
+		cloneOpts.Branch = ref.Branch
+		cloneOpts.Commit = ref.Commit
+		cloneOpts.Tag = ref.Tag
+		cloneOpts.SemVer = ref.SemVer
 	}
 
 	// Only if the object has an existing artifact in storage, attempt to
 	// short-circuit clone operation. reconcileStorage has already verified
 	// that the artifact exists.
 	if optimized && conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) {
 		if artifact := obj.GetArtifact(); artifact != nil {
-			checkoutOpts.LastRevision = artifact.Revision
+			cloneOpts.LastObservedCommit = artifact.Revision
 		}
 	}
 
 	gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
 
-	checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(gitCtx,
-		git.Implementation(obj.Spec.GitImplementation), checkoutOpts)
+	var gitReader git.RepositoryReader
+	var err error
+
+	switch obj.Spec.GitImplementation {
+	case sourcev1.LibGit2Implementation:
+		gitReader, err = libgit2.NewClient(dir, authOpts)
+	case sourcev1.GoGitImplementation:
+		gitReader, err = gogit.NewClient(dir, authOpts)
+	default:
+		err = fmt.Errorf("invalid Git implementation: %s", obj.Spec.GitImplementation)
+	}
 	if err != nil {
 		// Do not return err as recovery without changes is impossible.
-		e := &serror.Stalling{
-			Err:    fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
-			Reason: sourcev1.GitOperationFailedReason,
-		}
+		e := serror.NewStalling(
+			fmt.Errorf("failed to create Git client for implementation '%s': %w", obj.Spec.GitImplementation, err),
+			sourcev1.GitOperationFailedReason,
+		)
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return nil, e
 	}
+	defer gitReader.Close()
 
-	// 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
-		// 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 '%s' has invalid transport type, supported types are: http, https, ssh", obj.Spec.URL),
-				Reason: sourcev1.URLInvalidReason,
-			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
-			return nil, e
-		}
-	}
-
-	commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
+	commit, err := gitReader.Clone(gitCtx, obj.Spec.URL, cloneOpts)
 	if err != nil {
 		e := serror.NewGeneric(
 			fmt.Errorf("failed to checkout and determine revision: %w", err),
@@ -786,6 +786,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return nil, e
 	}
+
 	return commit, nil
 }
 

controllers/gitrepository_controller_test.go

@@ -56,13 +56,13 @@ import (
 	"github.com/fluxcd/pkg/ssh"
 	"github.com/fluxcd/pkg/testserver"
 
+	"github.com/fluxcd/pkg/git"
+	"github.com/fluxcd/pkg/git/libgit2/transport"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/features"
 	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 (
@@ -502,7 +502,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 				EventRecorder:               record.NewFakeRecorder(32),
 				Storage:                     testStorage,
 				features:                    features.FeatureGates(),
-				Libgit2TransportInitialized: managed.Enabled,
+				Libgit2TransportInitialized: transport.Enabled,
 			}
 
 			for _, i := range testGitImplementations {
@@ -731,7 +731,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 		EventRecorder:               record.NewFakeRecorder(32),
 		Storage:                     testStorage,
 		features:                    features.FeatureGates(),
-		Libgit2TransportInitialized: managed.Enabled,
+		Libgit2TransportInitialized: transport.Enabled,
 	}
 
 	for _, tt := range tests {
@@ -1404,7 +1404,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) {
 			},
 			wantErr: true,
 			assertConditions: []metav1.Condition{
-				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: failed to verify commit with any of the given key rings"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: unable to verify commit with any of the given key rings"),
 			},
 		},
 		{
@@ -1599,7 +1599,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) {
 				EventRecorder:               record.NewFakeRecorder(32),
 				Storage:                     testStorage,
 				features:                    features.FeatureGates(),
-				Libgit2TransportInitialized: managed.Enabled,
+				Libgit2TransportInitialized: transport.Enabled,
 			}
 
 			key := client.ObjectKeyFromObject(obj)

controllers/ocirepository_controller_test.go

@@ -57,6 +57,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/git"
 	"github.com/fluxcd/pkg/oci"
 	"github.com/fluxcd/pkg/runtime/conditions"
 	conditionscheck "github.com/fluxcd/pkg/runtime/conditions/check"
@@ -66,7 +67,6 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	serror "github.com/fluxcd/source-controller/internal/error"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
-	"github.com/fluxcd/source-controller/pkg/git"
 )
 
 func TestOCIRepository_Reconcile(t *testing.T) {

controllers/suite_test.go

@@ -37,6 +37,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	dcontext "github.com/distribution/distribution/v3/context"
+	"github.com/fluxcd/pkg/git/libgit2/transport"
 	"github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/testenv"
 	"github.com/fluxcd/pkg/testserver"
@@ -47,13 +48,12 @@ import (
 	dockerRegistry "github.com/distribution/distribution/v3/registry"
 	_ "github.com/distribution/distribution/v3/registry/auth/htpasswd"
 	_ "github.com/distribution/distribution/v3/registry/storage/driver/inmemory"
-	git2go "github.com/libgit2/git2go/v33"
+	git2go "github.com/libgit2/git2go/v34"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	"github.com/fluxcd/source-controller/internal/cache"
 	"github.com/fluxcd/source-controller/internal/features"
 	"github.com/fluxcd/source-controller/internal/helm/registry"
-	"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
 	// +kubebuilder:scaffold:imports
 )
 
@@ -237,7 +237,7 @@ func TestMain(m *testing.M) {
 		panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
 	}
 
-	if err = managed.InitManagedTransport(); err != nil {
+	if err = transport.InitManagedTransport(); err != nil {
 		panic(fmt.Sprintf("Failed to initialize libgit2 managed transport: %v", err))
 	}
 
@@ -247,7 +247,7 @@ func TestMain(m *testing.M) {
 		Metrics:                     testMetricsH,
 		Storage:                     testStorage,
 		features:                    features.FeatureGates(),
-		Libgit2TransportInitialized: managed.Enabled,
+		Libgit2TransportInitialized: transport.Enabled,
 	}).SetupWithManager(testEnv); err != nil {
 		panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err))
 	}