From 1a4c539d867712725ad748e13bed6e61cf287c65 Mon Sep 17 00:00:00 2001 From: cndoit18 Date: Sat, 22 May 2021 10:59:30 +0800 Subject: [PATCH] fix(helm): fix when using the randAlphaNum in Helm, the chart release is constantly upgraded Signed-off-by: cndoit18 --- changelog/fragments/helm-randalphanum.yaml | 4 + internal/helm/release/manager.go | 32 ++++---- internal/helm/release/manager_test.go | 82 +++++++++++++++++-- .../helm/release/testdata/simple/.helmignore | 21 +++++ .../helm/release/testdata/simple/Chart.yaml | 8 ++ .../testdata/simple/templates/NOTES.txt | 1 + .../testdata/simple/templates/_helpers.tpl | 7 ++ .../testdata/simple/templates/secrets.yaml | 8 ++ .../helm/release/testdata/simple/values.yaml | 1 + .../release/testdata/simpledf/.helmignore | 21 +++++ .../helm/release/testdata/simpledf/Chart.yaml | 8 ++ .../testdata/simpledf/templates/NOTES.txt | 1 + .../testdata/simpledf/templates/_helpers.tpl | 7 ++ .../testdata/simpledf/templates/secrets.yaml | 8 ++ .../release/testdata/simpledf/values.yaml | 2 + 15 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 changelog/fragments/helm-randalphanum.yaml create mode 100644 internal/helm/release/testdata/simple/.helmignore create mode 100644 internal/helm/release/testdata/simple/Chart.yaml create mode 100644 internal/helm/release/testdata/simple/templates/NOTES.txt create mode 100644 internal/helm/release/testdata/simple/templates/_helpers.tpl create mode 100644 internal/helm/release/testdata/simple/templates/secrets.yaml create mode 100644 internal/helm/release/testdata/simple/values.yaml create mode 100644 internal/helm/release/testdata/simpledf/.helmignore create mode 100644 internal/helm/release/testdata/simpledf/Chart.yaml create mode 100644 internal/helm/release/testdata/simpledf/templates/NOTES.txt create mode 100644 internal/helm/release/testdata/simpledf/templates/_helpers.tpl create mode 100644 internal/helm/release/testdata/simpledf/templates/secrets.yaml create mode 100644 internal/helm/release/testdata/simpledf/values.yaml diff --git a/changelog/fragments/helm-randalphanum.yaml b/changelog/fragments/helm-randalphanum.yaml new file mode 100644 index 00000000000..8124071c958 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go index 2d494f42c29..9489d377832 100644 --- a/internal/helm/release/manager.go +++ b/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) diff --git a/internal/helm/release/manager_test.go b/internal/helm/release/manager_test.go index a7105dd759f..77ccca43148 100644 --- a/internal/helm/release/manager_test.go +++ b/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 +} diff --git a/internal/helm/release/testdata/simple/.helmignore b/internal/helm/release/testdata/simple/.helmignore new file mode 100644 index 00000000000..f0c13194444 --- /dev/null +++ b/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 diff --git a/internal/helm/release/testdata/simple/Chart.yaml b/internal/helm/release/testdata/simple/Chart.yaml new file mode 100644 index 00000000000..786c77218ee --- /dev/null +++ b/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" diff --git a/internal/helm/release/testdata/simple/templates/NOTES.txt b/internal/helm/release/testdata/simple/templates/NOTES.txt new file mode 100644 index 00000000000..da9303003cb --- /dev/null +++ b/internal/helm/release/testdata/simple/templates/NOTES.txt @@ -0,0 +1 @@ +A simple charts \ No newline at end of file diff --git a/internal/helm/release/testdata/simple/templates/_helpers.tpl b/internal/helm/release/testdata/simple/templates/_helpers.tpl new file mode 100644 index 00000000000..e5fe9272385 --- /dev/null +++ b/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 -}} diff --git a/internal/helm/release/testdata/simple/templates/secrets.yaml b/internal/helm/release/testdata/simple/templates/secrets.yaml new file mode 100644 index 00000000000..4b61fbb7e49 --- /dev/null +++ b/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 }} \ No newline at end of file diff --git a/internal/helm/release/testdata/simple/values.yaml b/internal/helm/release/testdata/simple/values.yaml new file mode 100644 index 00000000000..ca762273d18 --- /dev/null +++ b/internal/helm/release/testdata/simple/values.yaml @@ -0,0 +1 @@ +# simple values \ No newline at end of file diff --git a/internal/helm/release/testdata/simpledf/.helmignore b/internal/helm/release/testdata/simpledf/.helmignore new file mode 100644 index 00000000000..f0c13194444 --- /dev/null +++ b/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 diff --git a/internal/helm/release/testdata/simpledf/Chart.yaml b/internal/helm/release/testdata/simpledf/Chart.yaml new file mode 100644 index 00000000000..786c77218ee --- /dev/null +++ b/internal/helm/release/testdata/simpledf/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" diff --git a/internal/helm/release/testdata/simpledf/templates/NOTES.txt b/internal/helm/release/testdata/simpledf/templates/NOTES.txt new file mode 100644 index 00000000000..da9303003cb --- /dev/null +++ b/internal/helm/release/testdata/simpledf/templates/NOTES.txt @@ -0,0 +1 @@ +A simple charts \ No newline at end of file diff --git a/internal/helm/release/testdata/simpledf/templates/_helpers.tpl b/internal/helm/release/testdata/simpledf/templates/_helpers.tpl new file mode 100644 index 00000000000..e5fe9272385 --- /dev/null +++ b/internal/helm/release/testdata/simpledf/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 -}} diff --git a/internal/helm/release/testdata/simpledf/templates/secrets.yaml b/internal/helm/release/testdata/simpledf/templates/secrets.yaml new file mode 100644 index 00000000000..4b61fbb7e49 --- /dev/null +++ b/internal/helm/release/testdata/simpledf/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 }} \ No newline at end of file diff --git a/internal/helm/release/testdata/simpledf/values.yaml b/internal/helm/release/testdata/simpledf/values.yaml new file mode 100644 index 00000000000..a99c72dc647 --- /dev/null +++ b/internal/helm/release/testdata/simpledf/values.yaml @@ -0,0 +1,2 @@ +# simple values +name: simpledf \ No newline at end of file