diff --git a/controller/appcontroller.go b/controller/appcontroller.go index fb835717f4..c468308dbe 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1062,6 +1062,9 @@ func (ctrl *ApplicationController) processAppOperationQueueItem() (processNext b }) message := fmt.Sprintf("Unable to delete application resources: %v", err.Error()) ctrl.logAppEvent(context.TODO(), app, argo.EventInfo{Reason: argo.EventReasonStatusRefreshed, Type: corev1.EventTypeWarning}, message) + } else { + // Clear DeletionError condition if deletion is progressing successfully + app.Status.SetConditions([]appv1.ApplicationCondition{}, map[appv1.ApplicationConditionType]bool{appv1.ApplicationConditionDeletionError: true}) } ts.AddCheckpoint("finalize_application_deletion_ms") } @@ -1203,17 +1206,21 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic if err != nil { return err } + + // Get destination cluster destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db) if err != nil { logCtx.WithError(err).Warn("Unable to get destination cluster") app.UnSetCascadedDeletion() app.UnSetPostDeleteFinalizerAll() + app.UnSetPreDeleteFinalizerAll() if err := ctrl.updateFinalizers(app); err != nil { return err } logCtx.Infof("Resource entries removed from undefined cluster") return nil } + clusterRESTConfig, err := destCluster.RESTConfig() if err != nil { return err @@ -1225,9 +1232,30 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic return fmt.Errorf("cannot apply impersonation: %w", err) } + // Handle PreDelete hooks - run them before any deletion occurs + if app.HasPreDeleteFinalizer() { + objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) + if err != nil { + return fmt.Errorf("error getting permitted app live objects: %w", err) + } + + done, err := ctrl.executePreDeleteHooks(app, proj, objsMap, config, logCtx) + if err != nil { + return fmt.Errorf("error executing pre-delete hooks: %w", err) + } + if !done { + // PreDelete hooks are still running - wait for them to complete + return nil + } + // PreDelete hooks are done - remove the finalizer so we can continue with deletion + app.UnSetPreDeleteFinalizer() + if err := ctrl.updateFinalizers(app); err != nil { + return fmt.Errorf("error updating pre-delete finalizers: %w", err) + } + } + if app.CascadedDeletion() { deletionApproved := app.IsDeletionConfirmed(app.DeletionTimestamp.Time) - logCtx.Infof("Deleting resources") // ApplicationDestination points to a valid cluster, so we may clean up the live objects objs := make([]*unstructured.Unstructured, 0) @@ -1304,6 +1332,23 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic return ctrl.updateFinalizers(app) } + if app.HasPreDeleteFinalizer("cleanup") { + objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) + if err != nil { + return fmt.Errorf("error getting permitted app live objects for pre-delete cleanup: %w", err) + } + + done, err := ctrl.cleanupPreDeleteHooks(objsMap, config, logCtx) + if err != nil { + return fmt.Errorf("error cleaning up pre-delete hooks: %w", err) + } + if !done { + return nil + } + app.UnSetPreDeleteFinalizer("cleanup") + return ctrl.updateFinalizers(app) + } + if app.HasPostDeleteFinalizer("cleanup") { objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) if err != nil { @@ -1321,7 +1366,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic return ctrl.updateFinalizers(app) } - if !app.CascadedDeletion() && !app.HasPostDeleteFinalizer() { + if !app.CascadedDeletion() && !app.HasPostDeleteFinalizer() && !app.HasPreDeleteFinalizer() { if err := ctrl.cache.SetAppManagedResources(app.Name, nil); err != nil { return err } @@ -1835,10 +1880,25 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo app.Status.SourceTypes = compareResult.appSourceTypes app.Status.ControllerNamespace = ctrl.namespace ts.AddCheckpoint("app_status_update_ms") - patchDuration = ctrl.persistAppStatus(origApp, &app.Status) - // This is a partly a duplicate of patch_ms, but more descriptive and allows to have measurement for the next step. - ts.AddCheckpoint("persist_app_status_ms") - if (compareResult.hasPostDeleteHooks != app.HasPostDeleteFinalizer() || compareResult.hasPostDeleteHooks != app.HasPostDeleteFinalizer("cleanup")) && + // Update finalizers BEFORE persisting status to avoid race condition where app shows "Synced" + // but doesn't have finalizers yet, which would allow deletion without running pre-delete hooks + if (compareResult.hasPreDeleteHooks != app.HasPreDeleteFinalizer() || + compareResult.hasPreDeleteHooks != app.HasPreDeleteFinalizer("cleanup")) && + app.GetDeletionTimestamp() == nil { + if compareResult.hasPreDeleteHooks { + app.SetPreDeleteFinalizer() + app.SetPreDeleteFinalizer("cleanup") + } else { + app.UnSetPreDeleteFinalizer() + app.UnSetPreDeleteFinalizer("cleanup") + } + + if err := ctrl.updateFinalizers(app); err != nil { + logCtx.Errorf("Failed to update pre-delete finalizers: %v", err) + } + } + if (compareResult.hasPostDeleteHooks != app.HasPostDeleteFinalizer() || + compareResult.hasPostDeleteHooks != app.HasPostDeleteFinalizer("cleanup")) && app.GetDeletionTimestamp() == nil { if compareResult.hasPostDeleteHooks { app.SetPostDeleteFinalizer() @@ -1849,10 +1909,13 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo } if err := ctrl.updateFinalizers(app); err != nil { - logCtx.WithError(err).Error("Failed to update finalizers") + logCtx.WithError(err).Error("Failed to update post-delete finalizers") } } ts.AddCheckpoint("process_finalizers_ms") + patchDuration = ctrl.persistAppStatus(origApp, &app.Status) + // This is a partly a duplicate of patch_ms, but more descriptive and allows to have measurement for the next step. + ts.AddCheckpoint("persist_app_status_ms") return processNext } diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 63697f2f38..49591af542 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -407,6 +407,37 @@ metadata: data: ` +var fakePreDeleteHook = ` +{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": { + "name": "pre-delete-hook", + "namespace": "default", + "labels": { + "app.kubernetes.io/instance": "my-app" + }, + "annotations": { + "argocd.argoproj.io/hook": "PreDelete" + } + }, + "spec": { + "containers": [ + { + "name": "pre-delete-hook", + "image": "busybox", + "restartPolicy": "Never", + "command": [ + "/bin/sh", + "-c", + "sleep 5 && echo hello from the pre-delete-hook pod" + ] + } + ] + } +} +` + var fakePostDeleteHook = ` { "apiVersion": "batch/v1", @@ -557,6 +588,15 @@ func newFakeCM() map[string]any { return cm } +func newFakePreDeleteHook() map[string]any { + var cm map[string]any + err := yaml.Unmarshal([]byte(fakePreDeleteHook), &cm) + if err != nil { + panic(err) + } + return cm +} + func newFakePostDeleteHook() map[string]any { var hook map[string]any err := yaml.Unmarshal([]byte(fakePostDeleteHook), &hook) @@ -1114,6 +1154,40 @@ func TestFinalizeAppDeletion(t *testing.T) { testShouldDelete(app3) }) + t.Run("PreDelete_HookIsCreated", func(t *testing.T) { + app := newFakeApp() + app.SetPreDeleteFinalizer() + app.Spec.Destination.Namespace = test.FakeArgoCDNamespace + ctrl := newFakeController(context.Background(), &fakeData{ + manifestResponses: []*apiclient.ManifestResponse{{ + Manifests: []string{fakePreDeleteHook}, + }}, + apps: []runtime.Object{app, &defaultProj}, + managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{}, + }, nil) + + patched := false + fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) + defaultReactor := fakeAppCs.ReactionChain[0] + fakeAppCs.ReactionChain = nil + fakeAppCs.AddReactor("get", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { + return defaultReactor.React(action) + }) + fakeAppCs.AddReactor("patch", "*", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) { + patched = true + return true, &v1alpha1.Application{}, nil + }) + err := ctrl.finalizeApplicationDeletion(app, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + require.NoError(t, err) + // finalizer is not deleted + assert.False(t, patched) + // pre-delete hook is created + require.Len(t, ctrl.kubectl.(*MockKubectl).CreatedResources, 1) + require.Equal(t, "pre-delete-hook", ctrl.kubectl.(*MockKubectl).CreatedResources[0].GetName()) + }) + t.Run("PostDelete_HookIsCreated", func(t *testing.T) { app := newFakeApp() app.SetPostDeleteFinalizer() @@ -1148,6 +1222,41 @@ func TestFinalizeAppDeletion(t *testing.T) { require.Equal(t, "post-delete-hook", ctrl.kubectl.(*MockKubectl).CreatedResources[0].GetName()) }) + t.Run("PreDelete_HookIsExecuted", func(t *testing.T) { + app := newFakeApp() + app.SetPreDeleteFinalizer() + app.Spec.Destination.Namespace = test.FakeArgoCDNamespace + liveHook := &unstructured.Unstructured{Object: newFakePreDeleteHook()} + require.NoError(t, unstructured.SetNestedField(liveHook.Object, "Succeeded", "status", "phase")) + ctrl := newFakeController(context.Background(), &fakeData{ + manifestResponses: []*apiclient.ManifestResponse{{ + Manifests: []string{fakePreDeleteHook}, + }}, + apps: []runtime.Object{app, &defaultProj}, + managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ + kube.GetResourceKey(liveHook): liveHook, + }, + }, nil) + + patched := false + fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) + defaultReactor := fakeAppCs.ReactionChain[0] + fakeAppCs.ReactionChain = nil + fakeAppCs.AddReactor("get", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { + return defaultReactor.React(action) + }) + fakeAppCs.AddReactor("patch", "*", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) { + patched = true + return true, &v1alpha1.Application{}, nil + }) + err := ctrl.finalizeApplicationDeletion(app, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + require.NoError(t, err) + // finalizer is removed + assert.True(t, patched) + }) + t.Run("PostDelete_HookIsExecuted", func(t *testing.T) { app := newFakeApp() app.SetPostDeleteFinalizer() diff --git a/controller/hook.go b/controller/hook.go index 8d1b41929d..3a4a613880 100644 --- a/controller/hook.go +++ b/controller/hook.go @@ -2,6 +2,8 @@ package controller import ( "context" + "fmt" + "strings" "github.com/argoproj/gitops-engine/pkg/health" "github.com/argoproj/gitops-engine/pkg/sync/common" @@ -14,26 +16,33 @@ import ( "github.com/argoproj/argo-cd/v3/util/lua" - "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" + appv1 "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" ) -var ( - postDeleteHook = "PostDelete" - postDeleteHooks = map[string]string{ - "argocd.argoproj.io/hook": postDeleteHook, +type HookType string + +const ( + PreDeleteHookType HookType = "PreDelete" + PostDeleteHookType HookType = "PostDelete" +) + +var hookTypeAnnotations = map[HookType]map[string]string{ + PreDeleteHookType: { + "argocd.argoproj.io/hook": string(PreDeleteHookType), + "helm.sh/hook": "pre-delete", + }, + PostDeleteHookType: { + "argocd.argoproj.io/hook": string(PostDeleteHookType), "helm.sh/hook": "post-delete", - } -) - -func isHook(obj *unstructured.Unstructured) bool { - return hook.IsHook(obj) || isPostDeleteHook(obj) + }, } -func isPostDeleteHook(obj *unstructured.Unstructured) bool { +func isHookOfType(obj *unstructured.Unstructured, hookType HookType) bool { if obj == nil || obj.GetAnnotations() == nil { return false } - for k, v := range postDeleteHooks { + + for k, v := range hookTypeAnnotations[hookType] { if val, ok := obj.GetAnnotations()[k]; ok && val == v { return true } @@ -41,11 +50,34 @@ func isPostDeleteHook(obj *unstructured.Unstructured) bool { return false } -func (ctrl *ApplicationController) executePostDeleteHooks(app *v1alpha1.Application, proj *v1alpha1.AppProject, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { +func isHook(obj *unstructured.Unstructured) bool { + if hook.IsHook(obj) { + return true + } + + for hookType := range hookTypeAnnotations { + if isHookOfType(obj, hookType) { + return true + } + } + return false +} + +func isPreDeleteHook(obj *unstructured.Unstructured) bool { + return isHookOfType(obj, PreDeleteHookType) +} + +func isPostDeleteHook(obj *unstructured.Unstructured) bool { + return isHookOfType(obj, PostDeleteHookType) +} + +// executeHooks is a generic function to execute hooks of a specified type +func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Application, proj *appv1.AppProject, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { appLabelKey, err := ctrl.settingsMgr.GetAppInstanceLabelKey() if err != nil { return false, err } + var revisions []string for _, src := range app.Spec.GetSources() { revisions = append(revisions, src.TargetRevision) @@ -55,44 +87,62 @@ func (ctrl *ApplicationController) executePostDeleteHooks(app *v1alpha1.Applicat if err != nil { return false, err } + + // Find existing hooks of the specified type runningHooks := map[kube.ResourceKey]*unstructured.Unstructured{} for key, obj := range liveObjs { - if isPostDeleteHook(obj) { + if isHookOfType(obj, hookType) { runningHooks[key] = obj } } + // Find expected hooks that need to be created expectedHook := map[kube.ResourceKey]*unstructured.Unstructured{} for _, obj := range targets { if obj.GetNamespace() == "" { obj.SetNamespace(app.Spec.Destination.Namespace) } - if !isPostDeleteHook(obj) { + if !isHookOfType(obj, hookType) { continue } if runningHook := runningHooks[kube.GetResourceKey(obj)]; runningHook == nil { expectedHook[kube.GetResourceKey(obj)] = obj } } + + // Create hooks that don't exist yet createdCnt := 0 for _, obj := range expectedHook { + // Add app instance label so the hook can be tracked and cleaned up + labels := obj.GetLabels() + if labels == nil { + labels = make(map[string]string) + } + labels[appLabelKey] = app.InstanceName(ctrl.namespace) + obj.SetLabels(labels) + _, err = ctrl.kubectl.CreateResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), obj, metav1.CreateOptions{}) if err != nil { return false, err } createdCnt++ } + if createdCnt > 0 { - logCtx.Infof("Created %d post-delete hooks", createdCnt) + logCtx.Infof("Created %d %s hooks", createdCnt, hookType) return false, nil } + + // Check health of running hooks resourceOverrides, err := ctrl.settingsMgr.GetResourceOverrides() if err != nil { return false, err } healthOverrides := lua.ResourceHealthOverrides(resourceOverrides) - progressingHooksCnt := 0 + progressingHooksCount := 0 + var failedHooks []string + var failedHookObjects []*unstructured.Unstructured for _, obj := range runningHooks { hookHealth, err := health.GetResourceHealth(obj, healthOverrides) if err != nil { @@ -110,19 +160,37 @@ func (ctrl *ApplicationController) executePostDeleteHooks(app *v1alpha1.Applicat Status: health.HealthStatusHealthy, } } - if hookHealth.Status == health.HealthStatusProgressing { - progressingHooksCnt++ + switch hookHealth.Status { + case health.HealthStatusProgressing: + progressingHooksCount++ + case health.HealthStatusDegraded: + failedHooks = append(failedHooks, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName())) + failedHookObjects = append(failedHookObjects, obj) } } - if progressingHooksCnt > 0 { - logCtx.Infof("Waiting for %d post-delete hooks to complete", progressingHooksCnt) + + if len(failedHooks) > 0 { + // Delete failed hooks to allow retry with potentially fixed hook definitions + logCtx.Infof("Deleting %d failed %s hook(s) to allow retry", len(failedHookObjects), hookType) + for _, obj := range failedHookObjects { + err = ctrl.kubectl.DeleteResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), metav1.DeleteOptions{}) + if err != nil { + logCtx.WithError(err).Warnf("Failed to delete failed hook %s/%s", obj.GetNamespace(), obj.GetName()) + } + } + return false, fmt.Errorf("%s hook(s) failed: %s", hookType, strings.Join(failedHooks, ", ")) + } + + if progressingHooksCount > 0 { + logCtx.Infof("Waiting for %d %s hooks to complete", progressingHooksCount, hookType) return false, nil } return true, nil } -func (ctrl *ApplicationController) cleanupPostDeleteHooks(liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { +// cleanupHooks is a generic function to clean up hooks of a specified type +func (ctrl *ApplicationController) cleanupHooks(hookType HookType, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { resourceOverrides, err := ctrl.settingsMgr.GetResourceOverrides() if err != nil { return false, err @@ -132,8 +200,10 @@ func (ctrl *ApplicationController) cleanupPostDeleteHooks(liveObjs map[kube.Reso pendingDeletionCount := 0 aggregatedHealth := health.HealthStatusHealthy var hooks []*unstructured.Unstructured + + // Collect hooks and determine overall health for _, obj := range liveObjs { - if !isPostDeleteHook(obj) { + if !isHookOfType(obj, hookType) { continue } hookHealth, err := health.GetResourceHealth(obj, healthOverrides) @@ -151,25 +221,60 @@ func (ctrl *ApplicationController) cleanupPostDeleteHooks(liveObjs map[kube.Reso hooks = append(hooks, obj) } + // Process hooks for deletion for _, obj := range hooks { - for _, policy := range hook.DeletePolicies(obj) { - if (policy != common.HookDeletePolicyHookFailed || aggregatedHealth != health.HealthStatusDegraded) && (policy != common.HookDeletePolicyHookSucceeded || aggregatedHealth != health.HealthStatusHealthy) { - continue + deletePolicies := hook.DeletePolicies(obj) + shouldDelete := false + + if len(deletePolicies) == 0 { + // If no delete policy is specified, always delete hooks during cleanup phase + shouldDelete = true + } else { + // Check if any delete policy matches the current hook state + for _, policy := range deletePolicies { + if (policy == common.HookDeletePolicyHookFailed && aggregatedHealth == health.HealthStatusDegraded) || + (policy == common.HookDeletePolicyHookSucceeded && aggregatedHealth == health.HealthStatusHealthy) { + shouldDelete = true + break + } } + } + + if shouldDelete { pendingDeletionCount++ if obj.GetDeletionTimestamp() != nil { continue } - logCtx.Infof("Deleting post-delete hook %s/%s", obj.GetNamespace(), obj.GetName()) + logCtx.Infof("Deleting %s hook %s/%s", hookType, obj.GetNamespace(), obj.GetName()) err = ctrl.kubectl.DeleteResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), metav1.DeleteOptions{}) if err != nil { return false, err } } } + if pendingDeletionCount > 0 { - logCtx.Infof("Waiting for %d post-delete hooks to be deleted", pendingDeletionCount) + logCtx.Infof("Waiting for %d %s hooks to be deleted", pendingDeletionCount, hookType) return false, nil } + return true, nil } + +// Execute and cleanup hooks for pre-delete and post-delete operations + +func (ctrl *ApplicationController) executePreDeleteHooks(app *appv1.Application, proj *appv1.AppProject, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { + return ctrl.executeHooks(PreDeleteHookType, app, proj, liveObjs, config, logCtx) +} + +func (ctrl *ApplicationController) cleanupPreDeleteHooks(liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { + return ctrl.cleanupHooks(PreDeleteHookType, liveObjs, config, logCtx) +} + +func (ctrl *ApplicationController) executePostDeleteHooks(app *appv1.Application, proj *appv1.AppProject, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { + return ctrl.executeHooks(PostDeleteHookType, app, proj, liveObjs, config, logCtx) +} + +func (ctrl *ApplicationController) cleanupPostDeleteHooks(liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) { + return ctrl.cleanupHooks(PostDeleteHookType, liveObjs, config, logCtx) +} diff --git a/controller/hook_test.go b/controller/hook_test.go new file mode 100644 index 0000000000..4ef25c6952 --- /dev/null +++ b/controller/hook_test.go @@ -0,0 +1,173 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestIsHookOfType(t *testing.T) { + tests := []struct { + name string + hookType HookType + annot map[string]string + expected bool + }{ + { + name: "ArgoCD PreDelete hook", + hookType: PreDeleteHookType, + annot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expected: true, + }, + { + name: "Helm PreDelete hook", + hookType: PreDeleteHookType, + annot: map[string]string{"helm.sh/hook": "pre-delete"}, + expected: true, + }, + { + name: "ArgoCD PostDelete hook", + hookType: PostDeleteHookType, + annot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expected: true, + }, + { + name: "Helm PostDelete hook", + hookType: PostDeleteHookType, + annot: map[string]string{"helm.sh/hook": "post-delete"}, + expected: true, + }, + { + name: "Not a hook", + hookType: PreDeleteHookType, + annot: map[string]string{"some-other": "annotation"}, + expected: false, + }, + { + name: "Wrong hook type", + hookType: PreDeleteHookType, + annot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expected: false, + }, + { + name: "Nil annotations", + hookType: PreDeleteHookType, + annot: nil, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj := &unstructured.Unstructured{} + obj.SetAnnotations(tt.annot) + result := isHookOfType(obj, tt.hookType) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestIsHook(t *testing.T) { + tests := []struct { + name string + annot map[string]string + expected bool + }{ + { + name: "ArgoCD PreDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expected: true, + }, + { + name: "ArgoCD PostDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expected: true, + }, + { + name: "ArgoCD PreSync hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PreSync"}, + expected: true, + }, + { + name: "Not a hook", + annot: map[string]string{"some-other": "annotation"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj := &unstructured.Unstructured{} + obj.SetAnnotations(tt.annot) + result := isHook(obj) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestIsPreDeleteHook(t *testing.T) { + tests := []struct { + name string + annot map[string]string + expected bool + }{ + { + name: "ArgoCD PreDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expected: true, + }, + { + name: "Helm PreDelete hook", + annot: map[string]string{"helm.sh/hook": "pre-delete"}, + expected: true, + }, + { + name: "ArgoCD PostDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj := &unstructured.Unstructured{} + obj.SetAnnotations(tt.annot) + result := isPreDeleteHook(obj) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestIsPostDeleteHook(t *testing.T) { + tests := []struct { + name string + annot map[string]string + expected bool + }{ + { + name: "ArgoCD PostDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expected: true, + }, + { + name: "Helm PostDelete hook", + annot: map[string]string{"helm.sh/hook": "post-delete"}, + expected: true, + }, + { + name: "ArgoCD PreDelete hook", + annot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj := &unstructured.Unstructured{} + obj.SetAnnotations(tt.annot) + result := isPostDeleteHook(obj) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/controller/state.go b/controller/state.go index 53a6903e8a..ee14fdcaec 100644 --- a/controller/state.go +++ b/controller/state.go @@ -95,6 +95,7 @@ type comparisonResult struct { timings map[string]time.Duration diffResultList *diff.DiffResultList hasPostDeleteHooks bool + hasPreDeleteHooks bool // revisionsMayHaveChanges indicates if there are any possibilities that the revisions contain changes revisionsMayHaveChanges bool } @@ -765,14 +766,26 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 } } } + hasPreDeleteHooks := false hasPostDeleteHooks := false + // Filter out PreDelete and PostDelete hooks from targetObjs since they should not be synced + // as regular resources. They are only executed during deletion. + var targetObjsForSync []*unstructured.Unstructured for _, obj := range targetObjs { + if isPreDeleteHook(obj) { + hasPreDeleteHooks = true + // Skip PreDelete hooks - they are not synced, only executed during deletion + continue + } if isPostDeleteHook(obj) { hasPostDeleteHooks = true + // Skip PostDelete hooks - they are not synced, only executed after deletion + continue } + targetObjsForSync = append(targetObjsForSync, obj) } - reconciliation := sync.Reconcile(targetObjs, liveObjByKey, app.Spec.Destination.Namespace, infoProvider) + reconciliation := sync.Reconcile(targetObjsForSync, liveObjByKey, app.Spec.Destination.Namespace, infoProvider) ts.AddCheckpoint("live_ms") compareOptions, err := m.settingsMgr.GetResourceCompareOptions() @@ -989,6 +1002,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 diffConfig: diffConfig, diffResultList: diffResults, hasPostDeleteHooks: hasPostDeleteHooks, + hasPreDeleteHooks: hasPreDeleteHooks, revisionsMayHaveChanges: revisionsMayHaveChanges, } diff --git a/controller/sync.go b/controller/sync.go index 9f8a505b5c..4a495180d6 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -330,6 +330,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp sync.WithResourcesFilter(func(key kube.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool { return (len(syncOp.Resources) == 0 || isPostDeleteHook(target) || + isPreDeleteHook(target) || argo.ContainsSyncResource(key.Name, key.Namespace, schema.GroupVersionKind{Kind: key.Kind, Group: key.Group}, syncOp.Resources)) && m.isSelfReferencedObj(live, target, app.GetName(), v1alpha1.TrackingMethod(trackingMethod), installationID) }), diff --git a/docs/user-guide/helm.md b/docs/user-guide/helm.md index f2c9760742..87d2b136d1 100644 --- a/docs/user-guide/helm.md +++ b/docs/user-guide/helm.md @@ -273,7 +273,7 @@ Argo CD supports many (most?) Helm hooks by mapping the Helm annotations onto Ar | Helm Annotation | Notes | | ------------------------------- |-----------------------------------------------------------------------------------------------| | `helm.sh/hook: crd-install` | Supported as equivalent to normal Argo CD CRD handling. | -| `helm.sh/hook: pre-delete` | Not supported. In Helm stable there are 3 cases used to clean up CRDs and 3 to clean-up jobs. | +| `helm.sh/hook: pre-delete` | Supported as equivalent to `argocd.argoproj.io/hook: PreDelete` | | `helm.sh/hook: pre-rollback` | Not supported. Never used in Helm stable. | | `helm.sh/hook: pre-install` | Supported as equivalent to `argocd.argoproj.io/hook: PreSync`. | | `helm.sh/hook: pre-upgrade` | Supported as equivalent to `argocd.argoproj.io/hook: PreSync`. | diff --git a/docs/user-guide/sync-waves.md b/docs/user-guide/sync-waves.md index aff496e1c5..130777a0c9 100644 --- a/docs/user-guide/sync-waves.md +++ b/docs/user-guide/sync-waves.md @@ -4,14 +4,15 @@ Sync phases and hooks define when resources are applied such as before or after Argo CD has the following hook types: -| Hook | Description | -|------|-------------| -| `PreSync` | Executes prior to the application of the manifests. | -| `Sync` | Executes after all `PreSync` hooks completed and were successful, at the same time as the application of the manifests. | -| `Skip` | Indicates to Argo CD to skip the application of the manifest. | -| `PostSync` | Executes after all `Sync` hooks completed and were successful, a successful application, and all resources in a `Healthy` state. | -| `SyncFail` | Executes when the sync operation fails. | -| `PostDelete` | Executes after all Application resources are deleted. _Available starting in v2.10._ | +| Hook | Description | +|--------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `PreSync` | Executes prior to the application of the manifests. | +| `Sync` | Executes after all `PreSync` hooks completed and were successful, at the same time as the application of the manifests. | +| `Skip` | Indicates to Argo CD to skip the application of the manifest. | +| `PostSync` | Executes after all `Sync` hooks completed and were successful, a successful application, and all resources in a `Healthy` state. | +| `SyncFail` | Executes when the sync operation fails. | +| `PreDelete` | Executes before Application resources are deleted. Only runs when the entire Application is being deleted, not during normal sync operations (even with pruning enabled. ) | +| `PostDelete` | Executes after all Application resources are deleted. _Available starting in v2.10._ | Adding the argocd.argoproj.io/hook annotation to a resource will assign it to a specific phase. During a Sync operation, Argo CD will apply the resource during the appropriate phase of the deployment. Hooks can be any type of Kubernetes resource kind, but tend to be Pod, Job or Argo Workflows. Multiple hooks can be specified as a comma separated list. @@ -41,14 +42,58 @@ Argo CD offers several methods to clean up hooks and decide how much history wil In the most basic case you can use the argocd.argoproj.io/hook-delete-policy to decide when a hook will be deleted. This can take the following values: -| Policy | Description | -|--------|-------------| -| `HookSucceeded` | The hook resource is deleted after the hook succeeded (e.g. Job/Workflow completed successfully). | -| `HookFailed` | The hook resource is deleted after the hook failed. | +| Policy | Description | +|----------------------|---------------------------------------------------------------------------------------------------------------------------------| +| `HookSucceeded` | The hook resource is deleted after the hook succeeded (e.g. Job/Workflow completed successfully). | +| `HookFailed` | The hook resource is deleted after the hook failed. | | `BeforeHookCreation` | Any existing hook resource is deleted before the new one is created (since v1.3). It is meant to be used with `/metadata/name`. | Note that if no deletion policy is specified, Argo CD will automatically assume `BeforeHookCreation` rules. +## PreDelete and PostDelete Hooks + +### PreDelete Hooks + +PreDelete hooks execute before an Application and its resources are deleted. They run only during Application deletion (e.g., `kubectl delete application` or `argocd app delete`), not during normal sync operations, even when pruning is enabled. + +**Behavior:** + +1. When an Application is deleted, Argo CD checks for PreDelete hooks defined in the Application's manifests +2. If PreDelete hooks exist, they are created and executed before any Application resources are deleted +3. Argo CD waits for all PreDelete hooks to reach a Healthy state before proceeding with deletion +4. Once all PreDelete hooks complete successfully, they are cleaned up and the Application resources are deleted + +**Failure Handling:** + +If a PreDelete hook fails (e.g., a Job fails or a Pod crashes), the Application deletion is blocked: + +- The Application will remain in a deleting state with a `DeletionError` condition +- Application resources will not be deleted until the hook succeeds +- The user can fix the failing hook by updating its manifest in the git repository +- After fixing the hook, Argo CD will automatically retry the deletion on the next reconciliation +- Alternatively, the user can manually delete the failing hook resource to allow deletion to proceed + + +### PostDelete Hooks + +PostDelete hooks execute after all Application resources have been deleted. They are useful for cleanup operations, notifications, or removing external resources. + +**Behavior:** + +1. Application resources are deleted first +2. Once all Application resources are fully removed, PostDelete hooks are created and executed +3. Argo CD waits for all PostDelete hooks to reach a Healthy state +4. Once all PostDelete hooks complete successfully, they are cleaned up and the Application is fully removed + +**Failure Handling:** + +If a PostDelete hook fails: + +- The Application is already deleted from the cluster, but the Application CR remains with a `DeletionError` condition +- The user can fix the failing hook by updating its manifest in the git repository +- After fixing the hook, Argo CD will automatically retry on the next reconciliation +- Alternatively, the user can manually delete the failing hook resource to complete the Application deletion + ## How sync waves work? Argo CD also offers an alternative method of changing the sync order of resources. These are sync waves. They are defined by the argocd.argoproj.io/sync-wave annotation. The value is an integer that defines the ordering (ArgoCD will start deploying from the lowest number and finish with the highest number). @@ -176,20 +221,20 @@ spec: Upgrading ingress-nginx controller (managed by helm) with ArgoCD 2.x sometimes fails to work resulting in: -.|. --|- -OPERATION|Sync -PHASE|Running -MESSAGE|waiting for completion of hook batch/Job/ingress-nginx-admission-create +| . | . | +|-----------|-------------------------------------------------------------------------| +| OPERATION | Sync | +| PHASE | Running | +| MESSAGE | waiting for completion of hook batch/Job/ingress-nginx-admission-create | -.|. --|- -KIND |batch/v1/Job -NAMESPACE|ingress-nginx -NAME |ingress-nginx-admission-create -STATUS |Running -HOOK |PreSync -MESSAGE |Pending deletion +| . | . | +|-----------|--------------------------------| +| KIND | batch/v1/Job | +| NAMESPACE | ingress-nginx | +| NAME | ingress-nginx-admission-create | +| STATUS | Running | +| HOOK | PreSync | +| MESSAGE | Pending deletion | To work around this, a helm user can add: diff --git a/pkg/apis/application/v1alpha1/application_defaults.go b/pkg/apis/application/v1alpha1/application_defaults.go index ad8112af8c..218b548567 100644 --- a/pkg/apis/application/v1alpha1/application_defaults.go +++ b/pkg/apis/application/v1alpha1/application_defaults.go @@ -9,6 +9,9 @@ const ( // ResourcesFinalizerName is the finalizer value which we inject to finalize deletion of an application ResourcesFinalizerName string = "resources-finalizer.argocd.argoproj.io" + // PreDeleteFinalizerName is the finalizer that controls pre-delete hooks execution + PreDeleteFinalizerName string = "pre-delete-finalizer.argocd.argoproj.io" + // PostDeleteFinalizerName is the finalizer that controls post-delete hooks execution PostDeleteFinalizerName string = "post-delete-finalizer.argocd.argoproj.io" diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 662c8f797f..792f0c22b2 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -3350,6 +3350,26 @@ func (app *Application) IsHydrateRequested() bool { return false } +func (app *Application) HasPreDeleteFinalizer(stage ...string) bool { + return getFinalizerIndex(app.ObjectMeta, strings.Join(append([]string{PreDeleteFinalizerName}, stage...), "/")) > -1 +} + +func (app *Application) SetPreDeleteFinalizer(stage ...string) { + setFinalizer(&app.ObjectMeta, strings.Join(append([]string{PreDeleteFinalizerName}, stage...), "/"), true) +} + +func (app *Application) UnSetPreDeleteFinalizer(stage ...string) { + setFinalizer(&app.ObjectMeta, strings.Join(append([]string{PreDeleteFinalizerName}, stage...), "/"), false) +} + +func (app *Application) UnSetPreDeleteFinalizerAll() { + for _, finalizer := range app.Finalizers { + if strings.HasPrefix(finalizer, PreDeleteFinalizerName) { + setFinalizer(&app.ObjectMeta, finalizer, false) + } + } +} + func (app *Application) HasPostDeleteFinalizer(stage ...string) bool { return getFinalizerIndex(app.ObjectMeta, strings.Join(append([]string{PostDeleteFinalizerName}, stage...), "/")) > -1 } diff --git a/test/e2e/hook_test.go b/test/e2e/hook_test.go index a350bfaadf..0a1601c377 100644 --- a/test/e2e/hook_test.go +++ b/test/e2e/hook_test.go @@ -52,6 +52,53 @@ func testHookSuccessful(t *testing.T, hookType HookType) { Expect(ResourceResultIs(ResourceResult{Version: "v1", Kind: "Pod", Namespace: DeploymentNamespace(), Images: []string{"quay.io/argoprojlabs/argocd-e2e-container:0.1"}, Name: "hook", Message: "pod/hook created", HookType: hookType, HookPhase: OperationSucceeded, SyncPhase: SyncPhase(hookType)})) } +func TestPreDeleteHook(t *testing.T) { + Given(t). + Path("pre-delete-hook"). + When(). + CreateApp(). + Sync(). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + And(func(_ *Application) { + _, err := KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Get( + t.Context(), "guestbook-ui", metav1.GetOptions{}, + ) + require.NoError(t, err) + }). + When(). + Delete(true). + Then(). + Expect(DoesNotExist()). + Expect(NotPod(func(p corev1.Pod) bool { + return p.Name == "hook" + })) +} + +func TestPreDeleteHookFailureAndRetry(t *testing.T) { + Given(t). + Path("pre-delete-hook"). + When(). + // Patch hook to make it fail + PatchFile("hook.yaml", `[{"op": "replace", "path": "/spec/containers/0/command/0", "value": "false"}]`). + CreateApp(). + Sync(). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + When(). + Delete(false). // Non-blocking delete + Then(). + // App should still exist because pre-delete hook failed + Expect(Condition(ApplicationConditionDeletionError, "")). + When(). + // Fix the hook by patching it to succeed + PatchFile("hook.yaml", `[{"op": "replace", "path": "/spec/containers/0/command", "value": ["sleep", "3"]}]`). + Refresh(RefreshTypeNormal). + Then(). + // After fixing the hook, deletion should eventually succeed + Expect(DoesNotExist()) +} + func TestPostDeleteHook(t *testing.T) { Given(t). Path("post-delete-hook"). @@ -61,12 +108,9 @@ func TestPostDeleteHook(t *testing.T) { Delete(true). Then(). Expect(DoesNotExist()). - AndAction(func() { - hooks, err := KubeClientset.CoreV1().Pods(DeploymentNamespace()).List(t.Context(), metav1.ListOptions{}) - require.NoError(t, err) - assert.Len(t, hooks.Items, 1) - assert.Equal(t, "hook", hooks.Items[0].Name) - }) + Expect(Pod(func(p corev1.Pod) bool { + return p.Name == "hook" + })) } // make sure that hooks do not appear in "argocd app diff" diff --git a/test/e2e/testdata/pre-delete-hook/hook.yaml b/test/e2e/testdata/pre-delete-hook/hook.yaml new file mode 100644 index 0000000000..3264ffb03e --- /dev/null +++ b/test/e2e/testdata/pre-delete-hook/hook.yaml @@ -0,0 +1,25 @@ +apiVersion: v1 +kind: Pod +metadata: + annotations: + argocd.argoproj.io/hook: PreDelete + labels: + argocd.argoproj.io/hook-type: PreDelete + name: hook +spec: + containers: + - command: + - "sleep" + - "3" + image: quay.io/argoprojlabs/argocd-e2e-container:0.1 + imagePullPolicy: IfNotPresent + name: main + restartPolicy: Never +--- +# Regular resource to be managed and deleted +apiVersion: v1 +kind: ConfigMap +metadata: + name: guestbook-ui +data: + app: "guestbook" \ No newline at end of file