Repository: operator-framework/operator-sdk (go)

Issue:
Helm operator logs full deploy yaml every reconcile cycle even when no changes after upgrading to 1.9 from 1.7
## Bug Report

We have an operator built on the operator-sdk and its helm-operator.  After upgrading to sdk 1.9 from 1.7, we are now seeing the operator dumps out the full deployment yaml in the info log at every reconcile cycle, even there are no changes at all.  There are no patch requests sent to the kube apiserver.  This behavior is new in 1.9; it didn't exist in 1.7.

#### What did you do?

Follow the procedure [here](https://github.com/turbonomic/t8c-install/blob/master/DEPLOY.md) to deploy the operator in a Kubernetes cluster.  Tailing the operator log and we will see it dumping out logs of the full deployment yaml every minute.  Here is an excerpt of the logging at the beginning of each reconcile cycle, and [attached](https://github.com/operator-framework/operator-sdk/files/6755322/t8c-operator.log) please find the entire log of a full deployment yaml printed out at each cycle.

```
{"level":"info","ts":1625235104.0440075,"logger":"helm.controller","msg":"Upgraded release","namespace":"turbonomic","name":"xl-release","apiVersion":"charts.helm.k8s.io/v1alpha1","kind":"Xl","release":"xl-release","force":false}
 ---
 # Source: xl/templates/telemetry-secret.yaml
 apiVersion: v1
 kind: Secret
 metadata:
   name: prometheus-datacloud-gateway
   labels:
     app.kubernetes.io/name: xl
     app.kubernetes.io/instance: xl-release
     app.kubernetes.io/managed-by: Helm
 type: Opaque
 data:
...
```

#### What did you expect to see?

We expect to see no such logging at the info level when there're no changes in the custom resource of the operator, just like what it was when we built with operator-sdk 1.7.  With 1.7, we only see the first line:
```
{"level":"info","ts":1625235104.0440075,"logger":"helm.controller","msg":"Upgraded release","namespace":"turbonomic","name":"xl-release","apiVersion":"charts.helm.k8s.io/v1alpha1","kind":"Xl","release":"xl-release","force":false}
```

#### What did you see instead? Under which circumstances?

We see logging of the full deployment yaml every minute.  [Attached](https://github.com/operator-framework/operator-sdk/files/6755322/t8c-operator.log) please find the entire log of a full deployment yaml printed out at each cycle.

#### Environment

**Operator type:**
helm operator

**Kubernetes cluster type:**
vanilla

`$ operator-sdk version`
```
operator-sdk version
operator-sdk version: "v1.9.0-6-g2781cf89", commit: "2781cf89a0d51a2783fa979a50ce8fdb22c820a9", kubernetes version: "v1.20.2", go version: "go1.16.3", GOOS: "darwin", GOARCH: "amd64"
```

`$ go version` (if language is Go)
N/A

`$ kubectl version`
```
Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.0", GitCommit:"cb303e613a121a29364f75cc67d3d580833a7479", GitTreeState:"clean", BuildDate:"2021-04-08T21:15:16Z", GoVersion:"go1.16.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.10", GitCommit:"62876fc6d93e891aa7fbe19771e6a6c03773b0f7", GitTreeState:"clean", BuildDate:"2020-10-15T01:43:56Z", GoVersion:"go1.13.15", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.21) and server (1.18) exceeds the supported minor version skew of +/-1
```

#### Additional context
We've done some investigation, and were able to isolate the difference was introduced in the following commit.  We don't know if it's intentional.
https://github.com/operator-framework/operator-sdk/commit/cae12a289d707156102ab4dd2a5f568c3d092e82

In particular, it is this line of the new code that made the difference:
https://github.com/operator-framework/operator-sdk/blob/master/internal/helm/release/manager.go#L146
It now compares `m.chart` and `deployedRelease.Chart`.  It used to just compare the `deployedRelease.Manifest` and `candidateRelease.Manifest`.  That seems intended, but [m.chart](https://github.com/operator-framework/operator-sdk/files/6755721/m.chart.txt) and
[deployedRelease.Chart](https://github.com/operator-framework/operator-sdk/files/6755727/deployedRelease.Chart.txt) seem very different, at least in the case of our operator (please see the attached).

Pull request:
fix: issue-5041
Signed-off-by: cndoit18 <cndoit18@outlook.com>

**Description of the change:**
fix #5041 

**Motivation for the change:**


**Checklist**

If the pull request includes user-facing changes, extra documentation is required:
- [x] Add a new changelog fragment in `changelog/fragments` (see [`changelog/fragments/00-template.yaml`](https://github.com/operator-framework/operator-sdk/tree/master/changelog/fragments/00-template.yaml))
- [ ] Add or update relevant sections of the docs website in [`website/content/en/docs`](https://github.com/operator-framework/operator-sdk/tree/master/website/content/en/docs)


Patch:
diff --git a/changelog/fragments/helm-deepequals.yaml b/changelog/fragments/helm-deepequals.yaml
new file mode 100644
index 00000000..0a331945
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go
index 7ee19a0b..ffeb40b2 100644
--- a/internal/helm/release/manager.go
+++ b/internal/helm/release/manager.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"reflect"
 	"strings"
 
 	jsonpatch "gomodules.xyz/jsonpatch/v3"
@@ -126,8 +127,10 @@ 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
 }
 
@@ -135,18 +138,41 @@ func notFoundErr(err error) bool {
 	return err != nil && strings.Contains(err.Error(), "not found")
 }
 
-func (m manager) isUpgrade(deployedRelease *rpb.Release) bool {
+// 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
 	}
 
 	// 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)
 
-	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) {


Test patch:
diff --git a/internal/helm/release/manager_test.go b/internal/helm/release/manager_test.go
index f930374e..70c5e5b5 100644
--- a/internal/helm/release/manager_test.go
+++ b/internal/helm/release/manager_test.go
@@ -15,6 +15,8 @@
 package release
 
 import (
+	"bytes"
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -247,11 +249,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) {
@@ -261,8 +272,9 @@ func TestManagerisUpgrade(t *testing.T) {
 				values:      test.values,
 				chart:       test.chart,
 			}
-			isUpgrade := m.isUpgrade(test.deployedRelease)
+			isUpgrade, err := m.isUpgrade(test.deployedRelease)
 			assert.Equal(t, test.want, isUpgrade)
+			assert.Equal(t, nil, err)
 		})
 	}
 }
@@ -273,13 +285,16 @@ 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,
 	})
+
+	buffer := &bytes.Buffer{}
+	_ = json.NewEncoder(buffer).Encode(chart)
+	_ = json.NewDecoder(buffer).Decode(release.Chart)
 	release.Config = values
 	return release
 }