Merge pull request #4663 from KawaiiNetworks/current

bgp: T7708: correct logic for route-reflector-client peer_as check

Author: dmbaturin

smoketest/scripts/cli/test_protocols_bgp.py

@@ -1438,17 +1438,64 @@ def test_bgp_26_ipv6_labeled_unicast_peer_group(self):
         self.assertIn(f'  neighbor {pg_ipv6} maximum-prefix {ipv6_max_prefix}', afiv6_config)
 
     def test_bgp_27_route_reflector_client(self):
-        self.cli_set(base_path + ['peer-group', 'peer1', 'address-family', 'l2vpn-evpn', 'route-reflector-client'])
-        with self.assertRaises(ConfigSessionError) as e:
-            self.cli_commit()
-
-        self.cli_set(base_path + ['peer-group', 'peer1', 'remote-as', 'internal'])
+        int_neighbors = ['192.0.2.2', '192.0.2.3', '192.0.2.4', '192.0.2.5']
+        int_interfaces = ['dum0', 'dum1', 'dum2', 'dum3']
+        int_pg_names = ['SMOKETESTINT0', 'SMOKETESTINT1', 'SMOKETESTINT2']
+        remote_as_types = ['external', 'internal']
+        for int_interface in int_interfaces:
+            self.cli_set(['interfaces', 'dummy', int_interface])
         self.cli_commit()
 
-        conf = self.getFRRconfig(f'router bgp {ASN}', endsection='^exit',
-                                 substring=' address-family l2vpn evpn', endsubsection='^ exit-address-family')
-
-        self.assertIn('neighbor peer1 route-reflector-client', conf)
+        def _set_neighbor_0(neighbor, remote_as_type):
+            # set route-reflector-client in neighbor and set remote-as in peer_group
+            interface_cmd = ['interface'] if neighbor.startswith('dum') else []
+            self.cli_set(base_path + ['peer-group', int_pg_names[0], 'remote-as', remote_as_type])
+            self.cli_set(base_path + ['neighbor', neighbor, 'address-family', 'ipv4-unicast', 'route-reflector-client'])
+            self.cli_set(base_path + ['neighbor', neighbor] + interface_cmd + ['peer-group', int_pg_names[0]])
+
+        def _set_neighbor_1(neighbor, remote_as_type):
+            # set route-reflector-client in peer_group and set remote-as in neighbor
+            interface_cmd = ['interface'] if neighbor.startswith('dum') else []
+            self.cli_set(base_path + ['peer-group', int_pg_names[1], 'address-family', 'ipv4-unicast', 'route-reflector-client'])
+            self.cli_set(base_path + ['neighbor', neighbor] + interface_cmd + ['remote-as', remote_as_type])
+            self.cli_set(base_path + ['neighbor', neighbor] + interface_cmd + ['peer-group', int_pg_names[1]])
+
+        def _set_neighbor_2(neighbor, remote_as_type):
+            # set route-reflector-client and remote-as in peer_group
+            interface_cmd = ['interface'] if neighbor.startswith('dum') else []
+            self.cli_set(base_path + ['peer-group', int_pg_names[2], 'remote-as', remote_as_type])
+            self.cli_set(base_path + ['peer-group', int_pg_names[2], 'address-family', 'ipv4-unicast', 'route-reflector-client'])
+            self.cli_set(base_path + ['neighbor', neighbor] + interface_cmd + ['peer-group', int_pg_names[2]])
+
+        def _set_neighbor_3(neighbor, remote_as_type):
+            # set route-reflector-client and remote-as in neighbor
+            interface_cmd = ['interface'] if neighbor.startswith('dum') else []
+            self.cli_set(base_path + ['neighbor', neighbor, 'address-family', 'ipv4-unicast', 'route-reflector-client'])
+            self.cli_set(base_path + ['neighbor', neighbor] + interface_cmd + ['remote-as', remote_as_type])
+            
+        set_neighbor_funcs = [_set_neighbor_0, _set_neighbor_1, _set_neighbor_2, _set_neighbor_3]
+        for remote_as_type in remote_as_types:
+            for func_count, set_neighbor_func in enumerate(set_neighbor_funcs):
+                for neighbors in [int_neighbors, int_interfaces]:
+                    set_neighbor_func(neighbors[func_count], remote_as_type)
+                    if remote_as_type == 'external':
+                        with self.assertRaises(ConfigSessionError) as e:
+                            self.cli_commit()
+                        self.cli_discard()
+                    else:
+                        self.cli_commit()
+        
+        frrconfig = self.getFRRconfig(f'router bgp {ASN}', endsection='^exit', substring=' address-family ipv4 unicast', endsubsection='^ exit-address-family')
+        neighbor_has_rr_client = [
+            int_neighbors[0], int_neighbors[3],
+            int_interfaces[0], int_interfaces[3],
+            int_pg_names[1], int_pg_names[2],
+        ]
+        [self.assertIn(f'neighbor {neighbor} route-reflector-client', frrconfig) for neighbor in neighbor_has_rr_client]
+
+        # tearDown dummy interfaces
+        self.cli_delete(['interfaces', 'dummy'])
+        self.cli_commit()
 
     def test_bgp_28_peer_group_member_all_internal_or_external(self):
         def _common_config_check(conf, include_ras=True):

src/conf_mode/protocols_bgp.py

@@ -316,6 +316,7 @@ def verify(config_dict):
                     Warning(f'BGP neighbor "{peer}" requires address-family!')
 
                 # Peer-group member cannot override remote-as of peer-group
+                peer_group = None
                 if 'peer_group' in peer_config:
                     peer_group = peer_config['peer_group']
                     if 'remote_as' in peer_config and 'remote_as' in bgp['peer_group'][peer_group]:
@@ -330,6 +331,27 @@ def verify(config_dict):
                             peer_group = peer_config['interface']['v6only']['peer_group']
                             if 'remote_as' in peer_config['interface']['v6only'] and 'remote_as' in bgp['peer_group'][peer_group]:
                                 raise ConfigError(f'Peer-group member "{peer}" cannot override remote-as of peer-group "{peer_group}"!')
+                            
+                for afi in ['ipv4_unicast', 'ipv4_multicast', 'ipv4_labeled_unicast', 'ipv4_flowspec',
+                            'ipv6_unicast', 'ipv6_multicast', 'ipv6_labeled_unicast', 'ipv6_flowspec',
+                            'l2vpn_evpn']:
+                    if dict_search(
+                        f'address_family.{afi}.route_reflector_client',
+                        peer_config,
+                    ) == {} or (
+                        peer_group
+                        and dict_search(
+                            f'peer_group.{peer_group}.address_family.{afi}.route_reflector_client',
+                            bgp,
+                        )
+                        == {}
+                    ):
+                        peer_as = verify_remote_as(peer_config, bgp)
+                        if peer_as != 'internal' and peer_as != bgp['system_as']:
+                            raise ConfigError('route-reflector-client only supported for iBGP peers')
+                    else:
+                        # It doesn’t make sense to check the remote-as of a peer group.
+                        pass
 
                 # Only checks for ipv4 and ipv6 neighbors
                 # Check if neighbor address is assigned as system interface address
@@ -412,20 +434,7 @@ def verify(config_dict):
                         if tmp in afi_config['route_map']:
                             verify_route_map(afi_config['route_map'][tmp], bgp)
 
-                if 'route_reflector_client' in afi_config:
-                    peer_as = peer_config.get('remote_as')
-
-                    if peer_as is not None and (peer_as != 'internal' and peer_as != bgp['system_as']):
-                        raise ConfigError('route-reflector-client only supported for iBGP peers')
-                    else:
-                        # Check into the peer group for the remote as, if we are in a peer group, check in peer itself
-                        if 'peer_group' in peer_config:
-                            peer_group_as = dict_search(f'peer_group.{peer_group}.remote_as', bgp)
-                        elif neighbor == 'peer_group':
-                            peer_group_as = peer_config.get('remote_as')
-                        
-                        if peer_group_as is None or (peer_group_as != 'internal' and peer_group_as != bgp['system_as']):
-                            raise ConfigError('route-reflector-client only supported for iBGP peers')
+                # route-reflector-client verification has been moved to neighbor-only part
 
             # T5833 not all AFIs are supported for VRF
             if 'vrf' in bgp and 'address_family' in peer_config: