Make sure reconcile request annotation works

The convention among GOTK controllers is to use a "reconcile request"
annotation to force a reconcilation, outside of spec or dependency
changes. This is used by e.g., the incoming webhooks handler. The
predicate `ChangePredicate`, already used by this controller, takes
this into account by allowing events that either caused the generation
to increment, _or_ changed the reconcile request annotation.

This commit adds a test that the automation will indeed run when the
annotation is set. This is a little delicate, because I have to rule
out _other_ reasons it might run. To do so, the test makes a change to
the git repo that will be overwritten by an automation run -- a commit
will not trigger a Reconcile call since it's entirely outside
Kubernetes.

Signed-off-by: Michael Bridgen <michael@weave.works>

Author: squaremo

api/v1alpha1/imageupdateautomation_types.go

@@ -102,7 +102,8 @@ type ImageUpdateAutomationStatus struct {
 	// +optional
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
 	// +optional
-	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	Conditions                  []metav1.Condition `json:"conditions,omitempty"`
+	meta.ReconcileRequestStatus `json:",inline"`
 }
 
 const (

api/v1alpha1/zz_generated.deepcopy.go

@@ -191,6 +191,10 @@ spec:
                   made).
                 format: date-time
                 type: string
+              lastHandledReconcileAt:
+                description: LastHandledReconcileAt holds the value of the most recent
+                  reconcile request value, so a change can be detected.
+                type: string
               observedGeneration:
                 format: int64
                 type: integer

config/crd/bases/image.toolkit.fluxcd.io_imageupdateautomations.yaml

@@ -87,6 +87,15 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// whatever else happens, we've now "seen" the reconcile
+	// annotation if it's there
+	if token, ok := meta.ReconcileAnnotationValue(auto.GetAnnotations()); ok {
+		auto.Status.SetLastHandledReconcileRequest(token)
+		if err := r.Status().Update(ctx, &auto); err != nil {
+			return ctrl.Result{Requeue: true}, err
+		}
+	}
+
 	if auto.Spec.Suspend {
 		msg := "ImageUpdateAutomation is suspended, skipping automation run"
 		imagev1.SetImageUpdateAutomationReadiness(

controllers/imageupdateautomation_controller.go

@@ -188,29 +188,9 @@ var _ = Describe("ImageUpdateAutomation", func() {
 			BeforeEach(func() {
 				// Insert a setter reference into the deployment file,
 				// before creating the automation object itself.
-				tmp, err := ioutil.TempDir("", "gotest-imageauto-setters")
-				Expect(err).ToNot(HaveOccurred())
-				defer os.RemoveAll(tmp)
-				repo, err := git.PlainClone(tmp, false, &git.CloneOptions{
-					URL:           repoURL,
-					ReferenceName: plumbing.NewBranchReferenceName(defaultBranch),
-				})
-				Expect(err).ToNot(HaveOccurred())
-
-				replaceMarker(tmp, policyKey)
-				worktree, err := repo.Worktree()
-				Expect(err).ToNot(HaveOccurred())
-				_, err = worktree.Add("deploy.yaml")
-				Expect(err).ToNot(HaveOccurred())
-				_, err = worktree.Commit("Install setter marker", &git.CommitOptions{
-					Author: &object.Signature{
-						Name:  "Testbot",
-						Email: "test@example.com",
-						When:  time.Now(),
-					},
+				commitInRepo(repoURL, "Install setter marker", func(tmp string) {
+					replaceMarker(tmp, policyKey)
 				})
-				Expect(err).ToNot(HaveOccurred())
-				Expect(repo.Push(&git.PushOptions{RemoteName: "origin"})).To(Succeed())
 
 				// pull the head commit we just pushed, so it's not
 				// considered a new commit when checking for a commit
@@ -229,6 +209,7 @@ var _ = Describe("ImageUpdateAutomation", func() {
 						Namespace: updateKey.Namespace,
 					},
 					Spec: imagev1.ImageUpdateAutomationSpec{
+						RunInterval: &metav1.Duration{Duration: 2 * time.Hour}, // this is to ensure any subsequent run should be outside the scope of the testing
 						Checkout: imagev1.GitCheckoutSpec{
 							GitRepositoryRef: corev1.LocalObjectReference{
 								Name: gitRepoKey.Name,
@@ -259,22 +240,9 @@ var _ = Describe("ImageUpdateAutomation", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(commit.Message).To(Equal(commitMessage))
 
-				tmp, err := ioutil.TempDir("", "gotest-imageauto")
-				Expect(err).ToNot(HaveOccurred())
-				defer os.RemoveAll(tmp)
-
-				expected, err := ioutil.TempDir("", "gotest-imageauto-expected")
-				Expect(err).ToNot(HaveOccurred())
-				defer os.RemoveAll(expected)
-				copy.Copy("testdata/appconfig-setters-expected", expected)
-				replaceMarker(expected, policyKey)
-
-				_, err = git.PlainClone(tmp, false, &git.CloneOptions{
-					URL:           repoURL,
-					ReferenceName: plumbing.NewBranchReferenceName(defaultBranch),
+				compareRepoWithExpected(repoURL, "testdata/appconfig-setters-expected", func(tmp string) {
+					replaceMarker(tmp, policyKey)
 				})
-				Expect(err).ToNot(HaveOccurred())
-				test.ExpectMatchingDirectories(tmp, expected)
 			})
 
 			It("stops updating when suspended", func() {
@@ -292,6 +260,52 @@ var _ = Describe("ImageUpdateAutomation", func() {
 					return ready != nil && ready.Status == metav1.ConditionFalse && ready.Reason == meta.SuspendedReason
 				}, timeout, time.Second).Should(BeTrue())
 			})
+
+			It("runs when the reconcile request annotation is added", func() {
+				// the automation has run, and is not expected to run
+				// again for 2 hours. Make a commit to the git repo
+				// which needs to be undone by automation, then add
+				// the annotation and make sure it runs again.
+				Expect(k8sClient.Get(context.Background(), updateKey, updateBySetters)).To(Succeed())
+				lastRun := updateBySetters.Status.LastAutomationRunTime
+				Expect(lastRun).ToNot(BeNil())
+
+				commitInRepo(repoURL, "Revert image update", func(tmp string) {
+					// revert the change made by copying the old version
+					// of the file back over then restoring the setter
+					// marker
+					copy.Copy("testdata/appconfig/deploy.yaml", filepath.Join(tmp, "deploy.yaml"))
+					replaceMarker(tmp, policyKey)
+				})
+				// check that it was reverted
+				compareRepoWithExpected(repoURL, "testdata/appconfig", func(tmp string) {
+					replaceMarker(tmp, policyKey)
+				})
+
+				ts := time.Now().String()
+				var updatePatch imagev1.ImageUpdateAutomation
+				updatePatch.Name = updateKey.Name
+				updatePatch.Namespace = updateKey.Namespace
+				updatePatch.ObjectMeta.Annotations = map[string]string{
+					meta.ReconcileRequestAnnotation: ts,
+				}
+				Expect(k8sClient.Patch(context.Background(), &updatePatch, client.Merge)).To(Succeed())
+
+				Eventually(func() bool {
+					if err := k8sClient.Get(context.Background(), updateKey, updateBySetters); err != nil {
+						return false
+					}
+					newLastRun := updateBySetters.Status.LastAutomationRunTime
+					return newLastRun != nil && newLastRun.Time.After(lastRun.Time)
+				}, timeout, time.Second).Should(BeTrue())
+				// check that the annotation was recorded as seen
+				Expect(updateBySetters.Status.LastHandledReconcileAt).To(Equal(ts))
+
+				// check that a new commit was made
+				compareRepoWithExpected(repoURL, "testdata/appconfig-setters-expected", func(tmp string) {
+					replaceMarker(tmp, policyKey)
+				})
+			})
 		})
 	})
 })
@@ -326,6 +340,51 @@ func waitForNewHead(repo *git.Repository) {
 	}, timeout, time.Second).Should(BeTrue())
 }
 
+func compareRepoWithExpected(repoURL, fixture string, changeFixture func(tmp string)) {
+	expected, err := ioutil.TempDir("", "gotest-imageauto-expected")
+	Expect(err).ToNot(HaveOccurred())
+	defer os.RemoveAll(expected)
+	copy.Copy(fixture, expected)
+	changeFixture(expected)
+
+	tmp, err := ioutil.TempDir("", "gotest-imageauto")
+	Expect(err).ToNot(HaveOccurred())
+	defer os.RemoveAll(tmp)
+	_, err = git.PlainClone(tmp, false, &git.CloneOptions{
+		URL:           repoURL,
+		ReferenceName: plumbing.NewBranchReferenceName(defaultBranch),
+	})
+	Expect(err).ToNot(HaveOccurred())
+	test.ExpectMatchingDirectories(tmp, expected)
+}
+
+func commitInRepo(repoURL, msg string, changeFiles func(path string)) {
+	tmp, err := ioutil.TempDir("", "gotest-imageauto")
+	Expect(err).ToNot(HaveOccurred())
+	defer os.RemoveAll(tmp)
+	repo, err := git.PlainClone(tmp, false, &git.CloneOptions{
+		URL:           repoURL,
+		ReferenceName: plumbing.NewBranchReferenceName(defaultBranch),
+	})
+	Expect(err).ToNot(HaveOccurred())
+
+	changeFiles(tmp)
+
+	worktree, err := repo.Worktree()
+	Expect(err).ToNot(HaveOccurred())
+	_, err = worktree.Add(".")
+	Expect(err).ToNot(HaveOccurred())
+	_, err = worktree.Commit(msg, &git.CommitOptions{
+		Author: &object.Signature{
+			Name:  "Testbot",
+			Email: "test@example.com",
+			When:  time.Now(),
+		},
+	})
+	Expect(err).ToNot(HaveOccurred())
+	Expect(repo.Push(&git.PushOptions{RemoteName: "origin"})).To(Succeed())
+}
+
 // Initialise a git server with a repo including the files in dir.
 func initGitRepo(gitServer *gittestserver.GitServer, fixture, repositoryPath string) error {
 	fs := memfs.New()