Remove stale remediated condition when in-sync

Remediation can roll back to a version that matches with the next good
config. In such situation, release will be in-sync and no action will be
performed. The status conditions will continue to show Remediated=True
and Released=False. Check and remove stale Remediated condition and add
a Released=True condition with message constructed from the latest
release.

Introduce replaceCondition() to replaces target condition with a
replacement condition, retaining the transition time. This helps ensure
that the last transition time of releases don't change when a release is
marked from remediated to released.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

Author: darkowlzz

internal/reconcile/atomic_release.go

@@ -25,6 +25,7 @@ import (
 
 	"helm.sh/helm/v3/pkg/kube"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 
@@ -315,6 +316,18 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 			return NewUpgrade(r.configFactory, r.eventRecorder), nil
 		}
 
+		// Since the release is in-sync, remove any remediated condition if
+		// present and replace it with upgrade succeeded condition.
+		// This can happen when the current release, which is the result of a
+		// rollback remediation, matches the new desired configuration due to
+		// having the same chart version and values. As a result, we are already
+		// in-sync without performing a release action.
+		if conditions.IsTrue(req.Object, v2.RemediatedCondition) {
+			cur := req.Object.Status.History.Latest()
+			msg := fmt.Sprintf(fmtUpgradeSuccess, cur.FullReleaseName(), cur.VersionedChartName())
+			replaceCondition(req.Object, v2.RemediatedCondition, v2.ReleasedCondition, v2.UpgradeSucceededReason, msg, metav1.ConditionTrue)
+		}
+
 		return nil, nil
 	case ReleaseStatusLocked:
 		log.Info(msgWithReason("release locked", state.Reason))
@@ -378,6 +391,21 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		return nil, nil
 	case ReleaseStatusUntested:
 		log.Info(msgWithReason("release has not been tested", state.Reason))
+
+		// Since an untested release indicates that the release is already
+		// in-sync, remove any remediated condition if present and replace it
+		// with upgrade succeeded condition.
+		// This can happen when an untested current release, which is the result
+		// of a rollback remediation, matches the new desired configuration due
+		// to having the same chart version and values, and has test enabled. As
+		// a result, we are already in-sync without performing a release action,
+		// the existing release needs to undergo testing.
+		if conditions.IsTrue(req.Object, v2.RemediatedCondition) {
+			cur := req.Object.Status.History.Latest()
+			msg := fmt.Sprintf(fmtUpgradeSuccess, cur.FullReleaseName(), cur.VersionedChartName())
+			replaceCondition(req.Object, v2.RemediatedCondition, v2.ReleasedCondition, v2.UpgradeSucceededReason, msg, metav1.ConditionTrue)
+		}
+
 		return NewTest(r.configFactory, r.eventRecorder), nil
 	case ReleaseStatusFailed:
 		log.Info(msgWithReason("release is in a failed state", state.Reason))
@@ -491,3 +519,24 @@ func timeoutForAction(action ActionReconciler, obj *v2.HelmRelease) time.Duratio
 		return obj.GetTimeout().Duration
 	}
 }
+
+// replaceCondition replaces existing target condition with replacement
+// condition, if present, for the given values, retaining the
+// LastTransitionTime.
+func replaceCondition(obj *v2.HelmRelease, target string, replacement string, reason string, msg string, status metav1.ConditionStatus) {
+	c := conditions.Get(obj, target)
+	if c != nil {
+		// Remove any existing replacement condition to retain the
+		// LastTransitionTime set here. If the state of the new condition
+		// changes an existing condition, the LastTransitionTime is updated to
+		// the current time.
+		// Refer https://github.com/fluxcd/pkg/blob/runtime/v0.43.0/runtime/conditions/setter.go#L54-L55.
+		conditions.Delete(obj, replacement)
+		c.Status = status
+		c.Type = replacement
+		c.Reason = reason
+		c.Message = msg
+		conditions.Set(obj, c)
+		conditions.Delete(obj, target)
+	}
+}

internal/reconcile/atomic_release_test.go

@@ -1015,15 +1015,16 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 
 func TestAtomicRelease_actionForState(t *testing.T) {
 	tests := []struct {
-		name        string
-		releases    []*helmrelease.Release
-		annotations map[string]string
-		spec        func(spec *v2.HelmReleaseSpec)
-		status      func(releases []*helmrelease.Release) v2.HelmReleaseStatus
-		state       ReleaseState
-		want        ActionReconciler
-		wantEvent   *corev1.Event
-		wantErr     error
+		name             string
+		releases         []*helmrelease.Release
+		annotations      map[string]string
+		spec             func(spec *v2.HelmReleaseSpec)
+		status           func(releases []*helmrelease.Release) v2.HelmReleaseStatus
+		state            ReleaseState
+		want             ActionReconciler
+		wantEvent        *corev1.Event
+		wantErr          error
+		assertConditions []metav1.Condition
 	}{
 		{
 			name: "in-sync release does not trigger any action",
@@ -1053,6 +1054,25 @@ func TestAtomicRelease_actionForState(t *testing.T) {
 			},
 			want: &Upgrade{},
 		},
+		{
+			name: "in-sync release with stale remediated condition",
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						{Version: 1},
+					},
+					Conditions: []metav1.Condition{
+						*conditions.FalseCondition(v2.ReleasedCondition, v2.UpgradeFailedReason, "upgrade failed"),
+						*conditions.TrueCondition(v2.RemediatedCondition, v2.RollbackSucceededReason, "rolled back"),
+					},
+				}
+			},
+			state: ReleaseState{Status: ReleaseStatusInSync},
+			want:  nil,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "upgrade succeeded"),
+			},
+		},
 		{
 			name:  "locked release triggers unlock action",
 			state: ReleaseState{Status: ReleaseStatusLocked},
@@ -1245,6 +1265,25 @@ func TestAtomicRelease_actionForState(t *testing.T) {
 			state: ReleaseState{Status: ReleaseStatusUntested},
 			want:  &Test{},
 		},
+		{
+			name: "untested release with stale remediated condition",
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						{Version: 1},
+					},
+					Conditions: []metav1.Condition{
+						*conditions.FalseCondition(v2.ReleasedCondition, v2.UpgradeFailedReason, "upgrade failed"),
+						*conditions.TrueCondition(v2.RemediatedCondition, v2.RollbackSucceededReason, "rolled back"),
+					},
+				}
+			},
+			state: ReleaseState{Status: ReleaseStatusUntested},
+			want:  &Test{},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "upgrade succeeded"),
+			},
+		},
 		{
 			name:  "failed release without active remediation triggers upgrade",
 			state: ReleaseState{Status: ReleaseStatusFailed},
@@ -1513,6 +1552,122 @@ func TestAtomicRelease_actionForState(t *testing.T) {
 			} else {
 				g.Expect(recorder.GetEvents()).To(BeEmpty())
 			}
+
+			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
+		})
+	}
+}
+
+func Test_replaceCondition(t *testing.T) {
+	g := NewWithT(t)
+	timestamp, err := time.Parse(time.UnixDate, "Wed Feb 25 11:06:39 GMT 2015")
+	g.Expect(err).ToNot(HaveOccurred())
+
+	tests := []struct {
+		name           string
+		conditions     []metav1.Condition
+		target         string
+		replacement    string
+		wantConditions []metav1.Condition
+	}{
+		{
+			name: "both conditions exist",
+			conditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionFalse,
+					Reason:             v2.UpgradeFailedReason,
+					Message:            "upgrade failed",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+				{
+					Type:               v2.RemediatedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.RollbackSucceededReason,
+					Message:            "rollback",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+			target:      v2.RemediatedCondition,
+			replacement: v2.ReleasedCondition,
+			wantConditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.UpgradeSucceededReason,
+					Message:            "foo",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+		},
+		{
+			name: "no existing replacement condition",
+			conditions: []metav1.Condition{
+				{
+					Type:               v2.RemediatedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.RollbackSucceededReason,
+					Message:            "rollback",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+			target:      v2.RemediatedCondition,
+			replacement: v2.ReleasedCondition,
+			wantConditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.UpgradeSucceededReason,
+					Message:            "foo",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+		},
+		{
+			name: "no existing target condition",
+			conditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionFalse,
+					Reason:             v2.UpgradeFailedReason,
+					Message:            "upgrade failed",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+			target:      v2.RemediatedCondition,
+			replacement: v2.ReleasedCondition,
+			wantConditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionFalse,
+					Reason:             v2.UpgradeFailedReason,
+					Message:            "upgrade failed",
+					ObservedGeneration: 1,
+					LastTransitionTime: metav1.NewTime(timestamp),
+				},
+			},
+		},
+		{
+			name:        "no existing target and replacement conditions",
+			target:      v2.RemediatedCondition,
+			replacement: v2.ReleasedCondition,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			obj := &v2.HelmRelease{}
+			obj.Generation = 1
+			obj.Status.Conditions = tt.conditions
+			replaceCondition(obj, tt.target, tt.replacement, v2.UpgradeSucceededReason, "foo", metav1.ConditionTrue)
+			g.Expect(obj.Status.Conditions).To(Equal(tt.wantConditions))
 		})
 	}
 }