Fix ImagePolicy reconciler checking ImageRepo Ready condition without need

Signed-off-by: Matheus Pimenta <[email protected]>

Author: matheuscscp

internal/controller/database.go

@@ -17,8 +17,11 @@ limitations under the License.
 package controller
 
 // DatabaseWriter implementations record the tags for an image repository.
+// It returns the adler32 checksum of the tags set, which can be used to
+// determine if the tags have changed since the last time they were
+// recorded.
 type DatabaseWriter interface {
-	SetTags(repo string, tags []string) error
+	SetTags(repo string, tags []string) (string, error)
 }
 
 // DatabaseReader implementations get the stored set of tags for an image

internal/controller/imagepolicy_controller.go

@@ -330,16 +330,6 @@ func (r *ImagePolicyReconciler) reconcile(ctx context.Context, sp *patch.SerialP
 		return
 	}
 
-	// Proceed only if the ImageRepository has scan result.
-	if !conditions.IsReady(repo) {
-		// Mark not ready but don't requeue. When the repository becomes ready,
-		// it'll trigger a policy reconciliation. No runtime error to prevent
-		// requeue.
-		conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.DependencyNotReadyReason, "referenced ImageRepository has not been scanned yet")
-		result, retErr = ctrl.Result{}, nil
-		return
-	}
-
 	// Check if the image is valid and mark stalled if not.
 	if _, err := registry.ParseImageReference(repo.Spec.Image, repo.Spec.Insecure); err != nil {
 		conditions.MarkStalled(obj, imagev1.ImageURLInvalidReason, "%s", err)
@@ -371,7 +361,7 @@ func (r *ImagePolicyReconciler) reconcile(ctx context.Context, sp *patch.SerialP
 		// If there's no tag in the database, mark not ready and retry.
 		if err == errNoTagsInDatabase {
 			conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.DependencyNotReadyReason, "%s", err)
-			result, retErr = ctrl.Result{}, err
+			result, retErr = ctrl.Result{}, nil
 			return
 		}
 

internal/controller/imagepolicy_controller_test.go

@@ -47,7 +47,7 @@ import (
 	"github.com/fluxcd/image-reflector-controller/internal/test"
 )
 
-func TestImagePolicyReconciler_imageRepoNotReady(t *testing.T) {
+func TestImagePolicyReconciler_imageRepoHasNoTags(t *testing.T) {
 	g := NewWithT(t)
 
 	namespaceName := "imagepolicy-" + randStringRunes(5)
@@ -65,19 +65,14 @@ func TestImagePolicyReconciler_imageRepoNotReady(t *testing.T) {
 			Name:      "repo",
 		},
 		Spec: imagev1.ImageRepositorySpec{
-			Image: "ghcr.io/stefanprodan/podinfo/foo:bar:zzz:qqq/aaa",
+			Image: "ghcr.io/doesnot/exist",
 		},
 	}
 	g.Expect(k8sClient.Create(ctx, imageRepo)).NotTo(HaveOccurred())
 	t.Cleanup(func() {
 		g.Expect(k8sClient.Delete(ctx, imageRepo)).NotTo(HaveOccurred())
 	})
 
-	g.Eventually(func() bool {
-		err := k8sClient.Get(ctx, client.ObjectKeyFromObject(imageRepo), imageRepo)
-		return err == nil && conditions.IsStalled(imageRepo)
-	}).Should(BeTrue())
-
 	imagePolicy := &imagev1.ImagePolicy{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespaceName,
@@ -87,6 +82,9 @@ func TestImagePolicyReconciler_imageRepoNotReady(t *testing.T) {
 			ImageRepositoryRef: meta.NamespacedObjectReference{
 				Name: imageRepo.Name,
 			},
+			Policy: imagev1.ImagePolicyChoice{
+				Alphabetical: &imagev1.AlphabeticalPolicy{},
+			},
 		},
 	}
 	g.Expect(k8sClient.Create(ctx, imagePolicy)).NotTo(HaveOccurred())
@@ -174,15 +172,17 @@ func TestImagePolicyReconciler_ignoresImageRepoNotReadyEvent(t *testing.T) {
 	}).Should(BeTrue())
 
 	// Check that the ImagePolicy is still ready and does not get updated.
-	err := k8sClient.Get(ctx, client.ObjectKeyFromObject(imagePolicy), imagePolicy)
-	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(conditions.IsReady(imagePolicy)).To(BeTrue())
+	g.Eventually(func() bool {
+		err := k8sClient.Get(ctx, client.ObjectKeyFromObject(imagePolicy), imagePolicy)
+		return err == nil && conditions.IsReady(imagePolicy)
+	}).Should(BeTrue())
 
 	// Wait a bit and check that the ImagePolicy remains ready.
 	time.Sleep(time.Second)
-	err = k8sClient.Get(ctx, client.ObjectKeyFromObject(imagePolicy), imagePolicy)
-	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(conditions.IsReady(imagePolicy)).To(BeTrue())
+	g.Eventually(func() bool {
+		err := k8sClient.Get(ctx, client.ObjectKeyFromObject(imagePolicy), imagePolicy)
+		return err == nil && conditions.IsReady(imagePolicy)
+	}).Should(BeTrue())
 }
 
 func TestImagePolicyReconciler_invalidImage(t *testing.T) {

internal/controller/imagerepository_controller.go

@@ -21,7 +21,9 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"slices"
 	"sort"
+	"strings"
 	"time"
 
 	"github.com/google/go-containerregistry/pkg/name"
@@ -192,6 +194,7 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser
 	oldObj := obj.DeepCopy()
 
 	var foundTags int
+	var tagsChecksum string
 	// Store a message about current reconciliation and next scan.
 	var nextScanMsg string
 	// Set a default next scan time before processing the object.
@@ -206,7 +209,7 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser
 			return true
 		}
 
-		readyMsg := fmt.Sprintf("successful scan: found %d tags", foundTags)
+		readyMsg := fmt.Sprintf("successful scan: found %d tags with checksum %s", foundTags, tagsChecksum)
 		rs := reconcile.NewResultFinalizer(isSuccess, readyMsg)
 		retErr = rs.Finalize(obj, result, retErr)
 
@@ -290,25 +293,27 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser
 	// Scan the repository if it's scan time. No scan is a no-op reconciliation.
 	// The next scan time is not reset in case of no-op reconciliation.
 	if ok {
-		reconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "scanning: %s", reasonMsg)
+		drift := reasonMsg == scanReasonEmptyDatabase
+		reconcile.ProgressiveStatus(drift, obj, meta.ProgressingReason, "scanning: %s", reasonMsg)
 		if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil {
 			result, retErr = ctrl.Result{}, err
 			return
 		}
 
-		tags, err := r.scan(ctx, obj, ref, opts)
+		tags, checksum, err := r.scan(ctx, obj, ref, opts)
 		if err != nil {
 			e := fmt.Errorf("scan failed: %w", err)
 			conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.ReadOperationFailedReason, "%s", e)
 			result, retErr = ctrl.Result{}, e
 			return
 		}
 		foundTags = tags
+		tagsChecksum = checksum
 
 		nextScanMsg = fmt.Sprintf("next scan in %s", when.String())
 		// Check if new tags were found.
-		if oldObj.Status.LastScanResult != nil &&
-			oldObj.Status.LastScanResult.TagCount == foundTags {
+		s := strings.Split(conditions.GetMessage(oldObj, meta.ReadyCondition), " tags with checksum ")
+		if len(s) == 2 && s[1] == checksum {
 			nextScanMsg = "no new tags found, " + nextScanMsg
 		} else {
 			// When new tags are found, this message will be suppressed by
@@ -405,7 +410,8 @@ func (r *ImageRepositoryReconciler) shouldScan(obj imagev1.ImageRepository, now
 
 // scan performs repository scanning and writes the scanned result in the
 // internal database and populates the status of the ImageRepository.
-func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.ImageRepository, ref name.Reference, options []remote.Option) (int, error) {
+// It returns the number of tags found, the checksum of the tags, and an error.
+func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.ImageRepository, ref name.Reference, options []remote.Option) (int, string, error) {
 	timeout := obj.GetTimeout()
 	ctx, cancel := context.WithTimeout(ctx, timeout)
 	defer cancel()
@@ -414,17 +420,20 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.Image
 
 	tags, err := remote.List(ref.Context(), options...)
 	if err != nil {
-		return 0, err
+		return 0, "", err
 	}
 
 	filteredTags, err := filterOutTags(tags, obj.GetExclusionList())
 	if err != nil {
-		return 0, err
+		return 0, "", err
 	}
 
+	slices.Sort(filteredTags)
+
 	canonicalName := ref.Context().String()
-	if err := r.Database.SetTags(canonicalName, filteredTags); err != nil {
-		return 0, fmt.Errorf("failed to set tags for %q: %w", canonicalName, err)
+	checksum, err := r.Database.SetTags(canonicalName, filteredTags)
+	if err != nil {
+		return 0, "", fmt.Errorf("failed to set tags for %q: %w", canonicalName, err)
 	}
 
 	scanTime := metav1.Now()
@@ -441,7 +450,7 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.Image
 		obj.Status.SetLastHandledReconcileRequest(token)
 	}
 
-	return len(filteredTags), nil
+	return len(filteredTags), checksum, nil
 }
 
 // reconcileDelete handles the deletion of the object.

internal/controller/imagerepository_controller_test.go

@@ -22,8 +22,10 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
+	"hash/adler32"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -56,12 +58,12 @@ type mockDatabase struct {
 }
 
 // SetTags implements the DatabaseWriter interface of the Database.
-func (db *mockDatabase) SetTags(repo string, tags []string) error {
+func (db *mockDatabase) SetTags(repo string, tags []string) (string, error) {
 	if db.WriteError != nil {
-		return db.WriteError
+		return "", db.WriteError
 	}
 	db.TagData = append(db.TagData, tags...)
-	return nil
+	return fmt.Sprintf("%v", adler32.Checksum([]byte(strings.Join(tags, ",")))), nil
 }
 
 // Tags implements the DatabaseReader interface of the Database.
@@ -393,7 +395,7 @@ func TestImageRepositoryReconciler_scan(t *testing.T) {
 				opts = append(opts, remote.WithTransport(tr))
 			}
 
-			tagCount, err := r.scan(context.TODO(), repo, ref, opts)
+			tagCount, _, err := r.scan(context.TODO(), repo, ref, opts)
 			if tt.wantErr != "" {
 				g.Expect(err).To(HaveOccurred())
 				g.Expect(err.Error()).To(ContainSubstring(tt.wantErr))

internal/database/badger.go

@@ -18,6 +18,7 @@ package database
 import (
 	"encoding/json"
 	"fmt"
+	"hash/adler32"
 
 	"github.com/dgraph-io/badger/v3"
 )
@@ -54,15 +55,19 @@ func (a *BadgerDatabase) Tags(repo string) ([]string, error) {
 // the repo.
 //
 // It overwrites existing tag sets for the provided repo.
-func (a *BadgerDatabase) SetTags(repo string, tags []string) error {
+func (a *BadgerDatabase) SetTags(repo string, tags []string) (string, error) {
 	b, err := marshal(tags)
 	if err != nil {
-		return err
+		return "", err
 	}
-	return a.db.Update(func(txn *badger.Txn) error {
+	err = a.db.Update(func(txn *badger.Txn) error {
 		e := badger.NewEntry(keyForRepo(tagsPrefix, repo), b)
 		return txn.SetEntry(e)
 	})
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%v", adler32.Checksum(b)), nil
 }
 
 func keyForRepo(prefix, repo string) []byte {

internal/database/badger_gc_test.go

@@ -42,8 +42,9 @@ func TestBadgerGarbageCollectorDoesStop(t *testing.T) {
 	time.Sleep(time.Second)
 
 	tags := []string{"latest", "v0.0.1", "v0.0.2"}
-	fatalIfError(t, db.SetTags(testRepo, tags))
-	_, err := db.Tags(testRepo)
+	_, err := db.SetTags(testRepo, tags)
+	fatalIfError(t, err)
+	_, err = db.Tags(testRepo)
 	fatalIfError(t, err)
 	t.Log("wrote tags successfully")
 

internal/database/badger_test.go

@@ -40,7 +40,7 @@ func TestSetTags(t *testing.T) {
 	db := createBadgerDatabase(t)
 	tags := []string{"latest", "v0.0.1", "v0.0.2"}
 
-	fatalIfError(t, db.SetTags(testRepo, tags))
+	fatalIfError(t, setTags(t, db, testRepo, tags, "1943865137"))
 
 	loaded, err := db.Tags(testRepo)
 	fatalIfError(t, err)
@@ -53,9 +53,9 @@ func TestSetTagsOverwrites(t *testing.T) {
 	db := createBadgerDatabase(t)
 	tags1 := []string{"latest", "v0.0.1", "v0.0.2"}
 	tags2 := []string{"latest", "v0.0.1", "v0.0.2", "v0.0.3"}
-	fatalIfError(t, db.SetTags(testRepo, tags1))
+	fatalIfError(t, setTags(t, db, testRepo, tags1, "1943865137"))
 
-	fatalIfError(t, db.SetTags(testRepo, tags2))
+	fatalIfError(t, setTags(t, db, testRepo, tags2, "3168012550"))
 
 	loaded, err := db.Tags(testRepo)
 	fatalIfError(t, err)
@@ -67,10 +67,10 @@ func TestSetTagsOverwrites(t *testing.T) {
 func TestGetOnlyFetchesForRepo(t *testing.T) {
 	db := createBadgerDatabase(t)
 	tags1 := []string{"latest", "v0.0.1", "v0.0.2"}
-	fatalIfError(t, db.SetTags(testRepo, tags1))
+	fatalIfError(t, setTags(t, db, testRepo, tags1, "1943865137"))
 	testRepo2 := "another/repo"
 	tags2 := []string{"v0.0.3", "v0.0.4"}
-	fatalIfError(t, db.SetTags(testRepo2, tags2))
+	fatalIfError(t, setTags(t, db, testRepo2, tags2, "728958008"))
 
 	loaded, err := db.Tags(testRepo)
 	fatalIfError(t, err)
@@ -96,6 +96,18 @@ func createBadgerDatabase(t *testing.T) *BadgerDatabase {
 	return NewBadgerDatabase(db)
 }
 
+func setTags(t *testing.T, db *BadgerDatabase, repo string, tags []string, expectedChecksum string) error {
+	t.Helper()
+	checksum, err := db.SetTags(repo, tags)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if checksum != expectedChecksum {
+		t.Fatalf("SetTags returned incorrect checksum, want %v, got %v", expectedChecksum, checksum)
+	}
+	return nil
+}
+
 func fatalIfError(t *testing.T, err error) {
 	t.Helper()
 	if err != nil {