From 46b5a542fe09b2e2b9888cc4676bcc66ac8c1a67 Mon Sep 17 00:00:00 2001 From: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:18:41 +0300 Subject: [PATCH] fix(api): let the garbage collector finalize Applications An Application and its HelmRelease share one UID, so the garbage collector keeps a single graph node for both and may finalize it through the Application endpoint. The API never returned finalizers on an Application, and every update restored the HelmRelease finalizers from the live object. When the collector came to drop foregroundDeletion or orphan through the Application, removeFinalizer found no such finalizer and returned as if it were already gone. The HelmRelease then kept foregroundDeletion after Helm had uninstalled the release, and the deletion never completed. The Application now carries the two garbage collection finalizers, and an update that adds or removes one of them applies the same change to the HelmRelease. Controller finalizers such as finalizers.fluxcd.io stay owned by the live object, so helm uninstall still runs before the release goes away. The deletion timestamp and grace period are copied from the live object, which keeps an update of a terminating release valid. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> --- pkg/registry/apps/application/rest.go | 43 +++- .../rest_garbage_collection_test.go | 196 ++++++++++++++++++ 2 files changed, 234 insertions(+), 5 deletions(-) create mode 100644 pkg/registry/apps/application/rest_garbage_collection_test.go diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index ef3fc63f7..0e666ffad 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -22,6 +22,7 @@ import ( "fmt" "maps" "net/http" + "slices" "sort" "strconv" "strings" @@ -523,6 +524,8 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje return nil, false, err } + previousFinalizers := slices.Clone(oldObj.(*appsv1alpha1.Application).Finalizers) + // Update the Application object newObj, err := objInfo.UpdatedObject(ctx, oldObj) if err != nil { @@ -587,7 +590,7 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje if helmRelease.ResourceVersion == "" { helmRelease.SetResourceVersion(cur.GetResourceVersion()) } - r.applyLiveMetadata(helmRelease, cur, app) + r.applyLiveMetadata(helmRelease, cur, app, previousFinalizers) // Suspension is not part of the Application, so the rebuilt object always // says suspend=false, and sending it as is resumes a release that an @@ -628,7 +631,7 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje return getErr } helmRelease = converted.DeepCopy() - r.applyLiveMetadata(helmRelease, cur, app) + r.applyLiveMetadata(helmRelease, cur, app, previousFinalizers) helmRelease.SetResourceVersion(cur.GetResourceVersion()) helmRelease.Spec.Suspend = cur.Spec.Suspend } @@ -1220,10 +1223,36 @@ func filterPrefixedMap(original map[string]string, prefix string) map[string]str return processed } +func garbageCollectionFinalizers(finalizers []string) []string { + var result []string + for _, finalizer := range finalizers { + if finalizer == metav1.FinalizerDeleteDependents || finalizer == metav1.FinalizerOrphanDependents { + result = append(result, finalizer) + } + } + return result +} + +func mergeGarbageCollectionFinalizers(current, previous, desired []string) []string { + result := slices.Clone(current) + for _, finalizer := range []string{metav1.FinalizerDeleteDependents, metav1.FinalizerOrphanDependents} { + before, after := slices.Contains(previous, finalizer), slices.Contains(desired, finalizer) + if before == after { + continue + } + if !after { + result = slices.DeleteFunc(result, func(f string) bool { return f == finalizer }) + } else if !slices.Contains(result, finalizer) { + result = append(result, finalizer) + } + } + return result +} + // applyLiveMetadata assembles the metadata of a HelmRelease rebuilt from an // Application, given the live object it is about to replace. The PUT is a full // replace, so anything not restated here is dropped: -// - finalizers: helm-controller's finalizers.fluxcd.io guarantees +// - controller finalizers: helm-controller's finalizers.fluxcd.io guarantees // `helm uninstall` runs on deletion. Stripping it lets a subsequent delete // remove the HelmRelease instantly, orphaning every resource of the release. // - ownerReferences: garbage collection and lineage. @@ -1234,8 +1263,10 @@ func filterPrefixedMap(original map[string]string, prefix string) map[string]str // // hr must be a fresh conversion output: the function overlays the live metadata // and is not idempotent against its own result. -func (r *REST) applyLiveMetadata(hr, cur *helmv2.HelmRelease, app *appsv1alpha1.Application) { - hr.Finalizers = cur.Finalizers +func (r *REST) applyLiveMetadata(hr, cur *helmv2.HelmRelease, app *appsv1alpha1.Application, previousFinalizers []string) { + hr.Finalizers = mergeGarbageCollectionFinalizers(cur.Finalizers, previousFinalizers, app.Finalizers) + hr.DeletionTimestamp = cur.DeletionTimestamp.DeepCopy() + hr.DeletionGracePeriodSeconds = cur.DeletionGracePeriodSeconds hr.OwnerReferences = cur.OwnerReferences hr.Labels = mergeMaps(omitPrefixedMap(cur.Labels, LabelPrefix), hr.Labels) hr.Annotations = mergeMaps(omitPrefixedMap(cur.Annotations, AnnotationPrefix), hr.Annotations) @@ -1513,6 +1544,7 @@ func (r *REST) convertHelmReleaseToApplication(ctx context.Context, hr *helmv2.H ResourceVersion: hr.GetResourceVersion(), CreationTimestamp: hr.CreationTimestamp, DeletionTimestamp: hr.DeletionTimestamp, + Finalizers: garbageCollectionFinalizers(hr.Finalizers), Labels: filterPrefixedMap(hr.Labels, LabelPrefix), Annotations: filterPrefixedMap(hr.Annotations, AnnotationPrefix), }, @@ -1744,6 +1776,7 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* Annotations: addPrefixedMap(app.Annotations, AnnotationPrefix), ResourceVersion: app.ResourceVersion, UID: app.UID, + Finalizers: garbageCollectionFinalizers(app.Finalizers), }, Spec: helmv2.HelmReleaseSpec{ ChartRef: &helmv2.CrossNamespaceSourceReference{ diff --git a/pkg/registry/apps/application/rest_garbage_collection_test.go b/pkg/registry/apps/application/rest_garbage_collection_test.go new file mode 100644 index 000000000..556a3fd38 --- /dev/null +++ b/pkg/registry/apps/application/rest_garbage_collection_test.go @@ -0,0 +1,196 @@ +package application + +import ( + "context" + "errors" + "slices" + "testing" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1" +) + +func TestApplicationGarbageCollection(t *testing.T) { + for _, finalizer := range []string{metav1.FinalizerDeleteDependents, metav1.FinalizerOrphanDependents} { + for _, helmFirst := range []bool{true, false} { + order := "gc-first" + if helmFirst { + order = "helm-first" + } + t.Run(finalizer+"/"+order, func(t *testing.T) { + hr := optionsTestHelmRelease() + hr.UID = "release-uid" + now := metav1.Now() + hr.DeletionTimestamp = &now + hr.Finalizers = []string{finalizer} + if !helmFirst { + hr.Finalizers = append(hr.Finalizers, "finalizers.fluxcd.io") + } + r := newOptionsTestREST(t, interceptor.Funcs{}, hr) + ctx := request.WithNamespace(context.Background(), optionsTestNamespace) + obj, err := r.Get(ctx, "example", &metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + app := obj.(*appsv1alpha1.Application) + if app.UID != hr.UID || app.DeletionTimestamp == nil { + t.Fatalf("deletion identity lost: %v", app.ObjectMeta) + } + if !slices.Contains(app.Finalizers, finalizer) { + t.Fatalf("garbage collector cannot see %q: %v", finalizer, app.Finalizers) + } + app.Finalizers = slices.DeleteFunc(app.Finalizers, func(f string) bool { return f == finalizer }) + if _, _, err := r.Update(ctx, app.Name, rest.DefaultUpdatedObjectInfo(app), nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + live := &helmv2.HelmRelease{} + err = r.c.Get(ctx, client.ObjectKeyFromObject(hr), live) + if helmFirst { + if !apierrors.IsNotFound(err) { + t.Fatalf("release still exists after cleanup: err=%v, finalizers=%v", err, live.Finalizers) + } + return + } + if err != nil { + t.Fatal(err) + } + if !slices.Equal(live.Finalizers, []string{"finalizers.fluxcd.io"}) { + t.Fatalf("Helm cleanup protection changed: %v", live.Finalizers) + } + live.Finalizers = nil + if err := r.c.Update(ctx, live); err != nil { + t.Fatal(err) + } + if err := r.c.Get(ctx, client.ObjectKeyFromObject(hr), live); !apierrors.IsNotFound(err) { + t.Fatalf("release still exists after Helm cleanup: %v", err) + } + }) + } + } +} + +func TestApplicationGarbageCollectionConflict(t *testing.T) { + for _, concurrent := range []struct { + name string + initial []string + requested []string + updated []string + expected []string + }{ + {"helm-completes", []string{metav1.FinalizerDeleteDependents, "finalizers.fluxcd.io", "test.example/hold"}, nil, []string{metav1.FinalizerDeleteDependents, "test.example/hold"}, []string{"test.example/hold"}}, + {"controller-adds-finalizer", []string{metav1.FinalizerDeleteDependents}, nil, []string{metav1.FinalizerDeleteDependents, "finalizers.fluxcd.io"}, []string{"finalizers.fluxcd.io"}}, + {"gc-starts-during-update", []string{"finalizers.fluxcd.io"}, nil, []string{"finalizers.fluxcd.io", metav1.FinalizerDeleteDependents}, []string{"finalizers.fluxcd.io", metav1.FinalizerDeleteDependents}}, + {"gc-finishes-during-update", []string{"finalizers.fluxcd.io", metav1.FinalizerDeleteDependents}, []string{metav1.FinalizerDeleteDependents}, []string{"finalizers.fluxcd.io"}, []string{"finalizers.fluxcd.io"}}, + } { + t.Run(concurrent.name, func(t *testing.T) { + hr := optionsTestHelmRelease() + hr.Finalizers = concurrent.initial + updates := 0 + r := newOptionsTestREST(t, interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updates++ + if updates > 1 { + return c.Update(ctx, obj, opts...) + } + live := &helmv2.HelmRelease{} + if err := c.Get(ctx, client.ObjectKeyFromObject(hr), live); err != nil { + return err + } + live.Finalizers = slices.Clone(concurrent.updated) + if err := c.Update(ctx, live); err != nil { + return err + } + return apierrors.NewConflict(helmv2.GroupVersion.WithResource("helmreleases").GroupResource(), hr.Name, errors.New("concurrent controller update")) + }, + }, hr) + ctx := request.WithNamespace(context.Background(), optionsTestNamespace) + obj, err := r.Get(ctx, "example", &metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + app := obj.(*appsv1alpha1.Application) + app.Finalizers = concurrent.requested + if _, _, err := r.Update(ctx, app.Name, rest.DefaultUpdatedObjectInfo(app), nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + live := &helmv2.HelmRelease{} + if err := r.c.Get(ctx, client.ObjectKeyFromObject(hr), live); err != nil { + t.Fatal(err) + } + if updates != 2 { + t.Fatalf("update calls = %d, want 2", updates) + } + if !slices.Equal(live.Finalizers, concurrent.expected) { + t.Fatalf("finalizers = %v, want %v", live.Finalizers, concurrent.expected) + } + }) + } +} + +func TestApplicationFinalizersKeepControllerProtection(t *testing.T) { + hr := optionsTestHelmRelease() + hr.Finalizers = []string{"finalizers.fluxcd.io", metav1.FinalizerDeleteDependents} + r := newOptionsTestREST(t, interceptor.Funcs{}, hr) + ctx := request.WithNamespace(context.Background(), optionsTestNamespace) + obj, err := r.Get(ctx, "example", &metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + app := obj.(*appsv1alpha1.Application) + if !slices.Equal(app.Finalizers, []string{metav1.FinalizerDeleteDependents}) { + t.Fatalf("unexpected application finalizers: %v", app.Finalizers) + } + app.Finalizers = []string{"other.example/ignored"} + if _, _, err := r.Update(ctx, app.Name, rest.DefaultUpdatedObjectInfo(app), nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + live := &helmv2.HelmRelease{} + if err := r.c.Get(ctx, client.ObjectKeyFromObject(hr), live); err != nil { + t.Fatal(err) + } + if !slices.Equal(live.Finalizers, []string{"finalizers.fluxcd.io"}) { + t.Fatalf("controller finalizers changed: %v", live.Finalizers) + } +} + +func TestApplicationFinalizersDoNotMutateCachedHelmRelease(t *testing.T) { + hr := optionsTestHelmRelease() + hr.Finalizers = []string{metav1.FinalizerDeleteDependents, "finalizers.fluxcd.io"} + r := newOptionsTestREST(t, interceptor.Funcs{}) + app, err := r.ConvertHelmReleaseToApplication(context.Background(), hr) + if err != nil { + t.Fatal(err) + } + app.Finalizers[0] = metav1.FinalizerOrphanDependents + if !slices.Equal(hr.Finalizers, []string{metav1.FinalizerDeleteDependents, "finalizers.fluxcd.io"}) { + t.Fatalf("conversion changed cached finalizers: %v", hr.Finalizers) + } +} + +func TestApplicationGarbageCollectionWithInPlaceUpdate(t *testing.T) { + hr := optionsTestHelmRelease() + hr.Finalizers = []string{metav1.FinalizerDeleteDependents} + now := metav1.Now() + hr.DeletionTimestamp = &now + r := newOptionsTestREST(t, interceptor.Funcs{}, hr) + ctx := request.WithNamespace(context.Background(), optionsTestNamespace) + update := rest.DefaultUpdatedObjectInfo(nil, func(_ context.Context, _, old runtime.Object) (runtime.Object, error) { + app := old.(*appsv1alpha1.Application) + app.Finalizers = nil + return app, nil + }) + if _, _, err := r.Update(ctx, "example", update, nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + if err := r.c.Get(ctx, client.ObjectKeyFromObject(hr), &helmv2.HelmRelease{}); !apierrors.IsNotFound(err) { + t.Fatalf("release still exists after in-place finalizer removal: %v", err) + } +}