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

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

Author: cndoit18

changelog/fragments/helm-randalphanum.yaml

@@ -0,0 +1,4 @@
+entries:
+  - description: >
+      For Helm-based operators, fixed a bug where deployed and candidate release comparison was always false when an RNG was used to derive some manifest value, resulting in the chart release constantly upgrading
+    kind: bugfix

internal/helm/release/manager.go

@@ -32,6 +32,7 @@ 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"
@@ -125,14 +126,7 @@ func (m *manager) Sync(ctx context.Context) error {
 	m.deployedRelease = deployedRelease
 	m.isInstalled = true
 
-	// 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 candidate release: %w", err)
-	}
-	if deployedRelease.Manifest != candidateRelease.Manifest {
-		m.isUpgradeRequired = true
-	}
+	m.isUpgradeRequired = m.isUpgrade(deployedRelease)
 
 	return nil
 }
@@ -141,6 +135,20 @@ func notFoundErr(err error) bool {
 	return err != nil && strings.Contains(err.Error(), "not found")
 }
 
+func (m manager) isUpgrade(deployedRelease *rpb.Release) bool {
+	if deployedRelease == nil {
+		return false
+	}
+
+	// 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
+}
+
 func (m manager) getDeployedRelease() (*rpb.Release, error) {
 	deployedRelease, err := m.storageBackend.Deployed(m.releaseName)
 	if err != nil {
@@ -152,14 +160,6 @@ 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

@@ -17,16 +17,17 @@ package release
 import (
 	"testing"
 
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	apitypes "k8s.io/apimachinery/pkg/types"
-	"k8s.io/cli-runtime/pkg/resource"
-
+	"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"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	apitypes "k8s.io/apimachinery/pkg/types"
+	"k8s.io/cli-runtime/pkg/resource"
 )
 
 func newTestUnstructured(containers []interface{}) *unstructured.Unstructured {
@@ -213,3 +214,72 @@ 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"},
+			chart:           newTestChart(t, "./testdata/simple"),
+			deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{"key": ""}, "deployed", "deployed-ns"),
+			want:            true,
+		},
+	}
+	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 := m.isUpgrade(test.deployedRelease)
+			assert.Equal(t, test.want, isUpgrade)
+		})
+	}
+}
+
+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 {
+	release := rpb.Mock(&rpb.MockReleaseOptions{
+		Name:      name,
+		Namespace: namespace,
+		Chart:     chart,
+		Version:   1,
+	})
+	release.Config = values
+	return release
+}

internal/helm/release/testdata/simple/.helmignore

@@ -0,0 +1,21 @@
+# Patterns to ignore when building packages.
+# This supports shell glob matching, relative path matching, and
+# negation (prefixed with !). Only one pattern per line.
+.DS_Store
+# Common VCS dirs
+.git/
+.gitignore
+.bzr/
+.bzrignore
+.hg/
+.hgignore
+.svn/
+# Common backup files
+*.swp
+*.bak
+*.tmp
+*~
+# Various IDEs
+.project
+.idea/
+*.tmproj

internal/helm/release/testdata/simple/Chart.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+description: a simple charts
+name: simple
+keywords:
+  - helm
+  - operator-sdk
+version: 0.0.1+master
+appVersion: "master"

internal/helm/release/testdata/simple/templates/NOTES.txt

@@ -0,0 +1 @@
+A simple charts

internal/helm/release/testdata/simple/templates/_helpers.tpl

@@ -0,0 +1,7 @@
+{{/* vim: set filetype=mustache: */}}
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "simple.fullname" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
+{{- end -}}

internal/helm/release/testdata/simple/templates/secrets.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: {{ template "simple.fullname" . }}
+  namespace: {{ .Release.Namespace }}
+type: Opaque
+data:
+  password: {{ randAlphaNum 10 | b64enc | quote }}

internal/helm/release/testdata/simple/values.yaml

@@ -0,0 +1 @@
+# simple values

internal/helm/release/testdata/simpledf/.helmignore

@@ -0,0 +1,21 @@
+# Patterns to ignore when building packages.
+# This supports shell glob matching, relative path matching, and
+# negation (prefixed with !). Only one pattern per line.
+.DS_Store
+# Common VCS dirs
+.git/
+.gitignore
+.bzr/
+.bzrignore
+.hg/
+.hgignore
+.svn/
+# Common backup files
+*.swp
+*.bak
+*.tmp
+*~
+# Various IDEs
+.project
+.idea/
+*.tmproj