bgp: support route reflector clients (RFC 4456) by gusbourg · Pull Request #133 · holo-routing/holo

gusbourg · 2026-07-05T22:00:09Z

Adds BGP route reflection (RFC 4456):

route-reflector neighbor config (client flag + configurable cluster-id, defaulting to the router-id) via the IETF ietf-bgp route-reflector container.
Reflection on advertisement: client-learned routes to all iBGP peers, non-client-learned routes to clients only.
ORIGINATOR_ID and CLUSTER_LIST set on reflected advertisements; received routes are rejected when either would form a reflection loop.
A route_reflector conformance topology.

🤖 Generated with Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Expose the IETF neighbor route-reflector container and use the client and cluster-id settings when disseminating iBGP routes. Reflected advertisements now carry ORIGINATOR_ID and CLUSTER_LIST, and received routes are rejected when ORIGINATOR_ID or CLUSTER_LIST would create a reflection loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Use the route reflector server cluster ID when reflecting iBGP routes, including advertisements to non-client peers, and add a conformance topology covering client reflection, cluster-loop rejection, and non-client reflection scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Paul-weqe

Surprising this wasn't already supported for route reflectors...good catch. In any case, here's my review below.

I'll take a deeper look at the logic in BGP's event, rib, and neighbor files, but these were the comments that stood out to me first.

Paul-weqe · 2026-07-10T12:29:07Z

    instance.state.schedule_decision_process(instance.tx);
 }

+fn schedule_decision_process_all_afs(instance: &mut Instance) {


Once we have the calls to this moved to the EventQueue, we will probably end up changing &mut Instance to mut InstanceUpView<'_>, but that should come later.

Paul-weqe · 2026-07-10T12:40:06Z

@@ -0,0 +1,5 @@
+{"RouterIdSub":{}}


Thanks for including conformance tests here-super helpful.

One suggestion: the tests would be more meaningful with more than one device. With only rt-1 and route reflectors configured, we're mainly validating that the configuration parses correctly, not that route reflection actually works between routers.

output/northbound-state.json can help confirm the expected final state in a multi-device setup. There's also a way to generate these conformance tests automatically from the holo munet topologies, which should make this easier going forward.

I'll update the README to document that process.

The README to holo-munet topologies has been updated:

https://github.com/holo-routing/holo-munet-topologies#conformance-tests

Paul-weqe · 2026-07-10T12:42:56Z

+
+            schedule_decision_process_all_afs(instance);
+        })
+        .delete_apply(|instance, args| {


Quick note: the YANG path

/ietf-routing:routing/control-plane-protocols/control-plane-protocol/ietf-bgp:bgp/neighbors/neighbor/route-reflector/client is a boolean, see here:

holo/holo-yang/modules/ietf/ietf-bgp-common-structure@2023-07-05.yang

Line 90 in 047398b

leaf client {

So it doesn't support the DELETE operation. Might be worth adjusting.

@gusbourg
A PR helping us to catch this during compilation has been opened: #158

Paul-weqe · 2026-07-10T12:44:42Z

+            let nbr_addr = args.list_entry.into_neighbor().unwrap();
+            let nbr = instance.neighbors.get_mut(&nbr_addr).unwrap();
+
+            let cluster_id = args.dnode.get_string();


Quick suggestion on fetching cluster_id: could we use get_u32() directly and convert to Ipv4Addr, rather than going through a string parse first? Should avoid an unnecessary conversion step.

Also, a question on the cluster_id type. Would making it an Option<u32> work here? It'd sidestep the extra parsing either way, though happy to hear if there's a reason for the current approach.

Paul-weqe · 2026-07-10T12:45:44Z

+            let cluster_id = cluster_id.parse::<Ipv4Addr>().unwrap_or_else(|_| Ipv4Addr::from(cluster_id.parse::<u32>().unwrap()));
+            nbr.config.route_reflector.cluster_id = Some(cluster_id);
+
+            schedule_decision_process_all_afs(instance);


This might fit better as an event added to the event queue. We could introduce a new variant in the Event enum, say ScheduleRRDecisionProcess and call schedule_decision_process_all_fs from there.
That would also keep it consistent with how holo handles events elsewhere in the configurations.

Paul-weqe · 2026-07-10T12:46:06Z

+
+            nbr.config.route_reflector.cluster_id = None;
+
+            schedule_decision_process_all_afs(instance);


Same as above...this should be an event added to the event queue

Paul-weqe · 2026-07-10T12:48:39Z

 // ===== global functions =====

 pub fn spawn_protocol_task<P>(
+    name: String,


As had been proposed in #132 this can all be one function and have the "testing" feature handle the extra fields and additional logic.

Not every YANG node structurally supports every CRUD operation (e.g. a leaf with a plain default and no when/case can't be deleted, list keys can't be modified or deleted). This was only checked at daemon startup (validate_callback), so an invalid registration like .delete_apply() on such a node compiled fine and only surfaced as a runtime crash. Make YangPath and CallbacksBuilder a typestate: yang_codegen now emits a zero-sized Caps marker per node and implements SupportsCreate/ SupportsModify/SupportsDelete/SupportsLookup on it based on the same CallbackOp::is_valid rules enforced at runtime today. CallbacksBuilder's create_*/modify_*/delete_*/lookup methods are now gated on the matching trait, so an invalid registration fails to compile instead of panicking at startup. Drop validate_callback, now fully superseded. validate_callbacks (checks for missing, not invalid, callbacks) is unaffected. Basically making sure invalid calls are caught at compile time not at runtime. Concern raised when invalid callbacks were made in a boolean Yang Path: PR: holo-routing#133 Comment: holo-routing#133 (comment) Signed-off-by: Paul Wekesa <paul1tw1@gmail.com>

gusbourg and others added 4 commits July 5, 2026 10:41

protocol: keep workspace tests buildable with testing feature

b0d252c

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

style: apply nightly rustfmt

40ca4f0

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Paul-weqe reviewed Jul 10, 2026

View reviewed changes

Paul-weqe mentioned this pull request Jul 10, 2026

northbound: enforce valid YANG CRUD callbacks at compile time #158

Open

Navigation Menu

Uh oh!

bgp: support route reflector clients (RFC 4456) - #133

bgp: support route reflector clients (RFC 4456)#133
gusbourg wants to merge 4 commits into
holo-routing:masterfrom
gusbourg:bgp-rr-server

gusbourg commented Jul 5, 2026

Uh oh!

Paul-weqe left a comment

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026 •

edited

Loading

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Paul-weqe Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		nbr.config.route_reflector.cluster_id = None;

		schedule_decision_process_all_afs(instance);

Uh oh!

Conversation

gusbourg commented Jul 5, 2026

Uh oh!

Paul-weqe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul-weqe Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Paul-weqe Jul 10, 2026 •

edited

Loading