Skip to content

Commit

Permalink
Use libgit2 for clone, fetch, push
Browse files Browse the repository at this point in the history 
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:

    #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 <[email protected]>
  • Loading branch information
@squaremo
squaremo committed Jun 2, 2021
1 parent 0ad5d28 commit 8818070
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 118 deletions.
26 changes: 0 additions & 26 deletions controllers/git_error_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
118 changes: 26 additions & 92 deletions controllers/imageupdateautomation_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -214,15 +213,15 @@ 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)
}

// When there's a push spec, the pushed-to branch is where commits
// 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 {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
}
Expand All @@ -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)
_, 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:
Expand All @@ -516,6 +514,7 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error {

return tree.Checkout(&gogit.CheckoutOptions{
Branch: localBranch,
Create: create,
})
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
73 changes: 73 additions & 0 deletions controllers/update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
}

Expand Down Expand Up @@ -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())
}

0 comments on commit 8818070

Please sign in to comment.