From 74420bd1a725cfcf59b6835108f810bcfbbd20b4 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 1 Jun 2022 19:51:49 +0530 Subject: [PATCH 1/4] refactor controller and git tests to use managed transport Signed-off-by: Sanskar Jaiswal --- controllers/git_test.go | 23 +-- .../imageupdateautomation_controller.go | 1 + controllers/suite_test.go | 4 + controllers/update_test.go | 136 +++++++++++++----- 4 files changed, 116 insertions(+), 48 deletions(-) diff --git a/controllers/git_test.go b/controllers/git_test.go index 8d8c5b95..a163030f 100644 --- a/controllers/git_test.go +++ b/controllers/git_test.go @@ -12,6 +12,7 @@ import ( libgit2 "github.com/libgit2/git2go/v33" "github.com/fluxcd/pkg/gittestserver" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) func populateRepoFromFixture(repo *libgit2.Repository, fixture string) error { @@ -128,21 +129,26 @@ func TestPushRejected(t *testing.T) { } // this is currently defined in update_test.go, but handy right here .. - if err = initGitRepo(gitServer, "testdata/appconfig", "main", "/appconfig.git"); err != nil { + if err = initGitRepo(gitServer, "testdata/appconfig", "master", "/appconfig.git"); err != nil { t.Fatal(err) } repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git" - repo, err := clone(repoURL, "origin", "main") + repo, err := clone(repoURL, "master") if err != nil { t.Fatal(err) } + defer repo.Free() - // This is here to guard against push in general being broken - err = push(context.TODO(), repo.Workdir(), "main", repoAccess{ - url: repoURL, - auth: nil, + transportOptsURL := "http://" + randStringRunes(5) + managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ + TargetURL: repoURL, }) + defer managed.RemoveTransportOptions(transportOptsURL) + repo.Remotes.SetUrl("origin", transportOptsURL) + + // This is here to guard against push in general being broken + err = push(context.TODO(), repo.Workdir(), "master", repoAccess{}) if err != nil { t.Fatal(err) } @@ -154,10 +160,7 @@ 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") } diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index 801be227..4aa99128 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/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{ diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 35e66600..7d6a2817 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -24,6 +24,7 @@ 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" @@ -31,6 +32,7 @@ import ( 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, diff --git a/controllers/update_test.go b/controllers/update_test.go index ab6ddf0a..bd1da1ce 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -54,6 +54,7 @@ import ( "github.com/fluxcd/pkg/gittestserver" "github.com/fluxcd/pkg/ssh" 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" "github.com/fluxcd/image-automation-controller/pkg/test" @@ -405,10 +406,9 @@ func TestImageAutomationReconciler_signedCommit(t *testing.T) { } func TestImageAutomationReconciler_e2e(t *testing.T) { - gitImpls := []string{sourcev1.GoGitImplementation, sourcev1.LibGit2Implementation} protos := []string{"http", "ssh"} - testFunc := func(t *testing.T, proto string, impl string) { + testFunc := func(t *testing.T, proto string) { g := NewWithT(t) const latestImage = "helloworld:1.0.1" @@ -462,10 +462,10 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { // in a secret. err = createSSHIdentitySecret(testEnv, gitSecretName, namespace, repoURL) g.Expect(err).ToNot(HaveOccurred()) - err = createGitRepository(testEnv, gitRepoName, namespace, impl, repoURL, gitSecretName) + err = createGitRepository(testEnv, gitRepoName, namespace, repoURL, gitSecretName) g.Expect(err).ToNot(HaveOccurred()) } else { - err = createGitRepository(testEnv, gitRepoName, namespace, impl, repoURL, "") + err = createGitRepository(testEnv, gitRepoName, namespace, repoURL, "") g.Expect(err).ToNot(HaveOccurred()) } @@ -480,8 +480,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { t.Run("PushSpec", func(t *testing.T) { // Clone the repo locally. - localRepo, err := clone(cloneLocalRepoURL, "origin", branch) + localRepo, err := clone(cloneLocalRepoURL, branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") + defer localRepo.Free() + origin, err := localRepo.Remotes.Lookup("origin") + g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") + defer origin.Free() // NB not testing the image reflector controller; this // will make a "fully formed" ImagePolicy object. @@ -613,8 +617,9 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { // test helper. When switching branches, the localRepo seems to get // stuck in one particular branch. As a workaround, create a // separate localRepo. - localRepo, err := clone(cloneLocalRepoURL, "origin", branch) + localRepo, err := clone(cloneLocalRepoURL, branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") + defer localRepo.Free() g.Expect(checkoutBranch(localRepo, branch)).ToNot(HaveOccurred()) err = createImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage) @@ -736,13 +741,10 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { }) } - // Run the protocol based e2e tests against the git implementations. - for _, gitImpl := range gitImpls { - for _, proto := range protos { - t.Run(fmt.Sprintf("%s_%s", gitImpl, proto), func(t *testing.T) { - testFunc(t, proto, gitImpl) - }) - } + for _, proto := range protos { + t.Run(fmt.Sprintf("%s", proto), func(t *testing.T) { + testFunc(t, proto) + }) } } @@ -861,8 +863,9 @@ func compareRepoWithExpected(g *WithT, repoURL, branch, fixture string, changeFi copy.Copy(fixture, expected) changeFixture(expected) - repo, err := clone(repoURL, "origin", branch) + repo, err := clone(repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) + defer repo.Free() // NOTE: The workdir contains a trailing /. Clean it to not confuse the // DiffDirectories(). actual := filepath.Clean(repo.Workdir()) @@ -872,10 +875,39 @@ func compareRepoWithExpected(g *WithT, repoURL, branch, fixture string, changeFi test.ExpectMatchingDirectories(g, actual, expected) } +// configureManagedTransportOptions registers the transport options for this repository +// and sets the remote url of origin to the transport options url. If repoURL is empty +// it tries to figure it out by looking at the remote url of origin. It returns a function +// which removes the transport options for this repo and sets the remote url of the origin +// back to the actual url. Callers are expected to call this function in a deferred manner. +func configureManagedTransportOptions(repo *libgit2.Repository, repoURL string) (func(), error) { + if repoURL == "" { + origin, err := repo.Remotes.Lookup(originRemote) + if err != nil { + return nil, err + } + defer origin.Free() + repoURL = origin.Url() + } + transportOptsURL := "http://" + randStringRunes(5) + managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ + TargetURL: repoURL, + }) + + err := repo.Remotes.SetUrl(originRemote, transportOptsURL) + if err != nil { + return nil, fmt.Errorf("could not set remote origin url: %v", err) + } + return func() { + managed.RemoveTransportOptions(transportOptsURL) + repo.Remotes.SetUrl(originRemote, repoURL) + }, nil +} + func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) { - originRemote := "origin" - repo, err := clone(repoURL, originRemote, branch) + repo, err := clone(repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) + defer repo.Free() changeFiles(repo.Workdir()) @@ -887,6 +919,11 @@ func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path s _, err = commitWorkDir(repo, branch, msg, sig) g.Expect(err).ToNot(HaveOccurred()) + cleanup, err := configureManagedTransportOptions(repo, repoURL) + if err != nil { + panic(err) + } + defer cleanup() origin, err := repo.Remotes.Lookup(originRemote) if err != nil { panic(fmt.Errorf("cannot find origin: %v", err)) @@ -941,7 +978,7 @@ func initGitRepoPlain(fixture, repositoryPath string) (*git2go.Repository, error } func headFromBranch(repo *git2go.Repository, branchName string) (*git2go.Commit, error) { - branch, err := repo.LookupBranch(branchName, git2go.BranchAll) + branch, err := repo.LookupBranch(branchName, git2go.BranchLocal) if err != nil { return nil, err } @@ -1123,40 +1160,53 @@ func mockSignature(time time.Time) *git2go.Signature { } } -func clone(repoURL, remoteName, branchName string) (*git2go.Repository, error) { +func clone(repoURL, branchName string) (*git2go.Repository, error) { dir, err := os.MkdirTemp("", "iac-clone-*") if err != nil { return nil, err } + transportOptsURL := "http://" + randStringRunes(5) + managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ + TargetURL: repoURL, + }) + defer managed.RemoveTransportOptions(transportOptsURL) + opts := &git2go.CloneOptions{ Bare: false, CheckoutBranch: branchName, CheckoutOptions: git2go.CheckoutOptions{ Strategy: git2go.CheckoutForce, }, - FetchOptions: git2go.FetchOptions{ - RemoteCallbacks: git2go.RemoteCallbacks{ - CertificateCheckCallback: func(cert *git2go.Certificate, valid bool, hostname string) error { - return nil - }, - }, - }, } + repo, err := git2go.Clone(transportOptsURL, dir, opts) - return git2go.Clone(repoURL, dir, opts) + // set the origin remote url to the actual repo url, since + // the origin remote will have transportOptsURl as the it's url + // because that's the url used to clone the repo. + err = repo.Remotes.SetUrl("origin", repoURL) + if err != nil { + return nil, err + } + return repo, nil } func waitForNewHead(g *WithT, repo *git2go.Repository, branch, preChangeHash string) { var commitToResetTo *git2go.Commit + cleanup, err := configureManagedTransportOptions(repo, "") + if err != nil { + panic(err) + } + defer cleanup() + + origin, err := repo.Remotes.Lookup("origin") + if err != nil { + panic("origin not set") + } + defer origin.Free() + // Now try to fetch new commits from that remote branch g.Eventually(func() bool { - origin, err := repo.Remotes.Lookup("origin") - if err != nil { - panic("origin not set") - } - defer origin.Free() - if err := origin.Fetch( []string{branchRefName(branch)}, &libgit2.FetchOptions{}, "", @@ -1205,6 +1255,12 @@ func commitIdFromBranch(repo *git2go.Repository, branchName string) string { } func getRemoteHead(repo *git2go.Repository, branchName string) (*git2go.Oid, error) { + cleanup, err := configureManagedTransportOptions(repo, "") + if err != nil { + return nil, err + } + defer cleanup() + remote, err := repo.Remotes.Lookup("origin") if err != nil { return nil, err @@ -1385,11 +1441,17 @@ func testWithCustomRepoAndImagePolicy( // Clone the repo. repoURL := gitServer.HTTPAddressWithCredentials() + repositoryPath - localRepo, err := clone(repoURL, "origin", args.branch) + localRepo, err := clone(repoURL, args.branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") + defer localRepo.Free() + + origin, err := localRepo.Remotes.Lookup("origin") + g.Expect(err).ToNot(HaveOccurred(), "failed to look up remote origin") + defer origin.Free() + localRepo.Remotes.SetUrl("origin", repoURL) // Create GitRepository resource for the above repo. - err = createGitRepository(kClient, args.gitRepoName, args.gitRepoNamespace, "", repoURL, "") + err = createGitRepository(kClient, args.gitRepoName, args.gitRepoNamespace, repoURL, "") g.Expect(err).ToNot(HaveOccurred(), "failed to create GitRepository resource") // Create ImagePolicy with populated latest image in the status. @@ -1440,7 +1502,7 @@ func createNamespace(kClient client.Client, name string) (cleanup, error) { return cleanup, nil } -func createGitRepository(kClient client.Client, name, namespace, impl, repoURL, secretRef string) error { +func createGitRepository(kClient client.Client, name, namespace, repoURL, secretRef string) error { gitRepo := &sourcev1.GitRepository{ Spec: sourcev1.GitRepositorySpec{ URL: repoURL, @@ -1452,9 +1514,7 @@ func createGitRepository(kClient client.Client, name, namespace, impl, repoURL, if secretRef != "" { gitRepo.Spec.SecretRef = &meta.LocalObjectReference{Name: secretRef} } - if impl != "" { - gitRepo.Spec.GitImplementation = impl - } + gitRepo.Spec.GitImplementation = sourcev1.LibGit2Implementation return kClient.Create(context.Background(), gitRepo) } From 5ee644676410504621e319d1979d80791cac1ed9 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 2 Jun 2022 14:11:59 +0530 Subject: [PATCH 2/4] fix regression in switchToBranch Fixes regression in which we fail to push to a branch after switching to a branch, if origin is ahead of local. Fixed by setting the upstream commit as the local branch target. Regression introduced in #330, and partially addressed in #369. Signed-off-by: Sanskar Jaiswal --- controllers/git_test.go | 99 ++++++++++++++++++- .../imageupdateautomation_controller.go | 29 ++++-- controllers/update_test.go | 13 +-- 3 files changed, 123 insertions(+), 18 deletions(-) diff --git a/controllers/git_test.go b/controllers/git_test.go index a163030f..9495835d 100644 --- a/controllers/git_test.go +++ b/controllers/git_test.go @@ -10,6 +10,9 @@ import ( "github.com/go-logr/logr" libgit2 "github.com/libgit2/git2go/v33" + "k8s.io/apimachinery/pkg/types" + + . "github.com/onsi/gomega" "github.com/fluxcd/pkg/gittestserver" "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" @@ -129,12 +132,12 @@ func TestPushRejected(t *testing.T) { } // this is currently defined in update_test.go, but handy right here .. - if err = initGitRepo(gitServer, "testdata/appconfig", "master", "/appconfig.git"); err != nil { + if err = initGitRepo(gitServer, "testdata/appconfig", "test", "/appconfig.git"); err != nil { t.Fatal(err) } repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git" - repo, err := clone(repoURL, "master") + repo, err := clone(repoURL, "test") if err != nil { t.Fatal(err) } @@ -148,7 +151,7 @@ func TestPushRejected(t *testing.T) { repo.Remotes.SetUrl("origin", transportOptsURL) // This is here to guard against push in general being broken - err = push(context.TODO(), repo.Workdir(), "master", repoAccess{}) + err = push(context.TODO(), repo.Workdir(), "test", repoAccess{}) if err != nil { t.Fatal(err) } @@ -165,3 +168,93 @@ func TestPushRejected(t *testing.T) { 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()) + + branch := "test" + g.Expect(initGitRepo(gitServer, "testdata/appconfig", branch, "/appconfig.git")).To(Succeed()) + + repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git" + repo, err := clone(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 + transportOptsURL := "http://" + randStringRunes(5) + managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ + TargetURL: repoURL, + }) + defer managed.RemoveTransportOptions(transportOptsURL) + repo.Remotes.SetUrl("origin", transportOptsURL) + + // 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("origin") + 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()) +} diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index 4aa99128..4a4cfa1f 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -749,7 +749,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, @@ -758,7 +757,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 } @@ -785,15 +784,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() @@ -811,6 +820,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) } diff --git a/controllers/update_test.go b/controllers/update_test.go index bd1da1ce..315489c2 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -904,7 +904,7 @@ func configureManagedTransportOptions(repo *libgit2.Repository, repoURL string) }, nil } -func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) { +func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) *libgit2.Oid { repo, err := clone(repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) defer repo.Free() @@ -916,21 +916,18 @@ func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path s Email: "test@example.com", When: time.Now(), } - _, err = commitWorkDir(repo, branch, msg, sig) + id, err := commitWorkDir(repo, branch, msg, sig) g.Expect(err).ToNot(HaveOccurred()) cleanup, err := configureManagedTransportOptions(repo, repoURL) - if err != nil { - panic(err) - } + g.Expect(err).ToNot(HaveOccurred()) defer cleanup() origin, err := repo.Remotes.Lookup(originRemote) - if err != nil { - panic(fmt.Errorf("cannot find origin: %v", err)) - } + g.Expect(err).ToNot(HaveOccurred()) defer origin.Free() g.Expect(origin.Push([]string{branchRefName(branch)}, &libgit2.PushOptions{})).To(Succeed()) + return id } // Initialise a git server with a repo including the files in dir. From 61f76d3f6dd830e84034b702b22f2c66a0449b75 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 2 Jun 2022 19:10:07 +0530 Subject: [PATCH 3/4] use context for cloning in tests Signed-off-by: Sanskar Jaiswal --- controllers/git_test.go | 47 +++++++++++++--------- controllers/update_test.go | 81 +++++++++++++++++++++----------------- 2 files changed, 72 insertions(+), 56 deletions(-) diff --git a/controllers/git_test.go b/controllers/git_test.go index 9495835d..85472e80 100644 --- a/controllers/git_test.go +++ b/controllers/git_test.go @@ -10,12 +10,10 @@ import ( "github.com/go-logr/logr" libgit2 "github.com/libgit2/git2go/v33" - "k8s.io/apimachinery/pkg/types" - . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" "github.com/fluxcd/pkg/gittestserver" - "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) func populateRepoFromFixture(repo *libgit2.Repository, fixture string) error { @@ -131,24 +129,29 @@ func TestPushRejected(t *testing.T) { t.Fatal(err) } - // this is currently defined in update_test.go, but handy right here .. + // 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, "test") + 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() - transportOptsURL := "http://" + randStringRunes(5) - managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ - TargetURL: repoURL, - }) - defer managed.RemoveTransportOptions(transportOptsURL) - repo.Remotes.SetUrl("origin", transportOptsURL) + 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(), "test", repoAccess{}) @@ -176,11 +179,18 @@ func Test_switchToBranch(t *testing.T) { 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" - repo, err := clone(repoURL, branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + repo, err := clone(cloneCtx, repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) defer repo.Free() @@ -190,12 +200,11 @@ func Test_switchToBranch(t *testing.T) { target := head.Target() // register transport options and update remote to transport url - transportOptsURL := "http://" + randStringRunes(5) - managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ - TargetURL: repoURL, - }) - defer managed.RemoveTransportOptions(transportOptsURL) - repo.Remotes.SetUrl("origin", transportOptsURL) + 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. @@ -217,7 +226,7 @@ func Test_switchToBranch(t *testing.T) { branch = "exists-on-origin" _, err = repo.CreateBranch(branch, cc, false) g.Expect(err).ToNot(HaveOccurred()) - origin, err := repo.Remotes.Lookup("origin") + origin, err := repo.Remotes.Lookup(originRemote) g.Expect(err).ToNot(HaveOccurred()) defer origin.Free() diff --git a/controllers/update_test.go b/controllers/update_test.go index 315489c2..2908858b 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -480,12 +480,11 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { t.Run("PushSpec", func(t *testing.T) { // Clone the repo locally. - localRepo, err := clone(cloneLocalRepoURL, branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + localRepo, err := clone(cloneCtx, cloneLocalRepoURL, branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") defer localRepo.Free() - origin, err := localRepo.Remotes.Lookup("origin") - g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") - defer origin.Free() // NB not testing the image reflector controller; this // will make a "fully formed" ImagePolicy object. @@ -617,7 +616,9 @@ func TestImageAutomationReconciler_e2e(t *testing.T) { // test helper. When switching branches, the localRepo seems to get // stuck in one particular branch. As a workaround, create a // separate localRepo. - localRepo, err := clone(cloneLocalRepoURL, branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + localRepo, err := clone(cloneCtx, cloneLocalRepoURL, branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") defer localRepo.Free() @@ -862,8 +863,9 @@ func compareRepoWithExpected(g *WithT, repoURL, branch, fixture string, changeFi copy.Copy(fixture, expected) changeFixture(expected) - - repo, err := clone(repoURL, branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + repo, err := clone(cloneCtx, repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) defer repo.Free() // NOTE: The workdir contains a trailing /. Clean it to not confuse the @@ -875,26 +877,28 @@ func compareRepoWithExpected(g *WithT, repoURL, branch, fixture string, changeFi test.ExpectMatchingDirectories(g, actual, expected) } -// configureManagedTransportOptions registers the transport options for this repository +// configureTransportOptsForRepo registers the transport options for this repository // and sets the remote url of origin to the transport options url. If repoURL is empty // it tries to figure it out by looking at the remote url of origin. It returns a function // which removes the transport options for this repo and sets the remote url of the origin // back to the actual url. Callers are expected to call this function in a deferred manner. -func configureManagedTransportOptions(repo *libgit2.Repository, repoURL string) (func(), error) { - if repoURL == "" { - origin, err := repo.Remotes.Lookup(originRemote) - if err != nil { - return nil, err - } - defer origin.Free() - repoURL = origin.Url() +func configureTransportOptsForRepo(repo *libgit2.Repository) (func(), error) { + origin, err := repo.Remotes.Lookup(originRemote) + if err != nil { + return nil, err } - transportOptsURL := "http://" + randStringRunes(5) + defer origin.Free() + repoURL := origin.Url() + u, err := url.Parse(repoURL) + if err != nil { + return nil, err + } + transportOptsURL := u.Scheme + "://" + randStringRunes(5) managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ TargetURL: repoURL, }) - err := repo.Remotes.SetUrl(originRemote, transportOptsURL) + err = repo.Remotes.SetUrl(originRemote, transportOptsURL) if err != nil { return nil, fmt.Errorf("could not set remote origin url: %v", err) } @@ -905,7 +909,9 @@ func configureManagedTransportOptions(repo *libgit2.Repository, repoURL string) } func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) *libgit2.Oid { - repo, err := clone(repoURL, branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + repo, err := clone(cloneCtx, repoURL, branch) g.Expect(err).ToNot(HaveOccurred()) defer repo.Free() @@ -919,7 +925,7 @@ func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path s id, err := commitWorkDir(repo, branch, msg, sig) g.Expect(err).ToNot(HaveOccurred()) - cleanup, err := configureManagedTransportOptions(repo, repoURL) + cleanup, err := configureTransportOptsForRepo(repo) g.Expect(err).ToNot(HaveOccurred()) defer cleanup() origin, err := repo.Remotes.Lookup(originRemote) @@ -951,8 +957,7 @@ func initGitRepo(gitServer *gittestserver.GitServer, fixture, branch, repository if err != nil { return err } - - return repo.Remotes.AddPush("origin", branchRefName(branch)) + return repo.Remotes.AddPush(originRemote, branchRefName(branch)) } func initGitRepoPlain(fixture, repositoryPath string) (*git2go.Repository, error) { @@ -1157,7 +1162,7 @@ func mockSignature(time time.Time) *git2go.Signature { } } -func clone(repoURL, branchName string) (*git2go.Repository, error) { +func clone(ctx context.Context, repoURL, branchName string) (*git2go.Repository, error) { dir, err := os.MkdirTemp("", "iac-clone-*") if err != nil { return nil, err @@ -1165,6 +1170,7 @@ func clone(repoURL, branchName string) (*git2go.Repository, error) { transportOptsURL := "http://" + randStringRunes(5) managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{ TargetURL: repoURL, + Context: ctx, }) defer managed.RemoveTransportOptions(transportOptsURL) @@ -1176,11 +1182,14 @@ func clone(repoURL, branchName string) (*git2go.Repository, error) { }, } repo, err := git2go.Clone(transportOptsURL, dir, opts) + if err != nil { + return nil, err + } // set the origin remote url to the actual repo url, since // the origin remote will have transportOptsURl as the it's url // because that's the url used to clone the repo. - err = repo.Remotes.SetUrl("origin", repoURL) + err = repo.Remotes.SetUrl(originRemote, repoURL) if err != nil { return nil, err } @@ -1190,16 +1199,12 @@ func clone(repoURL, branchName string) (*git2go.Repository, error) { func waitForNewHead(g *WithT, repo *git2go.Repository, branch, preChangeHash string) { var commitToResetTo *git2go.Commit - cleanup, err := configureManagedTransportOptions(repo, "") - if err != nil { - panic(err) - } + cleanup, err := configureTransportOptsForRepo(repo) + g.Expect(err).ToNot(HaveOccurred()) defer cleanup() - origin, err := repo.Remotes.Lookup("origin") - if err != nil { - panic("origin not set") - } + origin, err := repo.Remotes.Lookup(originRemote) + g.Expect(err).ToNot(HaveOccurred()) defer origin.Free() // Now try to fetch new commits from that remote branch @@ -1252,13 +1257,13 @@ func commitIdFromBranch(repo *git2go.Repository, branchName string) string { } func getRemoteHead(repo *git2go.Repository, branchName string) (*git2go.Oid, error) { - cleanup, err := configureManagedTransportOptions(repo, "") + cleanup, err := configureTransportOptsForRepo(repo) if err != nil { return nil, err } defer cleanup() - remote, err := repo.Remotes.Lookup("origin") + remote, err := repo.Remotes.Lookup(originRemote) if err != nil { return nil, err } @@ -1438,14 +1443,16 @@ func testWithCustomRepoAndImagePolicy( // Clone the repo. repoURL := gitServer.HTTPAddressWithCredentials() + repositoryPath - localRepo, err := clone(repoURL, args.branch) + cloneCtx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + localRepo, err := clone(cloneCtx, repoURL, args.branch) g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo") defer localRepo.Free() - origin, err := localRepo.Remotes.Lookup("origin") + origin, err := localRepo.Remotes.Lookup(originRemote) g.Expect(err).ToNot(HaveOccurred(), "failed to look up remote origin") defer origin.Free() - localRepo.Remotes.SetUrl("origin", repoURL) + localRepo.Remotes.SetUrl(originRemote, repoURL) // Create GitRepository resource for the above repo. err = createGitRepository(kClient, args.gitRepoName, args.gitRepoNamespace, repoURL, "") From 175f91ea0e030f01ac9e503026a433768703ad1c Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 2 Jun 2022 19:23:54 +0530 Subject: [PATCH 4/4] recover from panics in cloneInto Signed-off-by: Sanskar Jaiswal --- controllers/imageupdateautomation_controller.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index 4a4cfa1f..e53d9e85 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -541,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) @@ -980,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) + } +}