fix: issue-5041 (#5042)

Signed-off-by: cndoit18 <[email protected]>

Author: cndoit18

changelog/fragments/helm-deepequals.yaml

@@ -0,0 +1,4 @@
+entries:
+  - description: >
+      For Helm-based operators, fixed release equality comparison such that number values are compared and not their types to avoid unnecessary reconciliations.
+    kind: bugfix

internal/helm/release/manager.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"reflect"
 	"strings"
 
 	jsonpatch "gomodules.xyz/jsonpatch/v3"
@@ -126,27 +127,64 @@ func (m *manager) Sync(ctx context.Context) error {
 	m.deployedRelease = deployedRelease
 	m.isInstalled = true
 
-	m.isUpgradeRequired = m.isUpgrade(deployedRelease)
-
+	m.isUpgradeRequired, err = m.isUpgrade(deployedRelease)
+	if err != nil {
+		return fmt.Errorf("failed to get upgrade status: %w", err)
+	}
 	return nil
 }
 
 func notFoundErr(err error) bool {
 	return err != nil && strings.Contains(err.Error(), "not found")
 }
 
-func (m manager) isUpgrade(deployedRelease *rpb.Release) bool {
+func (m manager) getCandidateRelease(namespace, name string, chart *cpb.Chart,
+	values map[string]interface{}) (*rpb.Release, error) {
+	upgrade := action.NewUpgrade(m.actionConfig)
+	upgrade.Namespace = namespace
+	upgrade.DryRun = true
+	return upgrade.Run(name, chart, values)
+}
+
+// This is caused by the different logic of loading from local and loading from secret
+// For example, the Raw field, which has the tag `json:"-"`, causes the Unmarshal to be lost when it into Release
+// We need to make them follow the JSON tag
+// see: https://github.com/helm/helm/blob/cf0c6fed519d48101cd69ce01a355125215ee46f/pkg/storage/driver/util.go#L81
+func equalJSONStruct(a, b interface{}) (bool, error) {
+	if reflect.ValueOf(a).IsNil() || reflect.ValueOf(b).IsNil() {
+		return apiequality.Semantic.DeepEqual(a, b), nil
+	}
+
+	aBuf, bBuf := &bytes.Buffer{}, &bytes.Buffer{}
+	err := json.NewEncoder(aBuf).Encode(a)
+	if err != nil {
+		return false, err
+	}
+	err = json.NewEncoder(bBuf).Encode(b)
+	return aBuf.String() == bBuf.String(), err
+}
+
+func (m manager) isUpgrade(deployedRelease *rpb.Release) (bool, error) {
 	if deployedRelease == nil {
-		return false
+		return false, nil
+	}
+
+	candidateRelease, err := m.getCandidateRelease(m.namespace, m.releaseName, m.chart, m.values)
+	if err != nil {
+		return false, err
 	}
 
 	// Judging whether to skip updates
-	skip := m.namespace == deployedRelease.Namespace
-	skip = skip && m.releaseName == deployedRelease.Name
-	skip = skip && apiequality.Semantic.DeepEqual(m.chart, deployedRelease.Chart)
-	skip = skip && apiequality.Semantic.DeepEqual(m.values, deployedRelease.Config)
+	skip := candidateRelease.Namespace == deployedRelease.Namespace
+	skip = skip && candidateRelease.Name == deployedRelease.Name
 
-	return !skip
+	ok, err := equalJSONStruct(m.chart, deployedRelease.Chart)
+	if err != nil {
+		return false, err
+	}
+	skip = skip && ok
+	ok, err = equalJSONStruct(m.values, deployedRelease.Config)
+	return !(skip && ok), err
 }
 
 func (m manager) getDeployedRelease() (*rpb.Release, error) {

internal/helm/release/manager_test.go

@@ -15,12 +15,18 @@
 package release
 
 import (
+	"io/ioutil"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"helm.sh/helm/v3/pkg/action"
 	cpb "helm.sh/helm/v3/pkg/chart"
 	lpb "helm.sh/helm/v3/pkg/chart/loader"
+	"helm.sh/helm/v3/pkg/chartutil"
+	kubefake "helm.sh/helm/v3/pkg/kube/fake"
 	rpb "helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/storage"
+	"helm.sh/helm/v3/pkg/storage/driver"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -247,11 +253,20 @@ func TestManagerisUpgrade(t *testing.T) {
 			name:            "different values",
 			releaseName:     "deployed",
 			releaseNs:       "deployed-ns",
-			values:          map[string]interface{}{"key": "1"},
+			values:          map[string]interface{}{"key": "1", "int": int32(1)},
 			chart:           newTestChart(t, "./testdata/simple"),
-			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{"key": ""}, "deployed", "deployed-ns"),
+			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{"key": "", "int": int64(1)}, "deployed", "deployed-ns"),
 			want:            true,
 		},
+		{
+			name:            "nil values",
+			releaseName:     "deployed",
+			releaseNs:       "deployed-ns",
+			values:          nil,
+			chart:           newTestChart(t, "./testdata/simple"),
+			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{}, "deployed", "deployed-ns"),
+			want:            false,
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -260,9 +275,17 @@ func TestManagerisUpgrade(t *testing.T) {
 				namespace:   test.releaseNs,
 				values:      test.values,
 				chart:       test.chart,
+				actionConfig: &action.Configuration{
+					Releases:     storage.Init(driver.NewMemory()),
+					KubeClient:   &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}},
+					Capabilities: chartutil.DefaultCapabilities,
+					Log:          t.Logf,
+				},
 			}
-			isUpgrade := m.isUpgrade(test.deployedRelease)
+			assert.Equal(t, nil, m.actionConfig.Releases.Create(test.deployedRelease))
+			isUpgrade, err := m.isUpgrade(test.deployedRelease)
 			assert.Equal(t, test.want, isUpgrade)
+			assert.Equal(t, nil, err)
 		})
 	}
 }
@@ -273,13 +296,14 @@ func newTestChart(t *testing.T, path string) *cpb.Chart {
 	return chart
 }
 
-func newTestRelease(chart *cpb.Chart, values map[string]interface{}, name, namespace string) *rpb.Release {
+func newTestRelease(chart *cpb.Chart, values map[string]interface{}, name, namespace string) *rpb.Release { // nolint: unparam
 	release := rpb.Mock(&rpb.MockReleaseOptions{
 		Name:      name,
 		Namespace: namespace,
-		Chart:     chart,
 		Version:   1,
+		Chart:     chart,
 	})
+
 	release.Config = values
 	return release
 }