From cc3bd7e1daaa9c431d9b9e9fdc514953f1dfa8a5 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Fri, 2 Apr 2021 19:45:37 +0100 Subject: [PATCH 1/3] Test further commits to push branch This adds a test to check that should there be a further update to make, another commit is pushed to the "push branch". In this case, the image policy gets a new latest image. The test fails at present because the controller is not watching image policies (and will not run again on the long interval specified). Signed-off-by: Michael Bridgen --- controllers/update_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/controllers/update_test.go b/controllers/update_test.go index 4370e53d..5a313c79 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -564,7 +564,6 @@ Images: ) const latestImage = "helloworld:1.0.1" - const evenLatestImage = "helloworld:1.2.0" BeforeEach(func() { cloneLocalRepoURL = gitServer.HTTPAddressWithCredentials() + repositoryPath @@ -726,6 +725,22 @@ Images: Expect(commit.Message).To(Equal(commitMessage)) }) + It("pushes another commit to the existing push branch", func() { + // observe the first commit + waitForNewHead(localRepo, pushBranch) + head, err := localRepo.Reference(plumbing.NewRemoteReferenceName(originRemote, pushBranch), true) + headHash := head.String() + Expect(err).NotTo(HaveOccurred()) + + // 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()) }) From ddd0a8d8ed1606f20de2fe402768fc1acb84d790 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Fri, 2 Apr 2021 20:12:13 +0100 Subject: [PATCH 2/3] Watch ImagePolicy objects Prior to #27, controller indexed the automation objects against image policies, since an automation could depend on a specific image policy. That PR removed the references and the watch; however, automation objects still depend on image policy objects, just indirectly through the git repo. This commit reinstates the watch, and makes sure the generation change / reconcile request predicate applies only to the watch on automation object themselves. Signed-off-by: Michael Bridgen --- .../imageupdateautomation_controller.go | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index d18e4b75..6a9dec8e 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -44,6 +44,7 @@ import ( kuberecorder "k8s.io/client-go/tools/record" "k8s.io/client-go/tools/reference" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -70,7 +71,6 @@ const originRemote = "origin" const defaultMessageTemplate = `Update from image update automation` const repoRefKey = ".spec.gitRepository" -const imagePolicyKey = ".spec.update.imagePolicy" const signingSecretKey = "git.asc" @@ -294,9 +294,10 @@ func (r *ImageUpdateAutomationReconciler) SetupWithManager(mgr ctrl.Manager) err } return ctrl.NewControllerManagedBy(mgr). - For(&imagev1.ImageUpdateAutomation{}). - WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{})). + For(&imagev1.ImageUpdateAutomation{}, builder.WithPredicates( + predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{}))). Watches(&source.Kind{Type: &sourcev1.GitRepository{}}, handler.EnqueueRequestsFromMapFunc(r.automationsForGitRepo)). + Watches(&source.Kind{Type: &imagev1_reflect.ImagePolicy{}}, handler.EnqueueRequestsFromMapFunc(r.automationsForImagePolicy)). Complete(r) } @@ -351,6 +352,25 @@ func (r *ImageUpdateAutomationReconciler) automationsForGitRepo(obj client.Objec return reqs } +// automationsForImagePolicy fetches all the automation objects that +// might depend on a image policy object. Since the link is via +// markers in the git repo, _any_ automation object in the same +// namespace could be affected. +func (r *ImageUpdateAutomationReconciler) automationsForImagePolicy(obj client.Object) []reconcile.Request { + ctx := context.Background() + var autoList imagev1.ImageUpdateAutomationList + if err := r.List(ctx, &autoList, client.InNamespace(obj.GetNamespace())); err != nil { + return nil + } + reqs := make([]reconcile.Request, len(autoList.Items), len(autoList.Items)) + for i := range autoList.Items { + reqs[i].NamespacedName.Name = autoList.Items[i].GetName() + reqs[i].NamespacedName.Namespace = autoList.Items[i].GetNamespace() + } + println("[DEBUG] enqueuing autos for image policy", obj.GetName(), obj.GetNamespace(), len(reqs)) + return reqs +} + // --- git ops type repoAccess struct { From 40fb66a217ee8d1740bdb5f85c953871213f15b2 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Sat, 3 Apr 2021 15:11:20 +0100 Subject: [PATCH 3/3] Fetch remote branch before switching to it For the "push to branch" feature, the controller must either switch to the branch given, or create it starting at the checked-out HEAD. The func `switchBranch` encapsulates this decision -- but it assumes that if the branch exists at the remote, it will have been fetched when cloning, and this is not always true. In particular, cloning with go-git avoids fetching all refs: https://github.com/fluxcd/source-controller/blob/v0.11.0/pkg/git/gogit/checkout.go This commit adds a step to fetch the remote branch to a local branch, before attempting to switch to the local branch. This makes `switchBranch` a little simpler, and doesn't rely on any refs having been fetched ahead of time. Signed-off-by: Michael Bridgen --- .../imageupdateautomation_controller.go | 98 +++++++++++++++---- 1 file changed, 81 insertions(+), 17 deletions(-) diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index 6a9dec8e..80dbbf29 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -33,6 +33,7 @@ import ( libgit2 "github.com/libgit2/git2go/v31" 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" @@ -188,7 +189,11 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr // When there's a push spec, the pushed-to branch is where commits // shall be made if auto.Spec.Push != nil { - if err := switchBranch(repo, auto.Spec.Push.Branch); err != nil { + pushBranch := auto.Spec.Push.Branch + if err := fetch(ctx, tmp, repo, pushBranch, access, origin.Spec.GitImplementation); err != nil && err != errRemoteBranchMissing { + return failWithError(err) + } + if err = switchBranch(repo, pushBranch); err != nil { return failWithError(err) } } @@ -367,7 +372,6 @@ func (r *ImageUpdateAutomationReconciler) automationsForImagePolicy(obj client.O reqs[i].NamespacedName.Name = autoList.Items[i].GetName() reqs[i].NamespacedName.Namespace = autoList.Items[i].GetNamespace() } - println("[DEBUG] enqueuing autos for image policy", obj.GetName(), obj.GetNamespace(), len(reqs)) return reqs } @@ -409,6 +413,13 @@ func (r *ImageUpdateAutomationReconciler) getRepoAccess(ctx context.Context, rep return access, nil } +func (r repoAccess) remoteCallbacks() libgit2.RemoteCallbacks { + return libgit2.RemoteCallbacks{ + CertificateCheckCallback: r.auth.CertCallback, + CredentialsCallback: r.auth.CredCallback, + } +} + // cloneInto clones the upstream repository at the `branch` given, // using the git library indicated by `impl`. It returns a // `*gogit.Repository` regardless of the git library, since that is @@ -431,11 +442,10 @@ func cloneInto(ctx context.Context, access repoAccess, branch, path, impl string // branch given. If the branch does not exist, it is created using the // head as the starting point. func switchBranch(repo *gogit.Repository, pushBranch string) error { - remoteBranch := plumbing.NewRemoteReferenceName(originRemote, pushBranch) localBranch := plumbing.NewBranchReferenceName(pushBranch) - // is the remote branch already present? - branchHead, err := repo.Reference(remoteBranch, false) + // is the branch already present? + _, err := repo.Reference(localBranch, false) switch { case err == plumbing.ErrReferenceNotFound: // make a new branch, starting at HEAD @@ -450,17 +460,15 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error { case err != nil: return err default: - // make a local branch that references the remote branch - branchRef := plumbing.NewHashReference(localBranch, branchHead.Hash()) - if err = repo.Storer.SetReference(branchRef); err != nil { - return err - } + // local branch found, great + break } tree, err := repo.Worktree() if err != nil { return err } + return tree.Checkout(&gogit.CheckoutOptions{ Branch: localBranch, }) @@ -540,6 +548,62 @@ func (r *ImageUpdateAutomationReconciler) getSigningEntity(ctx context.Context, return entities[0], nil } +var errRemoteBranchMissing = errors.New("remote branch missing") + +// fetch gets the remote branch given and updates the local branch +// head of the same name, so it can be switched to. If the fetch +// completes, it returns nil; if the remote branch is missing, it +// 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 { + 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) + } +} + +func fetchLibgit2(repo *libgit2.Repository, refspec string, access repoAccess) error { + origin, err := repo.Remotes.Lookup(originRemote) + if err != nil { + return err + } + err = origin.Fetch( + []string{refspec}, + &libgit2.FetchOptions{ + RemoteCallbacks: access.remoteCallbacks(), + }, "", + ) + if err != nil && libgit2.IsErrorCode(err, libgit2.ErrorCodeNotFound) { + return errRemoteBranchMissing + } + 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 @@ -553,15 +617,18 @@ func push(ctx context.Context, path string, repo *gogit.Repository, branch strin } return pushLibgit2(lg2repo, access, branch) case sourcev1.GoGitImplementation: - return pushGoGit(ctx, repo, access) + 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) error { +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{ - Auth: access.auth.AuthMethod, + RemoteName: originRemote, + Auth: access.auth.AuthMethod, + RefSpecs: []config.RefSpec{refspec}, }) return gogitPushError(err) } @@ -590,10 +657,7 @@ func pushLibgit2(repo *libgit2.Repository, access repoAccess, branch string) err return err } err = origin.Push([]string{fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)}, &libgit2.PushOptions{ - RemoteCallbacks: libgit2.RemoteCallbacks{ - CertificateCheckCallback: access.auth.CertCallback, - CredentialsCallback: access.auth.CredCallback, - }, + RemoteCallbacks: access.remoteCallbacks(), }) return libgit2PushError(err) }