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

7 files changed, 199 insertions, 140 deletions

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 @@
 </node>

 <node name="listen">

   <properties>

-    <help>Listen for and accept BGP dynamic neighbors from range</help>

+    <help>BGP dynamic neighbors listen commands</help>

   </properties>

   <children>

     <leafNode name="limit">

@@ -297,7 +297,7 @@
     </leafNode>

     <tagNode name="range">

       <properties>

-        <help>BGP dynamic neighbors listen range</help>

+        <help>Dynamic neighbors listen range</help>

         <valueHelp>

           <format>ipv4net</format>

           <description>IPv4 dynamic neighbors listen range</description>

@@ -317,6 +317,18 @@
     </tagNode>

   </children>

 </node>

+<leafNode name="local-as">

+  <properties>

+    <help>Autonomous System Number (ASN)</help>

+    <valueHelp>

+      <format>u32:1-4294967294</format>

+      <description>Autonomous System Number</description>

+    </valueHelp>

+    <constraint>

+      <validator name="numeric" argument="--range 1-4294967294"/>

+    </constraint>

+  </properties>

+</leafNode>

 <tagNode name="neighbor">

   <properties>

     <help>BGP neighbor</help>

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 @@
 <interfaceDefinition>
   <node name="protocols">
     <children>
-      <tagNode name="bgp" owner="${vyos_conf_scripts_dir}/protocols_bgp.py">
+      <node name="bgp" owner="${vyos_conf_scripts_dir}/protocols_bgp.py">
         <properties>
           <help>Border Gateway Protocol (BGP)</help>
           <priority>820</priority>
-          <valueHelp>
-            <format>u32:1-4294967294</format>
-            <description>Autonomous System Number</description>
-          </valueHelp>
-          <constraint>
-            <validator name="numeric" argument="--range 1-4294967294"/>
-          </constraint>
         </properties>
         <children>
           #include <include/bgp/bgp-common-config.xml.i>
         </children>
-      </tagNode>
+      </node>
     </children>
   </node>
 </interfaceDefinition>
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 @@
               <help>Routing protocol parameters</help>
             </properties>
             <children>
-              <tagNode name="bgp" owner="${vyos_conf_scripts_dir}/protocols_bgp.py $VAR(../../@)">
+              <node name="bgp" owner="${vyos_conf_scripts_dir}/protocols_bgp.py $VAR(../../@)">
                 <properties>
                   <help>Border Gateway Protocol (BGP)</help>
                   <priority>821</priority>
-                  <valueHelp>
-                    <format>u32:1-4294967294</format>
-                    <description>Autonomous System Number</description>
-                  </valueHelp>
-                  <constraint>
-                    <validator name="numeric" argument="--range 1-4294967294"/>
-                  </constraint>
                 </properties>
                 <children>
                   #include <include/bgp/bgp-common-config.xml.i>
                 </children>
-              </tagNode>
+              </node>
               <node name="isis" owner="${vyos_conf_scripts_dir}/protocols_isis.py $VAR(../../@)">
                 <properties>
                   <help>Intermediate System to Intermediate System (IS-IS)</help>
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 <http://www.gnu.org/licenses/>.
+
+# 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)