Repository: operator-framework/operator-sdk
Pull request #4937 (2021-05-22 03:08:40)

fix(helm): properly compare existing and candidate releases
Signed-off-by: cndoit18 <cndoit18@outlook.com>

**Description of the change:**

Fix when using the randAlphaNum in Helm, the chart release is constantly upgraded

**Motivation for the change:**

fixes: #4936 

**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))

cndoit18: > It is not exactly clear why this fixes the issue described. In this case, comparing manifests will not work because the RNG will generate two different numbers, and everything else is the same?

yes. and I will add this unit test.
cndoit18: > It is not exactly clear why this fixes the issue described. In this case, comparing manifests will not work because the RNG will generate two different numbers, and everything else is the same?

Yes, It will cause the helm-operator always re-upgrade
openshift-ci[bot]: New changes are detected. LGTM label has been removed.
cndoit18: > Thanks @cndoit18!
> /lgtm

I resubmitted and fixed the problem with the e2e docs
cndoit18: > Thanks @cndoit18!
> /lgtm

Can you please perform ci first? Just like `need-test-ok` in the k8s community
cndoit18: > > Thanks @cndoit18!
> > /lgtm
> 
> I resubmitted and fixed the problem with the e2e docs

I'm having some problems with the ci and it doesn't look too much like I'm the cause. Can you help me? please
@estroz 

Patch:
diff --git a/README.md b/README.md
index fee59b1f..588b9bd1 100644
--- a/README.md
+++ b/README.md
@@ -3,7 +3,6 @@
 
 [![Build Status](https://github.com/operator-framework/operator-sdk/workflows/deploy/badge.svg)](https://github.com/operator-framework/operator-sdk/actions)
 [![License](http://img.shields.io/:license-apache-blue.svg)](http://www.apache.org/licenses/LICENSE-2.0.html)
-[![Go Report Card](https://goreportcard.com/badge/github.com/operator-framework/operator-sdk)](https://goreportcard.com/report/github.com/operator-framework/operator-sdk)
 
 ## Documentation
 
diff --git a/changelog/fragments/bugfix-ansible-runner.yaml b/changelog/fragments/bugfix-ansible-runner.yaml
new file mode 100644
index 00000000..1596e113
--- /dev/null
+++ b/changelog/fragments/bugfix-ansible-runner.yaml
@@ -0,0 +1,6 @@
+entries:
+  - description: >
+      Detect and report ansible-runner binary failures early and prevent symliking to artifacts.
+    kind: bugfix
+    pull_request_override: 4944
+
diff --git a/changelog/fragments/helm-randalphanum.yaml b/changelog/fragments/helm-randalphanum.yaml
new file mode 100644
index 00000000..ade97dfc
--- /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
diff --git a/internal/ansible/runner/runner.go b/internal/ansible/runner/runner.go
index 00a5f62a..afb9d097 100644
--- a/internal/ansible/runner/runner.go
+++ b/internal/ansible/runner/runner.go
@@ -48,6 +48,8 @@ const (
 	// to the ansible-runner command. This will override the value for a particular CR.
 	// Example usage "ansible.sdk.operatorframework.io/verbosity: 5"
 	AnsibleVerbosityAnnotation = "ansible.sdk.operatorframework.io/verbosity"
+
+	ansibleRunnerBin = "ansible-runner"
 )
 
 // Runner - a runnable that should take the parameters and name and namespace
@@ -180,6 +182,10 @@ type runner struct {
 }
 
 func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig string) (RunResult, error) {
+	if _, err := exec.LookPath(ansibleRunnerBin); err != nil {
+		return nil, err
+	}
+
 	timer := metrics.ReconcileTimer(r.GVK.String())
 	defer timer.ObserveDuration()
 
@@ -277,11 +283,16 @@ func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig stri
 		// link the current run to the `latest` directory under artifacts
 		currentRun := filepath.Join(inputDir.Path, "artifacts", ident)
 		latestArtifacts := filepath.Join(inputDir.Path, "artifacts", "latest")
-		if _, err = os.Lstat(latestArtifacts); err == nil {
-			if err = os.Remove(latestArtifacts); err != nil {
-				logger.Error(err, "Error removing the latest artifacts symlink")
+		if _, err = os.Lstat(latestArtifacts); err != nil {
+			if !errors.Is(err, os.ErrNotExist) {
+				logger.Error(err, "Latest artifacts dir has error")
+				return
 			}
+		} else if err = os.Remove(latestArtifacts); err != nil {
+			logger.Error(err, "Error removing the latest artifacts symlink")
+			return
 		}
+
 		if err = os.Symlink(currentRun, latestArtifacts); err != nil {
 			logger.Error(err, "Error symlinking latest artifacts")
 		}
diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go
index 2d494f42..9489d377 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)


Test patch:
diff --git a/internal/helm/release/manager_test.go b/internal/helm/release/manager_test.go
index a7105dd7..77ccca43 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 00000000..f0c13194
--- /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 00000000..786c7721
--- /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 00000000..da930300
--- /dev/null
+++ b/internal/helm/release/testdata/simple/templates/NOTES.txt
@@ -0,0 +1,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 00000000..e5fe9272
--- /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 00000000..4b61fbb7
--- /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 00000000..ca762273
--- /dev/null
+++ b/internal/helm/release/testdata/simple/values.yaml
@@ -0,0 +1,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 00000000..f0c13194
--- /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 00000000..786c7721
--- /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 00000000..da930300
--- /dev/null
+++ b/internal/helm/release/testdata/simpledf/templates/NOTES.txt
@@ -0,0 +1,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 00000000..e5fe9272
--- /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 00000000..4b61fbb7
--- /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 00000000..a99c72dc
--- /dev/null
+++ b/internal/helm/release/testdata/simpledf/values.yaml
@@ -0,0 +1,2 @@
+# simple values
+name: simpledf
\ No newline at end of file