From 73557d68b1cb54714fc0c3652a9ab29559da00ee Mon Sep 17 00:00:00 2001 From: Christian Poessinger Date: Mon, 29 Mar 2021 18:30:28 +0200 Subject: bgp: T1711: remove ASN tagNode and move to "local-as" Every time when set configuration bgp, you need set AS number. There is very less benefit in this system so the AS number is moved from a tagNode level down to a leafNode with the name "local-as", same as on the neighbor or peer-group level. This changes the CLI configuration from: set protocols bgp 100 neighbor 10.10.1.2 remote-as 200 to set protocols bgp local-as 100 set protocols bgp neighbor 10.10.1.2 remote-as 200 --- data/templates/frr/bgp.frr.tmpl | 2 +- .../include/bgp/bgp-common-config.xml.i | 16 +- interface-definitions/protocols-bgp.xml.in | 11 +- interface-definitions/vrf.xml.in | 11 +- smoketest/scripts/cli/test_protocols_bgp.py | 27 ++- src/conf_mode/protocols_bgp.py | 211 ++++++++++----------- src/migration-scripts/bgp/0-to-1 | 61 ++++++ 7 files changed, 199 insertions(+), 140 deletions(-) create mode 100755 src/migration-scripts/bgp/0-to-1 diff --git a/data/templates/frr/bgp.frr.tmpl b/data/templates/frr/bgp.frr.tmpl index 30e1ec082..53e62928b 100644 --- a/data/templates/frr/bgp.frr.tmpl +++ b/data/templates/frr/bgp.frr.tmpl @@ -185,7 +185,7 @@ {% endif %} {% endmacro %} ! -router bgp {{ asn }} {{ 'vrf ' + vrf if vrf is defined and vrf is not none }} +router bgp {{ local_as }} {{ 'vrf ' + vrf if vrf is defined and vrf is not none }} {% if parameters is defined and parameters.ebgp_requires_policy is defined %} bgp ebgp-requires-policy {% else %} diff --git a/interface-definitions/include/bgp/bgp-common-config.xml.i b/interface-definitions/include/bgp/bgp-common-config.xml.i index ae0e8b178..c89e2288e 100644 --- a/interface-definitions/include/bgp/bgp-common-config.xml.i +++ b/interface-definitions/include/bgp/bgp-common-config.xml.i @@ -280,7 +280,7 @@ - Listen for and accept BGP dynamic neighbors from range + BGP dynamic neighbors listen commands @@ -297,7 +297,7 @@ - BGP dynamic neighbors listen range + Dynamic neighbors listen range ipv4net IPv4 dynamic neighbors listen range @@ -317,6 +317,18 @@ + + + Autonomous System Number (ASN) + + u32:1-4294967294 + Autonomous System Number + + + + + + BGP neighbor diff --git a/interface-definitions/protocols-bgp.xml.in b/interface-definitions/protocols-bgp.xml.in index cf897d04f..d610f8dff 100644 --- a/interface-definitions/protocols-bgp.xml.in +++ b/interface-definitions/protocols-bgp.xml.in @@ -2,22 +2,15 @@ - + Border Gateway Protocol (BGP) 820 - - u32:1-4294967294 - Autonomous System Number - - - - #include - + diff --git a/interface-definitions/vrf.xml.in b/interface-definitions/vrf.xml.in index 7e5765c54..8a56b1bc0 100644 --- a/interface-definitions/vrf.xml.in +++ b/interface-definitions/vrf.xml.in @@ -33,22 +33,15 @@ Routing protocol parameters - + Border Gateway Protocol (BGP) 821 - - u32:1-4294967294 - Autonomous System Number - - - - #include - + Intermediate System to Intermediate System (IS-IS) diff --git a/smoketest/scripts/cli/test_protocols_bgp.py b/smoketest/scripts/cli/test_protocols_bgp.py index be99a0960..4f39948c0 100755 --- a/smoketest/scripts/cli/test_protocols_bgp.py +++ b/smoketest/scripts/cli/test_protocols_bgp.py @@ -24,7 +24,7 @@ from vyos.util import process_named_running PROCESS_NAME = 'bgpd' ASN = '64512' -base_path = ['protocols', 'bgp', ASN] +base_path = ['protocols', 'bgp'] route_map_in = 'foo-map-in' route_map_out = 'foo-map-out' @@ -213,6 +213,12 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): self.cli_set(base_path + ['parameters', 'router-id', router_id]) self.cli_set(base_path + ['parameters', 'log-neighbor-changes']) + + # Local AS number MUST be defined + with self.assertRaises(ConfigSessionError): + self.cli_commit() + self.cli_set(base_path + ['local-as', ASN]) + # Default local preference (higher = more preferred, default value is 100) self.cli_set(base_path + ['parameters', 'default', 'local-pref', local_pref]) # Deactivate IPv4 unicast for a peer by default @@ -251,6 +257,7 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): def test_bgp_02_neighbors(self): + self.cli_set(base_path + ['local-as', ASN]) # Test out individual neighbor configuration items, not all of them are # also available to a peer-group! for peer, peer_config in neighbor_config.items(): @@ -325,6 +332,7 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): self.verify_frr_config(peer, peer_config, frrconfig) def test_bgp_03_peer_groups(self): + self.cli_set(base_path + ['local-as', ASN]) # Test out individual peer-group configuration items for peer_group, config in peer_group_config.items(): if 'cap_dynamic' in config: @@ -395,6 +403,8 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): }, } + self.cli_set(base_path + ['local-as', ASN]) + # We want to redistribute ... redistributes = ['connected', 'isis', 'kernel', 'ospf', 'rip', 'static'] for redistribute in redistributes: @@ -441,6 +451,8 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): }, } + self.cli_set(base_path + ['local-as', ASN]) + # We want to redistribute ... redistributes = ['connected', 'kernel', 'ospfv3', 'ripng', 'static'] for redistribute in redistributes: @@ -482,7 +494,10 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): limit = '64' listen_ranges = ['192.0.2.0/25', '192.0.2.128/25'] peer_group = 'listenfoobar' + + self.cli_set(base_path + ['local-as', ASN]) self.cli_set(base_path + ['listen', 'limit', limit]) + for prefix in listen_ranges: self.cli_set(base_path + ['listen', 'range', prefix]) # check validate() - peer-group must be defined for range/prefix @@ -511,6 +526,9 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): def test_bgp_07_l2vpn_evpn(self): vnis = ['10010', '10020', '10030'] neighbors = ['192.0.2.10', '192.0.2.20', '192.0.2.30'] + + self.cli_set(base_path + ['local-as', ASN]) + self.cli_set(base_path + ['address-family', 'l2vpn-evpn', 'advertise-all-vni']) self.cli_set(base_path + ['address-family', 'l2vpn-evpn', 'advertise-default-gw']) self.cli_set(base_path + ['address-family', 'l2vpn-evpn', 'advertise-svi-ip']) @@ -539,14 +557,17 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): def test_bgp_08_vrf_simple(self): router_id = '127.0.0.3' vrfs = ['red', 'green', 'blue'] + # It is safe to assume that when the basic VRF test works, all - # other OSPF related features work, as we entirely inherit the CLI + # other BGP related features work, as we entirely inherit the CLI # templates and Jinja2 FRR template. table = '1000' + for vrf in vrfs: vrf_base = ['vrf', 'name', vrf] self.cli_set(vrf_base + ['table', table]) - self.cli_set(vrf_base + ['protocols', 'bgp', ASN, 'parameters', 'router-id', router_id]) + self.cli_set(vrf_base + ['protocols', 'bgp', 'local-as', ASN]) + self.cli_set(vrf_base + ['protocols', 'bgp', 'parameters', 'router-id', router_id]) table = str(int(table) + 1000) self.cli_commit() diff --git a/src/conf_mode/protocols_bgp.py b/src/conf_mode/protocols_bgp.py index 2f60795c1..6770865ff 100755 --- a/src/conf_mode/protocols_bgp.py +++ b/src/conf_mode/protocols_bgp.py @@ -62,21 +62,18 @@ def get_config(config=None): # prefix-lists and route-maps for instance. base = ['policy'] tmp = conf.get_config_dict(base, key_mangling=('-', '_')) - # As we only support one ASN (later checked in begin of verify()) we add the - # new information only to the first AS number - asn = next(iter(bgp)) # Merge policy dict into bgp dict - bgp[asn] = dict_merge(tmp, bgp[asn]) + bgp = dict_merge(tmp, bgp) return bgp -def verify_remote_as(peer_config, asn_config): +def verify_remote_as(peer_config, bgp_config): if 'remote_as' in peer_config: return peer_config['remote_as'] if 'peer_group' in peer_config: peer_group_name = peer_config['peer_group'] - tmp = dict_search(f'peer_group.{peer_group_name}.remote_as', asn_config) + tmp = dict_search(f'peer_group.{peer_group_name}.remote_as', bgp_config) if tmp: return tmp if 'interface' in peer_config: @@ -85,117 +82,106 @@ def verify_remote_as(peer_config, asn_config): if 'peer_group' in peer_config['interface']: peer_group_name = peer_config['interface']['peer_group'] - tmp = dict_search(f'peer_group.{peer_group_name}.remote_as', asn_config) + tmp = dict_search(f'peer_group.{peer_group_name}.remote_as', bgp_config) if tmp: return tmp return None def verify(bgp): - if not bgp: + if not bgp or 'deleted' in bgp: return None - # FRR bgpd only supports one Autonomous System Number, verify this! - asn = 0 - for key in bgp: - if key.isnumeric(): - asn +=1 - if asn > 1: - raise ConfigError('Only one BGP AS number can be defined!') - - for asn, asn_config in bgp.items(): - # Workaround for https://phabricator.vyos.net/T1711 - # We also have a vrf, and deleted key now - so we can only veriy "numbers" - if not asn.isnumeric(): + if 'local_as' not in bgp: + raise ConfigError('BGP local-as number must be defined!') + + # Common verification for both peer-group and neighbor statements + for neighbor in ['neighbor', 'peer_group']: + # bail out early if there is no neighbor or peer-group statement + # this also saves one indention level + if neighbor not in bgp: continue - # Common verification for both peer-group and neighbor statements - for neighbor in ['neighbor', 'peer_group']: - # bail out early if there is no neighbor or peer-group statement - # this also saves one indention level - if neighbor not in asn_config: - continue - - for peer, peer_config in asn_config[neighbor].items(): - # Only regular "neighbor" statement can have a peer-group set - # Check if the configure peer-group exists - if 'peer_group' in peer_config: - peer_group = peer_config['peer_group'] - if 'peer_group' not in asn_config or peer_group not in asn_config['peer_group']: - raise ConfigError(f'Specified peer-group "{peer_group}" for '\ - f'neighbor "{neighbor}" does not exist!') - - # ttl-security and ebgp-multihop can't be used in the same configration - if 'ebgp_multihop' in peer_config and 'ttl_security' in peer_config: - raise ConfigError('You can\'t set both ebgp-multihop and ttl-security hops') - - # Check spaces in the password - if 'password' in peer_config and ' ' in peer_config['password']: - raise ConfigError('You can\'t use spaces in the password') - - # Some checks can/must only be done on a neighbor and not a peer-group - if neighbor == 'neighbor': - # remote-as must be either set explicitly for the neighbor - # or for the entire peer-group - if not verify_remote_as(peer_config, asn_config): - raise ConfigError(f'Neighbor "{peer}" remote-as must be set!') - - # Only checks for ipv4 and ipv6 neighbors - # Check if neighbor address is assigned as system interface address - if is_ip(peer) and is_addr_assigned(peer): - raise ConfigError(f'Can\'t configure local address as neighbor "{peer}"') - - for afi in ['ipv4_unicast', 'ipv6_unicast', 'l2vpn_evpn']: - # Bail out early if address family is not configured - if 'address_family' not in peer_config or afi not in peer_config['address_family']: - continue - - afi_config = peer_config['address_family'][afi] - # Validate if configured Prefix list exists - if 'prefix_list' in afi_config: - for tmp in ['import', 'export']: - if tmp not in afi_config['prefix_list']: - # bail out early - continue - # get_config_dict() mangles all '-' characters to '_' this is legitimate, thus all our - # compares will run on '_' as also '_' is a valid name for a prefix-list - prefix_list = afi_config['prefix_list'][tmp].replace('-', '_') - if afi == 'ipv4_unicast': - if dict_search(f'policy.prefix_list.{prefix_list}', asn_config) == None: - raise ConfigError(f'prefix-list "{prefix_list}" used for "{tmp}" does not exist!') - elif afi == 'ipv6_unicast': - if dict_search(f'policy.prefix_list6.{prefix_list}', asn_config) == None: - raise ConfigError(f'prefix-list6 "{prefix_list}" used for "{tmp}" does not exist!') - - if 'route_map' in afi_config: - for tmp in ['import', 'export']: - if tmp in afi_config['route_map']: - # get_config_dict() mangles all '-' characters to '_' this is legitim, thus all our - # compares will run on '_' as also '_' is a valid name for a route-map - route_map = afi_config['route_map'][tmp].replace('-', '_') - if dict_search(f'policy.route_map.{route_map}', asn_config) == None: - raise ConfigError(f'route-map "{route_map}" used for "{tmp}" does not exist!') - - if 'route_reflector_client' in afi_config: - if 'remote_as' in peer_config and asn != peer_config['remote_as']: - raise ConfigError('route-reflector-client only supported for iBGP peers') - else: - if 'peer_group' in peer_config: - peer_group_as = dict_search(f'peer_group.{peer_group}.remote_as', asn_config) - if peer_group_as != None and peer_group_as != asn: - raise ConfigError('route-reflector-client only supported for iBGP peers') - - # Throw an error if a peer group is not configured for allow range - for prefix in dict_search('listen.range', asn_config) or []: - # we can not use dict_search() here as prefix contains dots ... - if 'peer_group' not in asn_config['listen']['range'][prefix]: - raise ConfigError(f'Listen range for prefix "{prefix}" has no peer group configured.') - - peer_group = asn_config['listen']['range'][prefix]['peer_group'] - if 'peer_group' not in asn_config or peer_group not in asn_config['peer_group']: - raise ConfigError(f'Peer-group "{peer_group}" for listen range "{prefix}" does not exist!') - - if not verify_remote_as(asn_config['listen']['range'][prefix], asn_config): - raise ConfigError(f'Peer-group "{peer_group}" requires remote-as to be set!') + for peer, peer_config in bgp[neighbor].items(): + # Only regular "neighbor" statement can have a peer-group set + # Check if the configure peer-group exists + if 'peer_group' in peer_config: + peer_group = peer_config['peer_group'] + if 'peer_group' not in bgp or peer_group not in bgp['peer_group']: + raise ConfigError(f'Specified peer-group "{peer_group}" for '\ + f'neighbor "{neighbor}" does not exist!') + + # ttl-security and ebgp-multihop can't be used in the same configration + if 'ebgp_multihop' in peer_config and 'ttl_security' in peer_config: + raise ConfigError('You can\'t set both ebgp-multihop and ttl-security hops') + + # Check spaces in the password + if 'password' in peer_config and ' ' in peer_config['password']: + raise ConfigError('You can\'t use spaces in the password') + + # Some checks can/must only be done on a neighbor and not a peer-group + if neighbor == 'neighbor': + # remote-as must be either set explicitly for the neighbor + # or for the entire peer-group + if not verify_remote_as(peer_config, bgp): + raise ConfigError(f'Neighbor "{peer}" remote-as must be set!') + + # Only checks for ipv4 and ipv6 neighbors + # Check if neighbor address is assigned as system interface address + if is_ip(peer) and is_addr_assigned(peer): + raise ConfigError(f'Can\'t configure local address as neighbor "{peer}"') + + for afi in ['ipv4_unicast', 'ipv6_unicast', 'l2vpn_evpn']: + # Bail out early if address family is not configured + if 'address_family' not in peer_config or afi not in peer_config['address_family']: + continue + + afi_config = peer_config['address_family'][afi] + # Validate if configured Prefix list exists + if 'prefix_list' in afi_config: + for tmp in ['import', 'export']: + if tmp not in afi_config['prefix_list']: + # bail out early + continue + # get_config_dict() mangles all '-' characters to '_' this is legitimate, thus all our + # compares will run on '_' as also '_' is a valid name for a prefix-list + prefix_list = afi_config['prefix_list'][tmp].replace('-', '_') + if afi == 'ipv4_unicast': + if dict_search(f'policy.prefix_list.{prefix_list}', bgp) == None: + raise ConfigError(f'prefix-list "{prefix_list}" used for "{tmp}" does not exist!') + elif afi == 'ipv6_unicast': + if dict_search(f'policy.prefix_list6.{prefix_list}', bgp) == None: + raise ConfigError(f'prefix-list6 "{prefix_list}" used for "{tmp}" does not exist!') + + if 'route_map' in afi_config: + for tmp in ['import', 'export']: + if tmp in afi_config['route_map']: + # get_config_dict() mangles all '-' characters to '_' this is legitim, thus all our + # compares will run on '_' as also '_' is a valid name for a route-map + route_map = afi_config['route_map'][tmp].replace('-', '_') + if dict_search(f'policy.route_map.{route_map}', bgp) == None: + raise ConfigError(f'route-map "{route_map}" used for "{tmp}" does not exist!') + + if 'route_reflector_client' in afi_config: + if 'remote_as' in peer_config and bgp['local_as'] != peer_config['remote_as']: + raise ConfigError('route-reflector-client only supported for iBGP peers') + else: + if 'peer_group' in peer_config: + peer_group_as = dict_search(f'peer_group.{peer_group}.remote_as', bgp) + if peer_group_as != None and peer_group_as != bgp['local_as']: + raise ConfigError('route-reflector-client only supported for iBGP peers') + + # Throw an error if a peer group is not configured for allow range + for prefix in dict_search('listen.range', bgp) or []: + # we can not use dict_search() here as prefix contains dots ... + if 'peer_group' not in bgp['listen']['range'][prefix]: + raise ConfigError(f'Listen range for prefix "{prefix}" has no peer group configured.') + + peer_group = bgp['listen']['range'][prefix]['peer_group'] + if 'peer_group' not in bgp or peer_group not in bgp['peer_group']: + raise ConfigError(f'Peer-group "{peer_group}" for listen range "{prefix}" does not exist!') + + if not verify_remote_as(bgp['listen']['range'][prefix], bgp): + raise ConfigError(f'Peer-group "{peer_group}" requires remote-as to be set!') return None @@ -204,14 +190,7 @@ def generate(bgp): bgp['new_frr_config'] = '' return None - # only one BGP AS is supported, so we can directly send the first key - # of the config dict - asn = list(bgp.keys())[0] - bgp[asn]['asn'] = asn - if 'vrf' in bgp: - bgp[asn]['vrf'] = bgp['vrf'] - - bgp['new_frr_config'] = render_to_string('frr/bgp.frr.tmpl', bgp[asn]) + bgp['new_frr_config'] = render_to_string('frr/bgp.frr.tmpl', bgp) return None def apply(bgp): diff --git a/src/migration-scripts/bgp/0-to-1 b/src/migration-scripts/bgp/0-to-1 new file mode 100755 index 000000000..a70b52909 --- /dev/null +++ b/src/migration-scripts/bgp/0-to-1 @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2021 VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# T3417: migrate IS-IS tagNode to node as we can only have one IS-IS process + +from sys import argv +from sys import exit + +from vyos.configtree import ConfigTree + +if (len(argv) < 1): + print("Must specify file name!") + exit(1) + +file_name = argv[1] + +with open(file_name, 'r') as f: + config_file = f.read() + +base = ['protocols', 'bgp'] +config = ConfigTree(config_file) + +if not config.exists(base): + # Nothing to do + exit(0) + +# Only one BGP process is supported, thus this operation is savea +asn = config.list_nodes(base) +bgp_base = base + asn +print(asn) + +# We need a temporary copy of the config +tmp_base = ['protocols', 'bgp2'] +config.copy(bgp_base, tmp_base) + +# Now it's save to delete the old configuration +config.delete(base) + +# Rename temporary copy to new final config and set new "local-as" option +config.rename(tmp_base, 'bgp') +config.set(base + ['local-as'], value=asn[0]) + +try: + with open(file_name, 'w') as f: + f.write(config.to_string()) +except OSError as e: + print(f'Failed to save the modified config: {e}') + exit(1) -- cgit v1.2.3