[quagga-dev,16135] Extend BGP_SEND_ASPATH_CHECK to cover confederations

Message ID 20160929172535.24695-1-thorvald@medallia.com
State Under Review
Headers show

Commit Message

Thorvald Natvig Sept. 29, 2016, 5:25 p.m.
Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations.
---
 bgpd/bgp_route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

cisystem@netdef.org Sept. 29, 2016, 7 p.m. | #1
Continous Integration Result: FAILED

See below for issues.
This is an EXPERIMENTAL automated CI system.
For questions and feedback, feel free to email
Martin Winter <mwinter@opensourcerouting.org>.

Patches applied :
  Patchwork 2074: http://patchwork.quagga.net/patch/2074
       [quagga-dev,16135] Extend BGP_SEND_ASPATH_CHECK to cover confederations
Tested on top of Git : 5f67888 (as of 20160429.234845 UTC)
CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/


Get source and apply patch from patchwork: Successful
----------------

Building Stage: Successful
----------------

Basic Tests: Failed
----------------
Static analyzer (clang): Successful
Ipv4 protocols on ubuntu 14.04: Successful


Regards,
  NetDEF/OpenSourceRouting Continous Integration (CI) System

---
OpenSourceRouting.org is a project of the Network Device Education Foundation,
For more information, see www.netdef.org and www.opensourcerouting.org
For questions in regards to this CI System, contact Martin Winter, mwinter@netdef.org
Martin Winter Sept. 29, 2016, 11:31 p.m. | #2
Hmm… my updated CI tests with a setup verification seems to fail 
including
any errors of it.

The issue here is that RIPng failed to bring up any neighbors (may have 
crashed
at startup), so no RIPng test were executed.

- Martin

On 29 Sep 2016, at 12:00, cisystem@netdef.org wrote:

> Continous Integration Result: FAILED
>
> See below for issues.
> This is an EXPERIMENTAL automated CI system.
> For questions and feedback, feel free to email
> Martin Winter <mwinter@opensourcerouting.org>.
>
> Patches applied :
>   Patchwork 2074: http://patchwork.quagga.net/patch/2074
>        [quagga-dev,16135] Extend BGP_SEND_ASPATH_CHECK to cover 
> confederations
> Tested on top of Git : 5f67888 (as of 20160429.234845 UTC)
> CI System Testrun URL: 
> https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/
>
> Get source and apply patch from patchwork: Successful
> ----------------
>
> Building Stage: Successful
> ----------------
>
> Basic Tests: Failed
> ----------------
> Static analyzer (clang): Successful
> Ipv4 protocols on ubuntu 14.04: Successful
>
> Regards,
>   NetDEF/OpenSourceRouting Continous Integration (CI) System
>
> ---
> OpenSourceRouting.org is a project of the Network Device Education 
> Foundation,
> For more information, see www.netdef.org and www.opensourcerouting.org
> For questions in regards to this CI System, contact Martin Winter, 
> mwinter@netdef.org
Thorvald Natvig Oct. 1, 2016, 7:28 p.m. | #3
Hi,

This is my first patch to Quagga, so I'm not quite familiar with the
process. What's the next action to help this get reviewed? Should I
resubmit the patch so it gets tested again?

- Thorvald


On Thu, Sep 29, 2016 at 4:31 PM, Martin Winter <
mwinter@opensourcerouting.org> wrote:

> Hmm… my updated CI tests with a setup verification seems to fail including
> any errors of it.
>
> The issue here is that RIPng failed to bring up any neighbors (may have
> crashed
> at startup), so no RIPng test were executed.
>
> - Martin
>
>
> On 29 Sep 2016, at 12:00, cisystem@netdef.org wrote:
>
> Continous Integration Result: FAILED
>>
>> See below for issues.
>> This is an EXPERIMENTAL automated CI system.
>> For questions and feedback, feel free to email
>> Martin Winter <mwinter@opensourcerouting.org>.
>>
>> Patches applied :
>>   Patchwork 2074: http://patchwork.quagga.net/patch/2074
>>        [quagga-dev,16135] Extend BGP_SEND_ASPATH_CHECK to cover
>> confederations
>> Tested on top of Git : 5f67888 (as of 20160429.234845 UTC)
>> CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/
>>
>> Get source and apply patch from patchwork: Successful
>> ----------------
>>
>> Building Stage: Successful
>> ----------------
>>
>> Basic Tests: Failed
>> ----------------
>> Static analyzer (clang): Successful
>> Ipv4 protocols on ubuntu 14.04: Successful
>>
>> Regards,
>>   NetDEF/OpenSourceRouting Continous Integration (CI) System
>>
>> ---
>> OpenSourceRouting.org is a project of the Network Device Education
>> Foundation,
>> For more information, see www.netdef.org and www.opensourcerouting.org
>> For questions in regards to this CI System, contact Martin Winter,
>> mwinter@netdef.org
>>
>
Martin Winter Oct. 3, 2016, 6:53 a.m. | #4
On 1 Oct 2016, at 12:28, Thorvald Natvig wrote:

> Hi,
>
> This is my first patch to Quagga, so I'm not quite familiar with the
> process. What's the next action to help this get reviewed? Should I
> resubmit the patch so it gets tested again?

If you are able to find the issue, then yes please just create a v2 
version
and submit it again.

If you can’t find the problem (i.e. in this case: You tried to bring 
up a
RIPng with another Quagga instance or another router and it worked for 
you),
then let me know and I’ll look into more details.

If you want to verify the code as tested by the CI system,
then look at the URL for your test run and click on the artifact link.
There you can download the source (Base source with your patch applied)
and packages for various OS’es).

Let me know if you need any more help.

> On Thu, Sep 29, 2016 at 4:31 PM, Martin Winter <
> mwinter@opensourcerouting.org> wrote:
>
>> Hmm… my updated CI tests with a setup verification seems to fail 
>> including
>> any errors of it.
>>
>> The issue here is that RIPng failed to bring up any neighbors (may 
>> have
>> crashed
>> at startup), so no RIPng test were executed.
>>
>> - Martin
>>
>>
>> On 29 Sep 2016, at 12:00, cisystem@netdef.org wrote:
>>
>> Continous Integration Result: FAILED
>>>
>>> See below for issues.
>>> This is an EXPERIMENTAL automated CI system.
>>> For questions and feedback, feel free to email
>>> Martin Winter <mwinter@opensourcerouting.org>.
>>>
>>> Patches applied :
>>>   Patchwork 2074: http://patchwork.quagga.net/patch/2074
>>>        [quagga-dev,16135] Extend BGP_SEND_ASPATH_CHECK to cover
>>> confederations
>>> Tested on top of Git : 5f67888 (as of 20160429.234845 UTC)
>>> CI System Testrun URL: 
>>> https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/
>>>
>>> Get source and apply patch from patchwork: Successful
>>> ----------------
>>>
>>> Building Stage: Successful
>>> ----------------
>>>
>>> Basic Tests: Failed
>>> ----------------
>>> Static analyzer (clang): Successful
>>> Ipv4 protocols on ubuntu 14.04: Successful
>>>
>>> Regards,
>>>   NetDEF/OpenSourceRouting Continous Integration (CI) System
>>>
>>> ---
>>> OpenSourceRouting.org is a project of the Network Device Education
>>> Foundation,
>>> For more information, see www.netdef.org and 
>>> www.opensourcerouting.org
>>> For questions in regards to this CI System, contact Martin Winter,
>>> mwinter@netdef.org
>>>
>>
Philippe Guibert Oct. 3, 2016, 2:45 p.m. | #5
On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig <thorvald@medallia.com> wrote:

Hello Thorvald,

> Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations.
..
> -#endif /* BGP_SEND_ASPATH_CHECK */
>
>    /* If we're a CONFED we need to loop check the CONFED ID too */
>    if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION))
> @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer *peer, struct prefix *p,
>           return 0;
>         }
>      }
> +#endif /* BGP_SEND_ASPATH_CHECK */
>

Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in
compilation process on quagga-dev continuous integration testing.
So, assuming the error depicted by CI is related to BGPd  (
interoperating with RIPng?), the problem could be due to the removal
of code that has been undefined by the patch.

Regarding the patch, confederation feature can be set by BGP VTY
through "bgp confederation-identifier <id>" command.
I understand that with modification, no more check is done at
announcement for condition to send BGP update to a conference-id
group.No ?

Is it your intention to not systematically test BGP conference id
updates at announcement ?

Best Regards,

Philippe
Thorvald Natvig Oct. 3, 2016, 9:26 p.m. | #6
Hi,

The #define is, by default, undefined. It's designed to cover
re-announcements of routes learned that contain your own AS.
Unfortunately, that #define only covers half the code; it's missing
for the code that deals with confederations.

Let's use an example:

My company has AS 123, and we have two locations.
Location "US" has subnet 1.2.3.0/24 using Router A.
Location "UK" has subnet 4.5.6.0/24 using Router B.
Connectivity between the two sites is over the the internet, passing
through one or more other AS on the way.

Behind the router B sits another router; let's call this the Problem
Router or PR. Let's assume I've assigned my PR the AS 65000.

If BGP_SEND_ASPATH_CHECK is defined, then Router B will detect that
the route to 1.2.3.0/24 contains its own AS (since both A and B are on
AS 123), and will not announce 1.2.3.0/24 to PR. Hence, PR can not
reach router A.

This is clearly a problem, which is why this logic is only
conditionally enabled if BGP_SEND_ASPATH_CHECK is defined. By default
it remains undefined, so B will announce the route to PR.

Let's reconfigure to a confederation, where B and PR form a
confederation with confederation-id 123. B will have have the internal
AS 65000, and PR has AS 65111.

There are now two checks for the aspath. The check inside the #define
checks for the confederation-internal AS. B will check: Does the route
to 1.2.3.0/24 contain the AS 65000? It doesn't, so we pass the first
check.
The second check, which is not protected by the #define, checks "does
it contain the confed-id". Clearly, as-path to 1.2.3.0/24 does contain
the confed-id, so this route gets suppressed.

All this patch does is make sure this second check, which is enabled
only for confederations, is covered by the same #define as the
non-confederation case.

- Thorvald


On Mon, Oct 3, 2016 at 7:45 AM, Philippe Guibert
<philippe.guibert@6wind.com> wrote:
> On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig <thorvald@medallia.com> wrote:
>
> Hello Thorvald,
>
>> Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations.
> ..
>> -#endif /* BGP_SEND_ASPATH_CHECK */
>>
>>    /* If we're a CONFED we need to loop check the CONFED ID too */
>>    if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION))
>> @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer *peer, struct prefix *p,
>>           return 0;
>>         }
>>      }
>> +#endif /* BGP_SEND_ASPATH_CHECK */
>>
>
> Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in
> compilation process on quagga-dev continuous integration testing.
> So, assuming the error depicted by CI is related to BGPd  (
> interoperating with RIPng?), the problem could be due to the removal
> of code that has been undefined by the patch.
>
> Regarding the patch, confederation feature can be set by BGP VTY
> through "bgp confederation-identifier <id>" command.
> I understand that with modification, no more check is done at
> announcement for condition to send BGP update to a conference-id
> group.No ?
>
> Is it your intention to not systematically test BGP conference id
> updates at announcement ?
>
> Best Regards,
>
> Philippe
Philippe Guibert Oct. 4, 2016, 6:46 a.m. | #7
On Mon, Oct 3, 2016 at 11:26 PM, Thorvald Natvig <thorvald@medallia.com> wrote:

Hello Thorvald,

Thanks for the detailed explanation.
In fact, confederation feature, once assigned, is used in all
transactions ( more info on rfc5065#section-4).

For both reasons, I understand the check at announcement should be
part of the BGP_SEND_ASPATH_CHECK,
since no specific check is planned to be done at announcement.

I would ack the fix, then.

However, there is still a problem with the testing status (
https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/ ).
I would recommend to send the patch once again to quagga-dev, to
trigger the running test, and see if the error still arises or not.

If it still arises, Martin's help would be great, to identify the
culprit ANVL test, or find out the root cause.
I don't know if there can be some deprecated test ( rfc5065 is 9 years).

Best Regards,

Philippe


> Hi,
>
> The #define is, by default, undefined. It's designed to cover
> re-announcements of routes learned that contain your own AS.
> Unfortunately, that #define only covers half the code; it's missing
> for the code that deals with confederations.
>
> Let's use an example:
>
> My company has AS 123, and we have two locations.
> Location "US" has subnet 1.2.3.0/24 using Router A.
> Location "UK" has subnet 4.5.6.0/24 using Router B.
> Connectivity between the two sites is over the the internet, passing
> through one or more other AS on the way.
>
> Behind the router B sits another router; let's call this the Problem
> Router or PR. Let's assume I've assigned my PR the AS 65000.
>
> If BGP_SEND_ASPATH_CHECK is defined, then Router B will detect that
> the route to 1.2.3.0/24 contains its own AS (since both A and B are on
> AS 123), and will not announce 1.2.3.0/24 to PR. Hence, PR can not
> reach router A.
>
> This is clearly a problem, which is why this logic is only
> conditionally enabled if BGP_SEND_ASPATH_CHECK is defined. By default
> it remains undefined, so B will announce the route to PR.
>
> Let's reconfigure to a confederation, where B and PR form a
> confederation with confederation-id 123. B will have have the internal
> AS 65000, and PR has AS 65111.
>
> There are now two checks for the aspath. The check inside the #define
> checks for the confederation-internal AS. B will check: Does the route
> to 1.2.3.0/24 contain the AS 65000? It doesn't, so we pass the first
> check.
> The second check, which is not protected by the #define, checks "does
> it contain the confed-id". Clearly, as-path to 1.2.3.0/24 does contain
> the confed-id, so this route gets suppressed.
>
> All this patch does is make sure this second check, which is enabled
> only for confederations, is covered by the same #define as the
> non-confederation case.
>
> - Thorvald
>
>
> On Mon, Oct 3, 2016 at 7:45 AM, Philippe Guibert
> <philippe.guibert@6wind.com> wrote:
>> On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig <thorvald@medallia.com> wrote:
>>
>> Hello Thorvald,
>>
>>> Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations.
>> ..
>>> -#endif /* BGP_SEND_ASPATH_CHECK */
>>>
>>>    /* If we're a CONFED we need to loop check the CONFED ID too */
>>>    if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION))
>>> @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer *peer, struct prefix *p,
>>>           return 0;
>>>         }
>>>      }
>>> +#endif /* BGP_SEND_ASPATH_CHECK */
>>>
>>
>> Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in
>> compilation process on quagga-dev continuous integration testing.
>> So, assuming the error depicted by CI is related to BGPd  (
>> interoperating with RIPng?), the problem could be due to the removal
>> of code that has been undefined by the patch.
>>
>> Regarding the patch, confederation feature can be set by BGP VTY
>> through "bgp confederation-identifier <id>" command.
>> I understand that with modification, no more check is done at
>> announcement for condition to send BGP update to a conference-id
>> group.No ?
>>
>> Is it your intention to not systematically test BGP conference id
>> updates at announcement ?
>>
>> Best Regards,
>>
>> Philippe

Patch hide | download patch | download mbox

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index c364372..30daa02 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -901,7 +901,6 @@  bgp_announce_check (struct bgp_info *ri, struct peer *peer, struct prefix *p,
 	      peer->host, peer->as);
       return 0;
     }
-#endif /* BGP_SEND_ASPATH_CHECK */
 
   /* If we're a CONFED we need to loop check the CONFED ID too */
   if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION))
@@ -916,6 +915,7 @@  bgp_announce_check (struct bgp_info *ri, struct peer *peer, struct prefix *p,
 	  return 0;
 	}      
     }
+#endif /* BGP_SEND_ASPATH_CHECK */
 
   /* Route-Reflect check. */
   if (from->sort == BGP_PEER_IBGP && peer->sort == BGP_PEER_IBGP)