Re: show negotiated capabilities in bgpctl show neighbor output

2021-04-26 Thread Denis Fondras
Le Mon, Apr 26, 2021 at 04:21:16PM +0200, Claudio Jeker a écrit :
> The bgpctl show neighbor output is a bit missleading for capabilities.
> It currently shows the capabilities sent by the neighbor and not the ones
> that then got selected for the session. This matters especially for the
> multiprotocol capability.
> 
> I added the negotiated capability in the output (which makes the output
> longer but hopefully less confusing). e.g.
> 

OK denis@

If you want to reduce the length, an alternative display could be to add '*'
next to the enabled capability.


> bgpctl show neighbor 2001:XXX
> BGP neighbor is 2001:XXX, remote AS 65195, Passive
>  Max-prefix: 604 (restart 15)
>   BGP version 4, remote router-id 0.0.0.1
>   BGP state = Established, up for 05w3d21h
>   Last read 00:00:15, holdtime 90s, keepalive interval 30s
>   Last write 00:00:14
>   Neighbor capabilities:
> Multiprotocol extensions: IPv4 unicast, IPv6 unicast, IPv4 vpn, IPv6 vpn
> 4-byte AS numbers
>   Negotiated capabilities:
> Multiprotocol extensions: IPv6 unicast
> 4-byte AS numbers
> 
> The neighbor here is exabgp and by default it just adds everything in the
> capabilities. Still the negotiated capabilites do not include anything but
> the IPv6 unicast AFI.
> 
> The JSON output already includes all 3 capabilities in its output so there
> no change is needed.
> -- 
> :wq Claudio
> 
> Index: output.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 output.c
> --- output.c  15 Apr 2021 14:12:05 -  1.15
> +++ output.c  25 Apr 2021 08:39:45 -
> @@ -132,14 +132,14 @@ show_summary(struct peer *p)
>  }
>  
>  static void
> -show_neighbor_capa_mp(struct peer *p)
> +show_neighbor_capa_mp(struct capabilities *capa)
>  {
>   int comma;
>   u_int8_ti;
>  
>   printf("Multiprotocol extensions: ");
>   for (i = 0, comma = 0; i < AID_MAX; i++)
> - if (p->capa.peer.mp[i]) {
> + if (capa->mp[i]) {
>   printf("%s%s", comma ? ", " : "", aid2str(i));
>   comma = 1;
>   }
> @@ -147,23 +147,23 @@ show_neighbor_capa_mp(struct peer *p)
>  }
>  
>  static void
> -show_neighbor_capa_restart(struct peer *p)
> +show_neighbor_capa_restart(struct capabilities *capa)
>  {
>   int comma;
>   u_int8_ti;
>  
>   printf("Graceful Restart");
> - if (p->capa.peer.grestart.timeout)
> - printf(": Timeout: %d, ", p->capa.peer.grestart.timeout);
> + if (capa->grestart.timeout)
> + printf(": Timeout: %d, ", capa->grestart.timeout);
>   for (i = 0, comma = 0; i < AID_MAX; i++)
> - if (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT) {
> + if (capa->grestart.flags[i] & CAPA_GR_PRESENT) {
>   if (!comma &&
> - p->capa.peer.grestart.flags[i] & CAPA_GR_RESTART)
> + capa->grestart.flags[i] & CAPA_GR_RESTART)
>   printf("restarted, ");
>   if (comma)
>   printf(", ");
>   printf("%s", aid2str(i));
> - if (p->capa.peer.grestart.flags[i] & CAPA_GR_FORWARD)
> + if (capa->grestart.flags[i] & CAPA_GR_FORWARD)
>   printf(" (preserved)");
>   comma = 1;
>   }
> @@ -286,12 +286,27 @@ show_neighbor_full(struct peer *p, struc
>   p->capa.peer.grestart.restart || p->capa.peer.as4byte) {
>   printf("  Neighbor capabilities:\n");
>   if (hascapamp)
> - show_neighbor_capa_mp(p);
> + show_neighbor_capa_mp(>capa.peer);
>   if (p->capa.peer.refresh)
>   printf("Route Refresh\n");
>   if (p->capa.peer.grestart.restart)
> - show_neighbor_capa_restart(p);
> + show_neighbor_capa_restart(>capa.peer);
>   if (p->capa.peer.as4byte)
> + printf("4-byte AS numbers\n");
> + }
> + for (i = 0; i < AID_MAX; i++)
> + if (p->capa.neg.mp[i])
> + hascapamp = 1;
> + if (hascapamp || p->capa.neg.refresh ||
> + p->capa.neg.grestart.restart || p->capa.neg.as4byte) {
> + printf("  Negotiated capabilities:\n");
> + if (hascapamp)
> + show_neighbor_capa_mp(>capa.neg);
> + if (p->capa.neg.refresh)
> + printf("Route Refresh\n");
> + if (p->capa.neg.grestart.restart)
> + show_neighbor_capa_restart(>capa.neg);
> + if (p->capa.neg.as4byte)
>   printf("4-byte AS numbers\n");
>   }
>   printf("\n");
> 



show negotiated capabilities in bgpctl show neighbor output

2021-04-26 Thread Claudio Jeker
The bgpctl show neighbor output is a bit missleading for capabilities.
It currently shows the capabilities sent by the neighbor and not the ones
that then got selected for the session. This matters especially for the
multiprotocol capability.

I added the negotiated capability in the output (which makes the output
longer but hopefully less confusing). e.g.

bgpctl show neighbor 2001:XXX
BGP neighbor is 2001:XXX, remote AS 65195, Passive
 Max-prefix: 604 (restart 15)
  BGP version 4, remote router-id 0.0.0.1
  BGP state = Established, up for 05w3d21h
  Last read 00:00:15, holdtime 90s, keepalive interval 30s
  Last write 00:00:14
  Neighbor capabilities:
Multiprotocol extensions: IPv4 unicast, IPv6 unicast, IPv4 vpn, IPv6 vpn
4-byte AS numbers
  Negotiated capabilities:
Multiprotocol extensions: IPv6 unicast
4-byte AS numbers

The neighbor here is exabgp and by default it just adds everything in the
capabilities. Still the negotiated capabilites do not include anything but
the IPv6 unicast AFI.

The JSON output already includes all 3 capabilities in its output so there
no change is needed.
-- 
:wq Claudio

Index: output.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
retrieving revision 1.15
diff -u -p -r1.15 output.c
--- output.c15 Apr 2021 14:12:05 -  1.15
+++ output.c25 Apr 2021 08:39:45 -
@@ -132,14 +132,14 @@ show_summary(struct peer *p)
 }
 
 static void
-show_neighbor_capa_mp(struct peer *p)
+show_neighbor_capa_mp(struct capabilities *capa)
 {
int comma;
u_int8_ti;
 
printf("Multiprotocol extensions: ");
for (i = 0, comma = 0; i < AID_MAX; i++)
-   if (p->capa.peer.mp[i]) {
+   if (capa->mp[i]) {
printf("%s%s", comma ? ", " : "", aid2str(i));
comma = 1;
}
@@ -147,23 +147,23 @@ show_neighbor_capa_mp(struct peer *p)
 }
 
 static void
-show_neighbor_capa_restart(struct peer *p)
+show_neighbor_capa_restart(struct capabilities *capa)
 {
int comma;
u_int8_ti;
 
printf("Graceful Restart");
-   if (p->capa.peer.grestart.timeout)
-   printf(": Timeout: %d, ", p->capa.peer.grestart.timeout);
+   if (capa->grestart.timeout)
+   printf(": Timeout: %d, ", capa->grestart.timeout);
for (i = 0, comma = 0; i < AID_MAX; i++)
-   if (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT) {
+   if (capa->grestart.flags[i] & CAPA_GR_PRESENT) {
if (!comma &&
-   p->capa.peer.grestart.flags[i] & CAPA_GR_RESTART)
+   capa->grestart.flags[i] & CAPA_GR_RESTART)
printf("restarted, ");
if (comma)
printf(", ");
printf("%s", aid2str(i));
-   if (p->capa.peer.grestart.flags[i] & CAPA_GR_FORWARD)
+   if (capa->grestart.flags[i] & CAPA_GR_FORWARD)
printf(" (preserved)");
comma = 1;
}
@@ -286,12 +286,27 @@ show_neighbor_full(struct peer *p, struc
p->capa.peer.grestart.restart || p->capa.peer.as4byte) {
printf("  Neighbor capabilities:\n");
if (hascapamp)
-   show_neighbor_capa_mp(p);
+   show_neighbor_capa_mp(>capa.peer);
if (p->capa.peer.refresh)
printf("Route Refresh\n");
if (p->capa.peer.grestart.restart)
-   show_neighbor_capa_restart(p);
+   show_neighbor_capa_restart(>capa.peer);
if (p->capa.peer.as4byte)
+   printf("4-byte AS numbers\n");
+   }
+   for (i = 0; i < AID_MAX; i++)
+   if (p->capa.neg.mp[i])
+   hascapamp = 1;
+   if (hascapamp || p->capa.neg.refresh ||
+   p->capa.neg.grestart.restart || p->capa.neg.as4byte) {
+   printf("  Negotiated capabilities:\n");
+   if (hascapamp)
+   show_neighbor_capa_mp(>capa.neg);
+   if (p->capa.neg.refresh)
+   printf("Route Refresh\n");
+   if (p->capa.neg.grestart.restart)
+   show_neighbor_capa_restart(>capa.neg);
+   if (p->capa.neg.as4byte)
printf("4-byte AS numbers\n");
}
printf("\n");