controller: unready dep should not bump obs gen

This ensures that any unfulfilled dependencies for which we requeue do
not prematurely bump the observed generation by introducing typed
errors.

These typed errors ensure that the logic to bump the observed generation
can continue to be the same, while ignoring them just in time before
returning the final error.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

internal/controller/helmrelease_controller.go

@@ -59,6 +59,7 @@ import (
 	"github.com/fluxcd/helm-controller/internal/action"
 	"github.com/fluxcd/helm-controller/internal/chartutil"
 	"github.com/fluxcd/helm-controller/internal/digest"
+	interrors "github.com/fluxcd/helm-controller/internal/errors"
 	"github.com/fluxcd/helm-controller/internal/features"
 	"github.com/fluxcd/helm-controller/internal/kube"
 	"github.com/fluxcd/helm-controller/internal/loader"
@@ -97,6 +98,11 @@ type HelmReleaseReconcilerOptions struct {
 	RateLimiter               ratelimiter.RateLimiter
 }
 
+var (
+	errWaitForDependency = errors.New("must wait for dependency")
+	errWaitForChart      = errors.New("must wait for chart")
+)
+
 func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opts HelmReleaseReconcilerOptions) error {
 	// Index the HelmRelease by the HelmChart references they point at
 	if err := mgr.GetFieldIndexer().IndexField(ctx, &v2.HelmRelease{}, v2.SourceIndexKey,
@@ -167,6 +173,13 @@ func (r *HelmReleaseReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
 		}
 
+		// We do not want to return these errors, but rather wait for the
+		// designated RequeueAfter to expire and try again.
+		// However, not returning an error will cause the patch helper to
+		// patch the observed generation, which we do not want. So we ignore
+		// these errors here after patching.
+		retErr = interrors.Ignore(retErr, errWaitForDependency, errWaitForChart)
+
 		if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
 			if !obj.DeletionTimestamp.IsZero() {
 				err = apierrutil.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) })
@@ -230,7 +243,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 
 			// Exponential backoff would cause execution to be prolonged too much,
 			// instead we requeue on a fixed interval.
-			return ctrl.Result{RequeueAfter: r.requeueDependency}, nil
+			return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency
 		}
 
 		log.Info("all dependencies are ready")
@@ -262,7 +275,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		conditions.MarkFalse(obj, meta.ReadyCondition, "HelmChartNotReady", msg)
 		// Do not requeue immediately, when the artifact is created
 		// the watcher should trigger a reconciliation.
-		return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), nil
+		return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), errWaitForChart
 	}
 
 	// Compose values based from the spec and references.
@@ -276,10 +289,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 	loadedChart, err := loader.SecureLoadChartFromURL(r.httpClient, hc.GetArtifact().URL, hc.GetArtifact().Digest)
 	if err != nil {
 		if errors.Is(err, loader.ErrFileNotFound) {
-			msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency)
+			msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency.String())
 			conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg)
 			log.Info(msg)
-			return ctrl.Result{RequeueAfter: r.requeueDependency}, nil
+			return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency
 		}
 
 		conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error()))

internal/controller/helmrelease_controller_test.go

@@ -108,7 +108,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
-		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(err).To(Equal(errWaitForDependency))
 		g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
@@ -222,7 +222,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
-		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(err).To(Equal(errWaitForChart))
 		g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
@@ -274,7 +274,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
-		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(err).To(Equal(errWaitForChart))
 		g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
@@ -326,7 +326,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
-		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(err).To(Equal(errWaitForChart))
 		g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
@@ -438,7 +438,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
-		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(err).To(Equal(errWaitForDependency))
 		g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{

internal/errors/ignore.go

@@ -0,0 +1,25 @@
+/*
+Copyright 2023 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package errors
+
+// Ignore returns nil if err is equal to any of the errs.
+func Ignore(err error, errs ...error) error {
+	if IsOneOf(err, errs...) {
+		return nil
+	}
+	return err
+}

internal/errors/ignore_test.go

@@ -0,0 +1,40 @@
+/*
+Copyright 2023 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package errors
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestIgnore(t *testing.T) {
+	err1 := errors.New("error1")
+	err2 := errors.New("error2")
+
+	if err := Ignore(err1, err1, err2); err != nil {
+		t.Errorf("Expected Ignore to return nil when the error is in the list, but got %v", err)
+	}
+
+	err3 := errors.New("error3")
+	if err := Ignore(err3, err1, err2); !errors.Is(err, err3) {
+		t.Errorf("Expected Ignore to return the error when it is not in the list, but got %v", err)
+	}
+
+	if err := Ignore(err1); !errors.Is(err, err1) {
+		t.Errorf("Expected Ignore to return the error with an empty list of errors, but got %v", err)
+	}
+}