From 80070a9f7b2f9e11b924355e51c47f8b6949a2c1 Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Sun, 12 Jul 2026 16:16:14 -0400 Subject: [PATCH 1/2] fix: retrigger EG translation on TPP spec changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A TrafficProtectionPolicy mode/spec change updates the extension server's local cache promptly, but Envoy Gateway only re-runs translation — and so the WAF-injecting extension hook — when a resource it natively watches changes. A TPP is not such a resource, so the edge keeps serving the stale WAF program until some unrelated EG-watched resource incidentally forces a re-translation, which in a quiet namespace never happens. Add a TPPReconciler in internal/extensionserver/retrigger symmetric to the existing Connector Reconciler: watch TrafficProtectionPolicy and, on a spec change, touch a trigger annotation on each owning Gateway so EG re-translates against the fresh cache. Key features: - Predicate admits creates and generation bumps (any spec change: mode, targetRefs, ruleSets, sampling); ignores status/metadata churn - Per-TPP annotation key so multiple TPPs targeting one Gateway never clobber each other's value and flip-flop it on resync - Merge patch with no preceding Get: unchanged generation is a server no-op, so it is idempotent and never triggers spurious re-translation - TargetRef name maps directly to the Gateway name (Gateway targetRefs name it directly; HTTPRoute targetRefs share the HTTPProxy name, which is also the Gateway name) Wire the controller into the extension-server run command alongside the Connector arm. --- internal/extensionserver/cmd/run.go | 10 +- internal/extensionserver/retrigger/tpp.go | 149 +++++++++++++++++ .../extensionserver/retrigger/tpp_test.go | 152 ++++++++++++++++++ 3 files changed, 309 insertions(+), 2 deletions(-) create mode 100644 internal/extensionserver/retrigger/tpp.go create mode 100644 internal/extensionserver/retrigger/tpp_test.go diff --git a/internal/extensionserver/cmd/run.go b/internal/extensionserver/cmd/run.go index cac6e9f9..59a035b9 100644 --- a/internal/extensionserver/cmd/run.go +++ b/internal/extensionserver/cmd/run.go @@ -246,11 +246,17 @@ func run(o options) { os.Exit(1) } - // --- Edge re-translation controller --- + // --- Edge re-translation controllers --- // Makes Envoy Gateway re-translate when a Connector's liveness changes, so // the hook re-runs against fresh liveness. See the retrigger package doc. if err := (&retrigger.Reconciler{Client: mgr.GetClient()}).SetupWithManager(mgr); err != nil { - log.Error("set up gateway re-translation controller", "err", err) + log.Error("set up connector re-translation controller", "err", err) + os.Exit(1) + } + // Symmetric arm for TrafficProtectionPolicy: a TPP is not EG-watched, so a + // mode/spec flip lands in the cache but never re-translates on its own. + if err := (&retrigger.TPPReconciler{Client: mgr.GetClient()}).SetupWithManager(mgr); err != nil { + log.Error("set up TPP re-translation controller", "err", err) os.Exit(1) } diff --git a/internal/extensionserver/retrigger/tpp.go b/internal/extensionserver/retrigger/tpp.go new file mode 100644 index 00000000..9a6805bb --- /dev/null +++ b/internal/extensionserver/retrigger/tpp.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +// This file adds the TrafficProtectionPolicy arm of the edge re-translation +// controller. It is symmetric to the Connector Reconciler in retrigger.go: +// Envoy Gateway only re-runs translation — and so the extension hook — when a +// resource it natively watches changes. A TrafficProtectionPolicy is not such a +// resource, so a mode/spec flip lands promptly in the extension server's local +// cache but the data plane keeps serving the stale WAF program until some +// unrelated EG-watched resource incidentally forces a re-translation. +// +// This controller watches TrafficProtectionPolicies and, on a spec change (which +// is exactly what the cache reads: mode, targetRefs, ruleSets, sampling), touches +// 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. + +package retrigger + +import ( + "context" + "fmt" + "strconv" + + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// tppTriggerAnnotationPrefix is the annotation key prefix patched onto a Gateway +// to make Envoy Gateway re-translate after a TPP spec change. The owning TPP's +// name is appended so each policy owns its own annotation slot: two TPPs +// targeting the same Gateway never clobber each other's value and flip-flop it +// on every resync. +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. +type TPPReconciler struct { + Client client.Client +} + +// tppTriggerAnnotationKey returns the per-TPP trigger annotation key. +func tppTriggerAnnotationKey(tppName string) string { + return tppTriggerAnnotationPrefix + tppName +} + +// 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. +// +// 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. +func (r *TPPReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + var tpp networkingv1alpha.TrafficProtectionPolicy + if err := r.Client.Get(ctx, req.NamespacedName, &tpp); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + value := strconv.FormatInt(tpp.Generation, 10) + annotationKey := tppTriggerAnnotationKey(tpp.Name) + + seen := make(map[string]struct{}, len(tpp.Spec.TargetRefs)) + var firstErr error + for _, ref := range tpp.Spec.TargetRefs { + gwName := string(ref.Name) + if gwName == "" { + continue + } + if _, dup := seen[gwName]; dup { + continue + } + seen[gwName] = struct{}{} + + gwKey := client.ObjectKey{Namespace: tpp.Namespace, Name: gwName} + if err := r.touchGateway(ctx, gwKey, annotationKey, value); err != nil { + logger.Error(err, "failed to touch gateway for TPP spec change", + "gateway", gwKey, "tpp", tpp.Name) + if firstErr == nil { + firstErr = err + } + continue + } + logger.Info("touched gateway to trigger EG re-translation", + "gateway", gwKey, "tpp", tpp.Name, "generation", value) + } + + return ctrl.Result{}, 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. +func (r *TPPReconciler) touchGateway(ctx context.Context, key client.ObjectKey, annotationKey, value string) error { + gw := &gatewayv1.Gateway{} + gw.Namespace = key.Namespace + gw.Name = key.Name + + body := fmt.Appendf(nil, `{"metadata":{"annotations":{%q:%q}}}`, annotationKey, value) + if err := r.Client.Patch(ctx, gw, client.RawPatch(types.MergePatchType, body)); err != nil { + return client.IgnoreNotFound(err) + } + return nil +} + +// 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. +func (r *TPPReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&networkingv1alpha.TrafficProtectionPolicy{}, builder.WithPredicates(tppSpecChangedPredicate())). + Named("extension-server-gateway-retrigger-tpp"). + Complete(r) +} + +// 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. +func tppSpecChangedPredicate() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return true }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldT, ok1 := e.ObjectOld.(*networkingv1alpha.TrafficProtectionPolicy) + newT, ok2 := e.ObjectNew.(*networkingv1alpha.TrafficProtectionPolicy) + if !ok1 || !ok2 { + return true + } + return oldT.Generation != newT.Generation + }, + } +} diff --git a/internal/extensionserver/retrigger/tpp_test.go b/internal/extensionserver/retrigger/tpp_test.go new file mode 100644 index 00000000..dceda321 --- /dev/null +++ b/internal/extensionserver/retrigger/tpp_test.go @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package retrigger + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/event" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const testTPP = "tpp-1" + +func gatewayTargetRef(name string) gatewayv1alpha2.LocalPolicyTargetReferenceWithSectionName { + return gatewayv1alpha2.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: gatewayv1.Kind("Gateway"), + Name: gatewayv1.ObjectName(name), + }, + } +} + +func tpp(mode networkingv1alpha.TrafficProtectionPolicyMode, generation int64, gwNames ...string) *networkingv1alpha.TrafficProtectionPolicy { + refs := make([]gatewayv1alpha2.LocalPolicyTargetReferenceWithSectionName, 0, len(gwNames)) + for _, n := range gwNames { + refs = append(refs, gatewayTargetRef(n)) + } + return &networkingv1alpha.TrafficProtectionPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: testTPP, Namespace: testNS, Generation: generation}, + Spec: networkingv1alpha.TrafficProtectionPolicySpec{ + Mode: mode, + TargetRefs: refs, + }, + } +} + +func reconcileTPP(t *testing.T, cl client.Client) { + t.Helper() + r := &TPPReconciler{Client: cl} + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: testNS, Name: testTPP}, + }) + require.NoError(t, err) +} + +func gatewayTPPTrigger(t *testing.T, cl client.Client, gwName string) string { + t.Helper() + var gw gatewayv1.Gateway + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{Namespace: testNS, Name: gwName}, &gw)) + return gw.Annotations[tppTriggerAnnotationKey(testTPP)] +} + +// TestTPPReconcile_StampsTargetedGateway: reconciling a TPP stamps its +// generation onto the trigger annotation of the Gateway it targets, so EG +// re-translates against the fresh cache. +func TestTPPReconcile_StampsTargetedGateway(t *testing.T) { + scheme := testScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(tpp(networkingv1alpha.TrafficProtectionPolicyEnforce, 7, testProxy), gateway()). + Build() + + reconcileTPP(t, cl) + + assert.Equal(t, "7", gatewayTPPTrigger(t, cl, testProxy), + "a targeted Gateway must be stamped with the TPP generation") +} + +// TestTPPReconcile_ModeFlipChangesTrigger: a mode flip bumps the generation, so +// the trigger annotation value changes and EG re-translates. +func TestTPPReconcile_ModeFlipChangesTrigger(t *testing.T) { + scheme := testScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(tpp(networkingv1alpha.TrafficProtectionPolicyObserve, 3, testProxy), gateway()). + Build() + + reconcileTPP(t, cl) + assert.Equal(t, "3", gatewayTPPTrigger(t, cl, testProxy)) + + var current networkingv1alpha.TrafficProtectionPolicy + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{Namespace: testNS, Name: testTPP}, ¤t)) + current.Spec.Mode = networkingv1alpha.TrafficProtectionPolicyEnforce + current.Generation = 4 + require.NoError(t, cl.Update(context.Background(), ¤t)) + reconcileTPP(t, cl) + + assert.Equal(t, "4", gatewayTPPTrigger(t, cl, testProxy), + "a spec change must bump the trigger annotation so EG re-translates") +} + +// TestTPPReconcile_NoGateway_NoError: a missing Gateway is ignored — EG reads +// the fresh cache when it later creates the Gateway. +func TestTPPReconcile_NoGateway_NoError(t *testing.T) { + scheme := testScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(tpp(networkingv1alpha.TrafficProtectionPolicyEnforce, 1, testProxy)). + Build() + + reconcileTPP(t, cl) // require.NoError inside +} + +// TestTPPReconcile_MultipleTargets stamps every targeted Gateway. +func TestTPPReconcile_MultipleTargets(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() + + reconcileTPP(t, cl) + + assert.Equal(t, "2", gatewayTPPTrigger(t, cl, testProxy)) + assert.Equal(t, "2", gatewayTPPTrigger(t, cl, "proxy-2")) +} + +// TestTPPReconcile_PerTPPKey: distinct TPPs targeting the same Gateway use +// distinct annotation keys and do not clobber each other. +func TestTPPReconcile_PerTPPKey(t *testing.T) { + assert.NotEqual(t, tppTriggerAnnotationKey("a"), tppTriggerAnnotationKey("b"), + "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. +func TestTPPSpecChangedPredicate(t *testing.T) { + p := tppSpecChangedPredicate() + + observe := tpp(networkingv1alpha.TrafficProtectionPolicyObserve, 1, testProxy) + enforce := tpp(networkingv1alpha.TrafficProtectionPolicyEnforce, 2, testProxy) + + 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.Update(event.UpdateEvent{ObjectOld: observe, ObjectNew: enforce}), + "a generation bump (spec change) must reconcile") + + noSpecChange := observe.DeepCopy() + noSpecChange.ResourceVersion = "999" + assert.False(t, p.Update(event.UpdateEvent{ObjectOld: observe, ObjectNew: noSpecChange}), + "a status/metadata-only update (same generation) must NOT reconcile") +} From f7f42cd96c043237a14542ba0833d7ca5bfa1a8a Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Sun, 12 Jul 2026 16:52:28 -0400 Subject: [PATCH 2/2] 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 --- internal/extensionserver/retrigger/tpp.go | 161 +++++++++++++++--- .../extensionserver/retrigger/tpp_test.go | 67 +++++++- 2 files changed, 203 insertions(+), 25 deletions(-) diff --git a/internal/extensionserver/retrigger/tpp.go b/internal/extensionserver/retrigger/tpp.go index 9a6805bb..18150e10 100644 --- a/internal/extensionserver/retrigger/tpp.go +++ b/internal/extensionserver/retrigger/tpp.go @@ -13,6 +13,16 @@ // 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 @@ -20,7 +30,9 @@ 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) diff --git a/internal/extensionserver/retrigger/tpp_test.go b/internal/extensionserver/retrigger/tpp_test.go index dceda321..71a09d3f 100644 --- a/internal/extensionserver/retrigger/tpp_test.go +++ b/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}, ¤t)) + require.NoError(t, cl.Delete(context.Background(), ¤t)) + 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}, ¤t)) + current.Spec.TargetRefs = []gatewayv1alpha2.LocalPolicyTargetReferenceWithSectionName{gatewayTargetRef(testProxy)} + current.Generation = 3 + require.NoError(t, cl.Update(context.Background(), ¤t)) + 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")