[helm] revert changes made to release equality comparison (#5097)

* Revert "fix: issue-5041 (#5042)"

This reverts commit 57ac0fe.

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>

* Revert "fix(helm): properly compare existing and candidate releases (#4937)"

This reverts commit cae12a2.

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>

* add changelog fragment

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>

changelog/fragments/revert-helm-equality-comparison.yaml

@@ -0,0 +1,20 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      For helm-based operators, reverted the following PRs which modified helm release
+      equality comparison.
+      - https://github.com/operator-framework/operator-sdk/pull/5042
+      - https://github.com/operator-framework/operator-sdk/pull/4937
+
+    # kind is one of:
+    # - addition
+    # - change
+    # - deprecation
+    # - removal
+    # - bugfix
+    kind: change
+
+    # Is this a breaking change?
+    breaking: false
+

internal/helm/release/manager.go

@@ -20,7 +20,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"reflect"
 	"strings"
 
 	jsonpatch "gomodules.xyz/jsonpatch/v3"
@@ -33,7 +32,6 @@ import (
 	"helm.sh/helm/v3/pkg/storage/driver"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
-	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -127,54 +125,22 @@ func (m *manager) Sync(ctx context.Context) error {
 	m.deployedRelease = deployedRelease
 	m.isInstalled = true
 
-	m.isUpgradeRequired, err = m.isUpgrade(deployedRelease)
+	// Get the next candidate release to determine if an upgrade is necessary.
+	candidateRelease, err := m.getCandidateRelease(m.namespace, m.releaseName, m.chart, m.values)
 	if err != nil {
-		return fmt.Errorf("failed to get upgrade status: %w", err)
+		return fmt.Errorf("failed to get candidate release: %w", err)
 	}
+	if deployedRelease.Manifest != candidateRelease.Manifest {
+		m.isUpgradeRequired = true
+	}
+
 	return nil
 }
 
 func notFoundErr(err error) bool {
 	return err != nil && strings.Contains(err.Error(), "not found")
 }
 
-// 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, nil
-	}
-
-	// Judging whether to skip updates
-	skip := m.namespace == deployedRelease.Namespace
-	skip = skip && m.releaseName == deployedRelease.Name
-
-	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) {
 	deployedRelease, err := m.storageBackend.Deployed(m.releaseName)
 	if err != nil {
@@ -186,6 +152,14 @@ func (m manager) getDeployedRelease() (*rpb.Release, error) {
 	return deployedRelease, nil
 }
 
+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)
+}
+
 // InstallRelease performs a Helm release install.
 func (m manager) InstallRelease(ctx context.Context, opts ...InstallOption) (*rpb.Release, error) {
 	install := action.NewInstall(m.actionConfig)

internal/helm/release/manager_test.go

@@ -15,21 +15,18 @@
 package release
 
 import (
-	"bytes"
-	"encoding/json"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
-	cpb "helm.sh/helm/v3/pkg/chart"
-	lpb "helm.sh/helm/v3/pkg/chart/loader"
-	rpb "helm.sh/helm/v3/pkg/release"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	apitypes "k8s.io/apimachinery/pkg/types"
+	"k8s.io/cli-runtime/pkg/resource"
+
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
+	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/runtime"
-	apitypes "k8s.io/apimachinery/pkg/types"
-	"k8s.io/cli-runtime/pkg/resource"
 )
 
 func newTestUnstructured(containers []interface{}) *unstructured.Unstructured {
@@ -216,85 +213,3 @@ func TestManagerGenerateStrategicMergePatch(t *testing.T) {
 		assert.Equal(t, test.patch, string(diff))
 	}
 }
-
-func TestManagerisUpgrade(t *testing.T) {
-	tests := []struct {
-		name            string
-		releaseName     string
-		releaseNs       string
-		values          map[string]interface{}
-		chart           *cpb.Chart
-		deployedRelease *rpb.Release
-		want            bool
-	}{
-		{
-			name:            "ok",
-			releaseName:     "deployed",
-			releaseNs:       "deployed-ns",
-			values:          map[string]interface{}{"key": "value"},
-			chart:           newTestChart(t, "./testdata/simple"),
-			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{"key": "value"}, "deployed", "deployed-ns"),
-			want:            false,
-		},
-		{
-			name:            "different chart",
-			releaseName:     "deployed",
-			releaseNs:       "deployed-ns",
-			values:          map[string]interface{}{"key": "value"},
-			chart:           newTestChart(t, "./testdata/simple"),
-			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simpledf"), map[string]interface{}{"key": "value"}, "deployed", "deployed-ns"),
-			want:            true,
-		},
-		{
-			name:            "different values",
-			releaseName:     "deployed",
-			releaseNs:       "deployed-ns",
-			values:          map[string]interface{}{"key": "1", "int": int32(1)},
-			chart:           newTestChart(t, "./testdata/simple"),
-			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) {
-			m := manager{
-				releaseName: test.releaseName,
-				namespace:   test.releaseNs,
-				values:      test.values,
-				chart:       test.chart,
-			}
-			isUpgrade, err := m.isUpgrade(test.deployedRelease)
-			assert.Equal(t, test.want, isUpgrade)
-			assert.Equal(t, nil, err)
-		})
-	}
-}
-
-func newTestChart(t *testing.T, path string) *cpb.Chart {
-	chart, err := lpb.Load(path)
-	assert.Nil(t, err)
-	return chart
-}
-
-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,
-		Version:   1,
-	})
-
-	buffer := &bytes.Buffer{}
-	_ = json.NewEncoder(buffer).Encode(chart)
-	_ = json.NewDecoder(buffer).Decode(release.Chart)
-	release.Config = values
-	return release
-}