fix: handle TPP delete and targetRef removal in retrigger

Complete the TrafficProtectionPolicy arm of the edge re-translation
controller so disabling protection reaches the data plane. The spec-change
arm only touched current targets, so two cases left stale WAF on the edge:
a deleted TPP whose Enforce program kept blocking, and a Gateway dropped
from spec.targetRefs that kept the old config.

Root cause: standard Reconcile receives only the object key on delete, not
the last-known object, so there are no targetRefs to resolve owning
Gateways from; and a targetRef edit only exposes the current targets.

Key changes:
- Record each TPP's owning Gateway set in an in-memory map keyed by
  namespaced name, populated by the create event every live TPP fires on
  startup so a restart re-learns the set before any later edit or delete.
- On delete, clear the trigger annotation on every previously-owning
  Gateway; on a targetRef edit, clear the Gateways that dropped out of the
  set. Removing the annotation is itself a Gateway change, so EG
  re-translates against the now-empty cache and drops the orphaned WAF.
- Admit delete events in the predicate (previously dropped).
- On a failed touch of a current target, retain the previous set so dropped
  Gateways are retried on requeue rather than forgotten.
- Tests for deletion, targetRef removal, and the delete predicate.

Closes #256

Author: ecv

internal/extensionserver/retrigger/tpp.go

@@ -13,14 +13,26 @@
 // a trigger annotation on each owning Gateway. Because the touch happens after
 // the new spec is already in the shared cache, EG re-translates against fresh
 // data and the mode change reaches the edge in seconds.
+//
+// Deletion and targetRef removal are the mirror image: when a TPP is deleted or
+// a Gateway is dropped from spec.targetRefs, the cache no longer programs any WAF
+// for that Gateway, but nothing forces EG to re-translate — so the edge keeps
+// serving the last (possibly Enforce, still-blocking) program. Standard
+// Reconcile receives only the object key on delete, not the last-known object,
+// so the controller records the owning Gateway set of each TPP in memory and, on
+// delete or a targetRef edit, clears the trigger annotation on every Gateway that
+// dropped out of the set. Removing the annotation is itself a Gateway change, so
+// EG re-translates against the now-empty cache and drops the orphaned WAF config.
 
 package retrigger
 
 import (
 	"context"
 	"fmt"
 	"strconv"
+	"sync"
 
+	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -42,9 +54,23 @@ const tppTriggerAnnotationPrefix = "networking.datumapis.com/tpp-generation-"
 
 // TPPReconciler touches the trigger annotation on every Gateway a
 // TrafficProtectionPolicy targets whenever the policy's spec changes, forcing EG
-// to re-translate and re-run the extension hook against the fresh cache.
+// to re-translate and re-run the extension hook against the fresh cache. On
+// delete or targetRef removal it clears the annotation on Gateways that dropped
+// out of the policy's target set so EG re-translates and drops the orphaned WAF.
 type TPPReconciler struct {
 	Client client.Client
+
+	// mu guards lastTargets. Reconciles for distinct TPPs run concurrently, so
+	// the previous-target map needs its own lock (controller-runtime only
+	// serialises reconciles that share a key).
+	mu sync.Mutex
+	// lastTargets records the owning Gateway set last stamped for each TPP,
+	// keyed by namespaced name. It is the source for "which Gateways did this
+	// TPP used to govern?" on a delete (where the object is gone) or a targetRef
+	// edit (where only the current targets are otherwise visible). Populated by
+	// the create event every live TPP fires on startup, so a restart re-learns
+	// the set before any subsequent edit or delete.
+	lastTargets map[types.NamespacedName][]string
 }
 
 // tppTriggerAnnotationKey returns the per-TPP trigger annotation key.
@@ -53,28 +79,36 @@ func tppTriggerAnnotationKey(tppName string) string {
 }
 
 // Reconcile stamps the TPP's observed generation onto the trigger annotation of
-// every Gateway it targets. A TPP, the Gateways it targets, and the HTTPProxies
-// that generate them share a namespace, and the Gateway is named after the
-// targetRef (Gateway targetRefs name the Gateway directly; HTTPRoute targetRefs
-// share the HTTPProxy name, which is also the Gateway name), so the
-// TPP→Gateway mapping is local and name-derived.
+// every Gateway it targets, and clears the annotation on every Gateway that
+// dropped out of its target set since the last reconcile (a removed targetRef,
+// or — when the TPP itself is gone — all of them). A TPP, the Gateways it
+// targets, and the HTTPProxies that generate them share a namespace, and the
+// Gateway is named after the targetRef (Gateway targetRefs name the Gateway
+// directly; HTTPRoute targetRefs share the HTTPProxy name, which is also the
+// Gateway name), so the TPP→Gateway mapping is local and name-derived.
 //
-// The Gateway patch is a merge patch with no preceding Get: if the generation is
-// unchanged the API server treats it as a no-op (no resourceVersion bump, no EG
-// event), so it is naturally idempotent and never triggers a spurious
-// re-translation.
+// Each Gateway patch is a merge patch with no preceding Get: an unchanged
+// generation (or clearing an already-absent annotation) is a server-side no-op
+// (no resourceVersion bump, no EG event), so it is naturally idempotent and
+// never triggers a spurious re-translation.
 func (r *TPPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
+	annotationKey := tppTriggerAnnotationKey(req.Name)
 
 	var tpp networkingv1alpha.TrafficProtectionPolicy
 	if err := r.Client.Get(ctx, req.NamespacedName, &tpp); err != nil {
-		return ctrl.Result{}, client.IgnoreNotFound(err)
+		if client.IgnoreNotFound(err) != nil {
+			return ctrl.Result{}, err
+		}
+		// Deleted: nudge every Gateway the policy used to govern so EG
+		// re-translates against the now-empty cache and drops the orphaned WAF.
+		return ctrl.Result{}, r.clearGateways(ctx, logger, req.NamespacedName, annotationKey, r.takeTargets(req.NamespacedName), "delete")
 	}
 
 	value := strconv.FormatInt(tpp.Generation, 10)
-	annotationKey := tppTriggerAnnotationKey(tpp.Name)
 
 	seen := make(map[string]struct{}, len(tpp.Spec.TargetRefs))
+	current := make([]string, 0, len(tpp.Spec.TargetRefs))
 	var firstErr error
 	for _, ref := range tpp.Spec.TargetRefs {
 		gwName := string(ref.Name)
@@ -85,6 +119,7 @@ func (r *TPPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 			continue
 		}
 		seen[gwName] = struct{}{}
+		current = append(current, gwName)
 
 		gwKey := client.ObjectKey{Namespace: tpp.Namespace, Name: gwName}
 		if err := r.touchGateway(ctx, gwKey, annotationKey, value); err != nil {
@@ -99,9 +134,37 @@ func (r *TPPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 			"gateway", gwKey, "tpp", tpp.Name, "generation", value)
 	}
 
+	// Clear Gateways that were targeted last time but are not now. On any error
+	// touching a current target, keep the previous set so the dropped Gateways
+	// are retried on requeue rather than forgotten.
+	prev := r.swapTargets(req.NamespacedName, current, firstErr == nil)
+	if err := r.clearGateways(ctx, logger, req.NamespacedName, annotationKey, removedTargets(prev, seen), "targetRef removal"); err != nil && firstErr == nil {
+		firstErr = err
+	}
+
 	return ctrl.Result{}, firstErr
 }
 
+// clearGateways removes the trigger annotation from each named Gateway in the
+// policy's namespace, returning the first error encountered.
+func (r *TPPReconciler) clearGateways(ctx context.Context, logger logr.Logger, tpp types.NamespacedName, annotationKey string, gwNames []string, reason string) error {
+	var firstErr error
+	for _, gwName := range gwNames {
+		gwKey := client.ObjectKey{Namespace: tpp.Namespace, Name: gwName}
+		if err := r.clearGateway(ctx, gwKey, annotationKey); err != nil {
+			logger.Error(err, "failed to clear gateway trigger for orphaned TPP target",
+				"gateway", gwKey, "tpp", tpp.Name, "reason", reason)
+			if firstErr == nil {
+				firstErr = err
+			}
+			continue
+		}
+		logger.Info("cleared gateway trigger to drop orphaned WAF config",
+			"gateway", gwKey, "tpp", tpp.Name, "reason", reason)
+	}
+	return firstErr
+}
+
 // touchGateway applies a merge patch setting the trigger annotation. A
 // non-existent Gateway is ignored: EG translates a Gateway when it is created,
 // reading the (already fresh) extension cache, so there is nothing to nudge yet.
@@ -117,9 +180,68 @@ func (r *TPPReconciler) touchGateway(ctx context.Context, key client.ObjectKey,
 	return nil
 }
 
+// clearGateway removes the trigger annotation via a merge patch (JSON null
+// deletes the key). Removing the annotation is a Gateway change, so EG
+// re-translates; clearing an already-absent key is a server-side no-op.
+func (r *TPPReconciler) clearGateway(ctx context.Context, key client.ObjectKey, annotationKey string) error {
+	gw := &gatewayv1.Gateway{}
+	gw.Namespace = key.Namespace
+	gw.Name = key.Name
+
+	body := fmt.Appendf(nil, `{"metadata":{"annotations":{%q:null}}}`, annotationKey)
+	if err := r.Client.Patch(ctx, gw, client.RawPatch(types.MergePatchType, body)); err != nil {
+		return client.IgnoreNotFound(err)
+	}
+	return nil
+}
+
+// swapTargets records the TPP's current owning Gateway set and returns the set
+// from the previous reconcile. When commit is false the previous set is left in
+// place (a touch failed, so the dropped Gateways must stay pending for retry)
+// but still returned for diffing.
+func (r *TPPReconciler) swapTargets(key types.NamespacedName, current []string, commit bool) []string {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	prev := r.lastTargets[key]
+	if !commit {
+		return prev
+	}
+	if r.lastTargets == nil {
+		r.lastTargets = make(map[types.NamespacedName][]string)
+	}
+	if len(current) == 0 {
+		delete(r.lastTargets, key)
+	} else {
+		r.lastTargets[key] = current
+	}
+	return prev
+}
+
+// takeTargets returns and forgets the TPP's recorded owning Gateway set (used on
+// delete, where there is no current set to keep).
+func (r *TPPReconciler) takeTargets(key types.NamespacedName) []string {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	prev := r.lastTargets[key]
+	delete(r.lastTargets, key)
+	return prev
+}
+
+// removedTargets returns the members of prev not present in current.
+func removedTargets(prev []string, current map[string]struct{}) []string {
+	var removed []string
+	for _, name := range prev {
+		if _, ok := current[name]; !ok {
+			removed = append(removed, name)
+		}
+	}
+	return removed
+}
+
 // SetupWithManager registers the controller. It reconciles a TPP only when its
-// spec changes (generation bump) — the extension server reads spec fields only
-// (mode, targetRefs, ruleSets, sampling), so status/metadata churn is ignored.
+// spec changes (generation bump) or it is deleted — the extension server reads
+// spec fields only (mode, targetRefs, ruleSets, sampling), so status/metadata
+// churn is ignored.
 func (r *TPPReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&networkingv1alpha.TrafficProtectionPolicy{}, builder.WithPredicates(tppSpecChangedPredicate())).
@@ -128,15 +250,14 @@ func (r *TPPReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 // tppSpecChangedPredicate admits creates (so TPPs already present when the
-// controller starts stamp their Gateways once) and updates that bump the
-// generation (any spec change: mode flip, targetRef edit, ruleset/sampling
-// change). Deletes are not handled here: a deleted TPP leaves no object to read
-// targetRefs from, and its Gateways are not re-translated by this arm — see the
-// known-limitation note in the PR.
+// controller starts stamp their Gateways, and their target set is learned before
+// any later edit or delete), updates that bump the generation (any spec change:
+// mode flip, targetRef edit, ruleset/sampling change), and deletes (so a removed
+// policy's Gateways are re-translated against the now-empty cache).
 func tppSpecChangedPredicate() predicate.Predicate {
 	return predicate.Funcs{
 		CreateFunc: func(event.CreateEvent) bool { return true },
-		DeleteFunc: func(event.DeleteEvent) bool { return false },
+		DeleteFunc: func(event.DeleteEvent) bool { return true },
 		UpdateFunc: func(e event.UpdateEvent) bool {
 			oldT, ok1 := e.ObjectOld.(*networkingv1alpha.TrafficProtectionPolicy)
 			newT, ok2 := e.ObjectNew.(*networkingv1alpha.TrafficProtectionPolicy)

internal/extensionserver/retrigger/tpp_test.go

@@ -47,7 +47,14 @@ func tpp(mode networkingv1alpha.TrafficProtectionPolicyMode, generation int64, g
 
 func reconcileTPP(t *testing.T, cl client.Client) {
 	t.Helper()
-	r := &TPPReconciler{Client: cl}
+	reconcileTPPWith(t, &TPPReconciler{Client: cl})
+}
+
+// reconcileTPPWith reconciles through a caller-owned reconciler so its in-memory
+// previous-target map survives across reconciles (needed to exercise delete and
+// targetRef removal).
+func reconcileTPPWith(t *testing.T, r *TPPReconciler) {
+	t.Helper()
 	_, err := r.Reconcile(context.Background(), ctrl.Request{
 		NamespacedName: client.ObjectKey{Namespace: testNS, Name: testTPP},
 	})
@@ -130,8 +137,58 @@ func TestTPPReconcile_PerTPPKey(t *testing.T) {
 		"each TPP must own a distinct annotation slot to avoid flip-flop churn")
 }
 
-// TestTPPSpecChangedPredicate verifies the controller reconciles on creates and
-// generation bumps, and ignores status/metadata churn and deletes.
+// TestTPPReconcile_DeletionClearsGateways: after a TPP is stamped onto its
+// Gateway, deleting the TPP clears the trigger annotation so EG re-translates
+// against the now-empty cache and drops the orphaned WAF program.
+func TestTPPReconcile_DeletionClearsGateways(t *testing.T) {
+	scheme := testScheme(t)
+	cl := fake.NewClientBuilder().WithScheme(scheme).
+		WithObjects(tpp(networkingv1alpha.TrafficProtectionPolicyEnforce, 5, testProxy), gateway()).
+		Build()
+	r := &TPPReconciler{Client: cl}
+
+	reconcileTPPWith(t, r)
+	require.Equal(t, "5", gatewayTPPTrigger(t, cl, testProxy))
+
+	var current networkingv1alpha.TrafficProtectionPolicy
+	require.NoError(t, cl.Get(context.Background(), client.ObjectKey{Namespace: testNS, Name: testTPP}, &current))
+	require.NoError(t, cl.Delete(context.Background(), &current))
+	reconcileTPPWith(t, r)
+
+	assert.Empty(t, gatewayTPPTrigger(t, cl, testProxy),
+		"deleting a TPP must clear its trigger annotation so EG drops the orphaned WAF")
+}
+
+// TestTPPReconcile_TargetRefRemovalClearsDropped: dropping a Gateway from
+// spec.targetRefs clears that Gateway's trigger annotation while the retained
+// target is re-stamped with the new generation.
+func TestTPPReconcile_TargetRefRemovalClearsDropped(t *testing.T) {
+	scheme := testScheme(t)
+	gwB := &gatewayv1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "proxy-2", Namespace: testNS}}
+	cl := fake.NewClientBuilder().WithScheme(scheme).
+		WithObjects(tpp(networkingv1alpha.TrafficProtectionPolicyEnforce, 2, testProxy, "proxy-2"), gateway(), gwB).
+		Build()
+	r := &TPPReconciler{Client: cl}
+
+	reconcileTPPWith(t, r)
+	require.Equal(t, "2", gatewayTPPTrigger(t, cl, testProxy))
+	require.Equal(t, "2", gatewayTPPTrigger(t, cl, "proxy-2"))
+
+	var current networkingv1alpha.TrafficProtectionPolicy
+	require.NoError(t, cl.Get(context.Background(), client.ObjectKey{Namespace: testNS, Name: testTPP}, &current))
+	current.Spec.TargetRefs = []gatewayv1alpha2.LocalPolicyTargetReferenceWithSectionName{gatewayTargetRef(testProxy)}
+	current.Generation = 3
+	require.NoError(t, cl.Update(context.Background(), &current))
+	reconcileTPPWith(t, r)
+
+	assert.Equal(t, "3", gatewayTPPTrigger(t, cl, testProxy),
+		"a retained target must be re-stamped with the new generation")
+	assert.Empty(t, gatewayTPPTrigger(t, cl, "proxy-2"),
+		"a dropped target must have its trigger annotation cleared")
+}
+
+// TestTPPSpecChangedPredicate verifies the controller reconciles on creates,
+// generation bumps, and deletes, and ignores status/metadata churn.
 func TestTPPSpecChangedPredicate(t *testing.T) {
 	p := tppSpecChangedPredicate()
 
@@ -140,8 +197,8 @@ func TestTPPSpecChangedPredicate(t *testing.T) {
 
 	assert.True(t, p.Create(event.CreateEvent{Object: observe}),
 		"create is always admitted so existing TPPs stamp their Gateway on startup")
-	assert.False(t, p.Delete(event.DeleteEvent{Object: observe}),
-		"delete is not handled by this arm")
+	assert.True(t, p.Delete(event.DeleteEvent{Object: observe}),
+		"delete is admitted so a removed TPP's Gateways are re-translated")
 	assert.True(t, p.Update(event.UpdateEvent{ObjectOld: observe, ObjectNew: enforce}),
 		"a generation bump (spec change) must reconcile")