Merge pull request #3245 from vyos/mergify/bp/sagitta/pr-3238

bgp: T5943: BGP Peer-group members must be all internal or all external (backport #3238)

2 files changed, 84 insertions, 0 deletions

diff --git a/smoketest/scripts/cli/test_protocols_bgp.py b/smoketest/scripts/cli/test_protocols_bgp.py
index 60c49b8b4..03daa34aa 100755
--- a/smoketest/scripts/cli/test_protocols_bgp.py
+++ b/smoketest/scripts/cli/test_protocols_bgp.py
@@ -1259,6 +1259,77 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase):
 
         self.assertIn('neighbor peer1 route-reflector-client', conf)
 
+    def test_bgp_28_peer_group_member_all_internal_or_external(self):
+        def _common_config_check(conf, include_ras=True):
+            if include_ras:
+                self.assertIn(f'neighbor {int_neighbors[0]} remote-as {ASN}', conf)
+                self.assertIn(f'neighbor {int_neighbors[1]} remote-as {ASN}', conf)
+                self.assertIn(f'neighbor {ext_neighbors[0]} remote-as {int(ASN) + 1}',conf)
+
+            self.assertIn(f'neighbor {int_neighbors[0]} peer-group {int_pg_name}', conf)
+            self.assertIn(f'neighbor {int_neighbors[1]} peer-group {int_pg_name}', conf)
+            self.assertIn(f'neighbor {ext_neighbors[0]} peer-group {ext_pg_name}', conf)
+
+        int_neighbors = ['192.0.2.2', '192.0.2.3']
+        ext_neighbors = ['192.122.2.2', '192.122.2.3']
+        int_pg_name, ext_pg_name = 'SMOKETESTINT', 'SMOKETESTEXT'
+
+        self.cli_set(base_path + ['neighbor', int_neighbors[0], 'peer-group', int_pg_name])
+        self.cli_set(base_path + ['neighbor', int_neighbors[0], 'remote-as', ASN])
+        self.cli_set(base_path + ['peer-group', int_pg_name, 'address-family', 'ipv4-unicast'])
+        self.cli_set(base_path + ['neighbor', ext_neighbors[0], 'peer-group', ext_pg_name])
+        self.cli_set(base_path + ['neighbor', ext_neighbors[0], 'remote-as', f'{int(ASN) + 1}'])
+        self.cli_set(base_path + ['peer-group', ext_pg_name, 'address-family', 'ipv4-unicast'])
+        self.cli_commit()
+
+        # test add external remote-as to internal group
+        self.cli_set(base_path + ['neighbor', int_neighbors[1], 'peer-group', int_pg_name])
+        self.cli_set(base_path + ['neighbor', int_neighbors[1], 'remote-as', f'{int(ASN) + 1}'])
+
+        with self.assertRaises(ConfigSessionError) as e:
+            self.cli_commit()
+        # self.assertIn('\nPeer-group members must be all internal or all external\n', str(e.exception))
+
+        # test add internal remote-as to internal group
+        self.cli_set(base_path + ['neighbor', int_neighbors[1], 'remote-as', ASN])
+        self.cli_commit()
+
+        conf = self.getFRRconfig(f'router bgp {ASN}')
+        _common_config_check(conf)
+
+        # test add internal remote-as to external group
+        self.cli_set(base_path + ['neighbor', ext_neighbors[1], 'peer-group', ext_pg_name])
+        self.cli_set(base_path + ['neighbor', ext_neighbors[1], 'remote-as', ASN])
+
+        with self.assertRaises(ConfigSessionError) as e:
+            self.cli_commit()
+        # self.assertIn('\nPeer-group members must be all internal or all external\n', str(e.exception))
+
+        # test add external remote-as to external group
+        self.cli_set(base_path + ['neighbor', ext_neighbors[1], 'remote-as', f'{int(ASN) + 2}'])
+        self.cli_commit()
+
+        conf = self.getFRRconfig(f'router bgp {ASN}')
+        _common_config_check(conf)
+        self.assertIn(f'neighbor {ext_neighbors[1]} remote-as {int(ASN) + 2}', conf)
+        self.assertIn(f'neighbor {ext_neighbors[1]} peer-group {ext_pg_name}', conf)
+
+        # test named remote-as
+        self.cli_set(base_path + ['neighbor', int_neighbors[0], 'remote-as', 'internal'])
+        self.cli_set(base_path + ['neighbor', int_neighbors[1], 'remote-as', 'internal'])
+        self.cli_set(base_path + ['neighbor', ext_neighbors[0], 'remote-as', 'external'])
+        self.cli_set(base_path + ['neighbor', ext_neighbors[1], 'remote-as', 'external'])
+        self.cli_commit()
+
+        conf = self.getFRRconfig(f'router bgp {ASN}')
+        _common_config_check(conf, include_ras=False)
+
+        self.assertIn(f'neighbor {int_neighbors[0]} remote-as internal', conf)
+        self.assertIn(f'neighbor {int_neighbors[1]} remote-as internal', conf)
+        self.assertIn(f'neighbor {ext_neighbors[0]} remote-as external', conf)
+        self.assertIn(f'neighbor {ext_neighbors[1]} remote-as external', conf)
+        self.assertIn(f'neighbor {ext_neighbors[1]} peer-group {ext_pg_name}', conf)
+
     def test_bgp_99_bmp(self):
         target_name = 'instance-bmp'
         target_address = '127.0.0.1'
diff --git a/src/conf_mode/protocols_bgp.py b/src/conf_mode/protocols_bgp.py
index 512fa26e9..2b16de775 100755
--- a/src/conf_mode/protocols_bgp.py
+++ b/src/conf_mode/protocols_bgp.py
@@ -285,6 +285,7 @@ def verify(bgp):
             elif tmp != 'default':
                 raise ConfigError(f'{error_msg} "{tmp}"!')
 
+    peer_groups_context = dict()
     # 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
@@ -301,6 +302,18 @@ def verify(bgp):
                     raise ConfigError(f'Specified peer-group "{peer_group}" for '\
                                       f'neighbor "{neighbor}" does not exist!')
 
+                if 'remote_as' in peer_config:
+                    is_ibgp = True
+                    if peer_config['remote_as'] != 'internal' and \
+                            peer_config['remote_as'] != bgp['system_as']:
+                        is_ibgp = False
+
+                    if peer_group not in peer_groups_context:
+                        peer_groups_context[peer_group] = is_ibgp
+                    elif peer_groups_context[peer_group] != is_ibgp:
+                        raise ConfigError(f'Peer-group members must be '
+                                          f'all internal or all external')
+
             if 'local_role' in peer_config:
                 #Ensure Local Role has only one value.
                 if len(peer_config['local_role']) > 1: