Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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

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
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

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

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
9 changes: 9 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r1/evpn_type3.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"101": {
"type": "L2",
"numRemoteVteps": 1,
"remoteVteps": [
"fd00:100::2"
]
}
}
15 changes: 15 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r1/frr.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
interface r1-eth0
ipv6 address fd00:100::1/64
!
router bgp 65000
bgp router-id 10.10.10.1
bgp log-neighbor-changes
no bgp default ipv4-unicast
neighbor fd00:100::2 remote-as 65000
neighbor fd00:100::2 capability extended-nexthop
!
address-family l2vpn evpn
neighbor fd00:100::2 activate
advertise-all-vni
exit-address-family
!
9 changes: 9 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r2/evpn_type3.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"101": {
"type": "L2",
"numRemoteVteps": 1,
"remoteVteps": [
"fd00:100::1"
]
}
}
15 changes: 15 additions & 0 deletions tests/topotests/bgp_evpn_v6_pmsi_tunnel/r2/frr.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
interface r2-eth0
ipv6 address fd00:100::2/64
!
router bgp 65000
bgp router-id 10.10.10.2
bgp log-neighbor-changes
no bgp default ipv4-unicast
neighbor fd00:100::1 remote-as 65000
neighbor fd00:100::1 capability extended-nexthop
!
address-family l2vpn evpn
neighbor fd00:100::1 activate
advertise-all-vni
exit-address-family
!
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
#!/usr/bin/env python
# SPDX-License-Identifier: ISC

#
# test_bgp_evpn_v6_pmsi_tunnel.py
#
# Copyright (c) 2025 Nvidia Inc.
# Donald Sharp

"""
Test that EVPN with an IPv6-only underlay correctly encodes and parses
the PMSI Tunnel attribute with an IPv6 tunnel identifier (length 21).

Type-3 IMET routes carry the PMSI Tunnel attribute, so this test verifies
that two FRR routers with IPv6-only EVPN peering and IPv6 VTEPs can
exchange type-3 routes and install remote VTEPs.
"""

import json
from functools import partial
import os
import sys
import pytest
import platform

CWD = os.path.dirname(os.path.realpath(__file__))
sys.path.append(os.path.join(CWD, "../"))

from lib import topotest
from lib.topogen import Topogen, get_topogen
from lib.topolog import logger

pytestmark = [pytest.mark.bgpd]


def build_topo(tgen):
tgen.add_router("r1")
tgen.add_router("r2")

switch = tgen.add_switch("s1")
switch.add_link(tgen.gears["r1"])
switch.add_link(tgen.gears["r2"])


def setup_module(mod):
tgen = Topogen(build_topo, mod.__name__)
tgen.start_topology()

krel = platform.release()
if topotest.version_cmp(krel, "4.18") < 0:
logger.info(
'BGP EVPN v6 PMSI tests require kernel >= 4.18 (have "{}")'.format(krel)
)
return pytest.skip("Kernel too old for EVPN NETNS test")

cmds_vxlan = [
"ip link add name bridge-101 up type bridge stp_state 0",
"ip link set dev bridge-101 up",
"ip link add name vxlan-101 type vxlan id 101 dstport 4789 dev {0}-eth0 local {1}",
"ip link set dev vxlan-101 master bridge-101",
"ip link set vxlan-101 up type bridge_slave learning off flood off mcast_flood off",
]

for rname, vtep_ip in [("r1", "fd00:100::1"), ("r2", "fd00:100::2")]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you add test for iPv4?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

there are already about 5-10 different topotests that show v4 behavior, I don't think I need to write a v4 specific test. I just wanted to ahve a new test that had absolutely no v4 addresses at all.

router = tgen.gears[rname]
for cmd in cmds_vxlan:
formatted = cmd.format(rname, vtep_ip)
logger.info("cmd to {}: {}".format(rname, formatted))
output = router.cmd_raises(formatted)
logger.info("result: " + output)

for rname, router in tgen.routers().items():
logger.info("Loading router %s" % rname)
router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname)))

tgen.start_router()


def teardown_module(_mod):
tgen = get_topogen()
tgen.stop_topology()


def test_bgp_convergence():
"""
Assert that BGP EVPN sessions come up between r1 and r2 over IPv6.
"""
tgen = get_topogen()
if tgen.routers_have_failure():
pytest.skip(tgen.errors)

for rname in ("r1", "r2"):
router = tgen.gears[rname]

expected = {"peers": {}}
if rname == "r1":
expected["peers"]["fd00:100::2"] = {"state": "Established"}
else:
expected["peers"]["fd00:100::1"] = {"state": "Established"}

test_func = partial(
topotest.router_json_cmp,
router,
"show bgp l2vpn evpn summary json",
expected,
)
_, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
assert result is None, '"{}" BGP session did not establish'.format(rname)


def test_evpn_type3_routes():
"""
Verify EVPN type-3 IMET routes are received. These carry the PMSI Tunnel
attribute. With IPv6-only peering, the PMSI tunnel identifier must be
encoded as 21 bytes (IPv6). Without the fix, these routes would be
rejected as malformed.
"""
tgen = get_topogen()
if tgen.routers_have_failure():
pytest.skip(tgen.errors)

for rname in ("r1", "r2"):
router = tgen.gears[rname]
json_file = "{}/{}/evpn_type3.json".format(CWD, rname)
expected = json.loads(open(json_file).read())

test_func = partial(
topotest.router_json_cmp,
router,
"show evpn vni json",
expected,
)
_, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
assert result is None, '"{}" EVPN type-3 route check failed'.format(rname)


def test_memory_leak():
tgen = get_topogen()
if not tgen.is_memleak_enabled():
pytest.skip("Memory leak test/report is disabled")

tgen.report_memory_leaks()


if __name__ == "__main__":
args = ["-s"] + sys.argv[1:]
sys.exit(pytest.main(args))
Loading