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) + } +}