diff --git a/controller/hook.go b/controller/hook.go index 9a731a9b35..7d565197bb 100644 --- a/controller/hook.go +++ b/controller/hook.go @@ -11,6 +11,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/argoproj/gitops-engine/pkg/utils/kube" log "github.com/sirupsen/logrus" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/rest" @@ -103,6 +104,7 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap revisions = append(revisions, src.TargetRevision) } + // Fetch target objects from Git to know which hooks should exist targets, _, _, err := ctrl.appStateManager.GetRepoObjs(context.Background(), app, app.Spec.GetSources(), appLabelKey, revisions, false, false, false, proj, true) if err != nil { return false, err @@ -125,14 +127,14 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap if !isHookOfType(obj, hookType) { continue } - if runningHook := runningHooks[kube.GetResourceKey(obj)]; runningHook == nil { + if _, alreadyExists := runningHooks[kube.GetResourceKey(obj)]; !alreadyExists { expectedHook[kube.GetResourceKey(obj)] = obj } } // Create hooks that don't exist yet createdCnt := 0 - for _, obj := range expectedHook { + for key, obj := range expectedHook { // Add app instance label so the hook can be tracked and cleaned up labels := obj.GetLabels() if labels == nil { @@ -141,8 +143,13 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap labels[appLabelKey] = app.InstanceName(ctrl.namespace) obj.SetLabels(labels) + logCtx.Infof("Creating %s hook resource: %s", hookType, key) _, err = ctrl.kubectl.CreateResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), obj, metav1.CreateOptions{}) if err != nil { + if apierrors.IsAlreadyExists(err) { + logCtx.Warnf("Hook resource %s already exists, skipping", key) + continue + } return false, err } createdCnt++ @@ -163,7 +170,8 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap progressingHooksCount := 0 var failedHooks []string var failedHookObjects []*unstructured.Unstructured - for _, obj := range runningHooks { + + for key, obj := range runningHooks { hookHealth, err := health.GetResourceHealth(obj, healthOverrides) if err != nil { return false, err @@ -180,12 +188,17 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap Status: health.HealthStatusHealthy, } } + switch hookHealth.Status { case health.HealthStatusProgressing: + logCtx.Debugf("Hook %s is progressing", key) progressingHooksCount++ case health.HealthStatusDegraded: + logCtx.Warnf("Hook %s is degraded: %s", key, hookHealth.Message) failedHooks = append(failedHooks, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName())) failedHookObjects = append(failedHookObjects, obj) + case health.HealthStatusHealthy: + logCtx.Debugf("Hook %s is healthy", key) } } @@ -194,7 +207,7 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap 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 { + if err != nil && !apierrors.IsNotFound(err) { logCtx.WithError(err).Warnf("Failed to delete failed hook %s/%s", obj.GetNamespace(), obj.GetName()) } } @@ -241,6 +254,10 @@ func (ctrl *ApplicationController) cleanupHooks(hookType HookType, liveObjs map[ hooks = append(hooks, obj) } + if len(hooks) == 0 { + return true, nil + } + // Process hooks for deletion for _, obj := range hooks { deletePolicies := hook.DeletePolicies(obj) @@ -267,7 +284,7 @@ func (ctrl *ApplicationController) cleanupHooks(hookType HookType, liveObjs map[ } 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 { + if err != nil && !apierrors.IsNotFound(err) { return false, err } } diff --git a/controller/hook_test.go b/controller/hook_test.go index 474e9dd161..3c5ca2b1f5 100644 --- a/controller/hook_test.go +++ b/controller/hook_test.go @@ -3,8 +3,10 @@ package controller import ( "testing" + "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" ) func TestIsHookOfType(t *testing.T) { @@ -312,3 +314,174 @@ func TestMultiHookOfType(t *testing.T) { }) } } + +func TestExecuteHooksAlreadyExistsLogic(t *testing.T) { + newObj := func(name string, annot map[string]string) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "batch", Version: "v1", Kind: "Job"}) + obj.SetName(name) + obj.SetNamespace("default") + obj.SetAnnotations(annot) + return obj + } + + tests := []struct { + name string + hookType []HookType + targetAnnot map[string]string + liveAnnot map[string]string // nil -> object doesn't exist in cluster + expectCreated bool + }{ + // PRE DELETE TESTS + { + name: "PreDelete (argocd): Not in cluster - should be created", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + liveAnnot: nil, + expectCreated: true, + }, + { + name: "PreDelete (helm): Not in cluster - should be created", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "pre-delete"}, + liveAnnot: nil, + expectCreated: true, + }, + { + name: "PreDelete (argocd): Already exists - should be skipped", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expectCreated: false, + }, + { + name: "PreDelete (argocd): Already exists - should be skipped", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "pre-delete"}, + liveAnnot: map[string]string{"helm.sh/hook": "pre-delete"}, + expectCreated: false, + }, + { + name: "PreDelete (helm+argocd): One of two already exists - should be skipped", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "pre-delete", "argocd.argoproj.io/hook": "PreDelete"}, + liveAnnot: map[string]string{"helm.sh/hook": "pre-delete"}, + expectCreated: false, + }, + { + name: "PreDelete (helm+argocd): One of two already exists - should be skipped", + hookType: []HookType{PreDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "pre-delete", "argocd.argoproj.io/hook": "PreDelete"}, + liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"}, + expectCreated: false, + }, + // POST DELETE TESTS + { + name: "PostDelete (argocd): Not in cluster - should be created", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + liveAnnot: nil, + expectCreated: true, + }, + { + name: "PostDelete (helm): Not in cluster - should be created", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete"}, + liveAnnot: nil, + expectCreated: true, + }, + { + name: "PostDelete (argocd): Already exists - should be skipped", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expectCreated: false, + }, + { + name: "PostDelete (helm): Already exists - should be skipped", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete"}, + liveAnnot: map[string]string{"helm.sh/hook": "post-delete"}, + expectCreated: false, + }, + { + name: "PostDelete (helm+argocd): Already exists - should be skipped", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"}, + liveAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"}, + expectCreated: false, + }, + { + name: "PostDelete (helm+argocd): One of two already exists - should be skipped", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"}, + liveAnnot: map[string]string{"helm.sh/hook": "post-delete"}, + expectCreated: false, + }, + { + name: "PostDelete (helm+argocd): One of two already exists - should be skipped", + hookType: []HookType{PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"}, + liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"}, + expectCreated: false, + }, + // MULTI HOOK TESTS - SKIP LOGIC + { + name: "Multi-hook (argocd): Target is (Pre,Post), Cluster has (Pre,Post) - should be skipped", + hookType: []HookType{PreDeleteHookType, PostDeleteHookType}, + targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete,PostDelete"}, + liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete,PostDelete"}, + expectCreated: false, + }, + { + name: "Multi-hook (helm): Target is (Pre,Post), Cluster has (Pre,Post) - should be skipped", + hookType: []HookType{PreDeleteHookType, PostDeleteHookType}, + targetAnnot: map[string]string{"helm.sh/hook": "post-delete,pre-delete"}, + liveAnnot: map[string]string{"helm.sh/hook": "post-delete,pre-delete"}, + expectCreated: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + targetObj := newObj("my-hook", tt.targetAnnot) + targetKey := kube.GetResourceKey(targetObj) + + liveObjs := make(map[kube.ResourceKey]*unstructured.Unstructured) + if tt.liveAnnot != nil { + liveObjs[targetKey] = newObj("my-hook", tt.liveAnnot) + } + + runningHooks := map[kube.ResourceKey]*unstructured.Unstructured{} + for key, obj := range liveObjs { + for _, hookType := range tt.hookType { + if isHookOfType(obj, hookType) { + runningHooks[key] = obj + } + } + } + + expectedHooksToCreate := map[kube.ResourceKey]*unstructured.Unstructured{} + targets := []*unstructured.Unstructured{targetObj} + + for _, obj := range targets { + for _, hookType := range tt.hookType { + if !isHookOfType(obj, hookType) { + continue + } + } + + objKey := kube.GetResourceKey(obj) + if _, alreadyExists := runningHooks[objKey]; !alreadyExists { + expectedHooksToCreate[objKey] = obj + } + } + + if tt.expectCreated { + assert.NotEmpty(t, expectedHooksToCreate, "Expected hook to be marked for creation") + } else { + assert.Empty(t, expectedHooksToCreate, "Expected hook to be skipped (already exists)") + } + }) + } +}