Fix: Trigger Helm upgrade when storage labels change even if manifests are identical

Author: jianzhangbjz

internal/operator-controller/applier/helm.go

@@ -117,7 +117,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 		return false, "", err
 	}
 
-	rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post)
+	rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post, storageLabels)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to get release state using server-side dry-run: %w", err)
 	}
@@ -289,7 +289,7 @@ func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExt
 	}, helmclient.AppendInstallPostRenderer(post))
 }
 
-func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) {
+func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer, storageLabels map[string]string) (*release.Release, *release.Release, string, error) {
 	currentRelease, err := cl.Get(ext.GetName())
 	if errors.Is(err, driver.ErrReleaseNotFound) {
 		desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
@@ -318,12 +318,25 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
 	relState := StateUnchanged
 	if desiredRelease.Manifest != currentRelease.Manifest ||
 		currentRelease.Info.Status == release.StatusFailed ||
-		currentRelease.Info.Status == release.StatusSuperseded {
+		currentRelease.Info.Status == release.StatusSuperseded ||
+		releaseLabelsChanged(currentRelease.Labels, storageLabels) {
 		relState = StateNeedsUpgrade
 	}
 	return currentRelease, desiredRelease, relState, nil
 }
 
+// releaseLabelsChanged checks if any of the storage labels have changed.
+// This ensures we upgrade the release when bundle metadata changes,
+// even if the rendered manifests are identical.
+func releaseLabelsChanged(currentLabels, desiredLabels map[string]string) bool {
+	for key, desiredValue := range desiredLabels {
+		if currentValue, exists := currentLabels[key]; !exists || currentValue != desiredValue {
+			return true
+		}
+	}
+	return false
+}
+
 type postrenderer struct {
 	labels  map[string]string
 	cascade postrender.PostRenderer

internal/operator-controller/applier/helm_test.go

@@ -622,11 +622,16 @@ func TestApply_Upgrade(t *testing.T) {
 
 	t.Run("fails during upgrade reconcile (StateUnchanged)", func(t *testing.T) {
 		// make sure desired and current are the same this time
-		testDesiredRelease := *testCurrentRelease
+		// Current release must have the same labels as storageLabels to trigger StateUnchanged
+		testCurrentWithLabels := &release.Release{
+			Info:   &release.Info{Status: release.StatusDeployed},
+			Labels: testStorageLabels,
+		}
+		testDesiredRelease := *testCurrentWithLabels
 
 		mockAcg := &mockActionGetter{
 			reconcileErr: errors.New("failed reconciling charts"),
-			currentRel:   testCurrentRelease,
+			currentRel:   testCurrentWithLabels,
 			desiredRel:   &testDesiredRelease,
 		}
 		mockPf := &mockPreflight{}
@@ -666,6 +671,47 @@ func TestApply_Upgrade(t *testing.T) {
 		require.True(t, installSucceeded)
 		require.Empty(t, installStatus)
 	})
+
+	t.Run("triggers upgrade when storage labels change but manifest is unchanged", func(t *testing.T) {
+		// Current release has old labels but same manifest as desired
+		testCurrentWithLabels := &release.Release{
+			Info:     &release.Info{Status: release.StatusDeployed},
+			Labels:   map[string]string{"bundle-version": "v1.0.0"},
+			Manifest: validManifest, // Same manifest as desired - only labels differ
+		}
+		// Desired release has same manifest as current (no manifest change)
+		testDesiredRelease := &release.Release{
+			Info:     &release.Info{Status: release.StatusDeployed},
+			Manifest: validManifest,
+		}
+
+		mockAcg := &mockActionGetter{
+			currentRel: testCurrentWithLabels,
+			desiredRel: testDesiredRelease,
+			// Set reconcileErr to ensure test fails if reconcile path is taken instead of upgrade
+			reconcileErr: errors.New("reconcile should not be called when labels change"),
+		}
+
+		helmApplier := applier.Helm{
+			ActionClientGetter:            mockAcg,
+			HelmChartProvider:             DummyHelmChartProvider,
+			HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
+			Manager: &mockManagedContentCacheManager{
+				cache: &mockManagedContentCache{},
+			},
+		}
+
+		// Use new storage labels that differ from current release labels
+		newStorageLabels := map[string]string{"bundle-version": "v2.0.0"}
+		installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, newStorageLabels)
+
+		// If the code correctly triggers upgrade (not reconcile) when labels change,
+		// the test succeeds. If it incorrectly takes the reconcile path, reconcileErr
+		// will cause the test to fail.
+		require.NoError(t, err)
+		require.True(t, installSucceeded)
+		require.Empty(t, installStatus)
+	})
 }
 
 func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {