Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/action/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const (
targetStorageNamespace = "storage namespace"
targetReleaseNamespace = "release namespace"
targetReleaseName = "release name"
targetChartName = "chart name"
TargetChartName = "chart name"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// ReleaseTargetChanged returns a reason and true if the given release and/or
Expand All @@ -68,7 +68,7 @@ func ReleaseTargetChanged(obj *v2.HelmRelease, chartName string) (string, bool)
case release.ShortenName(obj.GetReleaseName()) != cur.Name:
return targetReleaseName, true
case chartName != cur.ChartName:
return targetChartName, true
return TargetChartName, true
default:
return "", false
}
Expand Down
2 changes: 1 addition & 1 deletion internal/action/verify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func TestReleaseTargetChanged(t *testing.T) {
},
StorageNamespace: defaultNamespace,
},
wantReason: targetChartName,
wantReason: TargetChartName,
want: true,
},
{
Expand Down
3 changes: 2 additions & 1 deletion internal/controller/helmrelease_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ type HelmReleaseReconciler struct {
DefaultToRetryOnFailure bool
DirectSourceFetch bool
DisableChartDigestTracking bool
UninstallOnChartNameChange bool
}

const terminalErrorMessage = "Reconciliation failed terminally due to configuration error"
Expand Down Expand Up @@ -371,7 +372,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context,
// If the release target configuration has changed, we need to uninstall the
// previous release target first. If we did not do this, the installation would
// fail due to resources already existing.
if reason, changed := action.ReleaseTargetChanged(obj, loadedChart.Name()); changed {
if reason, changed := action.ReleaseTargetChanged(obj, loadedChart.Name()); changed && (reason != action.TargetChartName || r.UninstallOnChartNameChange) {
log.Info(fmt.Sprintf("release target configuration changed (%s): running uninstall for current release", reason))
if err = r.reconcileUninstall(ctx, getter, obj); err != nil && !errors.Is(err, intreconcile.ErrNoLatest) {
return ctrl.Result{}, err
Expand Down
103 changes: 103 additions & 0 deletions internal/controller/helmrelease_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,109 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
g.Expect(obj.Status.StorageNamespace).To(BeEmpty())
})

t.Run("Upgrades HelmRelease if chart name changed with feature gate set", func(t *testing.T) {
g := NewWithT(t)

// Create HelmChart mock.
chartMock := testutil.BuildChart()
chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root())
g.Expect(err).ToNot(HaveOccurred())

ns, err := testEnv.CreateNamespace(context.TODO(), "mock")
g.Expect(err).ToNot(HaveOccurred())
t.Cleanup(func() {
_ = testEnv.Delete(context.TODO(), ns)
})

hc := &sourcev1.HelmChart{
ObjectMeta: metav1.ObjectMeta{
Name: chartMock.Name(),
Namespace: ns.Name,
Generation: 1,
},
Spec: sourcev1.HelmChartSpec{
Chart: chartMock.Name(),
Version: chartMock.Metadata.Version,
},
Status: sourcev1.HelmChartStatus{
ObservedGeneration: 1,
Artifact: chartArtifact,
Conditions: []metav1.Condition{
{
Type: meta.ReadyCondition,
Status: metav1.ConditionTrue,
},
},
},
}

// Create a test Helm release storage mock.
rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
Name: "release",
Namespace: ns.Name,
Version: 1,
Chart: chartMock,
Status: helmreleasecommon.StatusDeployed,
})

snapshot := release.ObservedToSnapshot(release.ObserveRelease(rls))
snapshot.ChartName = "oldChartName"
obj := &v2.HelmRelease{
ObjectMeta: metav1.ObjectMeta{
Name: "release",
Namespace: ns.Name,
},
Spec: v2.HelmReleaseSpec{
ChartRef: &v2.CrossNamespaceSourceReference{
Kind: sourcev1.HelmChartKind,
Name: hc.Name,
},
},
Status: v2.HelmReleaseStatus{
StorageNamespace: ns.Name,
History: v2.Snapshots{
snapshot,
},
HelmChart: hc.Namespace + "/" + hc.Name,
},
}

c := fake.NewClientBuilder().
WithScheme(NewTestScheme()).
WithStatusSubresource(&v2.HelmRelease{}).
WithObjects(hc, obj).
Build()

r := &HelmReleaseReconciler{
Client: c,
GetClusterConfig: GetTestClusterConfig,
EventRecorder: record.NewFakeRecorder(32),
UninstallOnChartNameChange: false,
}

//Store the Helm release mock in the test namespace.
getter, err := r.buildRESTClientGetter(context.TODO(), obj)
g.Expect(err).ToNot(HaveOccurred())

cfg, err := action.NewConfigFactory(getter, action.WithStorage(helmdriver.SecretsDriverName, obj.Status.StorageNamespace))
g.Expect(err).ToNot(HaveOccurred())

store := helmstorage.Init(cfg.Driver)
g.Expect(store.Create(rls)).To(Succeed())

_, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj, nil)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s",
fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(),
chartMock.Metadata.Version)),
*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s",
fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(),
chartMock.Metadata.Version)),
}))
})

t.Run("resets failure counts on configuration change", func(t *testing.T) {
g := NewWithT(t)

Expand Down
8 changes: 8 additions & 0 deletions internal/features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ const (
// auto-clears failures on success, providing better UX especially
// when CancelHealthCheckOnNewRevision is enabled.
DefaultToRetryOnFailure = "DefaultToRetryOnFailure"

// UninstallOnChartNameChange controls if a release is uninstalled when the chart
// name changes resulting in a new install of the release. This is enabled by
// default. When disabled, an upgrade will be attempted
UninstallOnChartNameChange = "UninstallOnChartNameChange"
)

var features = map[string]bool{
Expand Down Expand Up @@ -132,6 +137,9 @@ var features = map[string]bool{
// DefaultToRetryOnFailure
// opt-in from v1.5.2
DefaultToRetryOnFailure: false,
// UninstallOnChartNameChange
// opt-in from v1.5.4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

UninstallOnChartNameChange: true,
}

func init() {
Expand Down
7 changes: 7 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ func main() {
"adoption of HelmRelease resources in legacy API versions is no longer supported")
}

uninstallOnChartNameChange, err := features.Enabled((features.UninstallOnChartNameChange))
if err != nil {
setupLog.Error(err, "unable to check feature gate "+features.UninstallOnChartNameChange)
os.Exit(1)
}

// Set the managedFields owner for resources reconciled from Helm charts.
kube.ManagedFieldsManager = controllerName

Expand Down Expand Up @@ -394,6 +400,7 @@ func main() {
ArtifactFetchRetries: httpRetry,
AllowExternalArtifact: allowExternalArtifact,
DisallowedFieldManagers: disallowedFieldManagers,
UninstallOnChartNameChange: uninstallOnChartNameChange,
}).SetupWithManager(ctx, mgr, controller.HelmReleaseReconcilerOptions{
RateLimiter: helper.GetRateLimiter(rateLimiterOptions),
WatchConfigs: watchConfigs,
Expand Down