From 229757f1950eaf19ec86335702aa31f934bcd9f6 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 12 Feb 2020 21:19:02 +0200 Subject: [PATCH 1/3] bgpd: Allow overriding ORIGIN for aggregate-address Override ORIGIN attribute if defined. E.g.: Cisco and Juniper set ORIGIN for aggregated address to IGP which is not what rfc4271 says. This enables the same behavior, optionally. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 126 ++++++++++++++++++++++++++++++++++++++++------- bgpd/bgp_route.h | 3 ++ bgpd/bgpd.h | 1 + 3 files changed, 112 insertions(+), 18 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 34580788bd..18bb1b9f7b 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6270,6 +6270,9 @@ void bgp_aggregate_route(struct bgp *bgp, struct prefix *p, else if (aggregate->egp_origin_count > 0) origin = BGP_ORIGIN_EGP; + if (aggregate->origin != BGP_ORIGIN_UNSPECIFIED) + origin = aggregate->origin; + if (aggregate->as_set) { if (aggregate->aspath) /* Retrieve aggregate route's as-path. @@ -6434,6 +6437,9 @@ static void bgp_add_route_to_aggregate(struct bgp *bgp, struct prefix *aggr_p, else if (aggregate->egp_origin_count > 0) origin = BGP_ORIGIN_EGP; + if (aggregate->origin != BGP_ORIGIN_UNSPECIFIED) + origin = aggregate->origin; + if (aggregate->as_set) { /* Compute aggregate route's as-path. */ @@ -6565,6 +6571,9 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, else if (aggregate->egp_origin_count > 0) origin = BGP_ORIGIN_EGP; + if (aggregate->origin != BGP_ORIGIN_UNSPECIFIED) + origin = aggregate->origin; + if (aggregate->as_set) { /* Retrieve aggregate route's as-path. */ @@ -6660,6 +6669,19 @@ void bgp_aggregate_decrement(struct bgp *bgp, struct prefix *p, #define AGGREGATE_AS_SET 1 #define AGGREGATE_AS_UNSET 0 +static const char *bgp_origin2str(uint8_t origin) +{ + switch (origin) { + case BGP_ORIGIN_IGP: + return "igp"; + case BGP_ORIGIN_EGP: + return "egp"; + case BGP_ORIGIN_INCOMPLETE: + return "incomplete"; + } + return "n/a"; +} + static int bgp_aggregate_unset(struct vty *vty, const char *prefix_str, afi_t afi, safi_t safi) { @@ -6753,8 +6775,9 @@ static int bgp_aggregate_unset(struct vty *vty, const char *prefix_str, } static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, - safi_t safi, const char *rmap, uint8_t summary_only, - uint8_t as_set) + safi_t safi, const char *rmap, + uint8_t summary_only, uint8_t as_set, + uint8_t origin) { VTY_DECLVAR_CONTEXT(bgp, bgp); int ret; @@ -6818,6 +6841,12 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, aggregate->as_set = as_set_new; aggregate->safi = safi; + /* Override ORIGIN attribute if defined. + * E.g.: Cisco and Juniper set ORIGIN for aggregated address + * to IGP which is not what rfc4271 says. + * This enables the same behavior, optionally. + */ + aggregate->origin = origin; if (rmap) { XFREE(MTYPE_ROUTE_MAP_NAME, aggregate->rmap.name); @@ -6837,7 +6866,7 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, DEFUN (aggregate_address, aggregate_address_cmd, - "aggregate-address A.B.C.D/M [] [route-map WORD]", + "aggregate-address A.B.C.D/M [] [route-map WORD] [origin ]", "Configure BGP aggregate entries\n" "Aggregate prefix\n" "Generate AS set path information\n" @@ -6845,12 +6874,17 @@ DEFUN (aggregate_address, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "A.B.C.D/M", &idx); char *prefix = argv[idx]->arg; char *rmap = NULL; + uint8_t origin = BGP_ORIGIN_UNSPECIFIED; int as_set = argv_find(argv, argc, "as-set", &idx) ? AGGREGATE_AS_SET : AGGREGATE_AS_UNSET; idx = 0; @@ -6863,13 +6897,23 @@ DEFUN (aggregate_address, if (idx) rmap = argv[idx]->arg; - return bgp_aggregate_set(vty, prefix, AFI_IP, bgp_node_safi(vty), - rmap, summary_only, as_set); + idx = 0; + if (argv_find(argv, argc, "origin", &idx)) { + if (strncmp(argv[idx + 1]->arg, "igp", 2) == 0) + origin = BGP_ORIGIN_IGP; + if (strncmp(argv[idx + 1]->arg, "egp", 1) == 0) + origin = BGP_ORIGIN_EGP; + if (strncmp(argv[idx + 1]->arg, "incomplete", 2) == 0) + origin = BGP_ORIGIN_INCOMPLETE; + } + + return bgp_aggregate_set(vty, prefix, AFI_IP, bgp_node_safi(vty), rmap, + summary_only, as_set, origin); } DEFUN (aggregate_address_mask, aggregate_address_mask_cmd, - "aggregate-address A.B.C.D A.B.C.D [] [route-map WORD]", + "aggregate-address A.B.C.D A.B.C.D [] [route-map WORD] [origin ]", "Configure BGP aggregate entries\n" "Aggregate address\n" "Aggregate mask\n" @@ -6878,7 +6922,11 @@ DEFUN (aggregate_address_mask, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "A.B.C.D", &idx); @@ -6886,6 +6934,7 @@ DEFUN (aggregate_address_mask, char *mask = argv[idx + 1]->arg; bool rmap_found; char *rmap = NULL; + uint8_t origin = BGP_ORIGIN_UNSPECIFIED; int as_set = argv_find(argv, argc, "as-set", &idx) ? AGGREGATE_AS_SET : AGGREGATE_AS_UNSET; idx = 0; @@ -6905,13 +6954,23 @@ DEFUN (aggregate_address_mask, return CMD_WARNING_CONFIG_FAILED; } + idx = 0; + if (argv_find(argv, argc, "origin", &idx)) { + if (strncmp(argv[idx + 1]->arg, "igp", 2) == 0) + origin = BGP_ORIGIN_IGP; + if (strncmp(argv[idx + 1]->arg, "egp", 1) == 0) + origin = BGP_ORIGIN_EGP; + if (strncmp(argv[idx + 1]->arg, "incomplete", 2) == 0) + origin = BGP_ORIGIN_INCOMPLETE; + } + return bgp_aggregate_set(vty, prefix_str, AFI_IP, bgp_node_safi(vty), - rmap, summary_only, as_set); + rmap, summary_only, as_set, origin); } DEFUN (no_aggregate_address, no_aggregate_address_cmd, - "no aggregate-address A.B.C.D/M [] [route-map WORD]", + "no aggregate-address A.B.C.D/M [] [route-map WORD] [origin ]", NO_STR "Configure BGP aggregate entries\n" "Aggregate prefix\n" @@ -6920,7 +6979,11 @@ DEFUN (no_aggregate_address, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "A.B.C.D/M", &idx); @@ -6930,7 +6993,7 @@ DEFUN (no_aggregate_address, DEFUN (no_aggregate_address_mask, no_aggregate_address_mask_cmd, - "no aggregate-address A.B.C.D A.B.C.D [] [route-map WORD]", + "no aggregate-address A.B.C.D A.B.C.D [] [route-map WORD] [origin ]", NO_STR "Configure BGP aggregate entries\n" "Aggregate address\n" @@ -6940,7 +7003,11 @@ DEFUN (no_aggregate_address_mask, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "A.B.C.D", &idx); @@ -6960,7 +7027,7 @@ DEFUN (no_aggregate_address_mask, DEFUN (ipv6_aggregate_address, ipv6_aggregate_address_cmd, - "aggregate-address X:X::X:X/M [] [route-map WORD]", + "aggregate-address X:X::X:X/M [] [route-map WORD] [origin ]", "Configure BGP aggregate entries\n" "Aggregate prefix\n" "Generate AS set path information\n" @@ -6968,13 +7035,18 @@ DEFUN (ipv6_aggregate_address, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "X:X::X:X/M", &idx); char *prefix = argv[idx]->arg; char *rmap = NULL; bool rmap_found; + uint8_t origin = BGP_ORIGIN_UNSPECIFIED; int as_set = argv_find(argv, argc, "as-set", &idx) ? AGGREGATE_AS_SET : AGGREGATE_AS_UNSET; @@ -6987,13 +7059,23 @@ DEFUN (ipv6_aggregate_address, if (rmap_found) rmap = argv[idx]->arg; + idx = 0; + if (argv_find(argv, argc, "origin", &idx)) { + if (strncmp(argv[idx + 1]->arg, "igp", 2) == 0) + origin = BGP_ORIGIN_IGP; + if (strncmp(argv[idx + 1]->arg, "egp", 1) == 0) + origin = BGP_ORIGIN_EGP; + if (strncmp(argv[idx + 1]->arg, "incomplete", 2) == 0) + origin = BGP_ORIGIN_INCOMPLETE; + } + return bgp_aggregate_set(vty, prefix, AFI_IP6, SAFI_UNICAST, rmap, - sum_only, as_set); + sum_only, as_set, origin); } DEFUN (no_ipv6_aggregate_address, no_ipv6_aggregate_address_cmd, - "no aggregate-address X:X::X:X/M [] [route-map WORD]", + "no aggregate-address X:X::X:X/M [] [route-map WORD] [origin ]", NO_STR "Configure BGP aggregate entries\n" "Aggregate prefix\n" @@ -7002,7 +7084,11 @@ DEFUN (no_ipv6_aggregate_address, "Filter more specific routes from updates\n" "Generate AS set path information\n" "Apply route map to aggregate network\n" - "Name of route map\n") + "Name of route map\n" + "BGP origin code\n" + "Remote EGP\n" + "Local IGP\n" + "Unknown heritage\n") { int idx = 0; argv_find(argv, argc, "X:X::X:X/M", &idx); @@ -12938,6 +13024,10 @@ void bgp_config_write_network(struct vty *vty, struct bgp *bgp, afi_t afi, if (bgp_aggregate->rmap.name) vty_out(vty, " route-map %s", bgp_aggregate->rmap.name); + if (bgp_aggregate->origin != BGP_ORIGIN_UNSPECIFIED) + vty_out(vty, " origin %s", + bgp_origin2str(bgp_aggregate->origin)); + vty_out(vty, "\n"); } } diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 8f31cd38dc..0ad656d133 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -338,6 +338,9 @@ struct bgp_aggregate { /* Count of routes of origin type egp under this aggregate. */ unsigned long egp_origin_count; + /* Optional modify flag to override ORIGIN */ + uint8_t origin; + /* Hash containing the communities of all the * routes under this aggregate. */ diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 548dfe4683..5f0a2d370d 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1489,6 +1489,7 @@ struct bgp_nlri { #define BGP_ORIGIN_IGP 0 #define BGP_ORIGIN_EGP 1 #define BGP_ORIGIN_INCOMPLETE 2 +#define BGP_ORIGIN_UNSPECIFIED 255 /* BGP notify message codes. */ #define BGP_NOTIFY_HEADER_ERR 1 From 561137b0e135fd04efd539664bf4d789a041523c Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 12 Feb 2020 22:32:55 +0200 Subject: [PATCH 2/3] tests: Add test case for `aggregate-address origin ` Signed-off-by: Donatas Abraitis --- .../bgp_aggregate-address_origin/__init__.py | 0 .../bgp_aggregate-address_origin/r1/bgpd.conf | 7 + .../r1/zebra.conf | 9 ++ .../bgp_aggregate-address_origin/r2/bgpd.conf | 4 + .../r2/zebra.conf | 6 + .../test_bgp_aggregate-address_origin.py | 128 ++++++++++++++++++ 6 files changed, 154 insertions(+) create mode 100644 tests/topotests/bgp_aggregate-address_origin/__init__.py create mode 100644 tests/topotests/bgp_aggregate-address_origin/r1/bgpd.conf create mode 100644 tests/topotests/bgp_aggregate-address_origin/r1/zebra.conf create mode 100644 tests/topotests/bgp_aggregate-address_origin/r2/bgpd.conf create mode 100644 tests/topotests/bgp_aggregate-address_origin/r2/zebra.conf create mode 100644 tests/topotests/bgp_aggregate-address_origin/test_bgp_aggregate-address_origin.py diff --git a/tests/topotests/bgp_aggregate-address_origin/__init__.py b/tests/topotests/bgp_aggregate-address_origin/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_aggregate-address_origin/r1/bgpd.conf b/tests/topotests/bgp_aggregate-address_origin/r1/bgpd.conf new file mode 100644 index 0000000000..528d02af36 --- /dev/null +++ b/tests/topotests/bgp_aggregate-address_origin/r1/bgpd.conf @@ -0,0 +1,7 @@ +router bgp 65000 + neighbor 192.168.255.2 remote-as 65001 + address-family ipv4 unicast + redistribute connected + aggregate-address 172.16.255.0/24 origin igp + exit-address-family +! diff --git a/tests/topotests/bgp_aggregate-address_origin/r1/zebra.conf b/tests/topotests/bgp_aggregate-address_origin/r1/zebra.conf new file mode 100644 index 0000000000..0a283c06d5 --- /dev/null +++ b/tests/topotests/bgp_aggregate-address_origin/r1/zebra.conf @@ -0,0 +1,9 @@ +! +interface lo + ip address 172.16.255.254/32 +! +interface r1-eth0 + ip address 192.168.255.1/24 +! +ip forwarding +! diff --git a/tests/topotests/bgp_aggregate-address_origin/r2/bgpd.conf b/tests/topotests/bgp_aggregate-address_origin/r2/bgpd.conf new file mode 100644 index 0000000000..73d4d0aeea --- /dev/null +++ b/tests/topotests/bgp_aggregate-address_origin/r2/bgpd.conf @@ -0,0 +1,4 @@ +router bgp 65001 + neighbor 192.168.255.1 remote-as 65000 + exit-address-family +! diff --git a/tests/topotests/bgp_aggregate-address_origin/r2/zebra.conf b/tests/topotests/bgp_aggregate-address_origin/r2/zebra.conf new file mode 100644 index 0000000000..606c17bec9 --- /dev/null +++ b/tests/topotests/bgp_aggregate-address_origin/r2/zebra.conf @@ -0,0 +1,6 @@ +! +interface r2-eth0 + ip address 192.168.255.2/24 +! +ip forwarding +! diff --git a/tests/topotests/bgp_aggregate-address_origin/test_bgp_aggregate-address_origin.py b/tests/topotests/bgp_aggregate-address_origin/test_bgp_aggregate-address_origin.py new file mode 100644 index 0000000000..be29d143dd --- /dev/null +++ b/tests/topotests/bgp_aggregate-address_origin/test_bgp_aggregate-address_origin.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python + +# +# bgp_aggregate-address_origin.py +# Part of NetDEF Topology Tests +# +# Copyright (c) 2020 by +# Donatas Abraitis +# +# Permission to use, copy, modify, and/or distribute this software +# for any purpose with or without fee is hereby granted, provided +# that the above copyright notice and this permission notice appear +# in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND NETDEF DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL NETDEF BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY +# DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, +# WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS +# ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE +# OF THIS SOFTWARE. +# + +""" +bgp_aggregate-address_origin.py: + +Test if works the following commands: +router bgp 65031 + address-family ipv4 unicast + aggregate-address 192.168.255.0/24 origin igp +""" + +import os +import sys +import json +import time +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, '../')) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, TopoRouter, get_topogen +from lib.topolog import logger +from mininet.topo import Topo + +class TemplateTopo(Topo): + def build(self, *_args, **_opts): + tgen = get_topogen(self) + + for routern in range(1, 3): + tgen.add_router('r{}'.format(routern)) + + switch = tgen.add_switch('s1') + switch.add_link(tgen.gears['r1']) + switch.add_link(tgen.gears['r2']) + +def setup_module(mod): + tgen = Topogen(TemplateTopo, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + + for i, (rname, router) in enumerate(router_list.iteritems(), 1): + router.load_config( + TopoRouter.RD_ZEBRA, + os.path.join(CWD, '{}/zebra.conf'.format(rname)) + ) + router.load_config( + TopoRouter.RD_BGP, + os.path.join(CWD, '{}/bgpd.conf'.format(rname)) + ) + + tgen.start_router() + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + +def test_bgp_aggregate_address_origin(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + router = tgen.gears['r2'] + + def _bgp_converge(router): + output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.1 json")) + expected = { + '192.168.255.1': { + 'bgpState': 'Established', + 'addressFamilyInfo': { + 'ipv4Unicast': { + 'acceptedPrefixCounter': 3 + } + } + } + } + return topotest.json_cmp(output, expected) + + def _bgp_aggregate_address_has_metric(router): + output = json.loads(router.vtysh_cmd("show ip bgp 172.16.255.0/24 json")) + expected = { + 'paths': [ + { + 'origin': 'IGP' + } + ] + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial(_bgp_converge, router) + success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + + assert result is None, 'Failed to see bgp convergence in "{}"'.format(router) + + test_func = functools.partial(_bgp_aggregate_address_has_metric, router) + success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + + assert result is None, 'Failed to see applied ORIGIN (igp) for aggregated prefix in "{}"'.format(router) + +if __name__ == '__main__': + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From a87d2ef7ec934509c7ab0bb6588f201dff5fcf5f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 12 Feb 2020 22:53:03 +0200 Subject: [PATCH 3/3] doc: Override ORIGIN for aggregate-address command Signed-off-by: Donatas Abraitis --- doc/user/bgp.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index bacb69b815..38ed78eb86 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -924,6 +924,11 @@ Route Aggregation-IPv4 Address Family Apply a route-map for an aggregated prefix. +.. index:: aggregate-address A.B.C.D/M origin +.. clicmd:: aggregate-address A.B.C.D/M origin + + Override ORIGIN for an aggregated prefix. + .. index:: aggregate-address A.B.C.D/M as-set .. clicmd:: aggregate-address A.B.C.D/M as-set @@ -971,6 +976,11 @@ Route Aggregation-IPv6 Address Family Apply a route-map for an aggregated prefix. +.. index:: aggregate-address X:X::X:X/M origin +.. clicmd:: aggregate-address X:X::X:X/M origin + + Override ORIGIN for an aggregated prefix. + .. index:: aggregate-address X:X::X:X/M as-set .. clicmd:: aggregate-address X:X::X:X/M as-set