Merge pull request #5536 from alexk37/bgp-default-local-pref-100

bgp: T9404: do not render "bgp default local-preference" when the value is 100

4 files changed, 76 insertions, 1 deletions

diff --git a/data/templates/frr/bgpd.frr.j2 b/data/templates/frr/bgpd.frr.j2
index e2def6e6f..7671576d1 100644
--- a/data/templates/frr/bgpd.frr.j2
+++ b/data/templates/frr/bgpd.frr.j2
@@ -619,7 +619,8 @@ router bgp {{ system_as }} {{ 'vrf ' ~ vrf if vrf is vyos_defined }} {{ 'as-nota
 {# Doesn't work in current FRR configuration; vtysh (bgp dampening 16 751 2001 61) #}
  bgp dampening {{ parameters.dampening.half_life }} {{ parameters.dampening.re_use if parameters.dampening.re_use is vyos_defined }} {{ parameters.dampening.start_suppress_time if parameters.dampening.start_suppress_time is vyos_defined }} {{ parameters.dampening.max_suppress_time if parameters.dampening.max_suppress_time is vyos_defined }}
 {% endif %}
-{% if parameters.default.local_pref is vyos_defined %}
+{# xml_default_local_pref is the XML default of parameters default local-pref, added to the dict by frrender #}
+{% if parameters.default.local_pref is vyos_defined and parameters.default.local_pref != xml_default_local_pref %}
  bgp default local-preference {{ parameters.default.local_pref }}
 {% endif %}
 {% if parameters.deterministic_med is vyos_defined %}
diff --git a/interface-definitions/include/bgp/protocol-common-config.xml.i b/interface-definitions/include/bgp/protocol-common-config.xml.i
index 44aa7f6f9..d4b46a731 100644
--- a/interface-definitions/include/bgp/protocol-common-config.xml.i
+++ b/interface-definitions/include/bgp/protocol-common-config.xml.i
@@ -1427,6 +1427,7 @@
               <validator name="numeric" argument="--range 0-4294967295"/>
             </constraint>
           </properties>
+          <defaultValue>100</defaultValue>
         </leafNode>
       </children>
     </node>
diff --git a/python/vyos/frrender.py b/python/vyos/frrender.py
index 69bb93a96..eac03e4d3 100644
--- a/python/vyos/frrender.py
+++ b/python/vyos/frrender.py
@@ -297,6 +297,12 @@ def get_frrender_dict(conf: Config, argv=None) -> dict:
                                    no_tag_node_value_mangle=True,
                                    with_recursive_defaults=True)
         bgp['dependent_vrfs'] = {}
+        # The XML default of "parameters default local-pref" is added to the
+        # dict, the template cannot query it while rendering. Only the node
+        # "parameters default" is asked for, not the whole BGP tree.
+        tmp = conf.get_config_defaults(bgp_cli_path + ['parameters', 'default'],
+                                       key_mangling=('-', '_'), get_first_key=True)
+        bgp['xml_default_local_pref'] = tmp.get('local_pref')
         dict.update({'bgp' : bgp})
     elif conf.exists_effective(bgp_cli_path):
         dict.update({'bgp' : {'deleted' : '', 'dependent_vrfs' : {}}})
@@ -522,6 +528,8 @@ def get_frrender_dict(conf: Config, argv=None) -> dict:
                 # merge in remaining default values
                 vrf_config['protocols']['bgp'] = config_dict_merge(default_values,
                                                                    vrf_config['protocols']['bgp'])
+                vrf_config['protocols']['bgp']['xml_default_local_pref'] = dict_search(
+                    'parameters.default.local_pref', default_values)
 
                 # Add this BGP VRF instance as dependency into the default VRF
                 if 'bgp' in dict:
diff --git a/smoketest/scripts/cli/test_protocols_bgp.py b/smoketest/scripts/cli/test_protocols_bgp.py
index 2ab8c8844..3e417a0f5 100755
--- a/smoketest/scripts/cli/test_protocols_bgp.py
+++ b/smoketest/scripts/cli/test_protocols_bgp.py
@@ -2126,5 +2126,70 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase):
             self.assertIn(f'interface {interface}', frrconfig)
             self.assertIn(f' mpls bgp l3vpn-multi-domain-switching', frrconfig)
 
+    def test_bgp_107_default_local_pref_default_value(self):
+        # T9404: FRR does not print "bgp default local-preference 100" in its
+        # running config because 100 is the default. If we render the line
+        # anyway, frr-reload finds it missing on every reload and sends it
+        # again, and FRR runs "clear bgp * soft in" for each of those. So the
+        # default value must not be rendered.
+        #
+        # This reads the generated FRR config file instead of getFRRconfig():
+        # FRR never prints the line for 100, so vtysh cannot tell if we render it.
+        frr_conf = '/run/frr/config/vyos.frr.conf'
+        local_pref = ' bgp default local-preference'
+
+        # the XML default (100) is always in the config dict, but it is the
+        # default, so nothing is rendered when local-pref is not configured
+        self.cli_commit()
+        frrconfig = read_file(frr_conf)
+        self.assertIn(f'router bgp {ASN}', frrconfig)
+        self.assertNotIn(local_pref, frrconfig)
+
+        self.cli_set(base_path + ['parameters', 'default', 'local-pref', '100'])
+        self.cli_commit()
+
+        frrconfig = read_file(frr_conf)
+        self.assertIn(f'router bgp {ASN}', frrconfig)
+        self.assertNotIn(local_pref, frrconfig)
+
+        # a value other than the default is still rendered and applied
+        self.cli_set(base_path + ['parameters', 'default', 'local-pref', '200'])
+        self.cli_commit()
+
+        self.assertIn(f'{local_pref} 200', read_file(frr_conf))
+        frrconfig = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit')
+        self.assertIn(f'{local_pref} 200', frrconfig)
+
+        # back to the default value, FRR must have the default again
+        self.cli_set(base_path + ['parameters', 'default', 'local-pref', '100'])
+        self.cli_commit()
+
+        self.assertNotIn(local_pref, read_file(frr_conf))
+        frrconfig = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit')
+        self.assertNotIn(local_pref, frrconfig)
+
+        # the VRF instance has the same default: 100 is not rendered in the
+        # VRF block, 200 is rendered in the VRF block only
+        vrf_header = f'router bgp {ASN} vrf {import_vrf}'
+        vrf_local_pref = import_vrf_base + [import_vrf, 'protocols', 'bgp', 'parameters', 'default', 'local-pref']
+        self.create_bgp_instances_for_import_test()
+        self.cli_set(vrf_local_pref + ['100'])
+        self.cli_commit()
+
+        frrconfig = read_file(frr_conf)
+        self.assertIn(vrf_header, frrconfig)
+        self.assertNotIn(local_pref, frrconfig)
+
+        self.cli_set(vrf_local_pref + ['200'])
+        self.cli_commit()
+
+        # the global block comes first in the file, the VRF block after it
+        global_block, _, vrf_block = read_file(frr_conf).partition(vrf_header)
+        self.assertIn(f'router bgp {ASN}', global_block)
+        self.assertNotIn(local_pref, global_block)
+        self.assertIn(f'{local_pref} 200', vrf_block)
+        frrconfig = self.getFRRconfig(vrf_header, stop_section='^exit')
+        self.assertIn(f'{local_pref} 200', frrconfig)
+
 if __name__ == '__main__':
     unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())