diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index d26324a78..7ae18c986 100644 --- a/internal/reconcile/atomic_release.go +++ b/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 } diff --git a/internal/reconcile/atomic_release_test.go b/internal/reconcile/atomic_release_test.go index 108f39154..7081a290d 100644 --- a/internal/reconcile/atomic_release_test.go +++ b/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 diff --git a/internal/reconcile/release.go b/internal/reconcile/release.go index 8e8564e66..269b1a2f8 100644 --- a/internal/reconcile/release.go +++ b/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 diff --git a/internal/reconcile/release_test.go b/internal/reconcile/release_test.go index 0cd320e78..d2cede74f 100644 --- a/internal/reconcile/release_test.go +++ b/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 { diff --git a/internal/reconcile/state.go b/internal/reconcile/state.go index be255d11c..247a752a7 100644 --- a/internal/reconcile/state.go +++ b/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 diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index bbd844f94..fb51b8648 100644 --- a/internal/reconcile/state_test.go +++ b/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) }