bgp: Support multiple labels in BGP-LU #19961

hedrok · 2025-11-04T06:21:50Z

Add support for multiple labels in BGP-LU + topological test using ExaBGP for that.

Absolute possible maximum number of labels according to RFC 3017/RFC 8277 (see comment to BGP_MAX_LABELS) is supported: max 10 labels.

Open questions:

I've added 8*3=24 bytes per struct bgp_labels - is that is a problem, should I make it dynamic like mpls_label_stack?
[answered] Not to break backward compatibility in json output of route_vty_out_detail (command like show ip bgp 33.10.10.0/30 json), I've added remoteLabels with array of labels, but left remoteLabel with top label in stack intact. Should I instead remove this duplicate field and change all topotests accordingly? (@ton31337 suggested to use CONFDATE check to issue warning in a year to remind that remoteLabel removal is needed)

Resolves #19506

ton31337 · 2025-11-05T12:28:35Z

Not to break backward compatibility in json output of route_vty_out_detail (command like show ip bgp 33.10.10.0/30 json), I've added remoteLabels with array of labels, but left remoteLabel with top label in stack intact. Should I instead remove this duplicate field and change all topotests accordingly?

We should eventually add CONFDATE (a deprecation for remoteLabel for one year, that would be quite enough).

donaldsharp · 2025-11-05T17:43:35Z

please put more verbiage in the commit message besides resolves ... that is not very useful for anyone who comes along behind you. Put in what you are doing, why and what your approach is please

hedrok · 2025-11-05T18:14:02Z

@donaldsharp I've updated commit message. If even more verbose commit message is needed (or any other changes), please say.

hedrok · 2025-11-06T11:41:26Z

I've also updated message for commit with test.

@ton31337 thanks for your answer, should I add

#if CONFDATE > 20261106
CPP_NOTICE("Remove 'remoteLabel' field")
#endif

Near the field in this PR?
I'm just a little bit confused with "We should eventually add" - should I do it now or it will be done later by someone else?

ton31337 · 2025-11-07T09:15:11Z

Yes, above remoteLabel JSON field.

hedrok · 2025-11-07T09:45:33Z

@ton31337 Done, thanks.

github-actions · 2025-11-07T20:56:44Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

ritika0313

The code changes look good overall:

Both sending and receiving of UPDATE messages with multiple labels is supported
Take care of BOS value being appropriately set in a case of single or multiple labels.( Some implicit setting BoS also exists which should have been verified in testing)
Correctly calculates the maximum possible number of labels and required size for labels to write in the stream
Also, provide backward compatibility for encode_label() used by Mpls VPN.

Few corrections and improvements have been suggested under particular line of code in individual comments

bgpd/bgp_route.c

bgpd/bgp_zebra.c

bgpd/bgp_label.c

lib/mpls.h

bgpd/bgp_label.h

bgpd/bgp_route.c

hedrok

@ritika0313 Thank you for thorough review. I've replied to all your comments and updated PR.

bgpd/bgp_label.c

bgpd/bgp_label.h

bgpd/bgp_label.c

bgpd/bgp_route.c

bgpd/bgp_zebra.c

lib/mpls.h

tests/topotests/bgp_lu_multiple_labels/test_bgp_lu_multiple_labels.py

ton31337 · 2025-11-09T15:19:23Z

tests/topotests/bgp_lu_multiple_labels/peer1/exa-send.py

+
+sleep(5)
+
+stdout.write("announce route 2001:db8:100::/64 next-hop fc00::2 label [777 10006]\n")


Can't we just announce these immediately without using a Python script (in the ExaBGP config directly)?

Yes, thank you, I experimented a little more and it works now - of course it is simpler this way.
And now 5 seconds less sleep :)

ton31337 · 2025-11-09T15:20:22Z

lib/stream.c

-	if (STREAM_WRITEABLE(s) < (psize_with_addpath + 3)) {
+	if (p->prefixlen + MPLS_LABEL_LEN_BYTES * 8 * num_labels >= 256) {
+		num_labels = (256 - p->prefixlen) / 8 / MPLS_LABEL_LEN_BYTES;
+		zlog_info("%s: Ignoring extra labels, can fit only %d in BGP-LU message with prefixlen %d",


Shouldn't this be a warning instead?

Yes, thank you.

lib/mpls.h

hedrok

Updated according to @ton31337 comments:

Replaced BGP_LABEL_BYTES -> MPLS_LABEL_LEN_BYTES globally not to have two constants
Chaned info to warning in case stream_put_labeled_prefix can't put so many labels.
Increased all timeouts in topotest to minimum 15 seconds.
Simplified ExaBGP configuration in topotest

I rebase my changes to master and use fixup of commits so that there are only two commits: one with actual changes, other one with tests. If any of reviewers prefers new commits for new changes and no history rewriting for easier review - please say, I'll do it this way then.

lib/mpls.h

hedrok · 2025-11-09T17:25:02Z

lib/stream.c

-	if (STREAM_WRITEABLE(s) < (psize_with_addpath + 3)) {
+	if (p->prefixlen + MPLS_LABEL_LEN_BYTES * 8 * num_labels >= 256) {
+		num_labels = (256 - p->prefixlen) / 8 / MPLS_LABEL_LEN_BYTES;
+		zlog_info("%s: Ignoring extra labels, can fit only %d in BGP-LU message with prefixlen %d",


Yes, thank you.

tests/topotests/bgp_lu_multiple_labels/test_bgp_lu_multiple_labels.py

hedrok · 2025-11-09T17:52:01Z

tests/topotests/bgp_lu_multiple_labels/peer1/exa-send.py

+
+sleep(5)
+
+stdout.write("announce route 2001:db8:100::/64 next-hop fc00::2 label [777 10006]\n")


Yes, thank you, I experimented a little more and it works now - of course it is simpler this way.
And now 5 seconds less sleep :)

ton31337

LGTM

ritika0313

Looks good

hedrok · 2025-11-10T20:27:15Z

@ton31337 thank you for approve, I've added one small change as @ritika0313 has a valid point: in hypothetic case of receiving more labels than BGP_MAX_LABELS I must set BOS for the last one we store.

This is the only change, here is full diff:

diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c
index 7046c61ff9..09a732e11e 100644
--- a/bgpd/bgp_label.c
+++ b/bgpd/bgp_label.c
@@ -459,6 +459,7 @@ static int bgp_nlri_get_labels(struct peer *peer, uint8_t *pnt, uint8_t plen, mp
 
 	if (label_depth > BGP_MAX_LABELS) {
 		*num_labels = BGP_MAX_LABELS;
+		label_set_bos(&labels[*num_labels - 1]);
 		zlog_info("%pBP rcvd UPDATE with label stack %d deep, using only first %d labels",
 			  peer, label_depth, BGP_MAX_LABELS);
 	}
diff --git a/bgpd/bgp_label.h b/bgpd/bgp_label.h
index b8e48cefaf..18967cb805 100644
--- a/bgpd/bgp_label.h
+++ b/bgpd/bgp_label.h
@@ -123,4 +123,12 @@ static inline uint8_t label_bos(mpls_label_t *label)
 	return (t[2] & 0x01);
 };
 
+/* Set BOS to 1 */
+static inline void label_set_bos(mpls_label_t *label)
+{
+	uint8_t *t = (uint8_t *)label;
+
+	t[2] |= 0x01;
+}
+
 #endif /* _BGP_LABEL_H */

dmytroshytyi-6WIND

LGTM, Thanks

bgpd/bgp_route.c

mjstapp · 2025-11-19T14:03:34Z

lib/stream.c

-int stream_put_labeled_prefix(struct stream *s, const struct prefix *p,
-			      mpls_label_t *label, bool addpath_capable,
-			      uint32_t addpath_tx_id)
+int stream_put_labeled_prefix(struct stream *s, const struct prefix *p, mpls_label_t *label,


why is this in lib/ - it sure looks as if it's a bgp-protocol function?

Logically I'm sure it should be in bgp code and it is used only once in bgpd/bgp_attr.c.
The function uses a lot of macro from lib/stream.c that I had to move to lib/stream.h for it to work.
I've done the move in separate commit for easier review and removal if not needed: bb14416

Please say whether this move is ok - if so, I'll squash the commit with main one and will be able to make two small refactorings:

Remove introduced MPLS_LABEL_LEN_BYTES and get back BGP_LABEL_BYTES - I've added new constant only because it was needed in this function that is in lib/stream.c, where bgp constant is unavailable.

Make use of label_set_bos instead of mpls_lse_decode + mpls_lse_encode

yes, ouch, I see what you mean - no, you should leave that change out.
the thing that's awkward about the lib/stream api is that it does all that detailed byte-building work. could we move the bit/byte code into bgp, so that bgp constructs a buffer with its own logic, and then uses just a vanilla stream api to add the bgp buffer? in any case, that's not really a responsibility of yours, for this PR.

This is great idea - it allowed me to implement two small refactorings mentioned earlier, merge stream_bgp_put_labeled_prefix and stream_put_prefix_addpath, overall code became much cleaner.

I tried my best not to introduced issues - now I've changed function that writes any BGP NLRI to stream... At least I hope that if I broke something topotests shoulds catch that, as it is core functionality, so I'm waiting for CI verdict with big interest...

hmm, I really sort of meant it when I said "you should leave that change out".
again, just let's see about getting the BGP-LU pieces through, and then if you want to adjust the stream code, that would be better as a separate PR (potentially of interest to a different set of reviewers, for example)

Sorry, I thought that "could we move the bit/byte code into bgp" was suggestion for action :)

Reverted, but left small preparations for this refactoring as I liked this change and I plan to create separate PR with it as soon as this PR is merged. And yes, it would be better to review it with extreme attention.

What I left is:

+/* TODO Move out label-specific logic to bgpd, there BGP_LABEL_BYTES is already defined */ +#define BGP_LABEL_BYTES 3

In lib/stream.c - hope it is not a problem, it will make refactoring easier.

the diff here is still ... large? it'd be clearer just to get the bgp parts in, and then look at the lib/stream part.

Ok.

I've reverted everything in lib/stream.*, reverted bgp_packet_mpattr_prefix_size (as now it is possible to send only one label) and updated bgp_packet_mpattr_prefix accordingly.

So now multiple labels won't work properly when use_local_label is false in bgp_adv_label (basically when PEER_FLAG_FORCE_NEXTHOP_SELF or PEER_FLAG_NEXTHOP_UNCHANGED are applied) - in such case only top label will be retransmitted and a warning issued.

Simpler case of just receiving multiple labels still works, which is covered by test.

mjstapp · 2025-11-19T14:09:11Z

bgpd/bgp_zebra.c

-						&exp, &bos);
+				uint8_t i;
+
+				api_nh->label_num = num_labels;


must validate the number of labels against the fixed-size array in the zapi nexthop?

lib/mpls.h

#define MPLS_MAX_LABELS 16

lib/zclient.h

struct zapi_nexthop { ... mpls_label_t labels[MPLS_MAX_LABELS];

And in bgp I've added theoretical maximum:

bgpd/bgp_label.h

#define BGP_MAX_LABELS 10

Of course I don't mind checking anyway, should I add something like

static_assert(BGP_MAX_LABELS <= MPLS_MAX_LABELS && sizeof(api_nh->labels) / sizeof(api_nh->labels[0]) == MPLS_MAX_LABELS, "Code expects BGP_MAX_LABELS <= MPLS_MAX_LABELS and api_nh->labels is array of MPLS_MAX_LABELS elements. If that changes, please adapt code below");

To guard against possible changes of these constants and that api_nh->labels is array of MPLS_MAX_LABELS elements?

I'd rather not have an assert - there's no real need to crash. I'd just like to see, in general, that the destination array limit be respected/checked during the iteration. An assumption about the code today may not always be correct in the future, and it'll always be safe to copy only as many values as there are destination cells?

Ok, I've just made obvious change:

if (api_nh->label_num > MPLS_MAX_LABELS) { zlog_warn("%s: too many labels provided for zebra (%d), copying first %d", __func__, num_labels, MPLS_MAX_LABELS); api_nh->label_num = MPLS_MAX_LABELS; }

But also added static_assert I've suggested above (after battle with checkpatch.sh and enduring awful in my personal opinion format changes from git clang-format).

I've suggested static_assert - compilation error, not crash.

I think this way message will be provided to the developer who tried to change these preconditions right away during compilation time and it will be much more obvious then losing some labels and warning log...

When changing BGP_MAX_LABELS to 20 I get during compilation:

In file included from ./lib/zebra.h:74, from bgpd/bgp_zebra.c:7: bgpd/bgp_zebra.c: In function 'bgp_zebra_announce_parse_nexthop': ./lib/assert/assert.h:30:23: error: static assertion failed: "Code expects BGP_MAX_LABELS <= MPLS_MAX_LABELS and api_nh->labels is an array of MPLS_MAX_LABELS elements. If that changes, please adapt code below" 30 | #define static_assert _Static_assert | ^~~~~~~~~~~~~~ bgpd/bgp_zebra.c:1425:33: note: in expansion of macro 'static_assert' 1425 | static_assert(BGP_MAX_LABELS <= MPLS_MAX_LABELS && | ^~~~~~~~~~~~~

Of course I can remove this if you think it is a bad idea.

I think that section just makes the code harder to read; I'm not sure we need to couple bgp and the FRR zapi library, really.
I think it's better and clearer just to ensure we don't overrun the target/destination array.

🫡 Sorry for long discussion. Changed to just checking destination array size, removed all static_assert part:

int max_elements = sizeof(api_nh->labels) / sizeof(api_nh->labels[0]); api_nh->label_num = num_labels; if (api_nh->label_num > max_elements) { zlog_warn("%s: too many labels provided for zebra (%d), copying first %d", __func__, num_labels, max_elements); api_nh->label_num = max_elements; } for (i = 0; i < api_nh->label_num; i++) {

bgpd/bgp_zebra.c

github-actions · 2025-11-21T14:40:51Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

mjstapp · 2025-11-21T21:18:41Z

tools/checkpatch.pl

 			my $to = $4;
 			my $newcomp = $comp;
 			if ($lead !~ /(?:$Operators|\.)\s*$/ &&
-			    $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&


hmm, no - don't try to extend this PR to include a tools change too. if you want to propose something like this, go ahead and put it in its own PR.

Sorry, will create separate PR. I just needed this to pass checkpatch.sh check so risked adding here. Don't need it anymore as I've removed static_assert.

Updated behaviour for SAFI_LABELED_UNICAST to support multiple labels: * Change BGP_MAX_LABELS 2 -> 10 (maximum possible number of labels for BGP-LU), previously for SAFI_LABELED_UNICAST only one label was supported * Save all labels when receiving an update event in `bgp_nlri_get_labels`, send them all to zebra in `bgp_zebra_announce_parse_nexthop` One of uses of multiple labels is SR-TE with BGP-LU. Resolves FRRouting#19506 Signed-off-by: Kyrylo Yatsenko <hedrok@gmail.com>

Announce three routes with label stacks 777/10006, 90, 11/22/33/44/55, check that `show bgp ipv6 PREFIX json` has proper labels in `remoteLabels`. Test for issue FRRouting#19506 Signed-off-by: Kyrylo Yatsenko <hedrok@gmail.com>

bgpd/bgp_label.c

hedrok · 2025-11-26T14:09:11Z

🥳 🎉 Thanks to everybody, this is the most reviewed PR in my life :)

github-actions bot added master size/XL labels Nov 4, 2025

hedrok mentioned this pull request Nov 4, 2025

T7766: FRR: Support multiple labels in BGP-LU vyos/vyos-build#1066

Merged

11 tasks

riw777 self-requested a review November 4, 2025 16:03

hedrok force-pushed the 19506-bgp-multiple-labels branch from 40a900b to f75c8bd Compare November 5, 2025 18:09

hedrok force-pushed the 19506-bgp-multiple-labels branch from f75c8bd to e56fe22 Compare November 6, 2025 11:34

frrbot bot added bgp tests Topotests, make check, etc labels Nov 6, 2025

hedrok force-pushed the 19506-bgp-multiple-labels branch from e56fe22 to c773136 Compare November 7, 2025 09:44

github-actions bot added the conflicts label Nov 7, 2025

hedrok force-pushed the 19506-bgp-multiple-labels branch from c773136 to e7b2644 Compare November 7, 2025 21:06

github-actions bot removed the conflicts label Nov 7, 2025

ritika0313 reviewed Nov 7, 2025

View reviewed changes

hedrok commented Nov 8, 2025

View reviewed changes

hedrok force-pushed the 19506-bgp-multiple-labels branch from e7b2644 to 5807236 Compare November 8, 2025 20:17

ton31337 reviewed Nov 9, 2025

View reviewed changes

hedrok commented Nov 9, 2025

View reviewed changes

hedrok force-pushed the 19506-bgp-multiple-labels branch from 5807236 to a90f728 Compare November 9, 2025 18:11

hedrok requested a review from ton31337 November 9, 2025 18:12

ton31337 approved these changes Nov 10, 2025

View reviewed changes

ritika0313 approved these changes Nov 10, 2025

View reviewed changes

hedrok force-pushed the 19506-bgp-multiple-labels branch from a90f728 to 00fe1bc Compare November 10, 2025 20:24

dmytroshytyi-6WIND approved these changes Nov 12, 2025

View reviewed changes

mjstapp reviewed Nov 18, 2025

View reviewed changes

bgpd/bgp_route.c Show resolved Hide resolved

hedrok force-pushed the 19506-bgp-multiple-labels branch from 4666f4d to 55358dd Compare November 18, 2025 21:38

mjstapp mentioned this pull request Nov 18, 2025

bgpd: bounds-check when parsing incoming label in nlri #20062

Merged

hedrok force-pushed the 19506-bgp-multiple-labels branch from 55358dd to fe68141 Compare November 18, 2025 22:11

mjstapp reviewed Nov 19, 2025

View reviewed changes

hedrok force-pushed the 19506-bgp-multiple-labels branch from fe68141 to bb14416 Compare November 19, 2025 21:25

hedrok requested a review from mjstapp November 19, 2025 21:32

hedrok force-pushed the 19506-bgp-multiple-labels branch from bb14416 to 5a5a172 Compare November 20, 2025 20:50

frrbot bot added bugfix tools labels Nov 20, 2025

hedrok force-pushed the 19506-bgp-multiple-labels branch 3 times, most recently from 24d211d to 910111f Compare November 21, 2025 05:03

github-actions bot added the conflicts label Nov 21, 2025

hedrok force-pushed the 19506-bgp-multiple-labels branch from 910111f to a6233a3 Compare November 21, 2025 16:08

github-actions bot removed the conflicts label Nov 21, 2025

mjstapp reviewed Nov 21, 2025

View reviewed changes

hedrok force-pushed the 19506-bgp-multiple-labels branch from a6233a3 to 74f496f Compare November 21, 2025 22:59

hedrok requested a review from mjstapp November 22, 2025 07:23

hedrok force-pushed the 19506-bgp-multiple-labels branch from 74f496f to 34a4e86 Compare November 24, 2025 17:41

hedrok added 2 commits November 24, 2025 19:44

tests: add test for multiple labels in BGP-LU

a2cdb46

Announce three routes with label stacks 777/10006, 90, 11/22/33/44/55, check that `show bgp ipv6 PREFIX json` has proper labels in `remoteLabels`. Test for issue FRRouting#19506 Signed-off-by: Kyrylo Yatsenko <hedrok@gmail.com>

hedrok force-pushed the 19506-bgp-multiple-labels branch from 34a4e86 to a2cdb46 Compare November 24, 2025 17:44

mjstapp reviewed Nov 25, 2025

View reviewed changes

bgpd/bgp_label.c Show resolved Hide resolved

mjstapp merged commit 6e2bd30 into FRRouting:master Nov 26, 2025
17 checks passed

hedrok deleted the 19506-bgp-multiple-labels branch November 26, 2025 14:09

hedrok mentioned this pull request Nov 27, 2025

lib, bgpd: Move BGP-specific funcs out of lib and refactor #20139

Merged


		sleep(5)

		stdout.write("announce route 2001:db8:100::/64 next-hop fc00::2 label [777 10006]\n")

bgp: Support multiple labels in BGP-LU #19961

bgp: Support multiple labels in BGP-LU #19961

Uh oh!

Conversation

hedrok commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ton31337 commented Nov 5, 2025

Uh oh!

donaldsharp commented Nov 5, 2025

Uh oh!

hedrok commented Nov 5, 2025

Uh oh!

hedrok commented Nov 6, 2025

Uh oh!

ton31337 commented Nov 7, 2025

Uh oh!

hedrok commented Nov 7, 2025

Uh oh!

github-actions bot commented Nov 7, 2025

Uh oh!

ritika0313 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hedrok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

hedrok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ton31337 left a comment

Choose a reason for hiding this comment

Uh oh!

ritika0313 left a comment

Choose a reason for hiding this comment

Uh oh!

hedrok commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmytroshytyi-6WIND left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedrok commented Nov 4, 2025 •

edited

Loading

ritika0313 left a comment •

edited

Loading

hedrok commented Nov 10, 2025 •

edited

Loading

hedrok Nov 19, 2025 •

edited

Loading