Merge pull request #977 from fluxcd/backport-972-to-release/v1.0.x

[release/v1.0.x] PostRenderersDigest observation improvements

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)
 			}