From 42bc3e8b0a0cd98b8210debae8f236bd5ddc11ac Mon Sep 17 00:00:00 2001
From: Sunny <darkowlzz@protonmail.com>
Date: Mon, 6 Feb 2023 10:43:25 +0000
Subject: [PATCH] helmrepo-oci: check before rec on type switching

When a HelmRepository with "default" spec.type is switched to "oci", the
existing HelmRepository is processed by HelmRepositoryReconciler by
running reconcileDelete() which removes all the previous status
information and allows the HelmRepositoryOCIReconciler to process the
object and add its own status data. But at times, when
HelmRepositoryOCIReconciler starts processing a HelmRepository with
stale status data from the client cache, it contains the stale
conditions that are owned only by HelmRepositoryReconciler and isn't
managed by HelmRepositoryOCIReconciler. This results in situations where
Ready is marked as True with the latest generation of the object and the
unmanaged stale conditions remain in the previous generation, resulting
in unexpected status conditions.

In the observed flaky tests,
`TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter` would
fail because of stale ArtifactInStorage condition with previous
generation value.

This change adds a check in the HelmRepositoryOCIReconciler to start
processing the object only once the stale unmanaged conditions have been
removed.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
---
 controllers/helmrepository_controller_oci.go  | 32 +++++++++++++++++++
 .../helmrepository_controller_oci_test.go     | 22 +++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go
index e971a11e..a0424c45 100644
--- a/controllers/helmrepository_controller_oci.go
+++ b/controllers/helmrepository_controller_oci.go
@@ -40,6 +40,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
+	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/oci"
 	"github.com/fluxcd/pkg/runtime/conditions"
@@ -82,6 +83,11 @@ type HelmRepositoryOCIReconciler struct {
 	RegistryClientGenerator RegistryClientGeneratorFunc
 
 	patchOptions []patch.Option
+
+	// unmanagedConditions are the conditions that are not managed by this
+	// reconciler and need to be removed from the object before taking ownership
+	// of the object being reconciled.
+	unmanagedConditions []string
 }
 
 // RegistryClientGeneratorFunc is a function that returns a registry client
@@ -95,6 +101,7 @@ func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 func (r *HelmRepositoryOCIReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmRepositoryReconcilerOptions) error {
+	r.unmanagedConditions = conditionsDiff(helmRepositoryReadyCondition.Owned, helmRepositoryOCIOwnedConditions)
 	r.patchOptions = getPatchOptions(helmRepositoryOCIOwnedConditions, r.ControllerName)
 
 	recoverPanic := true
@@ -124,6 +131,16 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// If the object contains any of the unmanaged conditions, requeue and wait
+	// for those conditions to be removed first before processing the object.
+	// NOTE: This will happen only when a HelmRepository's spec.type is switched
+	// from "default" to "oci".
+	if conditions.HasAny(obj, r.unmanagedConditions) {
+		r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "IncompleteTransition",
+			"object contains conditions managed by other reconciler")
+		return ctrl.Result{RequeueAfter: time.Second}, nil
+	}
+
 	// Record suspended status metric
 	r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
 
@@ -428,3 +445,18 @@ func makeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registry
 
 	return nil, nil
 }
+
+func conditionsDiff(a, b []string) []string {
+	bMap := make(map[string]struct{}, len(b))
+	for _, j := range b {
+		bMap[j] = struct{}{}
+	}
+
+	r := []string{}
+	for _, i := range a {
+		if _, exists := bMap[i]; !exists {
+			r = append(r, i)
+		}
+	}
+	return r
+}
diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go
index d73f0b8e..77ce2874 100644
--- a/controllers/helmrepository_controller_oci_test.go
+++ b/controllers/helmrepository_controller_oci_test.go
@@ -19,6 +19,7 @@ package controllers
 import (
 	"encoding/base64"
 	"fmt"
+	"strconv"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -320,3 +321,24 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) {
 		})
 	}
 }
+
+func TestConditionsDiff(t *testing.T) {
+	tests := []struct {
+		a, b, want []string
+	}{
+		{[]string{"a", "b", "c"}, []string{"b", "d"}, []string{"a", "c"}},
+		{[]string{"a", "b", "c"}, []string{}, []string{"a", "b", "c"}},
+		{[]string{}, []string{"b", "d"}, []string{}},
+		{[]string{}, []string{}, []string{}},
+		{[]string{"a", "b"}, nil, []string{"a", "b"}},
+		{nil, []string{"a", "b"}, []string{}},
+		{nil, nil, []string{}},
+	}
+
+	for i, tt := range tests {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			g := NewWithT(t)
+			g.Expect(conditionsDiff(tt.a, tt.b)).To(Equal(tt.want))
+		})
+	}
+}