636f76088d ("bgpd: router-id change reflect to vpn auto rd rt") has introduced an automatic configuration change of the rd vpn export and rt vpn export attributes after a router-id modification with the bgp router-id command.

For example, run the bgp_l3vpn_to_bgp_vrf topotest:

python3 -m pytest bgp_l3vpn_to_bgp_vrf -v --pause -s --shell=r1

Configure the following to r1:

ip link add r1-cust5 type vrf table 100 ip link set r1-cust5 up

vtysh conf t router bgp 5227 vrf r1-cust5 bgp router-id 192.168.1.1 ! address-family ipv4 unicast rd vpn export 10:1 rt vpn both 52:100 export vpn import vpn exit-address-family exit

Then change the router-id:

router bgp 5227 vrf r1-cust5 bgp router-id 192.168.1.11

Notice that the change rd/rt vpn export attributes:

r1(config-router)# do sh run bgp router bgp 5227 vrf r1-cust5 bgp router-id 192.168.1.11 ! address-family ipv4 unicast rd vpn export 192.168.1.11:3 rt vpn import 52:100 rt vpn export 192.168.1.11:3 export vpn import vpn exit-address-family

rd/rt vpn export attributes are in the following format:

r1(config-router-af)# rt vpn export ? RTLIST Space separated route target list (A.B.C.D:MN|EF:OPQR|GHJK:MN) r1(config-router-af)# rd vpn export ? ASN:NN_OR_IP-ADDRESS:NN Route Distinguisher (: | :)

The automatic change of the rd/rt vpn export attributes have only a benefit in the case the attributes are in the <router-id:number> format. It should not be the general case.

Add a bgp auto-rd-rt global option to explicitly request the rd/rt vpn export attributes to be automatically replaced by the <router-id:number> format after a bgp router-id modification.

Fixes: 636f76088d ("bgpd: router-id change reflect to vpn auto rd rt") Signed-off-by: Louis Scalbert [email protected]

May 11 '22 13:05 louis-6wind

Continuous Integration Result: FAILED

See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests Ubuntu 18.04 arm8 part 6: Failed (click for details)

Topotests Ubuntu 18.04 arm8 part 6: No useful log found

Topotests debian 10 amd64 part 6: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6DEB10AMD64-5314/test

Topology Tests failed for Topotests debian 10 amd64 part 6 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO6DEB10AMD64/ErrorLog/log_topotests.txt

Topotests debian 10 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-5314/test

Topology Tests failed for Topotests debian 10 amd64 part 9 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt

Topotests Ubuntu 18.04 amd64 part 6: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6U18AMD64-5314/test

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt

Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)

Topotests Ubuntu 18.04 arm8 part 9: No useful log found

Topotests Ubuntu 18.04 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-5314/test

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO9U18AMD64/ErrorLog/log_topotests.txt

Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-5314/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO9U18I386/ErrorLog/log_topotests.txt

Topotests Ubuntu 18.04 i386 part 6: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6U18I386-5314/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 6 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5314/artifact/TOPO6U18I386/ErrorLog/log_topotests.txt

Successful on other platforms/tests

Topotests Ubuntu 18.04 amd64 part 4
Topotests Ubuntu 18.04 arm8 part 1
Topotests Ubuntu 18.04 amd64 part 0
Topotests Ubuntu 18.04 amd64 part 5
Topotests Ubuntu 18.04 amd64 part 7
Addresssanitizer topotests part 2
Fedora 29 rpm pkg check
Topotests debian 10 amd64 part 5
Topotests debian 10 amd64 part 0
Topotests Ubuntu 18.04 arm8 part 2
Topotests Ubuntu 18.04 arm8 part 7
Addresssanitizer topotests part 3
Addresssanitizer topotests part 9
Addresssanitizer topotests part 8
CentOS 7 rpm pkg check
Topotests Ubuntu 18.04 arm8 part 8
Topotests Ubuntu 18.04 amd64 part 3
Topotests Ubuntu 18.04 i386 part 7
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 i386 part 5
Topotests Ubuntu 18.04 amd64 part 1
Addresssanitizer topotests part 6
Ubuntu 18.04 deb pkg check
Topotests Ubuntu 18.04 i386 part 0
Topotests Ubuntu 18.04 amd64 part 2
Topotests debian 10 amd64 part 7
Topotests Ubuntu 18.04 i386 part 8
Addresssanitizer topotests part 0
Topotests debian 10 amd64 part 8
Ubuntu 20.04 deb pkg check
Topotests Ubuntu 18.04 arm8 part 4
Debian 10 deb pkg check
Topotests debian 10 amd64 part 3
IPv6 protocols on Ubuntu 18.04
Addresssanitizer topotests part 1
Topotests Ubuntu 18.04 i386 part 4
Topotests Ubuntu 18.04 arm8 part 3
Topotests debian 10 amd64 part 4
Topotests Ubuntu 18.04 i386 part 3
Addresssanitizer topotests part 4
IPv4 protocols on Ubuntu 18.04
Debian 9 deb pkg check
Addresssanitizer topotests part 7
Static analyzer (clang)
Ubuntu 16.04 deb pkg check
Addresssanitizer topotests part 5
Topotests debian 10 amd64 part 1
Topotests debian 10 amd64 part 2
Topotests Ubuntu 18.04 i386 part 1
Topotests Ubuntu 18.04 amd64 part 8
Topotests Ubuntu 18.04 arm8 part 0
Topotests Ubuntu 18.04 arm8 part 5
IPv4 ldp protocol on Ubuntu 18.04

May 11 '22 16:05 NetDEF-CI

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5325/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

May 11 '22 18:05 NetDEF-CI

Continuous Integration Result: FAILED

See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5336/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)

Topotests Ubuntu 18.04 arm8 part 9: No useful log found

Successful on other platforms/tests

Topotests Ubuntu 18.04 arm8 part 7
Addresssanitizer topotests part 9
Topotests debian 10 amd64 part 0
Ubuntu 16.04 deb pkg check
IPv4 ldp protocol on Ubuntu 18.04
Topotests Ubuntu 18.04 arm8 part 2
Topotests debian 10 amd64 part 5
Topotests Ubuntu 18.04 i386 part 1
Topotests Ubuntu 18.04 arm8 part 8
Topotests Ubuntu 18.04 i386 part 7
Addresssanitizer topotests part 7
Topotests Ubuntu 18.04 i386 part 6
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 arm8 part 6
Topotests Ubuntu 18.04 amd64 part 5
Topotests Ubuntu 18.04 amd64 part 2
Topotests Ubuntu 18.04 amd64 part 7
Fedora 29 rpm pkg check
Topotests Ubuntu 18.04 i386 part 8
Topotests Ubuntu 18.04 i386 part 5
Topotests Ubuntu 18.04 i386 part 3
Addresssanitizer topotests part 3
Topotests Ubuntu 18.04 i386 part 0
Topotests debian 10 amd64 part 8
IPv6 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 arm8 part 4
CentOS 7 rpm pkg check
Topotests Ubuntu 18.04 amd64 part 4
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 amd64 part 0
Topotests Ubuntu 18.04 amd64 part 3
Topotests debian 10 amd64 part 9
Topotests Ubuntu 18.04 amd64 part 8
Topotests debian 10 amd64 part 7
IPv4 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 arm8 part 0
Static analyzer (clang)
Debian 9 deb pkg check
Topotests Ubuntu 18.04 amd64 part 9
Addresssanitizer topotests part 8
Topotests debian 10 amd64 part 2
Debian 10 deb pkg check
Topotests Ubuntu 18.04 arm8 part 5
Topotests Ubuntu 18.04 amd64 part 6
Addresssanitizer topotests part 6
Topotests Ubuntu 18.04 amd64 part 1
Ubuntu 20.04 deb pkg check
Ubuntu 18.04 deb pkg check
Topotests debian 10 amd64 part 6
Addresssanitizer topotests part 5
Topotests debian 10 amd64 part 1
Topotests Ubuntu 18.04 arm8 part 1
Addresssanitizer topotests part 4
Addresssanitizer topotests part 0
Topotests debian 10 amd64 part 4
Topotests debian 10 amd64 part 3
Topotests Ubuntu 18.04 arm8 part 3
Addresssanitizer topotests part 1
Topotests Ubuntu 18.04 i386 part 9
Topotests Ubuntu 18.04 i386 part 4

May 12 '22 11:05 NetDEF-CI

ci:rerun

May 13 '22 07:05 louis-6wind

Continuous Integration Result: FAILED

Test incomplete. See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5355/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Incomplete

Addresssanitizer topotests part 7: Incomplete

(check logs for details)

CentOS 7 rpm pkg check: Failed (click for details)

CentOS 7 rpm pkg check: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5355/artifact/CENTOS7RPM/ErrorLog/log_restart.txt CentOS 7 rpm pkg check: No useful log found

Successful on other platforms/tests

Topotests debian 10 amd64 part 9
Topotests Ubuntu 18.04 arm8 part 8
Topotests debian 10 amd64 part 4
Addresssanitizer topotests part 4
Topotests Ubuntu 18.04 i386 part 7
Topotests Ubuntu 18.04 arm8 part 3
Addresssanitizer topotests part 5
Topotests Ubuntu 18.04 amd64 part 2
Topotests Ubuntu 18.04 amd64 part 3
Topotests debian 10 amd64 part 3
Addresssanitizer topotests part 0
Topotests debian 10 amd64 part 8
Topotests Ubuntu 18.04 i386 part 6
Topotests Ubuntu 18.04 arm8 part 9
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 arm8 part 4
Ubuntu 20.04 deb pkg check
Ubuntu 16.04 deb pkg check
IPv4 ldp protocol on Ubuntu 18.04
IPv6 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 amd64 part 5
Debian 10 deb pkg check
Topotests Ubuntu 18.04 amd64 part 4
IPv4 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 i386 part 1
Topotests Ubuntu 18.04 i386 part 3
Addresssanitizer topotests part 9
Topotests Ubuntu 18.04 amd64 part 7
Topotests Ubuntu 18.04 arm8 part 2
Topotests debian 10 amd64 part 0
Topotests Ubuntu 18.04 i386 part 8
Topotests Ubuntu 18.04 arm8 part 7
Addresssanitizer topotests part 3
Topotests Ubuntu 18.04 amd64 part 9
Addresssanitizer topotests part 6
Topotests Ubuntu 18.04 arm8 part 5
Fedora 29 rpm pkg check
Topotests debian 10 amd64 part 1
Topotests Ubuntu 18.04 i386 part 9
Topotests Ubuntu 18.04 amd64 part 8
Topotests debian 10 amd64 part 2
Topotests debian 10 amd64 part 7
Topotests Ubuntu 18.04 arm8 part 0
Topotests Ubuntu 18.04 amd64 part 0
Static analyzer (clang)
Topotests Ubuntu 18.04 i386 part 0
Topotests Ubuntu 18.04 i386 part 5
Ubuntu 18.04 deb pkg check
Debian 9 deb pkg check
Addresssanitizer topotests part 1
Topotests Ubuntu 18.04 i386 part 4
Topotests Ubuntu 18.04 amd64 part 1
Topotests Ubuntu 18.04 arm8 part 6
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 arm8 part 1
Topotests debian 10 amd64 part 6
Addresssanitizer topotests part 8
Topotests Ubuntu 18.04 amd64 part 6
Topotests debian 10 amd64 part 5

May 13 '22 10:05 NetDEF-CI

I agree with the problem, but I think this is not the best solution. We don't need to introduce a new flag just for this and also this is confusing.

The right fix would be to check whether RD or RT are manual or auto configured. Do not change the RD/RT for any of these events (like router-id change etc.) if the RD or RT is manually configured. Modify them only if they are auto configured.

May 17 '22 14:05 srimohans

You can also use the below flags to check whether the configured RD is of router-id type or of AS type.

/* RD types */ #define RD_TYPE_AS 0 #define RD_TYPE_IP 1 #define RD_TYPE_AS4 2

May 17 '22 14:05 srimohans

I agree with the problem, but I think this is not the best solution. We don't need to introduce a new flag just for this and also this is confusing.

The right fix would be to check whether RD or RT are manual or auto configured. Do not change the RD/RT for any of these events (like router-id change etc.) if the RD or RT is manually configured. Modify them only if they are auto configured.

How do you determine whether rd and rt are manual ? Having them in the format :XX does not mean you want them to be automatically configured when router-id. It is confusing to have some other parameter changes when you only intend to change the BGP router ID.

May 17 '22 15:05 louis-6wind

I think you can use flag to determine this "VNI_FLAG_RD_CFGD" or the API if (is_rd_configured(vpn))

Currently FRR only supports router-id based RD for Auto option. i.e. when user chooses Auto RD, then FRR automatically computes the RD based on router-id:. So when router-id changes, it will change the RD as well and this is expected. If user doesn't want this, he can always choose manual RD option and control the life of RD.

May 17 '22 16:05 srimohans

I think you can use flag to determine this "VNI_FLAG_RD_CFGD" or the API if (is_rd_configured(vpn))

VNI means "VXLAN network identifier". This fix is not for EVPN.

Currently FRR only supports router-id based RD for Auto option. i.e. when user chooses Auto RD, then FRR automatically computes the RD based on router-id:. So when router-id changes, it will change the RD as well and this is expected. If user doesn't want this, he can always choose manual RD option and control the life of RD.

There is no manual mode for the moment.

Automatic by default is very confusing.

May 17 '22 16:05 louis-6wind

@chiragshah6 Do you have any inputs here ? I remember you had mentioned about some fixes in this area.

May 18 '22 04:05 srimohans

Automatic by default is very confusing.

yes ... I prefer the manual solution here

May 18 '22 19:05 riw777

lint test failures don't seem to be correct ... waiting on resolution for "how to do this" conversation

May 18 '22 19:05 riw777

ci failure doesn't seem to be related; rerunning

May 18 '22 19:05 riw777

ci:rerun

May 18 '22 19:05 riw777

Yes. Auto RD/RT assignment currently in FRR is bit confusing. So instead of introducing a flag to indicate whether the RD should be updated or not on router-id change, it is better to introduce the flag to distinguish between Auto RD/RT assignment and manual RD/RT assignment. If user chooses manual assignment for RD/RT, then do not update the RD or RT on router-id or AS num change. if user chooses Auto assignment, then RD or RT will change accordingly based on router-id or AS Num change.

May 18 '22 19:05 srimohans

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5468/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

May 18 '22 21:05 NetDEF-CI

I agree with the problem, but I think this is not the best solution. We don't need to introduce a new flag just for this and also this is confusing. The right fix would be to check whether RD or RT are manual or auto configured. Do not change the RD/RT for any of these events (like router-id change etc.) if the RD or RT is manually configured. Modify them only if they are auto configured.

How do you determine whether rd and rt are manual ? Having them in the format :XX does not mean you want them to be automatically configured when router-id. It is confusing to have some other parameter changes when you only intend to change the BGP router ID.

@louis-6wind I agree with @srimohans this is not an ideal solution to introduce a new configuration flag to override auto rd vs. user configured value.

Yes. Auto RD/RT assignment currently in FRR is bit confusing. So instead of introducing a flag to indicate whether the RD should be updated or not on router-id change, it is better to introduce the flag to distinguish between Auto RD/RT assignment and manual RD/RT assignment. If user chooses manual assignment for RD/RT, then do not update the RD or RT on router-id or AS num change. if user chooses Auto assignment, then RD or RT will change accordingly based on router-id or AS Num change.

@louis-6wind I second to @srimohans proposal, just like we check if route-target is user configured vs auto derived, introduce a flag to differentiate between the two and make use of it during router-id change.

May 19 '22 02:05 chiragshah6

@louis-6wind there was a commit to prohibit RD change when router-id is changed (either via cli or from zebra) where flag (BGP_VPN_POLICY_TOVPN_RD_SET) is used to make the call user vs auto configured

commit e65fe398f6ba1a47a04793f7e25d6557ee705b7f Author: Mark Stapp [email protected] Date: Tue Jun 11 13:47:15 2019 -0400

bgpd: auto router-id should not change configured vpn RD/RT

A router-id change that isn't explicitly configured (a change
from zebra, for example) should not replace a configured vpn
RD/RT.

Signed-off-by: Mark Stapp <[email protected]>

-void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw)
+void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw,
+                                bool is_config)
 {
        afi_t afi;
        int debug;
@@ -1536,6 +1537,20 @@ void vpn_handle_router_id_update(struct bgp *bgp, bool withdraw)

                        }
                } else {
+                       /*
+                        * Router-id changes that are not explicit config
+                        * changes should not replace configured RD/RT.
+                        */
+                       if (!is_config) {
+                               if (CHECK_FLAG(bgp->vpn_policy[afi].flags,
+                                              BGP_VPN_POLICY_TOVPN_RD_SET)) {
+                                       if (debug)
+                                               zlog_debug("%s: auto router-id change skipped",
+                                                          __func__);
+                                       goto postchange;
+                               }
+                       }
+

The flag is set when user configured RD value is set

af_rd_vpn_export_cmd()
        if (yes) {
                bgp->vpn_policy[afi].tovpn_rd = prd;
                SET_FLAG(bgp->vpn_policy[afi].flags,
                         BGP_VPN_POLICY_TOVPN_RD_SET);
        } else {
                UNSET_FLAG(bgp->vpn_policy[afi].flags,
                           BGP_VPN_POLICY_TOVPN_RD_SET);
        }

May 19 '22 02:05 chiragshah6

Do we need to call a meeting to discuss this one so we can figure out how to move it forward? It might be easier than tossing comments back and forth here?

May 27 '22 20:05 riw777

Yes. Once @chiragshah6 is back from vacation, we can have a quick meeting and conclude this.

May 27 '22 20:05 srimohans

Yes. Once @chiragshah6 is back from vacation, we can have a quick meeting and conclude this.

I am back. Let us meet at convenient time. Would 06/02/22 Thursday 9 AM EST work?

May 31 '22 21:05 chiragshah6

@louis-6wind and I had meeting on 07/06/22 and sort of in agreement to not proceed with aditional config knob but rather use a flag to avoid updating user provided L3VPN RD/RTs when a router-id changes.

Jul 13 '22 15:07 chiragshah6

@chiragshah6 @louis-6wind what's the next step here? I'll clear my approval and set to look at this again in a bit

Jul 19 '22 11:07 riw777

@louis-6wind what's the next step here?

Aug 22 '22 12:08 riw777

@louis-6wind what's the next step here?

We had a meeting but it is impossible to implement the feature in the way we decided. I have no time to continue the work at the moment.

Aug 22 '22 12:08 louis-6wind

@louis-6wind what's the next step here?

We had a meeting but it is impossible to implement the feature in the way we decided. I have no time to continue the work at the moment.

then we should probably close this for the moment and revisit it later ... ??

Sep 06 '22 14:09 riw777

@frrbot autoclose in 1 month

Sep 06 '22 14:09 riw777

frr
frr copied to clipboard

bgpd: explicitly request rd and rt auto change behavior after router-id change

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Incomplete

Continuous Integration Result: SUCCESSFUL

frr frr copied to clipboard

bgpd: explicitly request rd and rt auto change behavior after router-id change

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Incomplete

Continuous Integration Result: SUCCESSFUL

frr
frr copied to clipboard