diff --git a/changelog/fragments/helm-deepequals.yaml b/changelog/fragments/helm-deepequals.yaml new file mode 100644 index 00000000000..0a331945a40 --- /dev/null +++ b/changelog/fragments/helm-deepequals.yaml @@ -0,0 +1,4 @@ +entries: + - description: > + For Helm-based operators, fixed release equality comparison such that number values are compared and not their types to avoid unnecessary reconciliations. + kind: bugfix \ No newline at end of file diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go index 7ee19a0b3e0..ffeb40b2c83 100644 --- a/internal/helm/release/manager.go +++ b/internal/helm/release/manager.go @@ -20,6 +20,7 @@ import ( "encoding/json" "errors" "fmt" + "reflect" "strings" jsonpatch "gomodules.xyz/jsonpatch/v3" @@ -126,8 +127,10 @@ func (m *manager) Sync(ctx context.Context) error { m.deployedRelease = deployedRelease m.isInstalled = true - m.isUpgradeRequired = m.isUpgrade(deployedRelease) - + m.isUpgradeRequired, err = m.isUpgrade(deployedRelease) + if err != nil { + return fmt.Errorf("failed to get upgrade status: %w", err) + } return nil } @@ -135,18 +138,41 @@ func notFoundErr(err error) bool { return err != nil && strings.Contains(err.Error(), "not found") } -func (m manager) isUpgrade(deployedRelease *rpb.Release) bool { +// 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 + return false, nil } // 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 + 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) { diff --git a/internal/helm/release/manager_test.go b/internal/helm/release/manager_test.go index f930374e507..70c5e5b5144 100644 --- a/internal/helm/release/manager_test.go +++ b/internal/helm/release/manager_test.go @@ -15,6 +15,8 @@ package release import ( + "bytes" + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -247,11 +249,20 @@ func TestManagerisUpgrade(t *testing.T) { name: "different values", releaseName: "deployed", releaseNs: "deployed-ns", - values: map[string]interface{}{"key": "1"}, + values: map[string]interface{}{"key": "1", "int": int32(1)}, chart: newTestChart(t, "./testdata/simple"), - deployedRelease: newTestRelease(newTestChart(t, "./testdata/simple"), map[string]interface{}{"key": ""}, "deployed", "deployed-ns"), + 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) { @@ -261,8 +272,9 @@ func TestManagerisUpgrade(t *testing.T) { values: test.values, chart: test.chart, } - isUpgrade := m.isUpgrade(test.deployedRelease) + isUpgrade, err := m.isUpgrade(test.deployedRelease) assert.Equal(t, test.want, isUpgrade) + assert.Equal(t, nil, err) }) } } @@ -273,13 +285,16 @@ func newTestChart(t *testing.T, path string) *cpb.Chart { return chart } -func newTestRelease(chart *cpb.Chart, values map[string]interface{}, name, namespace string) *rpb.Release { +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, - Chart: chart, Version: 1, }) + + buffer := &bytes.Buffer{} + _ = json.NewEncoder(buffer).Encode(chart) + _ = json.NewDecoder(buffer).Decode(release.Chart) release.Config = values return release }