From d2f1f042e35adef9aaed11194fc2090d6ea51e49 Mon Sep 17 00:00:00 2001 From: Wido den Hollander Date: Fri, 22 Nov 2024 16:50:03 +0100 Subject: [PATCH 1/3] virtual router: Add route-maps to BGP peers for Routed Mode It is best practice, and mandatory in newer version of FRR, that route-maps should be applied to BGP peers. This is to prevent that mistakes can propogate through a network and cause outages. This change changes the route-maps where the VR will only accept IPv4 and IPv4 default gateways (0.0.0.0/0 and ::/0) to be sent by the upstream router to the VR. The other way around this change makes sure that FRR will not allow announcing anything else than the locally defined subnets to the upstream BGP router. --- .../debian/opt/cloud/bin/cs/CsBgpPeers.py | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py b/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py index 137279b74a9e..b02bcc2a2836 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py @@ -54,7 +54,9 @@ def process(self): self.frr_conf = CsFile(FRR_CONFIG) self.frr_conf.repopulate() self._pre_set() + self._access_list_set() self._process_peers() + self._route_map_set() self._post_set() if self.frr_conf.commit(): restart_frr = True @@ -75,13 +77,35 @@ def _process_dbag_item(self, item): self.peers[as_number]['ip6_peers'].append(item) def _pre_set(self): - self.frr_conf.add("frr version 6.0") self.frr_conf.add("frr defaults traditional") self.frr_conf.add("hostname {}".format(CsHelper.get_hostname())) self.frr_conf.add("service integrated-vtysh-config") self.frr_conf.add("ip nht resolve-via-default") return + def _access_list_set(self): + self.frr_conf.add("ip prefix-list all-v4 seq 1 permit any") + self.frr_conf.add("ip prefix-list default-v4 seq 1 permit 0.0.0.0/0") + self.frr_conf.add("ipv6 prefix-list all-v6 seq 1 permit any") + self.frr_conf.add("ipv6 prefix-list default-v6 seq 1 permit ::/0") + + for as_number in self.peers.keys(): + if self.peers[as_number]['ip4_peers']: + seq = 1 + ip4_cidrs = set({ip4_peer['guest_ip4_cidr'] for ip4_peer in self.peers[as_number]['ip4_peers']}) + for ip4_cidr in ip4_cidrs: + self.frr_conf.add("ip prefix-list local-v4 seq {} permit {}".format(seq, ip4_cidr)) + seq += 1 + + if self.peers[as_number]['ip6_peers']: + seq = 1 + ip6_cidrs = set({ip6_peer['guest_ip6_cidr'] for ip6_peer in self.peers[as_number]['ip6_peers']}) + for ip6_cidr in ip6_cidrs: + self.frr_conf.add("ipv6 prefix-list local-v6 seq {} permit {}".format(seq, ip6_cidr)) + seq += 1 + + return + def _process_peers(self): for as_number in self.peers.keys(): self.frr_conf.add("router bgp {}".format(as_number)) @@ -90,6 +114,8 @@ def _process_peers(self): self.frr_conf.add(" bgp default ipv6-unicast") for ip4_peer in self.peers[as_number]['ip4_peers']: self.frr_conf.add(" neighbor {} remote-as {}".format(ip4_peer['ip4_address'], ip4_peer['peer_as_number'])) + self.frr_conf.add(" neighbor {} route-map upstream-v4-in in".format(ip4_peer['ip4_address'])) + self.frr_conf.add(" neighbor {} route-map upstream-v4-out out".format(ip4_peer['ip4_address'])) if 'peer_password' in ip4_peer: self.frr_conf.add(" neighbor {} password {}".format(ip4_peer['ip4_address'], ip4_peer['peer_password'])) if 'details' in ip4_peer: @@ -97,6 +123,8 @@ def _process_peers(self): self.frr_conf.add(" neighbor {} ebgp-multihop {}".format(ip4_peer['ip4_address'], ip4_peer['details']['EBGP_MultiHop'])) for ip6_peer in self.peers[as_number]['ip6_peers']: self.frr_conf.add(" neighbor {} remote-as {}".format(ip6_peer['ip6_address'], ip6_peer['peer_as_number'])) + self.frr_conf.add(" neighbor {} route-map upstream-v6-in in".format(ip6_peer['ip6_address'])) + self.frr_conf.add(" neighbor {} route-map upstream-v6-out out".format(ip6_peer['ip6_address'])) if 'peer_password' in ip6_peer: self.frr_conf.add(" neighbor {} password {}".format(ip6_peer['ip6_address'], ip6_peer['peer_password'])) if 'details' in ip6_peer: @@ -115,5 +143,27 @@ def _process_peers(self): self.frr_conf.add(" network {}".format(ip6_cidr)) self.frr_conf.add(" exit-address-family") + def _route_map_set(self): + self.frr_conf.add("route-map upstream-v4-in permit 10") + self.frr_conf.add(" match ip address prefix-list default-v4") + self.frr_conf.add("route-map upstream-v4-in deny 1000") + self.frr_conf.add(" match ip address prefix-list all-v4") + + self.frr_conf.add("route-map upstream-v4-out permit 10") + self.frr_conf.add(" match ip address prefix-list local-v4") + self.frr_conf.add("route-map upstream-v4-out deny 1000") + self.frr_conf.add(" match ip address prefix-list all-v4") + + self.frr_conf.add("route-map upstream-v6-in permit 10") + self.frr_conf.add(" match ipv6 address prefix-list default-v6") + self.frr_conf.add("route-map upstream-v6-in deny 1000") + self.frr_conf.add(" match ipv6 address prefix-list all-v6") + + self.frr_conf.add("route-map upstream-v6-out permit 10") + self.frr_conf.add(" match ipv6 address prefix-list local-v6") + self.frr_conf.add("route-map upstream-v6-out deny 1000") + self.frr_conf.add(" match ipv6 address prefix-list all-v6") + return + def _post_set(self): self.frr_conf.add("line vty") From ebc221d833d52cc2e94f126f5d2780f49df9db48 Mon Sep 17 00:00:00 2001 From: Wido den Hollander Date: Thu, 13 Aug 2026 06:51:13 +0200 Subject: [PATCH 2/3] virtual router: Add unit tests for CsBgpPeers Add tests covering the FRR configuration generated for BGP peers in Routed Mode, including the prefix-lists and route-maps which are applied to the upstream BGP peers. --- systemvm/test/TestCsBgpPeers.py | 227 ++++++++++++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 systemvm/test/TestCsBgpPeers.py diff --git a/systemvm/test/TestCsBgpPeers.py b/systemvm/test/TestCsBgpPeers.py new file mode 100644 index 000000000000..32e6f10d4db3 --- /dev/null +++ b/systemvm/test/TestCsBgpPeers.py @@ -0,0 +1,227 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import unittest +try: + import mock +except ImportError: + from unittest import mock +from cs.CsBgpPeers import CsBgpPeers +from cs.CsFile import CsFile +import merge + + +class TestCsBgpPeers(unittest.TestCase): + + def setUp(self): + merge.DataBag.DPATH = "." + self.csbgppeers = CsBgpPeers("bgppeers", {}) + self.csbgppeers.peers = {} + self.csbgppeers.public_ip = "100.64.0.10" + self.csbgppeers.frr_conf = CsFile("frr.conf.test") + self.csbgppeers.frr_conf.repopulate() + + def _peer(self, **kwargs): + peer = { + 'peer_id': 1, + 'network_id': 100, + 'network_as_number': 64512, + 'peer_as_number': 64496 + } + peer.update(kwargs) + return peer + + def _frr_conf(self): + return [line.rstrip('\n') for line in self.csbgppeers.frr_conf.new_config] + + def test_init(self): + self.assertTrue(self.csbgppeers is not None) + + def test_process_dbag_item(self): + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24')) + self.csbgppeers._process_dbag_item(self._peer(ip6_address='2001:db8::1', + guest_ip6_cidr='2001:db8:100::/64')) + self.csbgppeers._process_dbag_item(self._peer(network_as_number=64513, + ip4_address='100.64.1.1', + guest_ip4_cidr='10.2.1.0/24')) + + self.assertEqual(sorted(self.csbgppeers.peers.keys()), [64512, 64513]) + self.assertEqual(len(self.csbgppeers.peers[64512]['ip4_peers']), 1) + self.assertEqual(len(self.csbgppeers.peers[64512]['ip6_peers']), 1) + self.assertEqual(len(self.csbgppeers.peers[64513]['ip4_peers']), 1) + self.assertEqual(len(self.csbgppeers.peers[64513]['ip6_peers']), 0) + + def test_process_dbag_item_dual_stack(self): + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24', + ip6_address='2001:db8::1', + guest_ip6_cidr='2001:db8:100::/64')) + + self.assertEqual(len(self.csbgppeers.peers[64512]['ip4_peers']), 1) + self.assertEqual(len(self.csbgppeers.peers[64512]['ip6_peers']), 1) + + def test_access_list_set(self): + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24', + ip6_address='2001:db8::1', + guest_ip6_cidr='2001:db8:100::/64')) + self.csbgppeers._access_list_set() + + config = self._frr_conf() + self.assertIn("ip prefix-list all-v4 seq 1 permit any", config) + self.assertIn("ip prefix-list default-v4 seq 1 permit 0.0.0.0/0", config) + self.assertIn("ipv6 prefix-list all-v6 seq 1 permit any", config) + self.assertIn("ipv6 prefix-list default-v6 seq 1 permit ::/0", config) + self.assertIn("ip prefix-list local-v4 seq 1 permit 10.1.1.0/24", config) + self.assertIn("ipv6 prefix-list local-v6 seq 1 permit 2001:db8:100::/64", config) + + def test_access_list_set_deduplicates_cidrs(self): + # Two peers announcing the same guest CIDR should only result in a single prefix-list entry + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24')) + self.csbgppeers._process_dbag_item(self._peer(peer_id=2, + ip4_address='100.64.0.2', + guest_ip4_cidr='10.1.1.0/24')) + self.csbgppeers._access_list_set() + + config = self._frr_conf() + local_v4 = [line for line in config if line.startswith("ip prefix-list local-v4")] + self.assertEqual(local_v4, ["ip prefix-list local-v4 seq 1 permit 10.1.1.0/24"]) + + def test_process_peers_ip4(self): + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24')) + self.csbgppeers._process_peers() + + config = self._frr_conf() + self.assertIn("router bgp 64512", config) + self.assertIn(" bgp router-id 100.64.0.10", config) + self.assertIn(" neighbor 100.64.0.1 remote-as 64496", config) + self.assertIn(" neighbor 100.64.0.1 route-map upstream-v4-in in", config) + self.assertIn(" neighbor 100.64.0.1 route-map upstream-v4-out out", config) + self.assertIn(" network 10.1.1.0/24", config) + self.assertNotIn(" bgp default ipv6-unicast", config) + + def test_process_peers_ip6(self): + self.csbgppeers._process_dbag_item(self._peer(ip6_address='2001:db8::1', + guest_ip6_cidr='2001:db8:100::/64')) + self.csbgppeers._process_peers() + + config = self._frr_conf() + self.assertIn(" bgp default ipv6-unicast", config) + self.assertIn(" neighbor 2001:db8::1 remote-as 64496", config) + self.assertIn(" neighbor 2001:db8::1 route-map upstream-v6-in in", config) + self.assertIn(" neighbor 2001:db8::1 route-map upstream-v6-out out", config) + self.assertIn(" network 2001:db8:100::/64", config) + + def test_process_peers_password_and_multihop(self): + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24', + peer_password='S3cr3t!', + details={'EBGP_MultiHop': 2})) + self.csbgppeers._process_peers() + + config = self._frr_conf() + self.assertIn(" neighbor 100.64.0.1 password S3cr3t!", config) + self.assertIn(" neighbor 100.64.0.1 ebgp-multihop 2", config) + + def test_route_map_set(self): + self.csbgppeers._route_map_set() + + expected = [ + "route-map upstream-v4-in permit 10", + " match ip address prefix-list default-v4", + "route-map upstream-v4-in deny 1000", + " match ip address prefix-list all-v4", + "route-map upstream-v4-out permit 10", + " match ip address prefix-list local-v4", + "route-map upstream-v4-out deny 1000", + " match ip address prefix-list all-v4", + "route-map upstream-v6-in permit 10", + " match ipv6 address prefix-list default-v6", + "route-map upstream-v6-in deny 1000", + " match ipv6 address prefix-list all-v6", + "route-map upstream-v6-out permit 10", + " match ipv6 address prefix-list local-v6", + "route-map upstream-v6-out deny 1000", + " match ipv6 address prefix-list all-v6" + ] + self.assertEqual(self._frr_conf(), expected) + + @mock.patch('cs.CsBgpPeers.CsHelper.get_hostname') + def test_full_frr_conf(self, mock_hostname): + mock_hostname.return_value = "r-1001-VM" + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24', + ip6_address='2001:db8::1', + guest_ip6_cidr='2001:db8:100::/64')) + self.csbgppeers._pre_set() + self.csbgppeers._access_list_set() + self.csbgppeers._process_peers() + self.csbgppeers._route_map_set() + self.csbgppeers._post_set() + + expected = [ + "frr defaults traditional", + "hostname r-1001-VM", + "service integrated-vtysh-config", + "ip nht resolve-via-default", + "ip prefix-list all-v4 seq 1 permit any", + "ip prefix-list default-v4 seq 1 permit 0.0.0.0/0", + "ipv6 prefix-list all-v6 seq 1 permit any", + "ipv6 prefix-list default-v6 seq 1 permit ::/0", + "ip prefix-list local-v4 seq 1 permit 10.1.1.0/24", + "ipv6 prefix-list local-v6 seq 1 permit 2001:db8:100::/64", + "router bgp 64512", + " bgp router-id 100.64.0.10", + " bgp default ipv6-unicast", + " neighbor 100.64.0.1 remote-as 64496", + " neighbor 100.64.0.1 route-map upstream-v4-in in", + " neighbor 100.64.0.1 route-map upstream-v4-out out", + " neighbor 2001:db8::1 remote-as 64496", + " neighbor 2001:db8::1 route-map upstream-v6-in in", + " neighbor 2001:db8::1 route-map upstream-v6-out out", + " address-family ipv4 unicast", + " network 10.1.1.0/24", + " exit-address-family", + " address-family ipv6 unicast", + " network 2001:db8:100::/64", + " exit-address-family", + "route-map upstream-v4-in permit 10", + " match ip address prefix-list default-v4", + "route-map upstream-v4-in deny 1000", + " match ip address prefix-list all-v4", + "route-map upstream-v4-out permit 10", + " match ip address prefix-list local-v4", + "route-map upstream-v4-out deny 1000", + " match ip address prefix-list all-v4", + "route-map upstream-v6-in permit 10", + " match ipv6 address prefix-list default-v6", + "route-map upstream-v6-in deny 1000", + " match ipv6 address prefix-list all-v6", + "route-map upstream-v6-out permit 10", + " match ipv6 address prefix-list local-v6", + "route-map upstream-v6-out deny 1000", + " match ipv6 address prefix-list all-v6", + "line vty" + ] + self.assertEqual(self._frr_conf(), expected) + + +if __name__ == '__main__': + unittest.main() From b7df9e53e205673a631cfbc6fe9477cf53be6117 Mon Sep 17 00:00:00 2001 From: Wido den Hollander Date: Thu, 13 Aug 2026 06:55:29 +0200 Subject: [PATCH 3/3] virtual router: Prevent prefix-list seq collisions with multiple AS numbers The local-v4 and local-v6 prefix-lists are shared between all AS numbers, but the sequence numbers restarted at 1 for each AS number. With peers for multiple AS numbers this caused entries to overwrite each other, denying the announcement of the guest networks of all but the last AS number. Collect the guest CIDRs of all AS numbers first and number them once. Sorting the CIDRs also makes the generated configuration deterministic between runs. --- .../debian/opt/cloud/bin/cs/CsBgpPeers.py | 21 ++++++++----------- systemvm/test/TestCsBgpPeers.py | 16 ++++++++++++++ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py b/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py index b02bcc2a2836..d90d31cba5fb 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsBgpPeers.py @@ -89,20 +89,17 @@ def _access_list_set(self): self.frr_conf.add("ipv6 prefix-list all-v6 seq 1 permit any") self.frr_conf.add("ipv6 prefix-list default-v6 seq 1 permit ::/0") + ip4_cidrs = set() + ip6_cidrs = set() for as_number in self.peers.keys(): - if self.peers[as_number]['ip4_peers']: - seq = 1 - ip4_cidrs = set({ip4_peer['guest_ip4_cidr'] for ip4_peer in self.peers[as_number]['ip4_peers']}) - for ip4_cidr in ip4_cidrs: - self.frr_conf.add("ip prefix-list local-v4 seq {} permit {}".format(seq, ip4_cidr)) - seq += 1 + ip4_cidrs.update(ip4_peer['guest_ip4_cidr'] for ip4_peer in self.peers[as_number]['ip4_peers']) + ip6_cidrs.update(ip6_peer['guest_ip6_cidr'] for ip6_peer in self.peers[as_number]['ip6_peers']) - if self.peers[as_number]['ip6_peers']: - seq = 1 - ip6_cidrs = set({ip6_peer['guest_ip6_cidr'] for ip6_peer in self.peers[as_number]['ip6_peers']}) - for ip6_cidr in ip6_cidrs: - self.frr_conf.add("ipv6 prefix-list local-v6 seq {} permit {}".format(seq, ip6_cidr)) - seq += 1 + for seq, ip4_cidr in enumerate(sorted(ip4_cidrs), start=1): + self.frr_conf.add("ip prefix-list local-v4 seq {} permit {}".format(seq, ip4_cidr)) + + for seq, ip6_cidr in enumerate(sorted(ip6_cidrs), start=1): + self.frr_conf.add("ipv6 prefix-list local-v6 seq {} permit {}".format(seq, ip6_cidr)) return diff --git a/systemvm/test/TestCsBgpPeers.py b/systemvm/test/TestCsBgpPeers.py index 32e6f10d4db3..2dddf7db0fd1 100644 --- a/systemvm/test/TestCsBgpPeers.py +++ b/systemvm/test/TestCsBgpPeers.py @@ -103,6 +103,22 @@ def test_access_list_set_deduplicates_cidrs(self): local_v4 = [line for line in config if line.startswith("ip prefix-list local-v4")] self.assertEqual(local_v4, ["ip prefix-list local-v4 seq 1 permit 10.1.1.0/24"]) + def test_access_list_set_multiple_as_numbers(self): + # Sequence numbers may not collide when there are peers for multiple AS numbers, + # otherwise entries overwrite each other in the shared local-v4/local-v6 prefix-lists + self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', + guest_ip4_cidr='10.1.1.0/24')) + self.csbgppeers._process_dbag_item(self._peer(peer_id=2, + network_as_number=64513, + ip4_address='100.64.1.1', + guest_ip4_cidr='10.2.1.0/24')) + self.csbgppeers._access_list_set() + + config = self._frr_conf() + local_v4 = [line for line in config if line.startswith("ip prefix-list local-v4")] + self.assertEqual(local_v4, ["ip prefix-list local-v4 seq 1 permit 10.1.1.0/24", + "ip prefix-list local-v4 seq 2 permit 10.2.1.0/24"]) + def test_process_peers_ip4(self): self.csbgppeers._process_dbag_item(self._peer(ip4_address='100.64.0.1', guest_ip4_cidr='10.1.1.0/24'))