helm operator for a chart with some dependencies keeps upgrading every sync loop

[wscheele]

Bug Report

What did you do?

Deploy a helm operator to manage a helm chart with a number of dependencies.

What did you expect to see?

Upgrades only when either the chart or the cr contained spec values change.

What did you see instead? Under which circumstances?

the isUpgrade method (in current master at)
internal/helm/release/manager.go:168
always returns true.

after some debugging, i figured that this happens because the chart from the manager (m.chart in that method) only contains Values coming from the main chart's values.yaml; while the chart from the release (deployedRelease.Chart in that code) contains (non-empty, non-null) defaults from the dependencies charts' values.yaml files as well.
therefore ok, err := equalJSONStruct(m.chart, deployedRelease.Chart) always sets ok to false (Values never matches).

Environment

Operator type:

/language helm

$ operator-sdk version

v1.8.0
but v1.8.1 operator image, as well as current master build have the same behavior.

$ go version (if language is Go)
go version go1.16.5 linux/amd64

Possible Solution

i don't have any experience with go (other than two or three debugging attempts).
am testing an approach where the manager_factory enriches the manager chart Values with the (non-null, non-empty) defaults from the dependencies charts as well. this does look promising. but having no knowledge in go, nor experience with helm apis, i find it very hard to determine whether that makes sense.

Additional context

something seems misaligned between the Values view of loader.LoadDir(f.chartDir) in manager_factory vs those returned by m.getDeployedRelease()

if there is something i need to adjust in either my chart definition, or the way i deploy the operator to align the values, i would love to know. but couldn't find.

[wscheele]

My tests with the following patch were successful:

diff --git a/internal/helm/release/manager_factory.go b/internal/helm/release/manager_factory.go
index 406c5dbf..3ccbd1f8 100644
--- a/internal/helm/release/manager_factory.go
+++ b/internal/helm/release/manager_factory.go
@@ -91,7 +91,20 @@ func (f managerFactory) NewManager(cr *unstructured.Unstructured, overrideValues
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse override values: %w", err)
 	}
-	values := mergeMaps(crValues, expOverrides)
+	configValues := mergeMaps(crValues, expOverrides)
+	var values = crChart.Values
+
+	// merge all dependency chart defaults.
+	// needs to be filtered to remove nil values, otherwise charts won't match.
+	for _, s := range crChart.Dependencies() {
+		//log.Info("merging values of dependency", "name", s.Name(), "values", s.Values)
+		var depValues = make(map[string]interface{})
+		depValues[s.Name()] = s.Values
+		var depValuesCopy = depValues[s.Name()].(map[string]interface{})
+		removeNilValuesFromMap(depValuesCopy)
+		values = mergeMaps(depValues, values)
+	}
+	crChart.Values = values
 
 	actionConfig := &action.Configuration{
 		RESTClientGetter: rcg,
@@ -109,11 +122,37 @@ func (f managerFactory) NewManager(cr *unstructured.Unstructured, overrideValues
 		namespace:   cr.GetNamespace(),
 
 		chart:  crChart,
-		values: values,
+		values: configValues,
 		status: types.StatusFor(cr),
 	}, nil
 }
 
+// run and recurse through structure, removing explicit nil values to prevent chart mismatches.
+func removeNilValuesFromMap(input map[string]interface{}) {
+	for key, value := range input {
+		switch c := value.(type) {
+		case map[string]interface{}:
+			removeNilValuesFromMap(c)
+		case []interface{}:
+			removeNilValuesFromList(c)
+		case nil:
+			//fmt.Printf("deleting nil for key: %q\n", key)
+			delete(input, key)
+		}
+	}
+}
+
+func removeNilValuesFromList(input []interface{}) {
+	for _, value := range input {
+		switch c := value.(type) {
+		case map[string]interface{}:
+			removeNilValuesFromMap(c)
+		case []interface{}:
+			removeNilValuesFromList(c)
+		}
+	}
+}
+
 // getReleaseName returns a release name for the CR.
 //
 // getReleaseName searches for a release using the CR name. If a release

releases were no longer upgraded on each cycle, only upon relevant changes.

[cndoit18]

Thank you very much, I will work with you on this issue recently. Do you have the chart to facilitate me to reproduce and troubleshoot it?

[cndoit18]

/assigne @cndoit18

[wscheele]

hi,

I cannot share the exact chart that produced this behavior. But any chart with:

dependencies:
  - name: postgresql-ha
    version: 7.7.1
    repository: https://charts.bitnami.com/bitnami
  - name: mongodb
    version: 10.20.5
    repository: https://charts.bitnami.com/bitnami
  - name: common
    version: 1.6.1
    repository: https://charts.bitnami.com/bitnami

should reproduce it. (i suppose any chart that has dependencies with their own default values).

[cndoit18]

I tried to generate a pr, can you help to verify it together?

[wscheele]

did you manage to reproduce the issue?

[wscheele]

will run my case against your pr to verify.

[cndoit18]

will run my case against your pr to verify.

Sad news, it did not solve #4936

[wscheele]

it also doesn't solve my case. keeps upgrading.

[wscheele]

the approach in the above patch works fine for me.

[cndoit18]

Hi, I updated it, can you verify it again? thanks @wscheele

[wscheele]

same problem, still keeps upgrading

[cndoit18]

I'll try to reproduce the problem by adding a unit test first. Can you help me to verify it again? @wscheele