bgp: T3311: remove remote-as from address-family

When moving from Quagga to FRR the BGP address-family was extended by an
invalid peer-group statement. FRR always moved a configured peer-group
from the AFI level down to the neighbor level.

With the migration to FRR reload we must take care about this by ourselves.

7 files changed, 606 insertions, 35 deletions

diff --git a/interface-definitions/include/bgp-afi-peer-group.xml.i b/interface-definitions/include/bgp-afi-peer-group.xml.i
deleted file mode 100644
index c98a91030..000000000
--- a/interface-definitions/include/bgp-afi-peer-group.xml.i
+++ /dev/null
@@ -1,7 +0,0 @@
-<!-- included start from bgp-afi-peer-group.xml.i -->
-<leafNode name="peer-group">
-  <properties>
-    <help>Peer group used for this neighbor</help>
-  </properties>
-</leafNode>
-<!-- included end -->
diff --git a/interface-definitions/include/bgp-neighbor-afi-ipv4-unicast.xml.i b/interface-definitions/include/bgp-neighbor-afi-ipv4-unicast.xml.i
index 8f6cf06b1..ece277fbf 100644
--- a/interface-definitions/include/bgp-neighbor-afi-ipv4-unicast.xml.i
+++ b/interface-definitions/include/bgp-neighbor-afi-ipv4-unicast.xml.i
@@ -12,7 +12,6 @@
         #include <include/bgp-afi-capability-orf.xml.i>
       </children>
     </node>
-    #include <include/bgp-afi-peer-group.xml.i>
     #include <include/bgp-afi-ipv4-prefix-list.xml.i>
     #include <include/bgp-afi-common.xml.i>
   </children>
diff --git a/interface-definitions/include/bgp-neighbor-afi-ipv6-unicast.xml.i b/interface-definitions/include/bgp-neighbor-afi-ipv6-unicast.xml.i
index aea10c20c..e43c34113 100644
--- a/interface-definitions/include/bgp-neighbor-afi-ipv6-unicast.xml.i
+++ b/interface-definitions/include/bgp-neighbor-afi-ipv6-unicast.xml.i
@@ -12,7 +12,6 @@
         #include <include/bgp-afi-capability-orf.xml.i>
       </children>
     </node>
-    #include <include/bgp-afi-peer-group.xml.i>
     #include <include/bgp-afi-ipv6-nexthop-local.xml.i>
     #include <include/bgp-afi-ipv6-prefix-list.xml.i>
     #include <include/bgp-afi-common.xml.i>
diff --git a/interface-definitions/include/bgp-shutdown.xml.i b/interface-definitions/include/bgp-shutdown.xml.i
index 330120bba..fefbfcebb 100644
--- a/interface-definitions/include/bgp-shutdown.xml.i
+++ b/interface-definitions/include/bgp-shutdown.xml.i
@@ -1,7 +1,7 @@
 <!-- included start from bgp-shutdown.xml.i -->
 <leafNode name="shutdown">
   <properties>
-    <help>Administratively shut down peer-group</help>
+    <help>Administratively shut down this neighbor</help>
     <valueless/>
   </properties>
 </leafNode>
diff --git a/smoketest/configs/bgp-bfd-communities b/smoketest/configs/bgp-bfd-communities
new file mode 100644
index 000000000..3b3056a51
--- /dev/null
+++ b/smoketest/configs/bgp-bfd-communities
@@ -0,0 +1,533 @@
+interfaces {
+    ethernet eth0 {
+        address 192.0.2.100/25
+        address 2001:db8::ffff/64
+    }
+    loopback lo {
+    }
+}
+policy {
+    large-community-list ANYCAST_ALL {
+        rule 10 {
+            action permit
+            description "Allow all anycast from anywhere"
+            regex "4242420696:100:.*"
+        }
+    }
+    large-community-list ANYCAST_INT {
+        rule 10 {
+            action permit
+            description "Allow all anycast from int"
+            regex 4242420696:100:1
+        }
+    }
+    prefix-list BGP-BACKBONE-IN {
+        description "Inbound backbone routes from other sites"
+        rule 10 {
+            action deny
+            description "Block default route"
+            prefix 0.0.0.0/0
+        }
+        rule 20 {
+            action deny
+            description "Block int primary"
+            ge 21
+            prefix 192.168.0.0/20
+        }
+        rule 30 {
+            action deny
+            description "Block loopbacks"
+            ge 25
+            prefix 192.168.253.0/24
+        }
+        rule 40 {
+            action deny
+            description "Block backbone peering"
+            ge 25
+            prefix 192.168.254.0/24
+        }
+        rule 999 {
+            action permit
+            description "Allow everything else"
+            ge 1
+            prefix 0.0.0.0/0
+        }
+    }
+    prefix-list BGP-BACKBONE-OUT {
+        description "Outbound backbone routes to other sites"
+        rule 10 {
+            action permit
+            description "Int primary"
+            ge 23
+            prefix 192.168.0.0/20
+        }
+    }
+    prefix-list GLOBAL {
+        description "Globally redistributed routes"
+        rule 10 {
+            action permit
+            prefix 192.168.100.1/32
+        }
+        rule 20 {
+            action permit
+            prefix 192.168.7.128/25
+        }
+    }
+    prefix-list6 BGP-BACKBONE-IN-V6 {
+        description "Inbound backbone routes from other sites"
+        rule 10 {
+            action deny
+            description "Block default route"
+            prefix ::/0
+        }
+        rule 20 {
+            action deny
+            description "Block int primary"
+            ge 53
+            prefix fd52:d62e:8011::/52
+        }
+        rule 30 {
+            action deny
+            description "Block peering and stuff"
+            ge 53
+            prefix fd52:d62e:8011:f000::/52
+        }
+        rule 999 {
+            action permit
+            description "Allow everything else"
+            ge 1
+            prefix ::/0
+        }
+    }
+    prefix-list6 BGP-BACKBONE-OUT-V6 {
+        description "Outbound backbone routes to other sites"
+        rule 10 {
+            action permit
+            ge 64
+            prefix fd52:d62e:8011::/52
+        }
+    }
+    prefix-list6 GLOBAL-V6 {
+        description "Globally redistributed routes"
+        rule 10 {
+            action permit
+            ge 64
+            prefix fd52:d62e:8011:2::/63
+        }
+    }
+    route-map BGP-REDISTRIBUTE {
+        rule 10 {
+            action permit
+            description "Prepend AS and allow VPN and modem"
+            match {
+                ip {
+                    address {
+                        prefix-list GLOBAL
+                    }
+                }
+            }
+            set {
+                as-path-prepend 4242420666
+            }
+        }
+        rule 20 {
+            action permit
+            description "Allow VPN"
+            match {
+                ipv6 {
+                    address {
+                        prefix-list GLOBAL-V6
+                    }
+                }
+            }
+        }
+    }
+    route-map BGP-BACKBONE-IN {
+        rule 10 {
+            action permit
+            match {
+                ip {
+                    address {
+                        prefix-list BGP-BACKBONE-IN
+                    }
+                }
+            }
+        }
+        rule 20 {
+            action permit
+            match {
+                ipv6 {
+                    address {
+                        prefix-list BGP-BACKBONE-IN-V6
+                    }
+                }
+            }
+        }
+        rule 30 {
+            action permit
+            match {
+                large-community {
+                    large-community-list ANYCAST_ALL
+                }
+            }
+        }
+    }
+    route-map BGP-BACKBONE-OUT {
+        rule 10 {
+            action permit
+            match {
+                ip {
+                    address {
+                        prefix-list BGP-BACKBONE-OUT
+                    }
+                }
+            }
+        }
+        rule 20 {
+            action permit
+            match {
+                ipv6 {
+                    address {
+                        prefix-list BGP-BACKBONE-OUT-V6
+                    }
+                }
+            }
+        }
+        rule 30 {
+            action permit
+            match {
+                large-community {
+                    large-community-list ANYCAST_INT
+                }
+            }
+            set {
+                as-path-prepend 4242420666
+            }
+        }
+    }
+}
+protocols {
+    bfd {
+        peer 192.168.253.1 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address 192.168.253.3
+            }
+        }
+        peer 192.168.253.2 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address 192.168.253.3
+            }
+        }
+        peer 192.168.253.6 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address 192.168.253.3
+            }
+        }
+        peer 192.168.253.7 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address 192.168.253.3
+            }
+        }
+        peer 192.168.253.12 {
+            interval {
+                receive 100
+                transmit 100
+            }
+            multihop
+            source {
+                address 192.168.253.3
+            }
+        }
+        peer fd52:d62e:8011:fffe:192:168:253:1 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address fd52:d62e:8011:fffe:192:168:253:3
+            }
+        }
+        peer fd52:d62e:8011:fffe:192:168:253:2 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address fd52:d62e:8011:fffe:192:168:253:3
+            }
+        }
+        peer fd52:d62e:8011:fffe:192:168:253:6 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address fd52:d62e:8011:fffe:192:168:253:3
+            }
+        }
+        peer fd52:d62e:8011:fffe:192:168:253:7 {
+            interval {
+                receive 50
+                transmit 50
+            }
+            multihop
+            source {
+                address fd52:d62e:8011:fffe:192:168:253:3
+            }
+        }
+        peer fd52:d62e:8011:fffe:192:168:253:12 {
+            interval {
+                receive 100
+                transmit 100
+            }
+            multihop
+            source {
+                address fd52:d62e:8011:fffe:192:168:253:3
+            }
+        }
+    }
+    bgp 4242420666 {
+        address-family {
+            ipv4-unicast {
+                redistribute {
+                    connected {
+                        route-map BGP-REDISTRIBUTE
+                    }
+                    static {
+                        route-map BGP-REDISTRIBUTE
+                    }
+                }
+            }
+            ipv6-unicast {
+                redistribute {
+                    connected {
+                        route-map BGP-REDISTRIBUTE
+                    }
+                }
+            }
+        }
+        neighbor 192.168.253.1 {
+            peer-group INT
+        }
+        neighbor 192.168.253.2 {
+            peer-group INT
+        }
+        neighbor 192.168.253.6 {
+            peer-group DAL13
+        }
+        neighbor 192.168.253.7 {
+            peer-group DAL13
+        }
+        neighbor 192.168.253.12 {
+            address-family {
+                ipv4-unicast {
+                    route-map {
+                        export BGP-BACKBONE-OUT
+                        import BGP-BACKBONE-IN
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd {
+            }
+            ebgp-multihop 2
+            remote-as 4242420669
+            update-source dum0
+        }
+        neighbor fd52:d62e:8011:fffe:192:168:253:1 {
+            address-family {
+                ipv6-unicast {
+                    peer-group INTv6
+                }
+            }
+        }
+        neighbor fd52:d62e:8011:fffe:192:168:253:2 {
+            address-family {
+                ipv6-unicast {
+                    peer-group INTv6
+                }
+            }
+        }
+        neighbor fd52:d62e:8011:fffe:192:168:253:6 {
+            address-family {
+                ipv6-unicast {
+                    peer-group DAL13v6
+                }
+            }
+        }
+        neighbor fd52:d62e:8011:fffe:192:168:253:7 {
+            address-family {
+                ipv6-unicast {
+                    peer-group DAL13v6
+                }
+            }
+        }
+        neighbor fd52:d62e:8011:fffe:192:168:253:12 {
+            address-family {
+                ipv6-unicast {
+                    route-map {
+                        export BGP-BACKBONE-OUT
+                        import BGP-BACKBONE-IN
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd {
+            }
+            ebgp-multihop 2
+            remote-as 4242420669
+            update-source dum0
+        }
+        parameters {
+            confederation {
+                identifier 4242420696
+                peers 4242420668
+                peers 4242420669
+            }
+            default {
+                no-ipv4-unicast
+            }
+            distance {
+                global {
+                    external 220
+                    internal 220
+                    local 220
+                }
+            }
+            graceful-restart {
+            }
+        }
+        peer-group DAL13 {
+            address-family {
+                ipv4-unicast {
+                    route-map {
+                        export BGP-BACKBONE-OUT
+                        import BGP-BACKBONE-IN
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd
+            ebgp-multihop 2
+            remote-as 4242420668
+            update-source dum0
+        }
+        peer-group DAL13v6 {
+            address-family {
+                ipv6-unicast {
+                    route-map {
+                        export BGP-BACKBONE-OUT
+                        import BGP-BACKBONE-IN
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd
+            ebgp-multihop 2
+            remote-as 4242420668
+            update-source dum0
+        }
+        peer-group INT {
+            address-family {
+                ipv4-unicast {
+                    default-originate {
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd
+            remote-as 4242420666
+            update-source dum0
+        }
+        peer-group INTv6 {
+            address-family {
+                ipv6-unicast {
+                    default-originate {
+                    }
+                    soft-reconfiguration {
+                        inbound
+                    }
+                }
+            }
+            bfd
+            remote-as 4242420666
+            update-source dum0
+        }
+    }
+}
+system {
+    config-management {
+        commit-revisions 200
+    }
+    console {
+        device ttyS0 {
+            speed 115200
+        }
+    }
+    host-name vyos
+    login {
+        user vyos {
+            authentication {
+                encrypted-password $6$2Ta6TWHd/U$NmrX0x9kexCimeOcYK1MfhMpITF9ELxHcaBU/znBq.X2ukQOj61fVI2UYP/xBzP4QtiTcdkgs7WOQMHWsRymO/
+                plaintext-password ""
+            }
+            level admin
+        }
+    }
+    ntp {
+        server 0.pool.ntp.org {
+        }
+        server 1.pool.ntp.org {
+        }
+        server 2.pool.ntp.org {
+        }
+    }
+    syslog {
+        global {
+            facility all {
+                level info
+            }
+            facility protocols {
+                level debug
+            }
+        }
+    }
+    time-zone Europe/Berlin
+}
+
+/* Warning: Do not remove the following line. */
+/* === vyatta-config-version: "broadcast-relay@1:cluster@1:config-management@1:conntrack-sync@1:conntrack@1:dhcp-relay@2:dhcp-server@5:dns-forwarding@1:firewall@5:ipsec@5:l2tp@1:mdns@1:nat@4:ntp@1:pptp@1:qos@1:quagga@6:snmp@1:ssh@1:system@10:vrrp@2:wanloadbalance@3:webgui@1:webproxy@1:webproxy@2:zone-policy@1" === */
+/* Release version: 1.2.6-S1 */
diff --git a/src/conf_mode/protocols_bgp.py b/src/conf_mode/protocols_bgp.py
index 54352460c..b5bb018ae 100755
--- a/src/conf_mode/protocols_bgp.py
+++ b/src/conf_mode/protocols_bgp.py
@@ -54,6 +54,26 @@ def get_config(config=None):
 
     return bgp
 
+def verify_remote_as(peer_config, asn_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)
+        if tmp: return tmp
+
+    if 'interface' in peer_config:
+        if 'remote_as' in peer_config['interface']:
+            return peer_config['interface']['remote_as']
+
+        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)
+            if tmp: return tmp
+
+    return None
+
 def verify(bgp):
     if not bgp:
         return None
@@ -79,18 +99,13 @@ def verify(bgp):
                         raise ConfigError(f'Specified peer-group "{peer_group}" for '\
                                           f'neighbor "{neighbor}" does not exist!')
 
-                # Some checks can/must only be done on a neighbor and nor a peer-group
+
+                # 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 'interface' in peer_config:
-                        if 'remote_as' not in peer_config['interface']:
-                            if 'peer_group' not in peer_config['interface'] or 'remote_as' not in asn_config['peer_group'][ peer_config['interface']['peer_group'] ]:
-                                raise ConfigError('Remote AS must be set for neighbor or peer-group!')
-
-                    elif 'remote_as' not in peer_config:
-                        if 'peer_group' not in peer_config or 'remote_as' not in asn_config['peer_group'][ peer_config['peer_group'] ]:
-                            raise ConfigError('Remote AS must be set for neighbor or peer-group!')
+                    if not verify_remote_as(peer_config, asn_config):
+                        raise ConfigError(f'Neighbor "{peer}" remote-as must be set!')
 
                 for afi in ['ipv4_unicast', 'ipv6_unicast', 'l2vpn_evpn']:
                     # Bail out early if address family is not configured
diff --git a/src/migration-scripts/quagga/6-to-7 b/src/migration-scripts/quagga/6-to-7
index f7aca0d2b..25cf5eebd 100755
--- a/src/migration-scripts/quagga/6-to-7
+++ b/src/migration-scripts/quagga/6-to-7
@@ -17,14 +17,17 @@
 # - T3037, BGP address-family ipv6-unicast capability dynamic does not exist in
 #   FRR, there is only a base, per neighbor dynamic capability, migrate config
 
-import sys
+from sys import argv
+from sys import exit
 from vyos.configtree import ConfigTree
+from vyos.template import is_ipv4
+from vyos.template import is_ipv6
 
-if (len(sys.argv) < 2):
+if (len(argv) < 2):
     print("Must specify file name!")
-    sys.exit(1)
+    exit(1)
 
-file_name = sys.argv[1]
+file_name = argv[1]
 
 with open(file_name, 'r') as f:
     config_file = f.read()
@@ -34,7 +37,7 @@ config = ConfigTree(config_file)
 
 if not config.exists(base):
     # Nothing to do
-    sys.exit(0)
+    exit(0)
 
 # Check if BGP is actually configured and obtain the ASN
 asn_list = config.list_nodes(base)
@@ -55,30 +58,59 @@ if asn_list:
                 config.delete(send_comm_path)
 
             cap_dynamic = False
+            peer_group = None
             for afi in ['ipv4-unicast', 'ipv6-unicast']:
-                afi_path = bgp_base + [neighbor_type, neighbor, 'address-family', afi, 'capability', 'dynamic']
-                if config.exists(afi_path):
+                afi_path = bgp_base + [neighbor_type, neighbor, 'address-family', afi]
+                # Exit loop early if AFI does not exist
+                if not config.exists(afi_path):
+                    continue
+
+                cap_path = afi_path + ['capability', 'dynamic']
+                if config.exists(cap_path):
                     cap_dynamic = True
-                    config.delete(afi_path)
+                    config.delete(cap_path)
+
+                    # We have now successfully migrated the address-family
+                    # specific dynamic capability to the neighbor/peer-group
+                    # level. If this has been the only option under the
+                    # address-family nodes, we can clean them up by checking if
+                    # no other nodes are left under that tree and if so, delete
+                    # the parent.
+                    #
+                    # We walk from the most inner node to the most outer one.
+                    cleanup = -1
+                    while len(config.list_nodes(cap_path[:cleanup])) == 0:
+                        config.delete(cap_path[:cleanup])
+                        cleanup -= 1
+
+                peer_group_path = afi_path + ['peer-group']
+                if config.exists(peer_group_path):
+                    if ((is_ipv4(neighbor) and afi == 'ipv4-unicast') or
+                        (is_ipv6(neighbor) and afi == 'ipv6-unicast')):
+                        peer_group = config.return_value(peer_group_path)
+
+                    config.delete(peer_group_path)
 
-                    # We have now successfully migrated the address-family specific
-                    # dynamic capability to the neighbor/peer-group level. If this
-                    # has been the only option under the address-family nodes, we
-                    # can clean them up by checking if no other nodes are left under
-                    # that tree and if so, delete the parent.
+                    # We have now successfully migrated the address-family
+                    # specific peer-group to the neighbor level. If this has
+                    # been the only option under the address-family nodes, we
+                    # can clean them up by checking if no other nodes are left
+                    # under that tree and if so, delete the parent.
                     #
                     # We walk from the most inner node to the most outer one.
                     cleanup = -1
-                    while len(config.list_nodes(afi_path[:cleanup])) == 0:
-                        config.delete(afi_path[:cleanup])
+                    while len(config.list_nodes(peer_group_path[:cleanup])) == 0:
+                        config.delete(peer_group_path[:cleanup])
                         cleanup -= 1
 
             if cap_dynamic:
                 config.set(bgp_base + [neighbor_type, neighbor, 'capability', 'dynamic'])
+            if peer_group:
+                config.set(bgp_base + [neighbor_type, neighbor, 'peer-group'], value=peer_group)
 
 try:
     with open(file_name, 'w') as f:
         f.write(config.to_string())
 except OSError as e:
     print("Failed to save the modified config: {}".format(e))
-    sys.exit(1)
+    exit(1)