BGP: Fix dynamic instance reconfiguration

Every dynamic BGP was torn down on reconfig because the inherited
configuration is a little bit different than the parent one. Fixed this
by applying the same changes before the reconfiguration's memcmp().

This bug was introduced in 56af56a5ab8929bf211bdbe8fef28cb37c2cb7fa.

Also fixed interface pattern reconfiguration which always restarted.
Added not only comparison but also actual reconfiguration of the pattern
itself so that one can update the pattern without restarting a running
BGP session.

Finally, extended documentation a bit to cover dynamic BGP scenarios a
little bit better. Yet, it probably deserves a separate section on
dynamic BGP.

diff --git a/doc/bird.sgml b/doc/bird.sgml
index d75640c6b753ed989fc880ccc62f71b5440db2bf..ca039ef87b315c7a165b3da2514083f5ce351435 100644 (file)
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3059,7 +3059,14 @@ protocol bgp [<name>] {
        instances are spawned for incoming BGP connections (if source address
        matches the network prefix). It is possible to mix regular BGP instances
        with dynamic BGP instances and have multiple dynamic BGP instances with
-       different ranges.
+        different ranges. These spawned dynamic BGP instances share the parent
+        configuration, therefore by reconfiguring the parent protocol forces
+        reconfiguration of the spawned protocols.
+
+        When the neighbor range is changed, all the spawned dynamic instances
+        shut down. Reconfiguration clears all dynamic instances which were
+        previously disabled by the <cf/disable/ CLI command. This may re-enable
+       connection of some clients otherwise blocked by the disabled instance.
 
        <tag><label id="bgp-iface">interface "<m/text/"</tag>
        Define interface we should use for link-local BGP IPv6 sessions.
@@ -3996,6 +4003,7 @@ direction (re-export of routes to the BGP neighbor):
        <item><cf/require hostname/
        <item><cf/require graceful restart/
        <item><cf/require long lived graceful restart/
+       <item><cf/interface range/ if the already existing option is updated to another value.
 </itemize>
 
 <p>Channel options that cause a restart when changed:
@@ -4023,6 +4031,10 @@ incompatible with the current protocol state):
        <item><cf/max long lived stale time/
 </itemize>
 
+<p>All these configuration changes apply also to the spawned dynamic BGP sessions
+and therefore reconfiguring the parent protocol may lead to shutdown of some or all
+of the dynamic BGP sessions.
+
 <sect1>Attributes
 <label id="bgp-attr">
 

diff --git a/nest/iface.c b/nest/iface.c
index 499822b6455f28185b0373df9680a07085bd7d9a..a91f4ecf39873c36dc875f71345ddc845f983cbd 100644 (file)
--- a/nest/iface.c
+++ b/nest/iface.c
@@ -771,7 +771,7 @@ iface_patt_find(list *l, struct iface *i, struct ifa *a)
   return NULL;
 }
 
-static int
+int
 iface_plists_equal(struct iface_patt *pa, struct iface_patt *pb)
 {
   struct iface_patt_node *x, *y;

diff --git a/nest/iface.h b/nest/iface.h
index 1e4225eeda0ea6b6b0a9dae62bedcbff4e535112..66af3d9183a3dfc3650058db4a6edbe5a9bcfd20 100644 (file)
--- a/nest/iface.h
+++ b/nest/iface.h
@@ -178,6 +178,7 @@ struct iface_patt {
 
 int iface_patt_match(struct iface_patt *ifp, struct iface *i, struct ifa *a);
 struct iface_patt *iface_patt_find(list *l, struct iface *i, struct ifa *a);
+int iface_plists_equal(struct iface_patt *pa, struct iface_patt *pb);
 int iface_patts_equal(list *, list *, int (*)(struct iface_patt *, struct iface_patt *));
 
 

diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
index 862756f2a336c4188297451eb0250a1244307dfd..d623eb4a88cff533f70aee79b2f804428cc18dda 100644 (file)
--- a/proto/bgp/bgp.c
+++ b/proto/bgp/bgp.c
@@ -1100,7 +1100,6 @@ bgp_spawn(struct bgp_proto *pp, sock *sk)
   cf->remote_ip = sk->daddr;
   cf->local_ip = sk->saddr;
   cf->iface = sk->iface;
-  cf->ipatt = NULL;
 
   struct bgp_proto *p = SKIP_BACK(struct bgp_proto, p, proto_spawn(sym->proto, 0));
   p->postponed_sk = sk;
@@ -1979,8 +1978,8 @@ bgp_start_neighbor(struct bgp_proto *p)
   bgp_initiate(p);
 }
 
-static void
-bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
+static bool
+bgp_iface_match(struct bgp_proto *p, struct iface *i)
 {
   int ps = p->p.proto_state;
 
@@ -1988,11 +1987,17 @@ bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
   ASSERT_DIE(p->cf->strict_bind);
 
   if ((ps == PS_DOWN) || (ps == PS_STOP))
-    return;
+    return false;
 
   if (!iface_patt_match(p->cf->ipatt, i, NULL))
-    return;
+    return false;
+
+  return true;
+}
 
+static void
+bgp_iface_update(struct bgp_proto *p, uint flags, struct iface *i)
+{
   struct bgp_socket_params params = {
     .iface = i,
     .vrf = p->p.vrf,
@@ -2026,7 +2031,8 @@ bgp_if_notify(struct proto *P, uint flags, struct iface *i)
 {
   struct bgp_proto *p = (struct bgp_proto *) P;
   ASSERT_DIE(ipa_zero(p->cf->local_ip));
-  bgp_iface_update(p, flags, i);
+  if (bgp_iface_match(p, i))
+    bgp_iface_update(p, flags, i);
 }
 
 static void
@@ -2035,10 +2041,33 @@ bgp_ifa_notify(struct proto *P, uint flags, struct ifa *i)
   struct bgp_proto *p = (struct bgp_proto *) P;
   ASSERT_DIE(!ipa_zero(p->cf->local_ip));
 
-  if (ipa_equal(i->ip, p->cf->local_ip))
+  if (ipa_equal(i->ip, p->cf->local_ip) && bgp_iface_match(p, i->iface))
     bgp_iface_update(p, flags, i->iface);
 }
 
+static void
+bgp_if_reload(struct bgp_proto *p, struct iface_patt *patt)
+{
+  struct iface *iface;
+  struct ifa *a;
+
+  WALK_LIST(iface, iface_list)
+  {
+    bool old = iface_patt_match(p->cf->ipatt, iface, NULL);
+    bool new = iface_patt_match(patt, iface, NULL);
+
+    if (old == new)
+      continue;
+
+    if (ipa_zero(p->cf->local_ip))
+      bgp_iface_update(p, old ? IF_CHANGE_DOWN : IF_CHANGE_UP, iface);
+    else
+      WALK_LIST(a, iface->addrs)
+       if (ipa_equal(a->ip, p->cf->local_ip))
+         bgp_iface_update(p, old ? IF_CHANGE_DOWN : IF_CHANGE_UP, iface);
+  }
+}
+
 static void
 bgp_neigh_notify(neighbor *n)
 {
@@ -2317,6 +2346,10 @@ bgp_start(struct proto *P)
   /* Initialize listening socket list */
   init_list(&p->listen);
 
+  /* Setup interface notification hooks */
+  P->if_notify = (cf->ipatt && ipa_zero(cf->local_ip)) ? bgp_if_notify : NULL;
+  P->ifa_notify = (cf->ipatt && !ipa_zero(cf->local_ip)) ? bgp_ifa_notify : NULL;
+
   /* Initialize TCP-AO keys */
   init_list(&p->ao.keys);
   if (cf->auth_type == BGP_AUTH_AO)
@@ -2476,9 +2509,6 @@ bgp_init(struct proto_config *CF)
   p->remote_ip = cf->remote_ip;
   p->remote_as = cf->remote_as;
 
-  P->if_notify = (cf->ipatt && ipa_zero(cf->local_ip)) ? bgp_if_notify : NULL;
-  P->ifa_notify = (cf->ipatt && !ipa_zero(cf->local_ip)) ? bgp_ifa_notify : NULL;
-
   /* Hack: We use cf->remote_ip just to pass remote_ip from bgp_spawn() */
   if (cf->c.parent)
     cf->remote_ip = IPA_NONE;
@@ -2959,15 +2989,40 @@ static int
 bgp_reconfigure(struct proto *P, struct proto_config *CF)
 {
   struct bgp_proto *p = (void *) P;
-  const struct bgp_config *new = (void *) CF;
+  struct bgp_config *new = (void *) CF;
   const struct bgp_config *old = p->cf;
 
+  /* XXX: There is a section in documentation describing which configuration
+   * changes force BGP restart. When changing this function, you have to update
+   * also that part of documentation. */
+
   if (proto_get_router_id(CF) != p->local_id)
     return 0;
 
   if (bstrcmp(proto_get_hostname(CF), p->hostname))
     return 0;
 
+  /* Fix the virtual configuration so that memcpy does not fail */
+  if (old->c.parent)
+  {
+    new->remote_ip = old->remote_ip;
+    new->local_ip = old->local_ip;
+
+    /* Pre-check interfaces */
+    if (new->ipatt)
+    {
+      if (!old->iface || !iface_patt_match(new->ipatt, old->iface, NULL))
+       return 0;
+    }
+    else if (new->iface)
+    {
+      if (old->iface != new->iface)
+       return 0;
+    }
+
+    new->iface = old->iface;
+  }
+
   int same = !memcmp(((byte *) old) + sizeof(struct proto_config),
                     ((byte *) new) + sizeof(struct proto_config),
                     // password item is last and must be checked separately
@@ -2978,7 +3033,19 @@ bgp_reconfigure(struct proto *P, struct proto_config *CF)
     && !bstrcmp(old->dynamic_name, new->dynamic_name)
     && (old->dynamic_name_digits == new->dynamic_name_digits);
 
-  /* Reconfigure TCP-AP */
+  /* Reconfigure interface notification hooks */
+  same = same && (!P->if_notify == !(new->ipatt && ipa_zero(new->local_ip)));
+  same = same && (!P->ifa_notify == !(new->ipatt && !ipa_zero(new->local_ip)));
+
+  /* Differing pattern lists cause an update of the listening sockets
+   * and also if the connection is up, then active sockets. */
+  bool need_if_reload = same && new->ipatt && old->ipatt && !iface_plists_equal(new->ipatt, old->ipatt);
+  if (need_if_reload && !bgp_is_dynamic(p) && (
+       p->incoming_conn.sk && !iface_patt_match(new->ipatt, p->incoming_conn.sk->iface, NULL) ||
+       p->outgoing_conn.sk && !iface_patt_match(new->ipatt, p->outgoing_conn.sk->iface, NULL)))
+    same = 0;
+
+  /* Reconfigure TCP-AO */
   same = same && bgp_reconfigure_ao_keys(p, new);
 
   /* FIXME: Move channel reconfiguration to generic protocol code ? */
@@ -3026,6 +3093,9 @@ bgp_reconfigure(struct proto *P, struct proto_config *CF)
   if (p->start_state > BSS_PREPARE)
     bgp_update_bfd(p, new->bfd);
 
+  if (need_if_reload)
+    bgp_if_reload(p, new->ipatt);
+
   return 1;
 }
 

diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index 7e70d745b95a2aaea545fea734a7a87c3c29d29b..9a206cfe818f480ed32f310856d8bb9fe265a5ed 100644 (file)
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -76,11 +76,12 @@ struct bgp_af_desc {
 
 struct bgp_config {
   struct proto_config c;
+  /* Beginning here, all options must be comparable by memcmp().
+   * Do not add anything above unless you want this comment to become futile. */
   u32 local_as, remote_as;
   ip_addr local_ip;                    /* Source address to use */
   ip_addr remote_ip;
   struct iface *iface;                 /* Interface for link-local addresses */
-  struct iface_patt *ipatt;            /* Interface pattern for dynamic strict bind */
   u16 local_port;                      /* Local listening port */
   u16 remote_port;                     /* Neighbor destination port */
   int peer_type;                       /* Internal or external BGP (BGP_PT_*, optional) */
@@ -147,6 +148,7 @@ struct bgp_config {
   const char *password;                        /* Password used for MD5 authentication */
   struct ao_config *ao_keys;           /* Keys for TCP-AO authentication */
   net_addr *remote_range;              /* Allowed neighbor range for dynamic BGP */
+  struct iface_patt *ipatt;            /* Interface patterns for dynamic strict bind */
   const char *dynamic_name;            /* Name pattern for dynamic BGP */
   int dynamic_name_digits;             /* Minimum number of digits for dynamic names */
   int check_link;                      /* Use iface link state for liveness detection */

diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index c8729544594c7374b0556307cfdbeeca80af957c..da28de59608c72dc3494b357ba9761f1d4371c40 100644 (file)
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -168,7 +168,7 @@ bgp_proto:
      BGP_CFG->remote_range = n;
    }
  | bgp_proto INTERFACE text ';' { BGP_CFG->iface = if_get_by_name($3); }
- | bgp_proto INTERFACE RANGE iface_patt ';' { BGP_CFG->ipatt = this_ipatt; }
+ | bgp_proto INTERFACE RANGE iface_patt ';' { BGP_CFG->ipatt =  this_ipatt; }
  | bgp_proto ONLINK bool ';' { BGP_CFG->onlink = $3; }
  | bgp_proto RR CLUSTER ID idval ';' { BGP_CFG->rr_cluster_id = $5; }
  | bgp_proto RR CLIENT bool ';' { BGP_CFG->rr_client = $4; }