feat: Prune resources in reverse order of syncwave during sync (#15074)

docs/user-guide/sync-waves.md

@@ -37,7 +37,7 @@ Hooks and resources are assigned to wave zero by default. The wave can be negati
 When Argo CD starts a sync, it orders the resources in the following precedence:
 
 * The phase
-* The wave they are in (lower values first)
+* The wave they are in (lower values first for creation & updation and higher values first for deletion)
 * By kind (e.g. [namespaces first and then other Kubernetes resources, followed by custom resources](https://github.com/argoproj/gitops-engine/blob/bc9ce5764fa306f58cf59199a94f6c968c775a2d/pkg/sync/sync_tasks.go#L27-L66))
 * By name 
 
@@ -49,6 +49,8 @@ It repeats this process until all phases and waves are in-sync and healthy.
 
 Because an application can have resources that are unhealthy in the first wave, it may be that the app can never get to healthy.
 
+During pruning of resources, resources from higher waves are processed first before moving to lower waves. If, for any reason, a resource isn't removed/pruned in a wave, the resources in next waves won't be processed. This is to ensure proper resource cleanup between waves.
+
 Note that there's currently a delay between each sync wave in order give other controllers a chance to react to the spec change
 that we just applied. This also prevent Argo CD from assessing resource health too quickly (against the stale object), causing
 hooks to fire prematurely. The current delay between each sync wave is 2 seconds and can be configured via environment

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/TomOnTime/utfutil v0.0.0-20180511104225-09c41003ee1d
 	github.com/alicebob/miniredis/v2 v2.30.4
 	github.com/antonmedv/expr v1.15.2
-	github.com/argoproj/gitops-engine v0.7.1-0.20240122213038-792124280fcc
+	github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757
 	github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9
 	github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1
 	github.com/aws/aws-sdk-go v1.44.317

go.sum

@@ -694,8 +694,8 @@ github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb
 github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
 github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU=
 github.com/appscode/go v0.0.0-20191119085241-0887d8ec2ecc/go.mod h1:OawnOmAL4ZX3YaPdN+8HTNwBveT1jMsqP74moa9XUbE=
-github.com/argoproj/gitops-engine v0.7.1-0.20240122213038-792124280fcc h1:Fv94Mi2WvtvPkEH5WoWC3iy/VoQRLeSsE0hyg0n2UkY=
-github.com/argoproj/gitops-engine v0.7.1-0.20240122213038-792124280fcc/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg=
+github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757 h1:5fKAhTQcTBom0vin56cz/UTPx2GMuvdb+lJRAUOPbHA=
+github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg=
 github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9 h1:1lt0VXzmLK7Vv0kaeal3S6/JIfzPyBORkUWXhiqF3l0=
 github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9/go.mod h1:E/vv4+by868m0mmflaRfGBmKBtAupoF+mmyfekP8QCk=
 github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo=

test/e2e/sync_waves_test.go

@@ -9,6 +9,8 @@ import (
 
 	"github.com/argoproj/gitops-engine/pkg/health"
 	. "github.com/argoproj/gitops-engine/pkg/sync/common"
+
+	v1 "k8s.io/api/core/v1"
 )
 
 func TestFixingDegradedApp(t *testing.T) {
@@ -100,3 +102,46 @@ func TestDegradedDeploymentIsSucceededAndSynced(t *testing.T) {
 		Expect(SyncStatusIs(SyncStatusCodeSynced)).
 		Expect(ResourceResultNumbering(1))
 }
+
+// resources should be pruned in reverse of creation order(syncwaves order)
+func TestSyncPruneOrderWithSyncWaves(t *testing.T) {
+	ctx := Given(t).Timeout(60)
+
+	// remove finalizer to ensure proper cleanup if test fails at early stage
+	defer func() {
+		_, _ = RunCli("app", "patch-resource", ctx.AppQualifiedName(),
+			"--kind", "Pod",
+			"--resource-name", "pod-with-finalizers",
+			"--patch", `[{"op": "remove", "path": "/metadata/finalizers"}]`,
+			"--patch-type", "application/json-patch+json", "--all",
+		)
+	}()
+
+	ctx.Path("syncwaves-prune-order").
+		When().
+		CreateApp().
+		// creation order: sa & role -> rolebinding -> pod
+		Sync().
+		Wait().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(HealthIs(health.HealthStatusHealthy)).
+		When().
+		// delete files to remove resources
+		DeleteFile("pod.yaml").
+		DeleteFile("rbac.yaml").
+		Refresh(RefreshTypeHard).
+		IgnoreErrors().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
+		When().
+		// prune order: pod -> rolebinding -> sa & role
+		Sync("--prune").
+		Wait().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(HealthIs(health.HealthStatusHealthy)).
+		Expect(NotPod(func(p v1.Pod) bool { return p.Name == "pod-with-finalizers" })).
+		Expect(ResourceResultNumbering(4))
+}

test/e2e/testdata/syncwaves-prune-order/README.md

@@ -0,0 +1,15 @@
+## Test Scenario
+
+This test example is for testing the reverse pruning of resources with syncwaves during sync operation.
+
+Resource creation happens in below order
+- wave 0: sa & role
+- wave 1: rolebinding
+- wave 2: pod
+
+They are setup in such a way that the resources will be cleaned up properly only if they are deleted in the reverse order of creation i.e
+- wave 0: pod
+- wave 1: rolebinding
+- wave 2: sa & role
+
+If above delete order is not followed the pod gets stuck in terminating state due to a finalizer which is supposed to be removed by k8s container lifecycle hook on delete if delete order is correct.

test/e2e/testdata/syncwaves-prune-order/pod.yaml

@@ -0,0 +1,41 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-with-finalizers
+  annotations:
+    argocd.argoproj.io/sync-wave: "2"
+  # remove this finalizers using container preStop lifecycle hook on delete
+  finalizers:
+  - example.com/block-delete
+spec:
+  serviceAccountName: modify-pods-sa  # sa with permissions to modify pods
+  terminationGracePeriodSeconds: 15
+  containers:
+  - name: container
+    image: nginx:alpine
+    command: ["/bin/sh", "-c"]
+    args: ["sleep 10h"]
+    env:
+      - name: POD_NAME
+        valueFrom:
+          fieldRef:
+            fieldPath: metadata.name
+      - name: NAMESPACE
+        valueFrom:
+          fieldRef:
+            fieldPath: metadata.namespace
+    lifecycle:
+      # remove finalizers for successful delete of pod
+      preStop:
+        exec:
+          command: 
+          - /bin/sh
+          - -c
+          - |
+            set -e
+
+            SERVICE_ACCOUNT_TOKEN=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)
+            POD_URL="https://kubernetes.default.svc/api/v1/namespaces/$NAMESPACE/pods/$POD_NAME"
+            PATCH_PAYLOAD='[{"op": "remove", "path": "/metadata/finalizers"}]'
+
+            curl -k -v -H "Authorization: Bearer $SERVICE_ACCOUNT_TOKEN" -H "Content-Type: application/json-patch+json" -X PATCH --data "$PATCH_PAYLOAD" $POD_URL

test/e2e/testdata/syncwaves-prune-order/rbac.yaml

@@ -0,0 +1,37 @@
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: modify-pods-sa
+  annotations:
+    argocd.argoproj.io/sync-wave: "0"
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: modify-pods-role
+  annotations:
+    argocd.argoproj.io/sync-wave: "0"
+rules:
+  - apiGroups: [""]
+    resources:
+      - pods
+    verbs:
+      - get
+      - list
+      - delete
+      - update
+      - patch
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: modify-pods-rolebinding
+  annotations:
+    argocd.argoproj.io/sync-wave: "1"
+subjects:
+  - kind: ServiceAccount
+    name: modify-pods-sa
+roleRef:
+  kind: Role
+  name: modify-pods-role
+  apiGroup: rbac.authorization.k8s.io