fix: Withdraw a load balancer rule's route after the rule is deleted

cmd/galactic-gateway/root.go

@@ -165,21 +165,23 @@ func runCmd(cfg *config.GatewayConfig) error {
 	// rewrites nothing, so a gateway node has no translation source of its own
 	// to publish.
 	if err := (&controller.NetworkGatewayReconciler{
-		Client:   mgr.GetClient(),
-		Scheme:   mgr.GetScheme(),
-		Engine:   gwEngine,
-		NodeName: nodeName,
-		Disabled: !cfg.DatapathEnabled,
+		Client:    mgr.GetClient(),
+		Scheme:    mgr.GetScheme(),
+		Engine:    gwEngine,
+		NodeName:  nodeName,
+		APIReader: mgr.GetAPIReader(),
+		Disabled:  !cfg.DatapathEnabled,
 	}).SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("setup NetworkGateway controller: %w", err)
 	}
 
 	// The rule reconciler: finalizer-guarded teardown ordering and the Accepted
 	// condition.
 	if err := (&controller.NetworkRuleReconciler{
-		Client:   mgr.GetClient(),
-		Scheme:   mgr.GetScheme(),
-		NodeName: nodeName,
+		Client:    mgr.GetClient(),
+		Scheme:    mgr.GetScheme(),
+		NodeName:  nodeName,
+		APIReader: mgr.GetAPIReader(),
 	}).SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("setup NetworkRule controller: %w", err)
 	}

docs/agents/ARCHITECTURE-GATEWAY.md

@@ -255,9 +255,12 @@ that converges this node's whole gateway engine:
    advertisement carries a `networkRuleLabel` label and a plain (not
    controller, no `blockOwnerDeletion`) owner reference to its
    `NetworkRule`, both backfilled on existing objects, and nothing is
-   created or updated for a rule that is being deleted. The owner reference
-   lets Kubernetes garbage collection delete an advertisement a stale-cache
-   pass created after teardown listed the rule's advertisements.
+   created or updated for a rule that is being deleted. A pass whose cache
+   has not yet seen that deletion can still create one after teardown has
+   finished. Each pass therefore withdraws this node's advertisements for
+   rules missing from its list once an uncached read confirms the rule is
+   gone (`sweepOrphanedAdvertisements`, #763). The owner reference lets
+   Kubernetes garbage collection delete any that remain.
 3. **Crash recovery.** Runs `Engine.ReconcileOrphans` (see
    [Crash recovery](#crash-recovery) below).
 
@@ -267,7 +270,9 @@ lifecycle pieces the aggregate pass above is the wrong place for:
 once gateway nodes exist for the namespace) and the
 finalizer-guarded teardown ordering on deletion (`reconcileDelete`: delete
 every `BGPAdvertisement` labelled with the rule's name, on any node,
-*before* releasing quota/reservation state, then remove the finalizer).
+*before* releasing quota/reservation state, then list them again through
+the manager's uncached API reader and, if another node created one the
+cache missed, delete it and requeue instead of removing the finalizer).
 Each gateway node removes the rule's `vip_table` rows only after those
 deletes and its drain delay (step 1 above), which is what the
 `Datapath.RemoveRule` contract requires: withdraw the route first, or

internal/controller/networkgateway_controller.go

@@ -87,6 +87,11 @@ type NetworkGatewayReconciler struct {
 
 	NodeName string
 
+	// APIReader reads from the API server, bypassing the informer cache.
+	// sweepOrphanedAdvertisements uses it to confirm a rule is really gone
+	// before withdrawing this node's route for it. Required.
+	APIReader client.Reader
+
 	// Disabled is set when this node's datapath is turned off by
 	// configuration. The reconciler then keeps every one of this node's VIP
 	// advertisements withdrawn and reports the node not ready, so the fabric
@@ -308,15 +313,21 @@ func (r *NetworkGatewayReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		logger.Error(updateErr, "update NetworkGateway status")
 	}
 
+	sweepErr := r.sweepOrphanedAdvertisements(ctx, gw.Namespace, ruleList.Items)
+	if sweepErr != nil {
+		logger.Error(sweepErr, "withdraw advertisements for deleted NetworkRules")
+	}
+
 	// Crash recovery. A failed sweep leaves orphaned vip_table state behind
 	// until a later pass succeeds, so it is returned for retry, after the
 	// status write above so the failure stays visible on the object.
 	if err := r.Engine.ReconcileOrphans(ctx, desired, cutoff); err != nil {
 		logger.Error(err, "reconcile orphaned vip_table state")
-		return ctrl.Result{}, errors.Join(advErr, ruleStatusErr, fmt.Errorf("reconcile orphaned vip_table state: %w", err))
+		return ctrl.Result{}, errors.Join(advErr, ruleStatusErr, sweepErr,
+			fmt.Errorf("reconcile orphaned vip_table state: %w", err))
 	}
 
-	if err := errors.Join(advErr, ruleStatusErr); err != nil {
+	if err := errors.Join(advErr, ruleStatusErr, sweepErr); err != nil {
 		return ctrl.Result{}, err
 	}
 	// Nothing else is sure to trigger the pass that drops a held rule once
@@ -1105,6 +1116,64 @@ func isNodeAdvertisement(adv *bgpv1alpha1.BGPAdvertisement, nodeName string) boo
 	return false
 }
 
+// sweepOrphanedAdvertisements deletes this node's BGPAdvertisements for rules
+// that no longer exist. rules is every NetworkRule in namespace as this pass
+// listed them.
+//
+// This node can create an advertisement from a cache that has not yet seen its
+// rule's deletion timestamp, after NetworkRuleReconciler's teardown has already
+// removed the finalizer. Teardown never sees that advertisement. Without this
+// sweep it would stay advertised, with no node serving the rule, until
+// Kubernetes garbage collection follows its owner reference. Here it is
+// withdrawn on the pass where this node's cache drops the rule, the same pass
+// that unloads the rule from the datapath.
+//
+// A rule missing from rules is looked up through APIReader first and kept if it
+// still exists, so a cache that has not yet seen a new rule never withdraws a
+// live route.
+func (r *NetworkGatewayReconciler) sweepOrphanedAdvertisements(
+	ctx context.Context, namespace string, rules []bgpv1alpha1.NetworkRule,
+) error {
+	known := make(map[string]bool, len(rules))
+	for i := range rules {
+		known[rules[i].Name] = true
+	}
+
+	advList := &bgpv1alpha1.BGPAdvertisementList{}
+	if err := r.List(ctx, advList,
+		client.InNamespace(namespace),
+		client.MatchingLabels{gatewayNodeLabel: gatewayNodeLabelValue(r.NodeName)},
+	); err != nil {
+		return fmt.Errorf("list BGPAdvertisements for gateway node %s: %w", r.NodeName, err)
+	}
+
+	var errs []error
+	for i := range advList.Items {
+		adv := &advList.Items[i]
+		if !isNodeAdvertisement(adv, r.NodeName) {
+			continue
+		}
+		ruleName := adv.Labels[networkRuleLabel]
+		if known[ruleName] {
+			continue
+		}
+		err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: ruleName},
+			&bgpv1alpha1.NetworkRule{})
+		switch {
+		case err == nil:
+			continue
+		case !apierrors.IsNotFound(err):
+			errs = append(errs, fmt.Errorf("check NetworkRule %s for BGPAdvertisement %s: %w", ruleName, adv.Name, err))
+			continue
+		}
+		if err := r.Delete(ctx, adv); err != nil && !apierrors.IsNotFound(err) {
+			errs = append(errs, fmt.Errorf("withdraw BGPAdvertisement %s for deleted NetworkRule %s: %w",
+				adv.Name, ruleName, err))
+		}
+	}
+	return errors.Join(errs...)
+}
+
 // withdrawNodeAdvertisements deletes every BGPAdvertisement that gateway node
 // nodeName created in namespace: each per-rule, per-address-family route it
 // advertised. It lists by gatewayNodeLabel and deletes those that

internal/controller/networkgateway_controller_test.go

@@ -126,7 +126,7 @@ func acceptRule(rule *bgpv1alpha1.NetworkRule) {
 func newGatewayReconciler(
 	c client.Client, scheme *runtime.Scheme, engine GatewayEngine, node string,
 ) *NetworkGatewayReconciler {
-	return &NetworkGatewayReconciler{Client: c, Scheme: scheme, Engine: engine, NodeName: node}
+	return &NetworkGatewayReconciler{Client: c, APIReader: c, Scheme: scheme, Engine: engine, NodeName: node}
 }
 
 // newTestRouter returns the BGPRouter targeting testNodeGWA, resolved
@@ -1494,3 +1494,58 @@ func TestApplyBGPAdvertisements_PrunesOlderAdvertisements(t *testing.T) {
 		}
 	}
 }
+
+// TestNetworkGatewayReconciler_WithdrawsOwnAdvertisementForDeletedRule is the
+// regression test for #763: an advertisement this node created from a stale
+// cache after the rule's teardown finished is withdrawn once the rule is gone.
+// Another node's advertisement is left for that node to withdraw.
+func TestNetworkGatewayReconciler_WithdrawsOwnAdvertisementForDeletedRule(t *testing.T) {
+	scheme := newRuleTestScheme(t)
+	own := newNodeAdvertisement(testRuleName, testNodeGWA, "v4")
+	other := newNodeAdvertisement(testRuleName, testNodeGWB, "v4")
+
+	fakeClient := newIndexedClientBuilder(scheme).
+		WithStatusSubresource(&bgpv1alpha1.NetworkGateway{}, &bgpv1alpha1.NetworkRule{}).
+		WithObjects(newTestGateway(testNodeGWA), newTestRouter(), own, other).
+		Build()
+
+	r := newGatewayReconciler(fakeClient, scheme, newFakeGatewayEngine(), testNodeGWA)
+	ctx := context.Background()
+	if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: testRuleKey(testNodeGWA)}); err != nil {
+		t.Fatalf("Reconcile: unexpected error: %v", err)
+	}
+
+	if err := fakeClient.Get(ctx, testRuleKey(own.Name), &bgpv1alpha1.BGPAdvertisement{}); !apierrors.IsNotFound(err) {
+		t.Errorf("BGPAdvertisement %s still exists (err=%v), want withdrawn", own.Name, err)
+	}
+	if err := fakeClient.Get(ctx, testRuleKey(other.Name), &bgpv1alpha1.BGPAdvertisement{}); err != nil {
+		t.Errorf("gw-b's BGPAdvertisement %s was removed: %v", other.Name, err)
+	}
+}
+
+// TestNetworkGatewayReconciler_KeepsAdvertisementForRuleMissingFromCache
+// covers the other side of the #763 sweep: a rule absent from this node's
+// cache but still present in the API keeps its advertisement.
+func TestNetworkGatewayReconciler_KeepsAdvertisementForRuleMissingFromCache(t *testing.T) {
+	scheme := newRuleTestScheme(t)
+	rule := newTestRule(testRuleName, "vpc-1", testVIP)
+	acceptRule(rule)
+	own := newNodeAdvertisement(testRuleName, testNodeGWA, "v4")
+
+	fakeClient := newIndexedClientBuilder(scheme).
+		WithStatusSubresource(&bgpv1alpha1.NetworkGateway{}, &bgpv1alpha1.NetworkRule{}).
+		WithObjects(newTestGateway(testNodeGWA), newTestRouter(), rule, own).
+		Build()
+
+	r := newGatewayReconciler(staleListClient{Client: fakeClient, hideRules: true},
+		scheme, newFakeGatewayEngine(), testNodeGWA)
+	r.APIReader = fakeClient
+	ctx := context.Background()
+	if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: testRuleKey(testNodeGWA)}); err != nil {
+		t.Fatalf("Reconcile: unexpected error: %v", err)
+	}
+
+	if err := fakeClient.Get(ctx, testRuleKey(own.Name), &bgpv1alpha1.BGPAdvertisement{}); err != nil {
+		t.Errorf("BGPAdvertisement %s for a live rule was removed: %v", own.Name, err)
+	}
+}

internal/controller/networkrule_controller.go

@@ -10,6 +10,7 @@ import (
 	"encoding/hex"
 	"fmt"
 	"strings"
+	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -94,8 +95,17 @@ type NetworkRuleReconciler struct {
 	client.Client
 	Scheme   *runtime.Scheme
 	NodeName string
+
+	// APIReader reads from the API server, bypassing the informer cache.
+	// Teardown uses it to find an advertisement another gateway node created
+	// after this node's cache was last updated. Required.
+	APIReader client.Reader
 }
 
+// staleAdvertisementRequeue is how long reconcileDelete waits before checking
+// again after it found an advertisement its cache had missed.
+const staleAdvertisementRequeue = time.Second
+
 // Reconcile reconciles one NetworkRule's finalizer and Accepted condition.
 // Engine convergence and advertisement wiring for accepted rules happen in
 // NetworkGatewayReconciler, which watches NetworkRule and re-lists on every
@@ -196,9 +206,14 @@ func (r *NetworkRuleReconciler) updateAcceptedCondition(ctx context.Context, rul
 // releasing the finalizer would need a cross-node protocol that does not
 // exist, so the finalizer does not wait on the nodes.
 //
-// An advertisement created from a stale cache after the list below runs is not
-// seen here. Its owner reference to the rule lets Kubernetes garbage collection
-// delete it once the rule is gone.
+// The cached list can miss an advertisement another gateway node created
+// from a cache that had not yet seen the deletion timestamp. Before removing
+// the finalizer the rule's advertisements are listed again through APIReader;
+// any found there are deleted and the rule is requeued, keeping the finalizer
+// until a pass finds none. A node that creates one after the finalizer is gone
+// deletes it itself once its cache drops the rule (sweepOrphanedAdvertisements),
+// and the advertisement's owner reference to the rule lets Kubernetes garbage
+// collection delete it as a last resort.
 func (r *NetworkRuleReconciler) reconcileDelete(
 	ctx context.Context, rule *bgpv1alpha1.NetworkRule,
 ) (ctrl.Result, error) {
@@ -212,19 +227,17 @@ func (r *NetworkRuleReconciler) reconcileDelete(
 	// gateway-node membership, which a name-based lookup would need. This finds
 	// every advertisement the rule ever caused, including one for a node that
 	// has since left.
-	advList := &bgpv1alpha1.BGPAdvertisementList{}
-	if err := r.List(ctx, advList,
-		client.InNamespace(rule.Namespace),
-		client.MatchingLabels{networkRuleLabel: rule.Name},
-	); err != nil {
-		return ctrl.Result{}, fmt.Errorf("list BGPAdvertisements for NetworkRule %s/%s teardown: %w",
-			rule.Namespace, rule.Name, err)
+	if _, err := deleteRuleAdvertisements(ctx, r.Client, r.Client, rule); err != nil {
+		return ctrl.Result{}, err
 	}
-	for i := range advList.Items {
-		adv := &advList.Items[i]
-		if delErr := r.Delete(ctx, adv); delErr != nil && !apierrors.IsNotFound(delErr) {
-			return ctrl.Result{}, fmt.Errorf("withdraw BGPAdvertisement %s: %w", adv.Name, delErr)
-		}
+	missed, err := deleteRuleAdvertisements(ctx, r.APIReader, r.Client, rule)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	if missed > 0 {
+		logger.Info("withdrew BGPAdvertisements the cache had missed; checking again before removing the finalizer",
+			"networkRule", rule.Name, "count", missed)
+		return ctrl.Result{RequeueAfter: staleAdvertisementRequeue}, nil
 	}
 
 	// TODO(edge-gateway): the QuotaEnforcer interface is real and wired, but
@@ -246,6 +259,37 @@ func (r *NetworkRuleReconciler) reconcileDelete(
 	return ctrl.Result{}, nil
 }
 
+// deleteRuleAdvertisements lists rule's BGPAdvertisements through reader and
+// deletes each one through c that is not already being deleted. It returns how
+// many it deleted.
+func deleteRuleAdvertisements(
+	ctx context.Context, reader client.Reader, c client.Client, rule *bgpv1alpha1.NetworkRule,
+) (int, error) {
+	advList := &bgpv1alpha1.BGPAdvertisementList{}
+	if err := reader.List(ctx, advList,
+		client.InNamespace(rule.Namespace),
+		client.MatchingLabels{networkRuleLabel: rule.Name},
+	); err != nil {
+		return 0, fmt.Errorf("list BGPAdvertisements for NetworkRule %s/%s teardown: %w",
+			rule.Namespace, rule.Name, err)
+	}
+	deleted := 0
+	for i := range advList.Items {
+		adv := &advList.Items[i]
+		if !adv.DeletionTimestamp.IsZero() {
+			continue
+		}
+		if err := c.Delete(ctx, adv); err != nil {
+			if apierrors.IsNotFound(err) {
+				continue
+			}
+			return deleted, fmt.Errorf("withdraw BGPAdvertisement %s: %w", adv.Name, err)
+		}
+		deleted++
+	}
+	return deleted, nil
+}
+
 // SetupWithManager registers the reconciler with the manager.
 //
 // The NetworkGateway watch is the mirror of NetworkGatewayReconciler's