PostRenderersDigest observation improvements

Move the post renderers digest set/update code from summarize() to
atomic release reconciler in order to update the observation only at the
end of a successful reconciliation. summarize() is for summarizing the
status conditions and is also called by all the other action
sub-reconcilers, which can update the post renderers digest observation
too early.
Updating the observed post renderers digest at the very end of a
reconciliation introduces an issue where a digest mismatch in
DetermineReleaseState() could result in the release to get stuck in a
loop as even after running an upgrade due to post renderers value, the
new observation isn't reflected immediately in the middle of atomic
reconciliation. This can be solved by checking post renderers digest
value only for new configurations where the object generation and the
ready status condition observed generations don't match, in other words
when the generation of a configuration has not be processed. This
assumes that an upgrade due to any other reason also takes into account
the post renderers value and need not be checked separately for the same
config generation.

Signed-off-by: Sunny <github@darkowlzz.space>
(cherry picked from commit 63f7a76319)

internal/reconcile/atomic_release.go

@@ -38,7 +38,9 @@ import (
 	v2 "github.com/fluxcd/helm-controller/api/v2"
 	"github.com/fluxcd/helm-controller/internal/action"
 	"github.com/fluxcd/helm-controller/internal/diff"
+	"github.com/fluxcd/helm-controller/internal/digest"
 	interrors "github.com/fluxcd/helm-controller/internal/errors"
+	"github.com/fluxcd/helm-controller/internal/postrender"
 )
 
 // OwnedConditions is a list of Condition types owned by the HelmRelease object.
@@ -210,6 +212,15 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 				// written to Ready.
 				summarize(req)
 
+				// remove stale post-renderers digest on successful reconciliation.
+				if conditions.IsReady(req.Object) {
+					req.Object.Status.ObservedPostRenderersDigest = ""
+					if req.Object.Spec.PostRenderers != nil {
+						// Update the post-renderers digest if the post-renderers exist.
+						req.Object.Status.ObservedPostRenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String()
+					}
+				}
+
 				return nil
 			}
 

internal/reconcile/atomic_release_test.go

@@ -43,7 +43,9 @@ import (
 
 	v2 "github.com/fluxcd/helm-controller/api/v2"
 	"github.com/fluxcd/helm-controller/internal/action"
+	"github.com/fluxcd/helm-controller/internal/digest"
 	"github.com/fluxcd/helm-controller/internal/kube"
+	"github.com/fluxcd/helm-controller/internal/postrender"
 	"github.com/fluxcd/helm-controller/internal/release"
 	"github.com/fluxcd/helm-controller/internal/testutil"
 )
@@ -1067,6 +1069,229 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 	}
 }
 
+func TestAtomicRelease_Reconcile_PostRenderers_Scenarios(t *testing.T) {
+	tests := []struct {
+		name              string
+		releases          func(namespace string) []*helmrelease.Release
+		spec              func(spec *v2.HelmReleaseSpec)
+		values            map[string]interface{}
+		status            func(releases []*helmrelease.Release) v2.HelmReleaseStatus
+		wantDigest        string
+		wantReleaseAction v2.ReleaseAction
+	}{
+		{
+			name: "addition of post renderers",
+			releases: func(namespace string) []*helmrelease.Release {
+				return []*helmrelease.Release{
+					testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+						Name:      mockReleaseName,
+						Namespace: namespace,
+						Version:   1,
+						Status:    helmrelease.StatusDeployed,
+						Chart:     testutil.BuildChart(),
+					}, testutil.ReleaseWithConfig(nil)),
+				}
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.PostRenderers = postRenderers
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
+					},
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 1,
+						},
+					},
+				}
+			},
+			wantDigest:        postrender.Digest(digest.Canonical, postRenderers).String(),
+			wantReleaseAction: v2.ReleaseActionUpgrade,
+		},
+		{
+			name: "config change and addition of post renderers",
+			releases: func(namespace string) []*helmrelease.Release {
+				return []*helmrelease.Release{
+					testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+						Name:      mockReleaseName,
+						Namespace: namespace,
+						Version:   1,
+						Status:    helmrelease.StatusDeployed,
+						Chart:     testutil.BuildChart(),
+					}, testutil.ReleaseWithConfig(nil)),
+				}
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.PostRenderers = postRenderers
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
+					},
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 1,
+						},
+					},
+				}
+			},
+			values:            map[string]interface{}{"foo": "baz"},
+			wantDigest:        postrender.Digest(digest.Canonical, postRenderers).String(),
+			wantReleaseAction: v2.ReleaseActionUpgrade,
+		},
+		{
+			name: "existing and new post renderers value",
+			releases: func(namespace string) []*helmrelease.Release {
+				return []*helmrelease.Release{
+					testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+						Name:      mockReleaseName,
+						Namespace: namespace,
+						Version:   1,
+						Status:    helmrelease.StatusDeployed,
+						Chart:     testutil.BuildChart(),
+					}, testutil.ReleaseWithConfig(nil)),
+				}
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.PostRenderers = postRenderers2
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
+					},
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 1,
+						},
+					},
+					ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
+				}
+			},
+			wantDigest:        postrender.Digest(digest.Canonical, postRenderers2).String(),
+			wantReleaseAction: v2.ReleaseActionUpgrade,
+		},
+		{
+			name: "post renderers mismatch remains in sync for processed config",
+			releases: func(namespace string) []*helmrelease.Release {
+				return []*helmrelease.Release{
+					testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+						Name:      mockReleaseName,
+						Namespace: namespace,
+						Version:   1,
+						Status:    helmrelease.StatusDeployed,
+						Chart:     testutil.BuildChart(),
+					}, testutil.ReleaseWithConfig(nil)),
+				}
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.PostRenderers = postRenderers2
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
+					},
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 2, // This is used to set processed config generation.
+						},
+					},
+					ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
+				}
+			},
+			wantDigest:        postrender.Digest(digest.Canonical, postRenderers2).String(),
+			wantReleaseAction: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace)
+			g.Expect(err).NotTo(HaveOccurred())
+			t.Cleanup(func() {
+				_ = testEnv.Delete(context.TODO(), namedNS)
+			})
+			releaseNamespace := namedNS.Name
+
+			releases := tt.releases(releaseNamespace)
+
+			obj := &v2.HelmRelease{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      mockReleaseName,
+					Namespace: releaseNamespace,
+					// Set a higher generation value to allow setting
+					// observations in previous generations.
+					Generation: 2,
+				},
+				Spec: v2.HelmReleaseSpec{
+					ReleaseName:      mockReleaseName,
+					TargetNamespace:  releaseNamespace,
+					StorageNamespace: releaseNamespace,
+					Timeout:          &metav1.Duration{Duration: 100 * time.Millisecond},
+				},
+			}
+
+			if tt.spec != nil {
+				tt.spec(&obj.Spec)
+			}
+			if tt.status != nil {
+				obj.Status = tt.status(releases)
+			}
+
+			getter, err := RESTClientGetterFromManager(testEnv.Manager, obj.GetReleaseNamespace())
+			g.Expect(err).ToNot(HaveOccurred())
+
+			cfg, err := action.NewConfigFactory(getter,
+				action.WithStorage(action.DefaultStorageDriver, obj.GetStorageNamespace()),
+			)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			store := helmstorage.Init(cfg.Driver)
+			for _, r := range releases {
+				g.Expect(store.Create(r)).To(Succeed())
+			}
+
+			// We use a fake client here to allow us to work with a minimal release
+			// object mock. As the fake client does not perform any validation.
+			// However, for the Helm storage driver to work, we need a real client
+			// which is therefore initialized separately above.
+			client := fake.NewClientBuilder().
+				WithScheme(testEnv.Scheme()).
+				WithObjects(obj).
+				WithStatusSubresource(&v2.HelmRelease{}).
+				Build()
+			patchHelper := patch.NewSerialPatcher(obj, client)
+			recorder := new(record.FakeRecorder)
+
+			req := &Request{
+				Object: obj,
+				Chart:  testutil.BuildChart(),
+				Values: tt.values,
+			}
+
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			g.Expect(obj.Status.ObservedPostRenderersDigest).To(Equal(tt.wantDigest))
+			g.Expect(obj.Status.LastAttemptedReleaseAction).To(Equal(tt.wantReleaseAction))
+		})
+	}
+}
+
 func TestAtomicRelease_actionForState(t *testing.T) {
 	tests := []struct {
 		name             string

internal/reconcile/release.go

@@ -28,8 +28,6 @@ import (
 
 	v2 "github.com/fluxcd/helm-controller/api/v2"
 	"github.com/fluxcd/helm-controller/internal/action"
-	"github.com/fluxcd/helm-controller/internal/digest"
-	"github.com/fluxcd/helm-controller/internal/postrender"
 	"github.com/fluxcd/helm-controller/internal/release"
 	"github.com/fluxcd/helm-controller/internal/storage"
 )
@@ -190,15 +188,6 @@ func summarize(req *Request) {
 		Message:            conds[0].Message,
 		ObservedGeneration: req.Object.Generation,
 	})
-
-	// remove stale post-renderers digest
-	if conditions.Get(req.Object, meta.ReadyCondition).Status == metav1.ConditionTrue {
-		req.Object.Status.ObservedPostRenderersDigest = ""
-		if req.Object.Spec.PostRenderers != nil {
-			// Update the post-renderers digest if the post-renderers exist.
-			req.Object.Status.ObservedPostRenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String()
-		}
-	}
 }
 
 // eventMessageWithLog returns an event message composed out of the given

internal/reconcile/release_test.go

@@ -31,8 +31,6 @@ import (
 
 	v2 "github.com/fluxcd/helm-controller/api/v2"
 	"github.com/fluxcd/helm-controller/internal/action"
-	"github.com/fluxcd/helm-controller/internal/digest"
-	"github.com/fluxcd/helm-controller/internal/postrender"
 )
 
 const (
@@ -50,14 +48,14 @@ var (
 							Kind: "Deployment",
 							Name: "test",
 						},
-						Patch: `|-
-						apiVersion: apps/v1
-						kind: Deployment
-						metadata:
-							name: test
-						spec:
-							replicas: 2
-					`,
+						Patch: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: test
+spec:
+  replicas: 2
+`,
 					},
 				},
 			},
@@ -73,14 +71,14 @@ var (
 							Kind: "Deployment",
 							Name: "test",
 						},
-						Patch: `|-
-						apiVersion: apps/v1
-						kind: Deployment
-						metadata:
-							name: test
-						spec:
-							replicas: 3
-					`,
+						Patch: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: test
+spec:
+  replicas: 3
+`,
 					},
 				},
 			},
@@ -509,96 +507,6 @@ func Test_summarize(t *testing.T) {
 				},
 			},
 		},
-		{
-			name:       "with postrender",
-			generation: 1,
-			status: v2.HelmReleaseStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:               v2.ReleasedCondition,
-						Status:             metav1.ConditionTrue,
-						Reason:             v2.InstallSucceededReason,
-						Message:            "Install complete",
-						ObservedGeneration: 1,
-					},
-				},
-				ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
-			},
-			spec: &v2.HelmReleaseSpec{
-				PostRenderers: postRenderers2,
-			},
-			expectedStatus: &v2.HelmReleaseStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:               meta.ReadyCondition,
-						Status:             metav1.ConditionTrue,
-						Reason:             v2.InstallSucceededReason,
-						Message:            "Install complete",
-						ObservedGeneration: 1,
-					},
-					{
-						Type:               v2.ReleasedCondition,
-						Status:             metav1.ConditionTrue,
-						Reason:             v2.InstallSucceededReason,
-						Message:            "Install complete",
-						ObservedGeneration: 1,
-					},
-				},
-				ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers2).String(),
-			},
-		},
-		{
-			name:       "with PostRenderers and Remediaction success",
-			generation: 1,
-			status: v2.HelmReleaseStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:               v2.ReleasedCondition,
-						Status:             metav1.ConditionFalse,
-						Reason:             v2.UpgradeFailedReason,
-						Message:            "Upgrade failure",
-						ObservedGeneration: 1,
-					},
-					{
-						Type:               v2.RemediatedCondition,
-						Status:             metav1.ConditionTrue,
-						Reason:             v2.RollbackSucceededReason,
-						Message:            "Uninstall complete",
-						ObservedGeneration: 1,
-					},
-				},
-				ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
-			},
-			spec: &v2.HelmReleaseSpec{
-				PostRenderers: postRenderers2,
-			},
-			expectedStatus: &v2.HelmReleaseStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:               meta.ReadyCondition,
-						Status:             metav1.ConditionFalse,
-						Reason:             v2.RollbackSucceededReason,
-						Message:            "Uninstall complete",
-						ObservedGeneration: 1,
-					},
-					{
-						Type:               v2.ReleasedCondition,
-						Status:             metav1.ConditionFalse,
-						Reason:             v2.UpgradeFailedReason,
-						Message:            "Upgrade failure",
-						ObservedGeneration: 1,
-					},
-					{
-						Type:               v2.RemediatedCondition,
-						Status:             metav1.ConditionTrue,
-						Reason:             v2.RollbackSucceededReason,
-						Message:            "Uninstall complete",
-						ObservedGeneration: 1,
-					},
-				},
-				ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
-			},
-		},
 	}
 
 	for _, tt := range tests {

internal/reconcile/state.go

@@ -21,6 +21,8 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/runtime/conditions"
 	"github.com/fluxcd/pkg/ssa/jsondiff"
 	"helm.sh/helm/v3/pkg/kube"
 	helmrelease "helm.sh/helm/v3/pkg/release"
@@ -143,13 +145,22 @@ func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req *
 			}
 		}
 
-		// Verify if postrender digest has changed
-		var postrenderersDigest string
-		if req.Object.Spec.PostRenderers != nil {
-			postrenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String()
-		}
-		if postrenderersDigest != req.Object.Status.ObservedPostRenderersDigest {
-			return ReleaseState{Status: ReleaseStatusOutOfSync, Reason: "postrender digest has changed"}, nil
+		// Verify if postrender digest has changed if config has not been
+		// processed. For the processed or partially processed generation, the
+		// updated observation will only be reflected at the end of a successful
+		// reconciliation.  Comparing here would result the reconciliation to
+		// get stuck in this check due to a mismatch forever.  The value can't
+		// change without a new generation. Hence, compare the observed digest
+		// for new generations only.
+		ready := conditions.Get(req.Object, meta.ReadyCondition)
+		if ready != nil && ready.ObservedGeneration != req.Object.Generation {
+			var postrenderersDigest string
+			if req.Object.Spec.PostRenderers != nil {
+				postrenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String()
+			}
+			if postrenderersDigest != req.Object.Status.ObservedPostRenderersDigest {
+				return ReleaseState{Status: ReleaseStatusOutOfSync, Reason: "postrenderers digest has changed"}, nil
+			}
 		}
 
 		// For the further determination of test results, we look at the

internal/reconcile/state_test.go

@@ -27,8 +27,10 @@ import (
 	helmrelease "helm.sh/helm/v3/pkg/release"
 	helmstorage "helm.sh/helm/v3/pkg/storage"
 	helmdriver "helm.sh/helm/v3/pkg/storage/driver"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
+	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/ssa/jsondiff"
 	ssanormalize "github.com/fluxcd/pkg/ssa/normalize"
 	ssautil "github.com/fluxcd/pkg/ssa/utils"
@@ -474,6 +476,13 @@ func Test_DetermineReleaseState(t *testing.T) {
 						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
 					},
 					ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 1,
+						},
+					},
 				}
 			},
 			chart:  testutil.BuildChart(),
@@ -482,6 +491,41 @@ func Test_DetermineReleaseState(t *testing.T) {
 				Status: ReleaseStatusOutOfSync,
 			},
 		},
+		{
+			name: "postRenderers mismatch ignored for processed generation",
+			releases: []*helmrelease.Release{
+				testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+					Name:      mockReleaseName,
+					Namespace: mockReleaseNamespace,
+					Version:   1,
+					Status:    helmrelease.StatusDeployed,
+					Chart:     testutil.BuildChart(),
+				}, testutil.ReleaseWithConfig(map[string]interface{}{"foo": "bar"})),
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.PostRenderers = postRenderers2
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
+					},
+					ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(),
+					Conditions: []metav1.Condition{
+						{
+							Type:               meta.ReadyCondition,
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 2,
+						},
+					},
+				}
+			},
+			chart:  testutil.BuildChart(),
+			values: map[string]interface{}{"foo": "bar"},
+			want: ReleaseState{
+				Status: ReleaseStatusInSync,
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -495,6 +539,10 @@ func Test_DetermineReleaseState(t *testing.T) {
 					StorageNamespace: mockReleaseNamespace,
 				},
 			}
+			// Set a non-zero generation so that old observations can be set on
+			// the object status.
+			obj.Generation = 2
+
 			if tt.spec != nil {
 				tt.spec(&obj.Spec)
 			}