From 573ad377401f66965e991aefba0bd57a1e17156f Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 21:23:03 -0400 Subject: [PATCH] fix: Withdraw a load balancer rule's route after the rule is deleted A gateway node working from a stale cache could create a BGPAdvertisement for a NetworkRule after NetworkRuleReconciler.reconcileDelete had listed the rule's advertisements, so the finalizer came off with that route still advertised. Since #758 the advertisement carries an owner reference to the rule and Kubernetes garbage collection eventually deletes it, but until then the fabric keeps sending VIP traffic to a node that has already unloaded the rule. Two windows close here. reconcileDelete still deletes what its cache sees, then lists the rule's advertisements again through the manager's uncached API reader; any found there are deleted and the rule is requeued after 1s, keeping the finalizer until a pass finds none. On the creating side, NetworkGatewayReconciler now runs sweepOrphanedAdvertisements on every pass: it lists this node's own advertisements by gateway-node label and deletes each one whose rule is missing from the pass's rule list and returns NotFound from the uncached reader. A rule the cache has not caught up with keeps its route. The sweep runs on the same pass that unloads the rule from the datapath, so a node that created a late advertisement withdraws it as soon as its cache drops the rule. The owner reference stays as the last resort. Both reconcilers take a required APIReader, wired to mgr.GetAPIReader() in galactic-gateway. A sweep failure is returned for retry and does not change the gateway's Ready condition. ARCHITECTURE-GATEWAY.md describes both changes. Fixes #763 Co-Authored-By: Claude Haiku 4.5 --- cmd/galactic-gateway/root.go | 18 ++-- docs/agents/ARCHITECTURE-GATEWAY.md | 13 ++- .../controller/networkgateway_controller.go | 73 +++++++++++++- .../networkgateway_controller_test.go | 57 ++++++++++- internal/controller/networkrule_controller.go | 74 ++++++++++++--- .../controller/networkrule_controller_test.go | 94 +++++++++++++++++-- 6 files changed, 291 insertions(+), 38 deletions(-) diff --git a/cmd/galactic-gateway/root.go b/cmd/galactic-gateway/root.go index 99a85f05..080470f7 100644 --- a/cmd/galactic-gateway/root.go +++ b/cmd/galactic-gateway/root.go @@ -165,11 +165,12 @@ 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) } @@ -177,9 +178,10 @@ func runCmd(cfg *config.GatewayConfig) error { // 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) } diff --git a/docs/agents/ARCHITECTURE-GATEWAY.md b/docs/agents/ARCHITECTURE-GATEWAY.md index a3380e91..64be54eb 100644 --- a/docs/agents/ARCHITECTURE-GATEWAY.md +++ b/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 diff --git a/internal/controller/networkgateway_controller.go b/internal/controller/networkgateway_controller.go index ba1f6fa2..f99b9be3 100644 --- a/internal/controller/networkgateway_controller.go +++ b/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 diff --git a/internal/controller/networkgateway_controller_test.go b/internal/controller/networkgateway_controller_test.go index d3e112e6..1158fae9 100644 --- a/internal/controller/networkgateway_controller_test.go +++ b/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) + } +} diff --git a/internal/controller/networkrule_controller.go b/internal/controller/networkrule_controller.go index a04a5b66..5703fbde 100644 --- a/internal/controller/networkrule_controller.go +++ b/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 diff --git a/internal/controller/networkrule_controller_test.go b/internal/controller/networkrule_controller_test.go index 4aa8f433..8941378d 100644 --- a/internal/controller/networkrule_controller_test.go +++ b/internal/controller/networkrule_controller_test.go @@ -8,11 +8,13 @@ import ( "context" "testing" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" 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/controller/controllerutil" @@ -165,7 +167,7 @@ func TestNetworkRuleReconciler_SetsFinalizerAndAccepted(t *testing.T) { WithObjects(gwA, gwB, rule). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { @@ -227,7 +229,7 @@ func TestNetworkRuleReconciler_UpdateAcceptedCondition_FalseWithNoGatewayNodes(t WithObjects(rule). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} if err := r.updateAcceptedCondition(context.Background(), rule); err != nil { t.Fatalf("updateAcceptedCondition: unexpected error: %v", err) } @@ -262,7 +264,7 @@ func TestNetworkRuleReconciler_GatewayAppearanceRequeuesParkedRule(t *testing.T) WithObjects(rule). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} // No NetworkGateway exists yet, so this is the zero-nodes branch (see // TestNetworkRuleReconciler_UpdateAcceptedCondition_FalseWithNoGatewayNodes @@ -325,7 +327,7 @@ func TestNetworkRuleReconciler_NoOpOnNonGatewayNode(t *testing.T) { Build() // compute-node-1 is not a registered NetworkGateway target. - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testComputeNodeName} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testComputeNodeName} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -370,7 +372,7 @@ func TestNetworkRuleReconciler_TeardownOrder_WithdrawsBGPBeforeRemovingFinalizer WithObjects(gwA, rule, adv). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -428,7 +430,7 @@ func TestNetworkRuleReconciler_TeardownWithdrawsAdvertisementForDepartedNode(t * WithObjects(gwA, rule, adv). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -473,7 +475,7 @@ func TestNetworkRuleReconciler_TeardownIgnoresAdvertisementForDifferentRule(t *t WithObjects(gwA, rule, otherAdv). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -499,9 +501,85 @@ func TestNetworkRuleReconciler_TeardownIsIdempotentWhenAdvertisementAlreadyGone( WithObjects(gwA, rule). Build() - r := &NetworkRuleReconciler{Client: fakeClient, Scheme: scheme, NodeName: testNodeGWA} + r := &NetworkRuleReconciler{Client: fakeClient, APIReader: fakeClient, Scheme: scheme, NodeName: testNodeGWA} req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error when no BGPAdvertisement exists: %v", err) } } + +// staleListClient hides every object of the wrapped kinds from List, the way +// an informer cache that has not yet seen another node's writes does. Get and +// every write go to the wrapped client. +type staleListClient struct { + client.Client + hideAdvertisements bool + hideRules bool +} + +func (c staleListClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + switch list.(type) { + case *bgpv1alpha1.BGPAdvertisementList: + if c.hideAdvertisements { + return nil + } + case *bgpv1alpha1.NetworkRuleList: + if c.hideRules { + return nil + } + } + return c.Client.List(ctx, list, opts...) +} + +// TestNetworkRuleReconciler_TeardownWithdrawsAdvertisementMissingFromCache is +// the regression test for #763: an advertisement another gateway node created +// after this node's cache was last updated must be found through APIReader +// and withdrawn, and the finalizer kept until a pass finds none. +func TestNetworkRuleReconciler_TeardownWithdrawsAdvertisementMissingFromCache(t *testing.T) { + scheme := newRuleTestScheme(t) + rule := newTestRule(testRuleName, "vpc-1", testVIP) + rule.Finalizers = []string{networkRuleFinalizer} + now := metav1.Now() + rule.DeletionTimestamp = &now + adv := newNodeAdvertisement(testRuleName, testNodeGWB, "v4") + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&bgpv1alpha1.NetworkRule{}). + WithObjects(newTestGateway(testNodeGWA), rule, adv). + Build() + + r := &NetworkRuleReconciler{ + Client: staleListClient{Client: fakeClient, hideAdvertisements: true}, + APIReader: fakeClient, + Scheme: scheme, + NodeName: testNodeGWA, + } + ctx := context.Background() + req := ctrl.Request{NamespacedName: testRuleKey(testRuleName)} + + res, err := r.Reconcile(ctx, req) + if err != nil { + t.Fatalf("Reconcile: unexpected error: %v", err) + } + if err := fakeClient.Get(ctx, testRuleKey(adv.Name), &bgpv1alpha1.BGPAdvertisement{}); !apierrors.IsNotFound(err) { + t.Fatalf("BGPAdvertisement %s still exists (err=%v), want withdrawn", adv.Name, err) + } + gotRule := &bgpv1alpha1.NetworkRule{} + if err := fakeClient.Get(ctx, req.NamespacedName, gotRule); err != nil { + t.Fatalf("NetworkRule removed on the pass that found a missed advertisement: %v", err) + } + if !controllerutil.ContainsFinalizer(gotRule, networkRuleFinalizer) { + t.Fatal("finalizer removed on the pass that found a missed advertisement") + } + if res.RequeueAfter == 0 { + t.Error("RequeueAfter = 0, want a requeue to check again") + } + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("second Reconcile: unexpected error: %v", err) + } + if err := fakeClient.Get(ctx, req.NamespacedName, &bgpv1alpha1.NetworkRule{}); !apierrors.IsNotFound(err) { + t.Fatalf("NetworkRule still exists after a pass found no advertisements (err=%v)", err) + } +}