bgp: fix case where confederation id same as member-as

currently the following configuration

dut:

!
interface ntfp2
 ip router isis 1
!
router bgp 200
 no bgp ebgp-requires-policy
 bgp confederation identifier 300
 bgp confederation peers 300
 neighbor 192.168.1.1 remote-as 100
 neighbor 192.168.2.2 remote-as 300
 !
 address-family ipv4 unicast
  neighbor 192.168.2.2 default-originate
 exit-address-family
!
router isis 1
 is-type level-2-only
 net 49.0001.0002.0002.0002.00
 redistribute ipv4 connected level-2
!
end

router:

!
interface ntfp2
 ip router isis 1
 isis circuit-type level-2-only
!
router bgp 300
 no bgp ebgp-requires-policy
 bgp confederation identifier 300
 bgp confederation peers 200
 neighbor 192.168.2.1 remote-as 200
 neighbor 192.168.3.2 remote-as 400
 !
 address-family ipv4 unicast
  network 3.3.3.0/24
 exit-address-family
!
router isis 1
 is-type level-2-only
 net 49.0001.0003.0003.0003.00
 redistribute ipv4 connected level-2
!
end

on dut result of show bgp ipv4 unicast command is:
show bgp ipv4 unicast

  BGP table version is 1, local router ID is 192.168.2.1, vrf id 0
  Default local pref 100, local AS 200
  Status codes:  s suppressed, d damped, h history, * valid, > best, = multipath,
                 i internal, r RIB-failure, S Stale, R Removed
  Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self
  Origin codes:  i - IGP, e - EGP, ? - incomplete
  RPKI validation codes: V valid, I invalid, N Not found

     Network          Next Hop            Metric LocPrf Weight Path
  *> 1.1.1.0/24       192.168.1.1              0             0 100 i

instead of

sho bgp ipv4 unicast
BGP table version is 3, local router ID is 192.168.2.1, vrf id 0
Default local pref 100, local AS 200
Status codes:  s suppressed, d damped, h history, * valid, > best, = multipath,
               i internal, r RIB-failure, S Stale, R Removed
Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self
Origin codes:  i - IGP, e - EGP, ? - incomplete
RPKI validation codes: V valid, I invalid, N Not found

   Network          Next Hop            Metric LocPrf Weight Path
*> 1.1.1.0/24       192.168.1.1              0             0 100 i
*> 3.3.3.0/24       192.168.2.2              0    100      0 (300) i
*> 4.4.4.0/24       192.168.3.2              0    100      0 (300) 400 i

Displayed  3 routes and 3 total paths

According to RFC 5065:the usage of one of the member AS number as the
confederation identifier is not forbidden.

fixes are the following

in bgp_route.c:
in bgp_update remove the test for presence of confederation id in
as_path since, this case is allowed;

in bgp_vty.c
bgp_confederation_peers, remove the test on peer as value

in bgpd.c
bgp_confederation_peers_add
remove the test on peer as value
invert the order of setting peer->sort value and peer->local_as,
since peer->sort is depending from current peer->local_as value

bgp_confederation_peers_remove
invert the order of setting peer->sort value and peer->local_as,
since peer->sort is depending from current peer->local_as value

Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>

diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c
index 06f6073781773be2e8255bf5711cc5bada11c976..85f09ccf0b6572b7c90566480b5e11ccca085dfd 100644 (file)
--- a/bgpd/bgp_aspath.c
+++ b/bgpd/bgp_aspath.c
@@ -1187,6 +1187,33 @@ int aspath_loop_check(struct aspath *aspath, as_t asno)
        return count;
 }
 
+/* AS path loop check.  If aspath contains asno
+ * that is a confed id then return >= 1.
+ */
+int aspath_loop_check_confed(struct aspath *aspath, as_t asno)
+{
+       struct assegment *seg;
+       int count = 0;
+
+       if (aspath == NULL || aspath->segments == NULL)
+               return 0;
+
+       seg = aspath->segments;
+
+       while (seg) {
+               unsigned int i;
+
+               for (i = 0; i < seg->length; i++)
+                       if (seg->type != AS_CONFED_SEQUENCE &&
+                           seg->type != AS_CONFED_SET && seg->as[i] == asno)
+                               count++;
+
+               seg = seg->next;
+       }
+       return count;
+}
+
+
 /* When all of AS path is private AS return 1.  */
 bool aspath_private_as_check(struct aspath *aspath)
 {

diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h
index 0b58e1adc4a77af8ae8168aae853b0d4a450255b..97bc7c0acada5ec7fc520b1ccd798b21b1140b71 100644 (file)
--- a/bgpd/bgp_aspath.h
+++ b/bgpd/bgp_aspath.h
@@ -111,6 +111,7 @@ extern unsigned int aspath_key_make(const void *p);
 extern unsigned int aspath_get_first_as(struct aspath *aspath);
 extern unsigned int aspath_get_last_as(struct aspath *aspath);
 extern int aspath_loop_check(struct aspath *aspath, as_t asno);
+extern int aspath_loop_check_confed(struct aspath *aspath, as_t asno);
 extern bool aspath_private_as_check(struct aspath *aspath);
 extern struct aspath *aspath_replace_specific_asn(struct aspath *aspath,
                                                  as_t target_asn,

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 130a0b4abd8bf8dc9e84d90337b3679451c714c1..24ce78a9acbe151ec4bb74ebc54c2cb628f55ae6 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -2197,7 +2197,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 
        /* If we're a CONFED we need to loop check the CONFED ID too */
        if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) {
-               if (aspath_loop_check(piattr->aspath, bgp->confed_id)) {
+               if (aspath_loop_check_confed(piattr->aspath, bgp->confed_id)) {
                        if (bgp_debug_update(NULL, p, subgrp->update_group, 0))
                                zlog_debug(
                                        "%pBP [Update:SEND] suppress announcement to peer AS %u is AS path.",
@@ -4113,16 +4113,23 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
 
        /* AS path loop check. */
        if (do_loop_check) {
-               if (aspath_loop_check(attr->aspath, bgp->as) > allowas_in ||
-                   (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION) &&
-                    (aspath_loop_check(attr->aspath, bgp->confed_id) >
-                     allowas_in))) {
+               if (aspath_loop_check(attr->aspath, bgp->as) >
+                   peer->allowas_in[afi][safi]) {
                        peer->stat_pfx_aspath_loop++;
                        reason = "as-path contains our own AS;";
                        goto filtered;
                }
        }
 
+       /* If we're a CONFED we need to loop check the CONFED ID too */
+       if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION) && do_loop_check)
+               if (aspath_loop_check_confed(attr->aspath, bgp->confed_id) >
+                   peer->allowas_in[afi][safi]) {
+                       peer->stat_pfx_aspath_loop++;
+                       reason = "as-path contains our own confed AS;";
+                       goto filtered;
+               }
+
        /* Route reflector originator ID check. If ACCEPT_OWN mechanism is
         * enabled, then take care of that too.
         */

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 1f66080e93ba02503eb3c02ff7d09cc6a3adc044..95c81909ce44b3ca74dc6e66c432cbdd1cd32264 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -1917,13 +1917,6 @@ DEFUN (bgp_confederation_peers,
 
        for (i = idx_asn; i < argc; i++) {
                as = strtoul(argv[i]->arg, NULL, 10);
-
-               if (bgp->as == as) {
-                       vty_out(vty,
-                               "%% Local member-AS not allowed in confed peer list\n");
-                       continue;
-               }
-
                bgp_confederation_peers_add(bgp, as);
        }
        return CMD_SUCCESS;

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 6ad1cf2c06f150c05f88d672ce555ff8db90b378..b198cd560ac2aee17986e48d14e7e6177ec4f79c 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -671,7 +671,7 @@ void bgp_confederation_peers_add(struct bgp *bgp, as_t as)
        struct peer *peer;
        struct listnode *node, *nnode;
 
-       if (bgp->as == as)
+       if (!bgp)
                return;
 
        if (bgp_confederation_peers_check(bgp, as))
@@ -687,8 +687,8 @@ void bgp_confederation_peers_add(struct bgp *bgp, as_t as)
        if (bgp_config_check(bgp, BGP_CONFIG_CONFEDERATION)) {
                for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
                        if (peer->as == as) {
-                               (void)peer_sort(peer);
                                peer->local_as = bgp->as;
+                               (void)peer_sort(peer);
                                if (BGP_IS_VALID_STATE_FOR_NOTIF(
                                            peer->status)) {
                                        peer->last_reset =
@@ -738,8 +738,8 @@ void bgp_confederation_peers_remove(struct bgp *bgp, as_t as)
        if (bgp_config_check(bgp, BGP_CONFIG_CONFEDERATION)) {
                for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
                        if (peer->as == as) {
-                               (void)peer_sort(peer);
                                peer->local_as = bgp->confed_id;
+                               (void)peer_sort(peer);
                                if (BGP_IS_VALID_STATE_FOR_NOTIF(
                                            peer->status)) {
                                        peer->last_reset =