From aa624b16c622096c96b08de7cec6fbd417526e73 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Wed, 2 Jun 2021 10:06:10 +0100 Subject: [PATCH] Use libgit2 for clone, fetch, push source-controller/pkg/git does shallow clones when using the go-git implementation, and apparently this causes problems when fetching a branch that has been merged at the origin: https://github.com/fluxcd/image-automation-controller/issues/164 So far as I can tell, getting a shallow clone breaks the automation, no matter whether go-git or libgit2 is used for operations after cloning. So: just use libgit2 for cloning, which means non-shallow clones; and, for fetch and push, since there's no functional difference between the implementations for those. Signed-off-by: Michael Bridgen --- controllers/git_error_test.go | 26 ---- .../imageupdateautomation_controller.go | 118 ++++-------------- controllers/update_test.go | 73 +++++++++++ 3 files changed, 99 insertions(+), 118 deletions(-) diff --git a/controllers/git_error_test.go b/controllers/git_error_test.go index 294657c9..d0b9b69c 100644 --- a/controllers/git_error_test.go +++ b/controllers/git_error_test.go @@ -74,29 +74,3 @@ func TestLibgit2ErrorUnchanged(t *testing.T) { t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage) } } - -func TestGoGitErrorReplace(t *testing.T) { - // this is what go-git uses as the error message is if the remote - // sends a blank first line - unknownMessage := `unknown error: remote: ` - err := errors.New(unknownMessage) - err = gogitPushError(err) - reformattedMessage := err.Error() - if reformattedMessage == unknownMessage { - t.Errorf("expected rewritten error, got %q", reformattedMessage) - } -} - -func TestGoGitErrorUnchanged(t *testing.T) { - // this is (roughly) what GitHub sends if the deploy key doesn't - // have write access; go-git passes this on verbatim - regularMessage := `remote: ERROR: deploy key does not have write access` - expectedReformat := regularMessage - err := errors.New(regularMessage) - err = gogitPushError(err) - reformattedMessage := err.Error() - // test that it's been rewritten, without checking the exact content - if len(reformattedMessage) > len(expectedReformat) { - t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage) - } -} diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index aef24071..00caaaf4 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -34,7 +34,6 @@ import ( "github.com/ProtonMail/go-crypto/openpgp" securejoin "github.com/cyphar/filepath-securejoin" - "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-logr/logr" @@ -214,7 +213,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr } var repo *gogit.Repository - if repo, err = cloneInto(ctx, access, ref, tmp, origin.Spec.GitImplementation); err != nil { + if repo, err = cloneInto(ctx, access, ref, tmp); err != nil { return failWithError(err) } @@ -222,7 +221,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr // shall be made if gitSpec.Push != nil { - if err := fetch(ctx, tmp, repo, pushBranch, access, origin.Spec.GitImplementation); err != nil && err != errRemoteBranchMissing { + if err := fetch(ctx, tmp, pushBranch, access); err != nil && err != errRemoteBranchMissing { return failWithError(err) } if err = switchBranch(repo, pushBranch); err != nil { @@ -307,7 +306,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr return failWithError(err) } } else { - if err := push(ctx, tmp, repo, pushBranch, access, origin.Spec.GitImplementation); err != nil { + if err := push(ctx, tmp, pushBranch, access); err != nil { return failWithError(err) } @@ -424,6 +423,10 @@ func (r *ImageUpdateAutomationReconciler) automationsForImagePolicy(obj client.O // --- git ops +// Note: libgit2 is always used for network operations; for cloning, +// it will do a non-shallow clone, and for anything else, it doesn't +// matter what is used. + type repoAccess struct { auth *git.Auth url string @@ -433,19 +436,21 @@ func (r *ImageUpdateAutomationReconciler) getRepoAccess(ctx context.Context, rep var access repoAccess access.auth = &git.Auth{} access.url = repository.Spec.URL - authStrat, err := gitstrat.AuthSecretStrategyForURL(access.url, git.CheckoutOptions{GitImplementation: repository.Spec.GitImplementation}) + + authStrat, err := gitstrat.AuthSecretStrategyForURL(access.url, git.CheckoutOptions{GitImplementation: sourcev1.LibGit2Implementation}) if err != nil { return access, err } if repository.Spec.SecretRef != nil && authStrat != nil { + name := types.NamespacedName{ Namespace: repository.GetNamespace(), Name: repository.Spec.SecretRef.Name, } var secret corev1.Secret - err := r.Client.Get(ctx, name, &secret) + err = r.Client.Get(ctx, name, &secret) if err != nil { err = fmt.Errorf("auth secret error: %w", err) return access, err @@ -468,11 +473,10 @@ func (r repoAccess) remoteCallbacks() libgit2.RemoteCallbacks { } // cloneInto clones the upstream repository at the `ref` given (which -// can be `nil`), using the git library indicated by `impl`. It -// returns a `*gogit.Repository` regardless of the git library, since -// that is used for committing changes. -func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef, path, impl string) (*gogit.Repository, error) { - checkoutStrat, err := gitstrat.CheckoutStrategyForRef(ref, git.CheckoutOptions{GitImplementation: impl}) +// can be `nil`). It returns a `*gogit.Repository` since that is used +// for committing changes. +func cloneInto(ctx context.Context, access repoAccess, ref *sourcev1.GitRepositoryRef, path string) (*gogit.Repository, error) { + checkoutStrat, err := gitstrat.CheckoutStrategyForRef(ref, git.CheckoutOptions{GitImplementation: sourcev1.LibGit2Implementation}) if err == nil { _, _, err = checkoutStrat.Checkout(ctx, path, access.url, access.auth) } @@ -490,18 +494,12 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error { localBranch := plumbing.NewBranchReferenceName(pushBranch) // is the branch already present? - _, err := repo.Reference(localBranch, false) + hash, err := repo.Reference(localBranch, true) + var create bool switch { case err == plumbing.ErrReferenceNotFound: // make a new branch, starting at HEAD - head, err := repo.Head() - if err != nil { - return err - } - branchRef := plumbing.NewHashReference(localBranch, head.Hash()) - if err = repo.Storer.SetReference(branchRef); err != nil { - return err - } + create = true case err != nil: return err default: @@ -516,6 +514,7 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error { return tree.Checkout(&gogit.CheckoutOptions{ Branch: localBranch, + Create: create, }) } @@ -608,23 +607,12 @@ var errRemoteBranchMissing = errors.New("remote branch missing") // returns errRemoteBranchMissing (this is to work in sympathy with // `switchBranch`, which will create the branch if it doesn't // exist). For any other problem it will return the error. -func fetch(ctx context.Context, path string, repo *gogit.Repository, branch string, access repoAccess, impl string) error { +func fetch(ctx context.Context, path string, branch string, access repoAccess) error { refspec := fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch) - switch impl { - case sourcev1.LibGit2Implementation: - lg2repo, err := libgit2.OpenRepository(path) - if err != nil { - return err - } - return fetchLibgit2(lg2repo, refspec, access) - case sourcev1.GoGitImplementation: - return fetchGoGit(ctx, repo, refspec, access) - default: - return fmt.Errorf("unknown git implementation %q", impl) + repo, err := libgit2.OpenRepository(path) + if err != nil { + return err } -} - -func fetchLibgit2(repo *libgit2.Repository, refspec string, access repoAccess) error { origin, err := repo.Remotes.Lookup(originRemote) if err != nil { return err @@ -641,69 +629,15 @@ func fetchLibgit2(repo *libgit2.Repository, refspec string, access repoAccess) e return err } -func fetchGoGit(ctx context.Context, repo *gogit.Repository, refspec string, access repoAccess) error { - err := repo.FetchContext(ctx, &gogit.FetchOptions{ - RemoteName: originRemote, - RefSpecs: []config.RefSpec{config.RefSpec(refspec)}, - Auth: access.auth.AuthMethod, - }) - if err == gogit.NoErrAlreadyUpToDate { - return nil - } - if _, ok := err.(gogit.NoMatchingRefSpecError); ok { - return errRemoteBranchMissing - } - return err -} - // push pushes the branch given to the origin using the git library // indicated by `impl`. It's passed both the path to the repo and a // gogit.Repository value, since the latter may as well be used if the // implementation is GoGit. -func push(ctx context.Context, path string, repo *gogit.Repository, branch string, access repoAccess, impl string) error { - switch impl { - case sourcev1.LibGit2Implementation: - lg2repo, err := libgit2.OpenRepository(path) - if err != nil { - return err - } - return pushLibgit2(lg2repo, access, branch) - case sourcev1.GoGitImplementation: - return pushGoGit(ctx, repo, access, branch) - default: - return fmt.Errorf("unknown git implementation %q", impl) - } -} - -func pushGoGit(ctx context.Context, repo *gogit.Repository, access repoAccess, branch string) error { - refspec := config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)) - err := repo.PushContext(ctx, &gogit.PushOptions{ - RemoteName: originRemote, - Auth: access.auth.AuthMethod, - RefSpecs: []config.RefSpec{refspec}, - }) - return gogitPushError(err) -} - -func gogitPushError(err error) error { - if err == nil { - return nil - } - switch strings.TrimSpace(err.Error()) { - case "unknown error: remote:": - // this unhelpful error arises because go-git takes the first - // line of the output on stderr, and for some git providers - // (GitLab, at least) the output has a blank line at the - // start. The rest of stderr is thrown away, so we can't get - // the actual error; but at least we know what was being - // attempted, and the likely cause. - return fmt.Errorf("push rejected; check git secret has write access") - default: +func push(ctx context.Context, path, branch string, access repoAccess) error { + repo, err := libgit2.OpenRepository(path) + if err != nil { return err } -} - -func pushLibgit2(repo *libgit2.Repository, access repoAccess, branch string) error { origin, err := repo.Remotes.Lookup(originRemote) if err != nil { return err diff --git a/controllers/update_test.go b/controllers/update_test.go index 3b447693..dc99565e 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -782,6 +782,29 @@ Images: Expect(head.String()).NotTo(Equal(headHash)) }) + It("still pushes to the push branch after it's merged", func() { + // observe the first commit + waitForNewHead(localRepo, pushBranch) + head, err := localRepo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), true) + headHash := head.String() + Expect(err).NotTo(HaveOccurred()) + + // merge the push branch into checkout branch, and push the merge commit + // upstream. + // waitForNewHead() leaves the repo at the head of the branch given, i.e., the + // push branch), so we have to check out the "main" branch first. + Expect(checkoutBranch(localRepo, branch)).To(Succeed()) + mergeBranchIntoHead(localRepo, pushBranch) + + // update the policy and expect another commit in the push branch + policy.Status.LatestImage = "helloworld:v1.3.0" + Expect(k8sClient.Status().Update(context.TODO(), policy)).To(Succeed()) + waitForNewHead(localRepo, pushBranch) + head, err = localRepo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), true) + Expect(err).NotTo(HaveOccurred()) + Expect(head.String()).NotTo(Equal(headHash)) + }) + AfterEach(func() { Expect(k8sClient.Delete(context.Background(), update)).To(Succeed()) }) @@ -1044,6 +1067,7 @@ func waitForNewHead(repo *git.Repository, branch string) { // remote, so it is a detached head. Expect(working.Reset(&git.ResetOptions{ Commit: remoteHead.Hash(), + Mode: git.HardReset, })).To(Succeed()) } @@ -1128,3 +1152,52 @@ func initGitRepo(gitServer *gittestserver.GitServer, fixture, branch, repository RefSpecs: []config.RefSpec{"refs/heads/*:refs/heads/*"}, }) } + +func checkoutBranch(repo *git.Repository, branch string) error { + working, err := repo.Worktree() + if err != nil { + return err + } + // check that there's no local changes, as a sanity check + status, err := working.Status() + if err != nil { + return err + } + if len(status) > 0 { + for path := range status { + println(path, "is changed") + } + } // the checkout next will fail if there are changed files + + if err = working.Checkout(&git.CheckoutOptions{ + Branch: plumbing.NewBranchReferenceName(branch), + Create: false, + }); err != nil { + return err + } + return nil +} + +// This merges the push branch into HEAD, and pushes upstream. This is +// to simulate e.g., a PR being merged. +func mergeBranchIntoHead(repo *git.Repository, pushBranch string) { + // hash of head + headRef, err := repo.Head() + Expect(err).NotTo(HaveOccurred()) + pushBranchRef, err := repo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), false) + Expect(err).NotTo(HaveOccurred()) + + // You need the worktree to be able to create a commit + worktree, err := repo.Worktree() + Expect(err).NotTo(HaveOccurred()) + _, err = worktree.Commit(fmt.Sprintf("Merge %s", pushBranch), &git.CommitOptions{ + Parents: []plumbing.Hash{headRef.Hash(), pushBranchRef.Hash()}, + }) + Expect(err).NotTo(HaveOccurred()) + + // push upstream + err = repo.Push(&git.PushOptions{ + RemoteName: originRemote, + }) + Expect(err).NotTo(HaveOccurred()) +}