Merge pull request #373 from aryan9600/managed-transport-default

controllers/git_test.go

@@ -10,6 +10,8 @@ import (
 
 	"github.com/go-logr/logr"
 	libgit2 "github.com/libgit2/git2go/v33"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/fluxcd/pkg/gittestserver"
 )
@@ -127,22 +129,32 @@ func TestPushRejected(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// this is currently defined in update_test.go, but handy right here ..
-	if err = initGitRepo(gitServer, "testdata/appconfig", "main", "/appconfig.git"); err != nil {
+	// We use "test" as the branch to init repo, to avoid potential conflicts
+	// with the default branch(main/master) of the system this test is running
+	// on. If, for e.g., we used main as the branch and the default branch is
+	// supposed to be main, this will fail as this would try to create a branch
+	// named main explicitly.
+	if err = initGitRepo(gitServer, "testdata/appconfig", "test", "/appconfig.git"); err != nil {
 		t.Fatal(err)
 	}
 
 	repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git"
-	repo, err := clone(repoURL, "origin", "main")
+	cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10)
+	defer cancel()
+	repo, err := clone(cloneCtx, repoURL, "test")
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer repo.Free()
+
+	cleanup, err := configureTransportOptsForRepo(repo)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
 
 	// This is here to guard against push in general being broken
-	err = push(context.TODO(), repo.Workdir(), "main", repoAccess{
-		url:  repoURL,
-		auth: nil,
-	})
+	err = push(context.TODO(), repo.Workdir(), "test", repoAccess{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -154,11 +166,104 @@ func TestPushRejected(t *testing.T) {
 
 	// This is supposed to fail, because the hook rejects the branch
 	// pushed to.
-	err = push(context.TODO(), repo.Workdir(), branch, repoAccess{
-		url:  repoURL,
-		auth: nil,
-	})
+	err = push(context.TODO(), repo.Workdir(), branch, repoAccess{})
 	if err == nil {
 		t.Error("push to a forbidden branch is expected to fail, but succeeded")
 	}
 }
+
+func Test_switchToBranch(t *testing.T) {
+	g := NewWithT(t)
+	gitServer, err := gittestserver.NewTempGitServer()
+	g.Expect(err).ToNot(HaveOccurred())
+	gitServer.AutoCreate()
+	g.Expect(gitServer.StartHTTP()).To(Succeed())
+
+	// We use "test" as the branch to init repo, to avoid potential conflicts
+	// with the default branch(main/master) of the system this test is running
+	// on. If, for e.g., we used main as the branch and the default branch is
+	// supposed to be main, this will fail as this would try to create a branch
+	// named main explicitly.
+	branch := "test"
+	g.Expect(initGitRepo(gitServer, "testdata/appconfig", branch, "/appconfig.git")).To(Succeed())
+
+	repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git"
+	cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10)
+	defer cancel()
+	repo, err := clone(cloneCtx, repoURL, branch)
+	g.Expect(err).ToNot(HaveOccurred())
+	defer repo.Free()
+
+	head, err := repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	defer head.Free()
+	target := head.Target()
+
+	// register transport options and update remote to transport url
+	cleanup, err := configureTransportOptsForRepo(repo)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
+
+	// calling switchToBranch with a branch that doesn't exist on origin
+	// should result in the branch being created and switched to.
+	branch = "not-on-origin"
+	switchToBranch(repo, context.TODO(), branch, repoAccess{})
+
+	head, err = repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	name, err := head.Branch().Name()
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(name).To(Equal(branch))
+
+	cc, err := repo.LookupCommit(head.Target())
+	g.Expect(err).ToNot(HaveOccurred())
+	defer cc.Free()
+	g.Expect(cc.Id().String()).To(Equal(target.String()))
+
+	// create a branch with the HEAD commit and push it to origin
+	branch = "exists-on-origin"
+	_, err = repo.CreateBranch(branch, cc, false)
+	g.Expect(err).ToNot(HaveOccurred())
+	origin, err := repo.Remotes.Lookup(originRemote)
+	g.Expect(err).ToNot(HaveOccurred())
+	defer origin.Free()
+
+	g.Expect(origin.Push(
+		[]string{fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)}, &libgit2.PushOptions{},
+	)).To(Succeed())
+
+	// push a new commit to the branch. this is done to test whether we properly
+	// sync our local branch with the remote branch, before switching.
+	policyKey := types.NamespacedName{
+		Name:      "policy",
+		Namespace: "ns",
+	}
+	commitID := commitInRepo(g, repoURL, branch, "Install setter marker", func(tmp string) {
+		g.Expect(replaceMarker(tmp, policyKey)).To(Succeed())
+	})
+
+	// calling switchToBranch with a branch that exists should make sure to fetch latest
+	// for that branch from origin, and then switch to it.
+	switchToBranch(repo, context.TODO(), branch, repoAccess{})
+	head, err = repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	name, err = head.Branch().Name()
+
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(name).To(Equal(branch))
+	g.Expect(head.Target().String()).To(Equal(commitID.String()))
+
+	// push a commit after switching to the branch, to check if the local
+	// branch is synced with origin.
+	replaceMarker(repo.Workdir(), policyKey)
+	sig := &libgit2.Signature{
+		Name:  "Testbot",
+		Email: "test@example.com",
+		When:  time.Now(),
+	}
+	_, err = commitWorkDir(repo, branch, "update policy", sig)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(push(context.TODO(), repo.Workdir(), branch, repoAccess{})).To(Succeed())
+}

controllers/imageupdateautomation_controller.go

@@ -510,6 +510,7 @@ type repoAccess struct {
 func (r *ImageUpdateAutomationReconciler) getRepoAccess(ctx context.Context, repository *sourcev1.GitRepository) (repoAccess, error) {
 	var access repoAccess
 	access.url = repository.Spec.URL
+	access.auth = &git.AuthOptions{}
 
 	if repository.Spec.SecretRef != nil {
 		name := types.NamespacedName{
@@ -540,12 +541,15 @@ func (r repoAccess) remoteCallbacks(ctx context.Context) libgit2.RemoteCallbacks
 // cloneInto clones the upstream repository at the `ref` given (which
 // can be `nil`). It returns a `*libgit2.Repository` since that is used
 // for committing changes.
-func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef, path string) (*libgit2.Repository, error) {
+func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef,
+	path string) (_ *libgit2.Repository, err error) {
+	defer recoverPanic(&err)
+
 	opts := git.CheckoutOptions{}
 	if ref != nil {
 		opts.Tag = ref.Tag
 		opts.SemVer = ref.SemVer
-		opts.Tag = ref.Tag
+		opts.Commit = ref.Commit
 		opts.Branch = ref.Branch
 	}
 	checkoutStrat, err := gitstrat.CheckoutStrategyForImplementation(ctx, sourcev1.LibGit2Implementation, opts)
@@ -748,7 +752,6 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		callbacks = managed.RemoteCallbacks()
 	}
 
-	branchRef := fmt.Sprintf("origin/%s", branch)
 	// Force the fetching of the remote branch.
 	err = origin.Fetch([]string{branch}, &libgit2.FetchOptions{
 		RemoteCallbacks: callbacks,
@@ -757,7 +760,7 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		return fmt.Errorf("cannot fetch remote branch: %w", err)
 	}
 
-	remoteBranch, err := repo.LookupBranch(branchRef, libgit2.BranchRemote)
+	remoteBranch, err := repo.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", branch))
 	if err != nil && !libgit2.IsErrorCode(err, libgit2.ErrorCodeNotFound) {
 		return err
 	}
@@ -784,15 +787,25 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 	}
 	defer commit.Free()
 
-	localBranch, err := repo.LookupBranch(branch, libgit2.BranchLocal)
+	localBranch, err := repo.References.Lookup(fmt.Sprintf("refs/heads/%s", branch))
 	if err != nil && !libgit2.IsErrorCode(err, libgit2.ErrorCodeNotFound) {
 		return fmt.Errorf("cannot lookup branch '%s': %w", branch, err)
 	}
 	if localBranch == nil {
-		localBranch, err = repo.CreateBranch(branch, commit, false)
-	}
-	if localBranch == nil {
-		return fmt.Errorf("cannot create local branch '%s': %w", branch, err)
+		lb, err := repo.CreateBranch(branch, commit, false)
+		if err != nil {
+			return fmt.Errorf("cannot create branch '%s': %w", branch, err)
+		}
+		defer lb.Free()
+		// We could've done something like:
+		// localBranch = lb.Reference
+		// But for some reason, calling `lb.Free()` AND using it, causes a really
+		// nasty crash. Since, we can't avoid calling `lb.Free()`, in order to prevent
+		// memory leaks, we don't use `lb` and instead manually lookup the ref.
+		localBranch, err = repo.References.Lookup(fmt.Sprintf("refs/heads/%s", branch))
+		if err != nil {
+			return fmt.Errorf("cannot lookup branch '%s': %w", branch, err)
+		}
 	}
 	defer localBranch.Free()
 
@@ -810,6 +823,12 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		return fmt.Errorf("cannot checkout tree for branch '%s': %w", branch, err)
 	}
 
+	ref, err := localBranch.SetTarget(commit.Id(), "")
+	if err != nil {
+		return fmt.Errorf("cannot update branch '%s' to be at target commit: %w", branch, err)
+	}
+	ref.Free()
+
 	return repo.SetHead("refs/heads/" + branch)
 }
 
@@ -964,3 +983,9 @@ func templateMsg(messageTemplate string, templateValues *TemplateData) (string,
 	}
 	return b.String(), nil
 }
+
+func recoverPanic(err *error) {
+	if r := recover(); r != nil {
+		*err = fmt.Errorf("recovered from git2go panic: %v", r)
+	}
+}

controllers/suite_test.go

@@ -24,13 +24,15 @@ import (
 	"testing"
 	"time"
 
+	"github.com/go-logr/logr"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1beta1"
 	"github.com/fluxcd/pkg/runtime/testenv"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
+	"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
 
 	imagev1 "github.com/fluxcd/image-automation-controller/api/v1beta1"
 	// +kubebuilder:scaffold:imports
@@ -61,6 +63,8 @@ func TestMain(m *testing.M) {
 		filepath.Join("testdata", "crds"),
 	))
 
+	managed.InitManagedTransport(logr.Discard())
+
 	controllerName := "image-automation-controller"
 	if err := (&ImageUpdateAutomationReconciler{
 		Client:        testEnv,