Leave the route server peers alone when no router nodes match

[frobware]

An empty router node list is reconciled like any other. reconcileRouteServerPeers builds nodesByAZ from the node list, so an empty list leaves the desired address set empty for every zone, and the loop that deletes managed peers absent from that set deletes all of them. Every BGP session in the cluster goes down together.

The selector matching nothing is not a rare state. Moving the router label during a rollout does it, and so does any window where the labelled nodes are briefly gone. Nothing has asked for the peers to be released at that point, and recovering means recreating every peer and renegotiating every session, so the reconcile now returns early and leaves them as it found them. Cleanup already does the releasing, and it runs on deletion, where the intent is unambiguous.

The guard sits in ReconcileNodes rather than in reconcileRouteServerPeers, and that placement is the point. A zone that loses its last node while other zones still have one has genuinely stale peers, and those must still be deleted; it is only the case of no nodes anywhere that is meaningless. Keeping the guard a level up leaves the lower function's "not in the desired set means stale" semantics untouched, which is why TestReconcilePeers_DeleteStalePeer still passes unmodified: it passes nil nodes to that function directly and asserts the peer is deleted.

GCP has the same defect and the same fix, in #63. This is separate so that it can land on its own rather than behind a GCP review.

Worth knowing: #13 is open in draft and rewrites reconcileRouteServerPeers substantially. It keeps the same desiredIPs := nodesByAZ[az] shape, so the bug survives that rewrite intact. The two should not conflict textually, since this change is in the calling function, and the guard would still protect the rewritten code.

Test plan

Two unit tests, written red first. TestReconcileNodes_EmptyNodeListLeavesPeersAlone fails before the change with deleted 1 peers, want 0 and passes after. TestReconcileNodes_ShrunkNodeListStillDeletes passes both before and after, and exists so the early return cannot quietly widen: one node in us-east-1a and none in us-east-1b must still delete peer-ep-b1.

go test ./internal/... passes, make lint reports no issues.

This has not been exercised against a live AWS cluster. The equivalent fix in #63 was verified on a live GCP cluster, where stripping the router label from every node left the estate intact and restoring it brought all sessions back with no cloud operations at all.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 4a2c3375-70fc-4a67-87d9-8f6c2a7521a4

📥 Commits

Reviewing files that changed from the base of the PR and between 25a6f39 and 3a993ca.

📒 Files selected for processing (2)

• internal/platform/aws/aws.go
• internal/platform/aws/aws_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

ReconcileNodes now treats an empty router-node selection as a no-op and preserves existing managed peers. Reduced node lists still trigger reconciliation and remove stale peers. Tests add a reusable managed-peer mock and cover both behaviors.

Possibly related PRs

• openshift/bgp-cloud-connector#64: Changes AWS reconciliation in the same ReconcileNodes function.

Suggested reviewers: alebedev87

Merge Risk: ⚪ Minimal · up to 3a993

The change preserves existing route-server peers when no router nodes match while retaining stale-peer cleanup when only a zone becomes empty; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: preserving route-server peers when no router nodes match.
Description check	✅ Passed	The description directly explains the bug, implementation, test coverage, and validation for the route-server peer reconciliation change.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The added tests use static Go test names and no Ginkgo title constructs; node names, zones, IPs, and peer IDs occur only in test bodies.
Test Structure And Quality	✅ Passed	The PR adds standard Go unit tests, not Ginkgo tests. They use local mocks, create no cluster resources, have no waits, and include diagnostic assertion messages consistent with nearby tests.
Microshift Test Compatibility	✅ Passed	The added tests are standard Go testing functions, not Ginkgo e2e tests, and they reference only AWS mocks and platform types; no MicroShift-unavailable APIs or assumptions are introduced.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The diff adds only standard Go func Test... unit tests in internal/platform/aws; it adds no Ginkgo e2e tests or SNO-sensitive multi-node assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The diff only changes AWS route-server peer reconciliation and tests; it adds no manifests, workload scheduling constraints, replica/PDB settings, or topology targeting.
Ote Binary Stdout Contract	✅ Passed	The diff adds only an Info log inside controller reconciliation and standard Go tests; it adds no stdout write in main/init or Ginkgo/OTE suite setup.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds standard Go unit tests in internal/platform/aws/aws_test.go, not new Ginkgo e2e tests; the IPv6/disconnected-network check is therefore inapplicable.
No-Weak-Crypto	✅ Passed	The PR changes only node reconciliation and tests. The diff adds no crypto code, prohibited algorithms, custom crypto, or secret/token comparisons.
Container-Privileges	✅ Passed	The diff changes only AWS Go code and tests; it adds no container/Kubernetes manifests or privilege settings such as hostPID, SYS_ADMIN, or allowPrivilegeEscalation.
No-Sensitive-Data-In-Logs	✅ Passed	The only new runtime log is a fixed message with no credentials, identifiers, addresses, hostnames, or customer data; existing value-bearing logs are unchanged.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [frobware]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knobunc]

/lgtm

[openshift-merge-bot]

Pipeline controller notification

No second-stage tests were triggered for this PR.

This can happen when:

• The changed files don't match any pipeline_run_if_changed patterns
• All files match pipeline_skip_if_only_changed patterns
• No pipeline-controlled jobs are defined for the main branch

Use /test ? to see all available tests.