IGP metric: Split out local_metric again

When removing BIRD 2 RTA, I joined rta->igp_metric with
ea_gen_igp_metric, while these two attributes actually have different
semantics. Reintroducing the distinction but with the first one
named local_metric.

This join caused quite some confusion as the igp_metric attribute set in
import filters gets rewritten by recursive nexthop resolution in BIRD 3
up to 3.1.x. Now the igp_metric stays intact and the local_metric is
the original rta->igp_metric which always meant "cost of the local part
of the whole route".

The local_metric attribute also represents the "interior cost" as used
in RFC 4271. When using BGP as an underlay / IGP, one should explicitly
set the igp_metric in the import filter, while the local_metric will be
updated by next hop resolver independently.

Fixes: #154

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index e91047452826dfe423a6ac3b575a0774bfccb68f..4a1b2708e82dfafac573e32da77fa769ffc56671 100644 (file)
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1467,6 +1467,7 @@ build-netlab:
     - cd $TOOLS_DIR
     - sudo git clean -fx
     - git pull --ff-only
+    - git checkout -f origin/data-v3.2
     - "mv $DIR/build-netlab/* netlab/common/"
     - ln -s $STAYRTR_BINARY netlab/common/stayrtr
     - cd netlab
@@ -1480,6 +1481,7 @@ build-netlab:
     - "mkdir $DIR/netlab-failure"
     - git status --porcelain > $DIR/netlab-failure.log
     - for f in $(git status --porcelain | sed -rn 's#^.[^DRT] netlab/##p'); do mkdir -p $DIR/netlab-failure/$(dirname $f); sudo chmod a+rw $f; sudo mv $f $DIR/netlab-failure/$(dirname $f); done
+    - git checkout -f master
   artifacts:
     when: on_failure
     untracked: true

diff --git a/doc/bird.sgml b/doc/bird.sgml
index b0ac094ae40a944f784dafe78b0ec447612dcc4d..0023e2dd849272bb6b755ee132fe0640c5a5177c 100644 (file)
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -2277,8 +2277,21 @@ Common route attributes are:
        <tag><label id="rta-igp-metric"><m/int/ igp_metric</tag>
        The optional attribute that can be used to specify a distance to the
        network for routes that do not have a native protocol metric attribute
-       (like <cf/ospf_metric1/ for OSPF routes). It is used mainly by BGP to
-       compare internal distances to boundary routers (see below).
+       (like <cf/ospf_metric1/ for OSPF routes). It is used to fill in
+       <ref id="rta-local-metric" name="local_metric"> attribute when recursive
+       nexthops are resolved, so that BGP can compare internal distances
+       to boundary routers (see below).
+
+       <tag><label id="rta-local-metric"><m/int/ local_metric</tag>
+       Attribute storing the route's metric to the resolved recursive nexthop,
+       mainly used by BGP best route selection. Also known as "interior cost"
+       in RFC 4271.
+        It's the copy of the underlying protocol's metric, e.g.
+        <cf/ospf_metric1/, or the generic <cf/igp_metric/. For recursive nexthops,
+       setting the <cf/local_metric/ attribute in the import filters is futile,
+       as the nexthop resolver overwrites it.
+       Note: In versions 3.0.x and 3.1.x, the <cf/igp_metric/ attribute was confusingly
+       used for this purpose.
 
        <tag><label id="rta-mpls-label"><m/int/ mpls_label</tag>
        Local MPLS label attached to the route. This attribute is produced by
@@ -3006,7 +3019,7 @@ choose among them and so on.
        <item>Prefer IGP origin over EGP and EGP origin over incomplete.
        <item>Prefer the lowest value of the Multiple Exit Discriminator.
        <item>Prefer routes received via eBGP over ones received via iBGP.
-       <item>Prefer routes with lower internal distance to a boundary router.
+       <item>Prefer routes with lower <ref id="rta-local-metric" name="internal distance"> to a boundary router.
        <item>Prefer the route with the lowest value of router ID of the
        advertising router.
 </itemize>

diff --git a/filter/test.conf b/filter/test.conf
index 64d130645ed06b3f36227bdfea5194bb77c69480..758a7d0c7aa950658ee32b49745998f73c991488 100644 (file)
--- a/filter/test.conf
+++ b/filter/test.conf
@@ -2482,6 +2482,7 @@ bool t;
        test_ca_lclist7 = ---empty---;
 
        igp_metric = 53;
+       local_metric = 75;
        babel_metric = 64;
        t = defined(babel_router_id);
 

diff --git a/lib/route.h b/lib/route.h
index 21b46820867fbec7e8d39b8f13d093c58250dec7..4b7a74b086fdaa41e8ef09079c133b17194031d7 100644 (file)
--- a/lib/route.h
+++ b/lib/route.h
@@ -454,12 +454,15 @@ extern struct ea_class ea_gen_preference;
 static inline u32 rt_get_preference(const rte *rt)
 { return ea_get_int(rt->attrs, &ea_gen_preference, 0); }
 
-/* IGP metric: second-order comparison */
+/* IGP metric: explicit metric to be used as local_metric in case of recursive nexthop */
 extern struct ea_class ea_gen_igp_metric;
 u32 rt_get_igp_metric(const rte *rt);
 #define IGP_METRIC_UNKNOWN 0x80000000  /* Default igp_metric used when no other
                                           protocol-specific metric is availabe */
 
+/* local metric: second-order comparison */
+extern struct ea_class ea_gen_local_metric;
+
 /* From: Advertising router */
 extern struct ea_class ea_gen_from;
 

diff --git a/misc/gitlab/template.yml.j2 b/misc/gitlab/template.yml.j2
index 7b1fd57a338d2fdd52ce8e5a4da439a33439143e..99c6feba785cf81f910965c57d0a60565e7c07ba 100644 (file)
--- a/misc/gitlab/template.yml.j2
+++ b/misc/gitlab/template.yml.j2
@@ -448,6 +448,7 @@ build-netlab:
     - cd $TOOLS_DIR
     - sudo git clean -fx
     - git pull --ff-only
+    - git checkout -f origin/data-v3.2
     - "mv $DIR/build-netlab/* netlab/common/"
     - ln -s $STAYRTR_BINARY netlab/common/stayrtr
     - cd netlab
@@ -461,6 +462,7 @@ build-netlab:
     - "mkdir $DIR/netlab-failure"
     - git status --porcelain > $DIR/netlab-failure.log
     - for f in $(git status --porcelain | sed -rn 's#^.[^DRT] netlab/##p'); do mkdir -p $DIR/netlab-failure/$(dirname $f); sudo chmod a+rw $f; sudo mv $f $DIR/netlab-failure/$(dirname $f); done
+    - git checkout -f master
   artifacts:
     when: on_failure
     untracked: true

diff --git a/nest/rt-attr.c b/nest/rt-attr.c
index b28744af3a813a1817767c06439109c7bd75b55d..a42a337daf28bb930ad4020fff84959e927c8b40 100644 (file)
--- a/nest/rt-attr.c
+++ b/nest/rt-attr.c
@@ -66,6 +66,11 @@ struct ea_class ea_gen_igp_metric = {
   .type = T_INT,
 };
 
+struct ea_class ea_gen_local_metric = {
+  .name = "local_metric",
+  .type = T_INT,
+};
+
 struct ea_class ea_gen_preference = {
   .name = "preference",
   .type = T_INT,
@@ -1323,10 +1328,10 @@ ea_show(struct cli *c, const eattr *e)
     switch (e->type)
       {
        case T_INT:
-         if ((cls == &ea_gen_igp_metric) && e->u.data >= IGP_METRIC_UNKNOWN)
-           return;
-
-         bsprintf(pos, "%u", e->u.data);
+         if ((cls == &ea_gen_local_metric) && e->u.data >= IGP_METRIC_UNKNOWN)
+           bsprintf(pos, "unknown");
+         else
+           bsprintf(pos, "%u", e->u.data);
          break;
        case T_OPAQUE:
          opaque_format(ad, pos, end - pos);
@@ -1744,6 +1749,7 @@ rta_init(void)
   /* Other generic route attributes */
   ea_register_init(&ea_gen_preference);
   ea_register_init(&ea_gen_igp_metric);
+  ea_register_init(&ea_gen_local_metric);
   ea_register_init(&ea_gen_from);
   ea_register_init(&ea_gen_source);
   ea_register_init(&ea_gen_flowspec_valid);

diff --git a/nest/rt-table.c b/nest/rt-table.c
index 9141ddc6c2da5eb39e530e88bf024f8e8974e42d..c153f3c2ae5e5f3455fcfff44a1707e23e882a13 100644 (file)
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -4179,8 +4179,6 @@ rta_apply_hostentry(ea_list **to, struct hostentry_adata *head)
 
     /* Jump-away block for applying the actual attributes */
     do {
-      ea_set_attr_u32(to, &ea_gen_igp_metric, 0, he->igp_metric);
-
       ea_list *src = atomic_load_explicit(&he->src, memory_order_acquire);
       if (!src)
       {
@@ -4188,6 +4186,8 @@ rta_apply_hostentry(ea_list **to, struct hostentry_adata *head)
        break;
       }
 
+      ea_set_attr_u32(to, &ea_gen_local_metric, 0, he->igp_metric);
+
       eattr *he_nh_ea = ea_find(src, &ea_gen_nexthop);
       ASSERT_DIE(he_nh_ea);
 

diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index f072e4c99ed698d283580cb40d8d3e6dd5d4dd84..8658651e035f9cf1da63011cf32a74817621881f 100644 (file)
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -387,7 +387,7 @@ bgp_total_aigp_metric_(const rte *e, u64 *metric, const struct adata **ad)
     return 0;
 
   u64 aigp = get_u64(b + 3);
-  u64 step = rt_get_igp_metric(e);
+  u64 step = ea_get_int(e->attrs, &ea_gen_local_metric, IGP_METRIC_UNKNOWN);
 
   if (!rte_resolvable(e) || (step >= IGP_METRIC_UNKNOWN))
     step = BGP_AIGP_MAX;
@@ -2622,8 +2622,8 @@ bgp_rte_better(const rte *new, const rte *old)
     return 1;
 
   /* RFC 4271 9.1.2.2. e) Compare IGP metrics */
-  n = new_bgp->cf->igp_metric ? rt_get_igp_metric(new) : 0;
-  o = old_bgp->cf->igp_metric ? rt_get_igp_metric(old) : 0;
+  n = new_bgp->cf->igp_metric ? ea_get_int(new->attrs, &ea_gen_local_metric, 0) : 0;
+  o = old_bgp->cf->igp_metric ? ea_get_int(old->attrs, &ea_gen_local_metric, 0) : 0;
   if (n < o)
     return 1;
   if (n > o)
@@ -2731,8 +2731,8 @@ bgp_rte_mergable(const rte *pri, const rte *sec)
     return 0;
 
   /* RFC 4271 9.1.2.2. e) Compare IGP metrics */
-  p = pri_bgp->cf->igp_metric ? rt_get_igp_metric(pri) : 0;
-  s = sec_bgp->cf->igp_metric ? rt_get_igp_metric(sec) : 0;
+  p = pri_bgp->cf->igp_metric ? ea_get_int(pri->attrs, &ea_gen_local_metric, 0) : 0;
+  s = sec_bgp->cf->igp_metric ? ea_get_int(sec->attrs, &ea_gen_local_metric, 0) : 0;
   if (p != s)
     return 0;
 
@@ -3014,19 +3014,20 @@ bgp_get_route_info(const rte *e, byte *buf)
     if (rte_stale(e))
       buf += bsprintf(buf, "s");
 
+    eattr *ic;
     u64 metric = bgp_total_aigp_metric(e);
     if (metric < BGP_AIGP_MAX)
     {
       buf += bsprintf(buf, "/%lu", metric);
     }
-    else if (metric = rt_get_igp_metric(e))
+    else if (ic = ea_find(e->attrs, &ea_gen_local_metric))
     {
       if (!rte_resolvable(e))
        buf += bsprintf(buf, "/-");
-      else if (metric >= IGP_METRIC_UNKNOWN)
+      else if (ic->u.i >= IGP_METRIC_UNKNOWN)
        buf += bsprintf(buf, "/?");
-      else
-       buf += bsprintf(buf, "/%d", metric);
+      else if (ic->u.i > 0)
+       buf += bsprintf(buf, "/%d", ic->u.i);
     }
   }
   buf += bsprintf(buf, ") [");

diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index bdbeabd12e454408ff104af616ef1f11dd3c262e..a17cd6162f8a2f79c6af57802460f6d635ef6b46 100644 (file)
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -1132,7 +1132,8 @@ bgp_apply_next_hop(struct bgp_parse_state *s, ea_list **to, ip_addr gw, ip_addr
     if (nbr->scope == SCOPE_HOST)
       WITHDRAW(BAD_NEXT_HOP " - address %I is local", nbr->addr);
 
-    ea_set_attr_u32(to, &ea_gen_igp_metric, 0, c->cf->cost);
+    if (c->cf->cost)
+      ea_set_attr_u32(to, &ea_gen_local_metric, 0, c->cf->cost);
 
     struct nexthop_adata_mpls nam;
     memset(&nam, 0, sizeof nam);

diff --git a/proto/l3vpn/l3vpn.c b/proto/l3vpn/l3vpn.c
index ce59ec08783fd8193da262d0c684b764223f0044..e85772ac473795a436bfbfdf9151ff324bd49606 100644 (file)
--- a/proto/l3vpn/l3vpn.c
+++ b/proto/l3vpn/l3vpn.c
@@ -243,9 +243,9 @@ l3vpn_rt_notify(struct proto *P, struct channel *c0, const net_addr *n0, rte *ne
        ea_set_attr_data(&new->attrs, &ea_gen_nexthop, 0, nhad.ad.data, nhad.ad.length);
       }
 
-      /* Drop original IGP metric on export;
+      /* Drop original local metric on export;
        * kept on import as a base for L3VPN metric */
-      ea_unset_attr(&new->attrs, 0, &ea_gen_igp_metric);
+      ea_unset_attr(&new->attrs, 0, &ea_gen_local_metric);
     }
 
     rte_update(dst, n, new, src);
@@ -326,18 +326,25 @@ l3vpn_reload_routes(struct channel *C, struct rt_feeding_request *rfr)
   return 1;
 }
 
+static inline u32
+l3vpn_metric(const rte *e)
+{
+  eattr *a = ea_find(e->attrs, &ea_gen_igp_metric) ?: ea_find(e->attrs, &ea_gen_local_metric);
+  return a ? a->u.i : IGP_METRIC_UNKNOWN;
+}
+
 static int
 l3vpn_rte_better(const rte *new, const rte *old)
 {
   /* This is hack, we should have full BGP-style comparison */
-  return rt_get_igp_metric(new) < rt_get_igp_metric(old);
+  return l3vpn_metric(new) < l3vpn_metric(old);
 }
 
 static void
 l3vpn_get_route_info(const rte *rte, byte *buf)
 {
   u32 pref = rt_get_preference(rte);
-  u32 metric = rt_get_igp_metric(rte);
+  u32 metric = l3vpn_metric(rte);
 
   if (metric < IGP_METRIC_UNKNOWN)
     bsprintf(buf, " (%u/%u)", pref, metric);