fix(server): Avoid error when attempting a second delete operation (cherry-pick #27495 for 3.3) (#27501)

Signed-off-by: Taylor Thomas <[email protected]>
Co-authored-by: Taylor Thomas <[email protected]>

server/application/application.go

@@ -1166,7 +1166,10 @@ func (s *Server) Delete(ctx context.Context, q *application.ApplicationDeleteReq
 		if policyFinalizer == "" {
 			return nil, status.Errorf(codes.InvalidArgument, "invalid propagation policy: %s", *q.PropagationPolicy)
 		}
-		if !a.IsFinalizerPresent(policyFinalizer) {
+		// Kubernetes forbids adding finalizers to an object that is already being deleted,
+		// so skip the patch if the app is mid-deletion. The underlying Delete call below is
+		// still issued to keep the RPC idempotent for callers retrying a cascade delete.
+		if !a.IsFinalizerPresent(policyFinalizer) && a.DeletionTimestamp == nil {
 			a.SetCascadedDeletion(policyFinalizer)
 			patchFinalizer = true
 		}

server/application/application_test.go

@@ -1869,6 +1869,46 @@ func TestDeleteApp(t *testing.T) {
 	})
 }
 
+func TestDeleteAppAlreadyDeleting(t *testing.T) {
+	ctx := t.Context()
+	appServer := newTestAppServer(t)
+	createReq := application.ApplicationCreateRequest{
+		Application: newTestApp(),
+	}
+	app, err := appServer.Create(ctx, &createReq)
+	require.NoError(t, err)
+
+	fakeAppCs := appServer.appclientset.(*deepCopyAppClientset).GetUnderlyingClientSet().(*apps.Clientset)
+	// Drop the default reactor so we can observe exactly which actions Delete issues.
+	fakeAppCs.ReactionChain = nil
+	patched := false
+	deleted := false
+	fakeAppCs.AddReactor("patch", "applications", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) {
+		patched = true
+		return true, nil, nil
+	})
+	fakeAppCs.AddReactor("delete", "applications", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) {
+		deleted = true
+		return true, nil, nil
+	})
+	now := metav1.Now()
+	fakeAppCs.AddReactor("get", "applications", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) {
+		return true, &v1alpha1.Application{
+			ObjectMeta: metav1.ObjectMeta{
+				DeletionTimestamp: &now,
+			},
+			Spec: v1alpha1.ApplicationSpec{Source: &v1alpha1.ApplicationSource{}},
+		}, nil
+	})
+	appServer.appclientset = fakeAppCs
+
+	trueVar := true
+	_, err = appServer.Delete(ctx, &application.ApplicationDeleteRequest{Name: &app.Name, Cascade: &trueVar})
+	require.NoError(t, err)
+	assert.False(t, patched, "finalizer patch must not be attempted on an app already being deleted")
+	assert.True(t, deleted, "delete call should still be issued so the RPC stays idempotent")
+}
+
 func TestDeleteResourcesRBAC(t *testing.T) {
 	ctx := t.Context()
 	//nolint:staticcheck