New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: add basic support for ETI and ESI for BGP EVPN #2035

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
7 participants
@vincentbernat
Copy link
Contributor

vincentbernat commented Apr 6, 2018

Ethernet Segment ID (ESI) and Ethernet Tag ID (ETI) are both part of
the prefix. They cannot just be ignored as they need to be be used
when checking for prefix uniqueness. Moreover, when using Quagga as a
route reflector, we need to keep their values. Therefore, we correctly
parse and encode them.

This is needed with implementations using a fixed RD (like JunOS).

@LabN-CI

This comment has been minimized.

Copy link

LabN-CI commented Apr 6, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2035 f193f15
Date 04/06/2018
Start 09:55:21
Finish 10:18:19
Run-Time 22:58
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-04-06-09:55:21.txt
Log autoscript-2018-04-06-09:56:08.log.bz2

For details, please contact louberger

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

NetDEF-CI commented Apr 6, 2018

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-FRRPULLREQ-3233/

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


Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for bgp_evpn.c
===============================================
< ERROR: return is not a function, parentheses are not required
< #3700: FILE: /tmp/f1-24124/bgp_evpn.c:3700:
< +	return (buf);
< 
< WARNING: line over 80 characters
< #3733: FILE: /tmp/f1-24124/bgp_evpn.c:3733:
< +		stream_putl(s, evp->prefix.eth_tag);	    /* Ethernet Tag ID */
< 
--
< WARNING: line over 80 characters
< #3749: FILE: /tmp/f1-24124/bgp_evpn.c:3749:
< +		stream_putl(s, evp->prefix.eth_tag);		 /* Ethernet Tag ID */
< 

CLANG Static Analyzer Summary

  • Github Pull Request 2035, comparing to Git base SHA 52483fa

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3233/artifact/shared/static_analysis/index.html

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

NetDEF-CI commented Apr 6, 2018

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-FRRPULLREQ-3234/

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


Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for bgp_evpn.c
===============================================
< ERROR: return is not a function, parentheses are not required
< #3700: FILE: /tmp/f1-3025/bgp_evpn.c:3700:
< +	return (buf);
< 
< WARNING: line over 80 characters
< #3733: FILE: /tmp/f1-3025/bgp_evpn.c:3733:
< +		stream_putl(s, evp->prefix.eth_tag);	    /* Ethernet Tag ID */
< 
--
< WARNING: line over 80 characters
< #3749: FILE: /tmp/f1-3025/bgp_evpn.c:3749:
< +		stream_putl(s, evp->prefix.eth_tag);		 /* Ethernet Tag ID */
< 

CLANG Static Analyzer Summary

  • Github Pull Request 2035, comparing to Git base SHA 52483fa

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3234/artifact/shared/static_analysis/index.html

@@ -3623,6 +3632,8 @@ void bgp_evpn_route2json(struct prefix_evpn *p, json_object *json)
} else {
/* Currently, this is to cater to other AF_ETHERNET code. */
}

free(esi);

This comment has been minimized.

@qlyoung

qlyoung Apr 6, 2018

Member

This should be XFREE(MTYPE_TMP, esi);

@@ -3671,6 +3696,7 @@ char *bgp_evpn_route2str(struct prefix_evpn *p, char *buf, int len)
p->prefix.route_type);
}

free(esi);

This comment has been minimized.

@qlyoung

qlyoung Apr 6, 2018

Member

XFREE(MTYPE_TMP, esi);

This comment has been minimized.

@vincentbernat

vincentbernat Apr 6, 2018

Contributor

PR updated!

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/no-etag-esi-ignore branch from f193f15 to 4177b1b Apr 6, 2018

@LabN-CI

This comment has been minimized.

Copy link

LabN-CI commented Apr 6, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2035 4177b1b
Date 04/06/2018
Start 12:55:19
Finish 13:18:17
Run-Time 22:58
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-04-06-12:55:20.txt
Log autoscript-2018-04-06-12:56:07.log.bz2

For details, please contact louberger

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

NetDEF-CI commented Apr 6, 2018

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-FRRPULLREQ-3236/

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


Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for bgp_evpn.c
===============================================
< ERROR: return is not a function, parentheses are not required
< #3700: FILE: /tmp/f1-16336/bgp_evpn.c:3700:
< +	return (buf);
< 
< WARNING: line over 80 characters
< #3733: FILE: /tmp/f1-16336/bgp_evpn.c:3733:
< +		stream_putl(s, evp->prefix.eth_tag);	    /* Ethernet Tag ID */
< 
--
< WARNING: line over 80 characters
< #3749: FILE: /tmp/f1-16336/bgp_evpn.c:3749:
< +		stream_putl(s, evp->prefix.eth_tag);		 /* Ethernet Tag ID */
< 

CLANG Static Analyzer Summary

  • Github Pull Request 2035, comparing to Git base SHA 52483fa

No Changes in Static Analysis warnings compared to base

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3236/artifact/shared/static_analysis/index.html

@donaldsharp

This comment has been minimized.

Copy link
Member

donaldsharp commented Apr 7, 2018

@vincentbernat I've asked @vivek-cumulus and @mkanjari to review this PR. In the mean time I believe we need to fix the 'signed-off-by:..' line in the commit message as well as the ERROR reported in bgp_evpn.c on line 3700.

The code looks fine to me otherwise, but I want someone who fully understands EVPN to comment on this as well.

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/no-etag-esi-ignore branch from 4177b1b to 1c5bff5 Apr 7, 2018

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

NetDEF-CI commented Apr 7, 2018

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-FRRPULLREQ-3247/

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


Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for bgp_evpn.c
===============================================
< ERROR: return is not a function, parentheses are not required
< #3700: FILE: /tmp/f1-27111/bgp_evpn.c:3700:
< +	return (buf);
< 
< WARNING: line over 80 characters
< #3733: FILE: /tmp/f1-27111/bgp_evpn.c:3733:
< +		stream_putl(s, evp->prefix.eth_tag);	    /* Ethernet Tag ID */
< 
--
< WARNING: line over 80 characters
< #3749: FILE: /tmp/f1-27111/bgp_evpn.c:3749:
< +		stream_putl(s, evp->prefix.eth_tag);		 /* Ethernet Tag ID */
< 

CLANG Static Analyzer Summary

  • Github Pull Request 2035, comparing to Git base SHA 8227cf9

Fixed warnings:

  • Logic error: Dereference of null pointer in ospf6d/ospf6_intra.c, function ospf6_intra_prefix_lsa_remove, line 1851

Static Analysis warning summary compared to base:

  • Fixed warnings: 1
  • New warnings: 0

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3247/artifact/shared/static_analysis/index.html

@LabN-CI

This comment has been minimized.

Copy link

LabN-CI commented Apr 7, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2035 1c5bff5
Date 04/07/2018
Start 02:24:41
Finish 02:47:49
Run-Time 23:08
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-04-07-02:24:41.txt
Log autoscript-2018-04-07-02:25:31.log.bz2

For details, please contact louberger

@mkanjari
Copy link
Contributor

mkanjari left a comment

@vincentbernat : ESI is not a part of prefix for type-2 route.
From section 7.2 (https://tools.ietf.org/html/rfc7432#section-7.2)
"For the purpose of BGP route key processing, only the Ethernet Tag
ID, MAC Address Length, MAC Address, IP Address Length, and IP
Address fields are considered to be part of the prefix in the NLRI.
The Ethernet Segment Identifier, MPLS Label1, and MPLS Label2 fields
are to be treated as route attributes as opposed to being part of the
"route". Both the IP and MAC address lengths are in bits."

It is a part of the prefix for only type-1 and type-4 route. We will have to think through this a bit. May be a we need a union in the prefix_evpn ?

This is not present in frr currently because we dont support type-1 and type-4 routes yet.

@vincentbernat

This comment has been minimized.

Copy link
Contributor

vincentbernat commented Apr 9, 2018

Since it is never part of the prefix for us, I can just move it out. Otherwise, there is some work to be done in lib/prefix.c as well: an union would break uses of memcmp() and sizeof().

bgpd: add basic support for ETI and ESI for BGP EVPN
Ethernet Tag ID (ETI) is part of the prefix. It cannot just be ignored
as it needs to be used when checking for prefix uniqueness. Moreover,
when using Quagga as a route reflector, we need to keep its
value. Therefore, we correctly parse and encode it. We also parse
ESI. While not part of the prefix, it needs to be reflected correctly
by Quagga.

Signed-off-by: Vincent Bernat <vincent@bernat.im>

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/no-etag-esi-ignore branch from 1c5bff5 to 554cd77 Apr 9, 2018

@vincentbernat

This comment has been minimized.

Copy link
Contributor

vincentbernat commented Apr 9, 2018

I have amended the PR to not include ESI as part of the prefix. I have also removed it from the output (JSON and plain) of "show route". Unfortunately, the display functions all take a prefix, so I cannot display the ESI in the JSON output.

@LabN-CI

This comment has been minimized.

Copy link

LabN-CI commented Apr 9, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2035 554cd77
Date 04/09/2018
Start 06:18:34
Finish 06:41:39
Run-Time 23:05
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-04-09-06:18:34.txt
Log autoscript-2018-04-09-06:19:24.log.bz2

For details, please contact louberger

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

NetDEF-CI commented Apr 9, 2018

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-FRRPULLREQ-3257/

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


Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for bgp_evpn.c
===============================================
< WARNING: line over 80 characters
< #3721: FILE: /tmp/f1-17771/bgp_evpn.c:3721:
< +		stream_putl(s, evp->prefix.eth_tag);	    /* Ethernet Tag ID */
< 
--
< WARNING: line over 80 characters
< #3737: FILE: /tmp/f1-17771/bgp_evpn.c:3737:
< +		stream_putl(s, evp->prefix.eth_tag);		 /* Ethernet Tag ID */
< 

CLANG Static Analyzer Summary

  • Github Pull Request 2035, comparing to Git base SHA 8227cf9

Fixed warnings:

  • Logic error: Dereference of null pointer in ospf6d/ospf6_intra.c, function ospf6_intra_prefix_lsa_remove, line 1851

Static Analysis warning summary compared to base:

  • Fixed warnings: 1
  • New warnings: 0

19 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3257/artifact/shared/static_analysis/index.html

@vincentbernat

This comment has been minimized.

Copy link
Contributor

vincentbernat commented Apr 9, 2018

After testing more, this doesn't work as expected. I would need to investigate a bit.

@donaldsharp

This comment has been minimized.

Copy link
Member

donaldsharp commented Apr 12, 2018

@vincentbernat @mkanjari @qlyoung any updates here?

@vincentbernat

This comment has been minimized.

Copy link
Contributor

vincentbernat commented Apr 12, 2018

For me, this one is OK now. I took the lazy option of not putting ESI at all in the prefix, which is valid for type-2, type-3 and type-5 prefixes (the only ones we support). In conjuction with #234, I get working connectivity with QFX and MX when they don't use multi-homing.

@qlyoung
Copy link
Member

qlyoung left a comment

looks fine to me

@mkanjari
Copy link
Contributor

mkanjari left a comment

Looks good to me. We will have to make relevant changes when we start supporting type-1 and type-4 routes. For now this looks good.

@donaldsharp

This comment has been minimized.

Copy link
Member

donaldsharp commented Apr 17, 2018

@mkanjari @vincentbernat is this one ready? I can push in if it is

@vincentbernat

This comment has been minimized.

Copy link
Contributor

vincentbernat commented Apr 17, 2018

For me, this one is ready.

@mkanjari

This comment has been minimized.

Copy link
Contributor

mkanjari commented Apr 17, 2018

Looks good to me.

@donaldsharp donaldsharp merged commit e49b64d into FRRouting:master Apr 17, 2018

1 check passed

default Tested by NetDEF/OpenSourceRouting CI System
Details
@vjardin

This comment has been minimized.

Copy link

vjardin commented Jun 23, 2018

Same, just record Vincent's blog for those who need to use EVPN
https://vincent.bernat.im/fr/blog/2017-vxlan-bgp-evpn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment