Support helm release upgrade on helm chart name change

[MichaelMorrisEst]

Summary

Closes #870

Proposes to add a new feature gate UninstallOnChartNameChange.

When enabled (default), the current behaviour of uninstalling a Helm Release when the Helm chart name is changed shall remain unchanged. This results in a new Helm Release install.
When disabled, the Helm Release will not be uninstalled when the Helm chart name is changed. This results in a Helm upgrade being performed. This was the behaviour before Flux 2.2.0

This re-introduces support that was lost going from Flux 2.1.x -> 2.2.x but controls the behaviour through a feature gate so either the previous or current behaviour (default) can be achieved

Changes

• Define UninstallOnChartNameChange feature gate in internal/features/features.go
• Wire the feature gate from main.go to HelmReleaseReconciler
• When the flag is false and the chart name has changed do not uninstall the HelmRelease
• Change the visibility of action.targetChartName to enable it to be referenced in the HelmRelease controller
• Test added for verify new behaviour

[hiddeco]

Feedback beyond the contents of the PR: I do wonder if there are not other changes (e.g., release name/namespace or storage namespace) where there could be a desire of control. Given this, I do wonder if what this PR attempts to do isn't too narrow.

[hiddeco]

The chance of this going into a patch release is unlikely, this should thus indicate 1.6.0.

[hiddeco]

This list of constants was always meant to be a descriptive reason (factored out to a const for code readability), not an identifier. Additionally, only exposing one of them creates a weak contract. I would prefer a solution where:

1. All identifiers are public
2. Identifiers do not contain spaces

[hiddeco]

Some additional thoughts:

• For storage namespace changes, we could actually do the more intelligent thing and attempt to migrate instead of going through an uninstall cycle. This would open a surface for "Secret smuggling" however, and integrity would likely have to be verified as part of this.
• In hindsight, the biggest design mistake I ever introduced in the helm-controller (and something that still bites me as an operator once in a while) is not having the storageNamespace field mirror the releaseNamespace unless explicitly overwritten. If we are rethinking how this all works, this might be worth addressing as well (behind a feature gate).
• Some of these changes may actually require an uninstall (or juggling with Helm install options). As changing a release name would make the resources stay put, which could then cause the subsequent Helm install to fail.

[artem-nefedov]

How about having this setting on resource level rather than a global feature flag?
I feel like that's more correct approach, but don't know about the complexity involved to support that.

[MichaelMorrisEst]

How about having this setting on resource level rather than a global feature flag? I feel like that's more correct approach, but don't know about the complexity involved to support that.

Hi @artem-nefedov Are you suggesting some on the CRD? e.g. maybe adding something to the 'Uninstall' element of 'HelmRelease'?

[MichaelMorrisEst]

Hi @hiddeco
Thats for your feedback.

I dont think Helm itself supports changing the helm release name, namespace or storage namespace and, as you state yourself, this might require an uninstall anyway. The proposed scope of chart name changes only I believe is consistent with what Helm supports. I'd be concerned about the complexity of implementing this for other changes particularly if there isnt anybody looking for it. Having said that, I am happy to work on it if you feel its the way to go, but I would need your advice on how it should be implemented.

In relation to the default storageNamespace. If I understand you correctly, you are saying it would be better for it to default to targetNamespace and this change should be controlled by a feature gate? I am happy to implement such a change, but would it be better in a separate PR as its not directly related to this proposal?

[artem-nefedov]

Hi @artem-nefedov Are you suggesting some on the CRD? e.g. maybe adding something to the 'Uninstall' element of 'HelmRelease'?

Sorry for the delayed reply. Yes, I meant something like this, only rather than 'Uninstall', I think the better place is 'Upgrade'. For example:

spec:
  upgrade:
    chartNameChangePolicy: InPlaceUpdate # or "Reinstall" (default)