Bug 6841: a neighbor becomes inactivated in the routing engine after its peer-group is removed

Bug 6981: BGP commit failure with shutdown and peer-group attributes

rework of the peer-groups implementation.

2 files changed, 144 insertions, 60 deletions

diff --git a/lib/Vyatta/Quagga/Config.pm b/lib/Vyatta/Quagga/Config.pm
index 3c3187e0..f07ffa11 100644..100755
--- a/lib/Vyatta/Quagga/Config.pm
+++ b/lib/Vyatta/Quagga/Config.pm
@@ -122,6 +122,18 @@ sub deleteConfigTreeRecursive {
   return 0;
 }
 
+# Explicitly re-add an unchanged node to the list to be re-inserted into Quagga
+# Note that this method is not recursive!
+sub reInsertNode
+{
+	my ($self, $level) = @_;
+
+	_pdebug(1, "_reAddNode: level: $level");
+	if (! _qtree("$level", 'oneshot')) {
+		return 0;
+	}
+}
+
 ### End Public methods -
 ### Private methods
 sub _pdebug {
@@ -289,7 +301,8 @@ sub _qVarReplace {
   my $node = shift;
   my $qcommand = shift;
 
-  _pdebug(2, "_qVarReplace entry: node - $node\n_qVarReplace entry: qcommand - $qcommand");
+  _pdebug(2, "_qVarReplace entry: node - $node");
+  _pdebug(2, "_qVarReplace entry: qcommand - $qcommand");
 
   my @nodes = split /\s/, $node;
   my @qcommands = split /\s/, $qcommand;
@@ -367,34 +380,39 @@ sub _qCommandFind {
 # translate the adds/changes in a Vyatta config tree into Quagga vtysh commands.
 # recursively walks the tree.  
 # input:  $1 - the level of the Vyatta config tree to start at
-#         $2 - the action (set|delete)
+#			   ex: protocols bgp 1 neighbor 1.1.1.1 remote-as 10
+#         $2 - the action (set|delete|oneshot)
 # output: none - creates the %vtysh that contains the Quagga add commands
 sub _qtree {
   my ($level, $action) = @_;
   my @nodes;
-  my ($qcom, $vtysh);
-
-  $qcom = $_qcomref;
-  
-  # Would love to reference a global config and just reset Levels,
-  # but Vyatta::Config isn't recursion safe.
+  my $oneshot = 0;
+  my $qcom = $_qcomref;
+  my $vtysh = \%_vtysh;
   my $config = new Vyatta::Config;
   $config->setLevel($level);
-
-  # setup references for set or delete action
-  if ($action eq 'set') {
-    $vtysh = \%_vtysh;
+  
+  # setup data structures and references for various action types
+  if ($action eq 'oneshot') {
+	# split the last parameter out of the level string
+	$level =~ s/\s+/ /g;
+	my $index = rindex($level, " ");
+	my $node = substr($level, $index + 1);
+	$level = substr($level, 0, (-1 * length($node)));
+	chop $level;
+	$config->setLevel($level);
+	push @nodes, $node;
+	$action = 'set';
+	$oneshot = 1;
+  } elsif ($action eq 'set') {
     @nodes = $config->listNodes();
-  }
-  else {
+  } else {
     $vtysh = \%_vtyshdel;
     @nodes = $config->listDeleted();
 
     # handle special case for multi-nodes values being deleted
-    # listDeleted() doesn't return the node as deleted if it is a multi
-    # unless all values are deleted.
-    # TODO: fix listDeleted() in Config.pm
-    # This is really, really fugly.
+    # listDeleted() doesn't return the node as being deleted if 
+    # it is a multi unless all values are deleted.  This is suboptimal.
     my @all_nodes = $config->listNodes();
     foreach my $node (@all_nodes) {
       my @array = split /\s+/, $level;
@@ -409,7 +427,6 @@ sub _qtree {
         }
       }
     }
-
   } ## end else {
 
   _pdebug(1, "_qtree - action: $action\tlevel: $level");
@@ -420,8 +437,9 @@ sub _qtree {
       _pdebug(2, "_qtree - foreach node loop - node $node");
 
       # for set action, need to check that the node was actually changed.  Otherwise
-      # we end up re-writing every node to Quagga every commit, which is bad. Mmm' ok?
-      if (($action eq 'del') || ($config->isChanged("$node"))) {
+      # we end up re-writing every node to Quagga every commit.  For the other action
+      # types, just do it. 
+      if ($config->isChanged("$node") || ($action eq 'del') || $oneshot) {
         # is there a Quagga command template?
         # TODO: need to add function reference support to qcom hash for complicated nodes
         my $qcommand = _qCommandFind("$level $node", $action, $qcom);
@@ -460,20 +478,21 @@ sub _qtree {
             foreach my $val (@vals) {
               my $var = _qVarReplace("$level $node $val", $qcom->{$qcommand}->{$action});
               push @{$vtysh->{"$qcommand"}}, "$level $node $val  \036  $var";
-              _pdebug(1, "_qtree leaf node command: set $level $action $node $val \n\t\t\t\t\t$var");
+              _pdebug(1, "_qtree leaf node command: $action $level $node $val \n\t\t\t\t\t$var");
             }
-          }
-
-          else {
+          } else {
             my $var = _qVarReplace("$level $node", $qcom->{$qcommand}->{$action});
             push @{$vtysh->{"$qcommand"}}, "$level $node  \036  $var";
-            _pdebug(1, "_qtree node command: set $level $action $node \n\t\t\t\t$var");
+            _pdebug(1, "_qtree node command: $action $level $node \n\t\t\t\t$var");
           }
         }
       }
+      
       # recurse to next level in tree
-      _qtree("$level $node", 'del');
-      _qtree("$level $node", 'set');
+      if (! $oneshot) {
+      	_qtree("$level $node", 'del');
+      	_qtree("$level $node", 'set');
+ 	  }
     }
   }
 
diff --git a/scripts/bgp/vyatta-bgp.pl b/scripts/bgp/vyatta-bgp.pl
index 2ccf4724..73d35a98 100755
--- a/scripts/bgp/vyatta-bgp.pl
+++ b/scripts/bgp/vyatta-bgp.pl
@@ -406,7 +406,8 @@ my %qcom = (
   },
   'protocols bgp var neighbor var peer-group' => {
       set => 'router bgp #3 ; neighbor #5 peer-group #7',
-      del => 'router bgp #3 ; no neighbor #5 peer-group #7',
+      del => 'router bgp #3 ; no neighbor #5 peer-group #7 ; neighbor #5 activate',
+      noerr => 'del',
   },
   'protocols bgp var neighbor var port' => {
       set => 'router bgp #3 ; neighbor #5 port #7',
@@ -1041,6 +1042,8 @@ my %qcom = (
   },
 );
 
+my $qconfig = new Vyatta::Quagga::Config('protocols', \%qcom);
+
 my ( $pg, $as, $neighbor );
 my ( $main, $peername, $isneighbor, $checkpeergroups, $checksource, $isiBGPpeer, $wasiBGPpeer, $confedibgpasn);
 
@@ -1093,7 +1096,7 @@ sub check_peergroup_name {
     # Quagga treats the first byte as a potential IPv6 address
     # so we can't use it as a peer group name.  So let's check for it.
     if (/^[A-Fa-f]{1,4}$/) {
-	die "malformed peer-group name $neighbor\n";
+		die "malformed peer-group name $neighbor\n";
     }
 }
 
@@ -1165,9 +1168,72 @@ sub bgp_type_change {
     }
 }
 
+# delete a neighbor from a peer-group
+sub neighborFromPeerGroup
+{
+	my ($level) = @_;
+
+	my @overwritelist = ('allowas-in', 'allowas-in number', 'capability dynamic', 'capability orf', 
+							'disable-connected-check', 'distribute-list import', 
+							'disable-capability-negotiation', 'ebgp-multihop', 'filter-list import', 
+							'maximum-prefix', 'override-capability', 'passive', 'password', 'port', 
+							'prefix-list import', 'route-map import', 'shutdown', 
+							'soft-reconfiguration inbound', 'strict-capability-match',
+							'update-source', 'weight');
+
+	my $config = new Vyatta::Config;
+    $config->setLevel("protocols bgp $level");
+    
+	# make sure to re-add the overwrite nodes if they exist.
+	foreach my $node (@overwritelist) {
+		if ($config->exists($node)) {
+			$qconfig->reInsertNode("protocols bgp $level $node");
+		}
+	}
+}
+
+# add a neighbor to a peer-group
+sub neighborToPeerGroup
+{
+	my ($level) = @_;
+	
+	my @bannedlist = ('advertisement-interval', 'attribute-unchanged', 'capability orf',
+						'default-originate', 'distribute-list export', 'filter-list export',
+						'local-as', 'nexthop-self', 'prefix-list export', 'remove-private-as',
+						'route-map export', 'route-reflector-client', 'route-server-client',
+						'disable-send-community', 'timers', 'ttl-security', 'unsuppress-map');
+						
+	my @overwritelist = ('allowas-in', 'allowas-in number', 'capability dynamic', 'capability orf', 
+							'disable-connected-check', 'distribute-list import', 
+							'disable-capability-negotiation', 'ebgp-multihop', 'filter-list import', 
+							'maximum-prefix', 'override-capability', 'passive', 'password', 'port', 
+							'prefix-list import', 'route-map import', 'shutdown', 
+							'soft-reconfiguration inbound', 'strict-capability-match',
+							'update-source', 'weight');
+
+	my $config = new Vyatta::Config;
+    $config->setLevel("protocols bgp $level");
+
+	# first check that the neighbor doesn't contain anything in the bannedlist
+	foreach my $node (@bannedlist) {
+		if ($config->exists($node)) {
+			print "[ protocols bgp $level ]\n  parameter $node is incompatible with neighbors in a peer-group\n";
+			exit 1;
+		}
+	}
+	
+	# now make sure to re-add the overwrite nodes if they exist.
+	foreach my $node (@overwritelist) {
+		if ($config->exists($node)) {
+			$qconfig->reInsertNode("protocols bgp $level $node");
+		}
+	}
+}
+
 # check that changed neighbors have a remote-as or peer-group defined
 # and that all permutations of parameters and BGP type are correct
-sub check_remote_as {
+sub check_neighbor_parameters 
+{
     my $config = new Vyatta::Config;
     $config->setLevel('protocols bgp');
 
@@ -1217,45 +1283,43 @@ sub check_remote_as {
 
       # check neighbor if remote-as or peer-group has been changed
       my @neighbors = $config->listNodes("$as neighbor");
+      
       foreach my $neighbor (@neighbors) {
-	  next unless ( $config->isChanged("$as neighbor $neighbor remote-as")  ||
-                        $config->isDeleted("$as neighbor $neighbor remote-as")  ||
-                        $config->isChanged("$as neighbor $neighbor peer-group") ||
-                        $config->isDeleted("$as neighbor $neighbor peer-group") );
-
-          if ($config->isDeleted("$as neighbor $neighbor remote-as")) {
-            my @neighbor_params = undef;
-            @neighbor_params = $config->listNodes("$as neighbor $neighbor");
-            die "[protocols bgp $as neighbor $neighbor]\n  must delete the neighbor first if changing the remote-as\n"
-              if (@neighbor_params);
-          }
+      	next unless ($config->isChanged("$as neighbor $neighbor remote-as")  ||
+      					$config->isChanged("$as neighbor $neighbor peer-group") || 
+      					$config->isDeleted("$as neighbor $neighbor remote-as")  ||
+      					$config->isDeleted("$as neighbor $neighbor peer-group") );
 
-          # First check that we have a remote-as defined in the neighbor or that
-          # the neighbor is a member of a peer-group that has a remote-as defined
+          # remote-as checks: Make sure the neighbor has a remote-as defined locally or in the peer-group
           my ($remoteas, $peergroup, $peergroupas);
           $remoteas = $config->returnValue("$as neighbor $neighbor remote-as");
           if ($config->exists("$as neighbor $neighbor peer-group")) {
-            $peergroup = $config->returnValue("$as neighbor $neighbor peer-group");
+			$peergroup = $config->returnValue("$as neighbor $neighbor peer-group");
             if ($config->exists("$as peer-group $peergroup remote-as")) {
-              $peergroupas = $config->returnValue("$as peer-group $peergroup remote-as");
+				$peergroupas = $config->returnValue("$as peer-group $peergroup remote-as");
             }
           }
 
-          die "[protocols bgp $as neighbor $neighbor]\n  must define a remote-as or peer-group\n"
-            unless ($peergroup || $remoteas);
+          die "[ protocols bgp $as neighbor $neighbor ]\n  must set remote-as or peer-group with remote-as defined\n"
+			unless ($remoteas || $peergroupas);
 
-          die "[protocols bgp $as neighbor $neighbor]\n  remote-as should not be defined in both neighbor and peer-group\n"
+          die "[ protocols bgp $as neighbor $neighbor ]\n  remote-as should not be defined in both neighbor and peer-group\n"
             if ($remoteas && $peergroupas);
-     
-          die "[protocols bgp $as neighbor $neighbor]\n  must define a remote-as in neighbor or peer-group $peergroup\n"
-            if ( (! $remoteas) && (! $peergroupas) );
+          ## end remote-as checks
 
-          # now check if changing remote-as type from/to i/eBGP
+          # If the peer-group has changed since the last commit, update overwritable nodes
+          # We do this because Quagga removes nodes silently while vyatta-cfg does not.  These
+          # functions actually make Vyatta implentation of peer-groups more consistent.
+          if ($config->isChanged("$as neighbor $neighbor peer-group")) {
+          	neighborToPeerGroup("$as neighbor $neighbor");
+          } elsif ($config->isDeleted("$as neighbor $neighbor peer-group")) {
+          	neighborFromPeerGroup("$as neighbor $neighbor");
+          }            
+
+          # Check if changing BGP peer type from/to i/eBGP
           my $error = bgp_type_change($neighbor, $as, "neighbor");
           if ($error) { die "[protocols bgp $as neighbor $neighbor]\n  $error\n"; }
-
       } ## end foreach my $neighbor (@neighbors)
-
     } ## end foreach my $as (@asns)
 }
 
@@ -1364,21 +1428,22 @@ sub check_source {
     }
 }
 
-sub main {
+sub main 
+{
    # initialize the Quagga Config object with data from Vyatta config tree
-   my $qconfig = new Vyatta::Quagga::Config('protocols', \%qcom);
+   # my $qconfig = new Vyatta::Quagga::Config('protocols', \%qcom);
 
    # debug routines
-   #$qconfig->setDebugLevel('3');
+   $qconfig->setDebugLevel('3');
    #$qconfig->_reInitialize();
 
    # check that all changed neighbors have a proper remote-as or peer-group defined
    # and that migrations to/from iBGP eBGP are valid
-   check_remote_as();
+   check_neighbor_parameters();
 
    ## deletes with priority
    # delete everything in neighbor, ordered nodes last 
-   my @ordered = ('remote-as', 'shutdown', 'route-map', 'prefix-list', 'filter-list', 'distribute-list', 'unsuppress-map');  
+   my @ordered = ('remote-as', 'peer-group', 'shutdown', 'route-map', 'prefix-list', 'filter-list', 'distribute-list', 'unsuppress-map');  
    # notice the extra space in the level string.  keeps the parent from being deleted.
    $qconfig->deleteConfigTreeRecursive('protocols bgp var neighbor var ', undef, \@ordered) || die "exiting $?\n";
    $qconfig->deleteConfigTreeRecursive('protocols bgp var peer-group var ', undef, \@ordered) || die "exiting $?\n";