Skip to content

Pmsi tunnel v6 length #21488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
donaldsharp wants to merge 3 commits into from
Closed

Pmsi tunnel v6 length #21488

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c775dc0
bgpd: Actually write the pmsi tunnel length based upon v4 or v6
donaldsharp Apr 9, 2026
d29fb11
bgpd: Ensure backwards compatability with older versions of FRR
donaldsharp Apr 9, 2026
6bb1602
tests: Add a v6 pmsi tunnel only test.
donaldsharp Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions bgpd/bgp_attr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3773,7 +3773,8 @@ bgp_attr_pmsi_tunnel(struct bgp_attr_parser_args *args)
args->total);
}
if (tnl_type == PMSI_TNLTYPE_INGR_REPL) {
if (length != 9) {
if (length != BGP_ATTR_PMSI_TUNNEL_V4_LENGTH &&
length != BGP_ATTR_PMSI_TUNNEL_V6_LENGTH) {
flog_err(EC_BGP_ATTR_PMSI_LEN,
"Bad PMSI tunnel attribute length %d for IR",
length);
Expand Down Expand Up @@ -5654,13 +5655,22 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, struct strea
if (bgp_attr_exists(attr, BGP_ATTR_PMSI_TUNNEL)) {
stream_putc(s, BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS);
stream_putc(s, BGP_ATTR_PMSI_TUNNEL);
stream_putc(s, 9); // Length
stream_putc(s, 0); // Flags
stream_putc(s, bgp_attr_get_pmsi_tnl_type(attr));
stream_put(s, &(attr->label),
BGP_LABEL_BYTES); // MPLS Label / VXLAN VNI
stream_put_ipv4(s, attr->nexthop.s_addr);
// Unicast tunnel endpoint IP address
if (attr->nexthop.s_addr == INADDR_ANY &&
Copy link
Copy Markdown
Member

@ton31337 ton31337 Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check for the attribute flag (bgp_attr_exists()) instead?

Copy link
Copy Markdown
Member Author

@donaldsharp donaldsharp Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so from a quick scan of the source code the attr->mp_nexthop_len is just checked like I do here, the same with attr->nexthop. So I was just following coding conventions. If you think we should switch to a different way of doing it, that is fine but I think it belongs in it's own cleanup work since we would need to touch a bunch of different spots.

(attr->mp_nexthop_len == BGP_ATTR_NHLEN_IPV6_GLOBAL ||
attr->mp_nexthop_len == BGP_ATTR_NHLEN_IPV6_GLOBAL_AND_LL)) {
stream_putc(s, BGP_ATTR_PMSI_TUNNEL_V6_LENGTH);
stream_putc(s, 0);
stream_putc(s, bgp_attr_get_pmsi_tnl_type(attr));
stream_put(s, &attr->label, BGP_LABEL_BYTES);
stream_put(s, &attr->mp_nexthop_global, 16);
} else {
stream_putc(s, BGP_ATTR_PMSI_TUNNEL_V4_LENGTH); // Length
stream_putc(s, 0); // Flags
stream_putc(s, bgp_attr_get_pmsi_tnl_type(attr));
stream_put(s, &(attr->label),
BGP_LABEL_BYTES); // MPLS Label / VXLAN VNI
stream_put_ipv4(s, attr->nexthop.s_addr);
}
}

/* OTC */
Expand Down
3 changes: 3 additions & 0 deletions bgpd/bgp_attr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ enum pta_type {
PMSI_TNLTYPE_MAX = PMSI_TNLTYPE_MLDP_MP2MP
};

#define BGP_ATTR_PMSI_TUNNEL_V4_LENGTH 9
#define BGP_ATTR_PMSI_TUNNEL_V6_LENGTH 21

Copy link
Copy Markdown
Contributor

@nick-bouliane nick-bouliane Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lengths look specific to PMSI_TNLTYPE_INGR_REPL. Are they valid for other PMSI tunnel types too? If not, should the constant names include INGR_REPL to make that scope explicit?

Copy link
Copy Markdown
Member Author

@donaldsharp donaldsharp Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From some very superficial reading of the RFC's associated here (rfc6514 and rfc7988) I believe that it applies to a bunch of pmsi tunnel types but since we do not have those programmed yet I did not do anything other than make the generic tunnel lengths. Hopefully someone will program the other types in the near future

/*
* Prefix-SID type-4
* SRv6-VPN-SID-TLV
Expand Down