From: Maria Matejka <mq@ucw.cz>
Date: Thu, 18 Dec 2025 11:39:42 +0000 (+0100)
Subject: IGP metric: Split out local_metric again
X-Git-Tag: v3.2.0~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fb6987a671549dff303470302404a77a7351044e;p=thirdparty%2Fbird.git

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 e91047452..4a1b2708e 100644
--- 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 b0ac094ae..0023e2dd8 100644
--- 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 64d130645..758a7d0c7 100644
--- 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 21b468208..4b7a74b08 100644
--- 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 7b1fd57a3..99c6feba7 100644
--- 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 b28744af3..a42a337da 100644
--- 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 9141ddc6c..c153f3c2a 100644
--- 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 f072e4c99..8658651e0 100644
--- 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 bdbeabd12..a17cd6162 100644
--- 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 ce59ec087..e85772ac4 100644
--- 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);