bgpd: move labels from extra to extra->labels and add them to adj-rib-in and adj-rib-out #15434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
louis-6wind
force-pushed
the
labels-hash
branch
2 times, most recently
from
ce4971d
to
5e36280
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
louis-6wind
force-pushed
the
labels-hash
branch
3 times, most recently
from
8b97634
to
3673400
Compare
ton31337
approved these changes
May 21, 2024
|
failing for this--probably needs to be fixed: |
The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.
bgp_path_info stores the MPLS labels, as shown below:
> struct bgp_path_info {
> struct bgp_path_info_extra *extra;
> [...]
> struct bgp_path_info_extra {
> mpls_label_t label[BGP_MAX_LABELS];
> uint32_t num_labels;
> [...]
To solve those issues, a solution would be to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. The idea is to reference a common label
pointer in all these three structures. And to store the data in a hash
list in order to save memory.
However, an issue in the code prevents us from setting clean data
without a rework. The extra->num_labels field, which is intended to
indicate the number of labels in extra->label[], is not reliably checked
or set. The code often incorrectly assumes that if the extra pointer is
present, then a label must also be present, leading to direct access to
extra->label[] without verifying extra->num_labels. This assumption
usually works because extra->label[0] is set to MPLS_INVALID_LABEL when
a new bgp_path_info_extra is created, but it is technically incorrect.
Cleanup the label code by setting num_labels each time values are set in
extra->label[] and checking extra->num_labels before accessing the
labels.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add bgp_path_has_valid_label to check that a path_info has a valid label. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
No need to init labels at extra allocation. num_labels is the number of set labels in label[] and is initialized to 0 by default. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
In bgp_update(), path_info *new has just been created and has void labels. bgp_labels_same() is always false. Do not compare previous labels before setting them. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add bgp_path_info_labels_same() to compare labels with labels from path_info. Remove labels_same() that was used for mplsvpn only. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
num_labels cannot be greater than BGP_MAX_LABELS by design. Remove the check and the override. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
In route_vty_out_detail(), tag_buf stores a string representation of the VNI label. Rename tag_buf to vni_buf for clarity and rework the code a little bit to prepare the following commits. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add bgp_path_info_num_labels() to get the number of labels stored in a path_info structure. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Small rework to fix the following checkpatch warning: > < WARNING: Too many leading tabs - consider code refactoring > < FRRouting#2142: FILE: /tmp/f1-1616988/vnc_import_bgp.c:2142: Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
8 bits are sufficient to store the number of labels because the current maximum is 2. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Clarify bgp_vpnv4_ebgp Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
This test ensures that when r1 changes the label value, then the new value is automatically propagated to remote peer. This demonstrates that the ADJ-RIB-OUT to r2 has been correctly updated. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
The test is done on r2. A BGP update is received on r2, and is filtered on r2. The RIB of r2 does not have the BGP update stored, but the ADJ-RIB-IN is yet present. To demonstrate this, if the inbound route-map is removed, then the BGP update should be copied from the the ADJ-RIB-IN and added to the RIB with the label value. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add bgp_labels type and hash list. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Move labels from extra to extra->labels. Labels are now stored in a hash list. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Get rid of has_valid_label in bgp_update() to prepare the next commits. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
In a BGP L3VPN context using ADJ-RIB-IN (ie. enabled with 'soft-reconfiguration inbound'), after applying a deny route-map and removing it, the remote MPLS label information is lost. As a result, BGP is unable to re-install the related routes in the RIB. For example, > router bgp 65500 > [..] > neighbor 192.0.2.2 remote-as 65501 > address-family ipv4 vpn > neighbor 192.0.2.2 activate > neighbor 192.0.2.2 soft-reconfiguration inbound The 192.168.0.0/24 prefix has a remote label value of 102 in the BGP RIB. > # show bgp ipv4 vpn 192.168.0.0/24 > BGP routing table entry for 444:1:192.168.0.0/24, version 2 > [..] > 192.168.0.0 from 192.0.2.2 > Origin incomplete, metric 0, valid, external, best (First path received) > Extended Community: RT:52:100 > Remote label: 102 A route-map now filter all incoming BGP updates: > route-map rmap deny 1 > router bgp 65500 > address-family ipv4 vpn > neighbor 192.0.2.2 route-map rmap in The prefix is now filtered: > # show bgp ipv4 vpn 192.168.0.0/24 > # The route-map is detached: > router bgp 65500 > address-family ipv4 vpn > no neighbor 192.168.0.1 route-map rmap in The BGP RIB entry is present but the remote label is lost: > # show bgp ipv4 vpn 192.168.0.0/24 > BGP routing table entry for 444:1:192.168.0.0/24, version 2 > [..] > 192.168.0.0 from 192.0.2.2 > Origin incomplete, metric 0, valid, external, best (First path received) > Extended Community: RT:52:100 The reason for the loose is that labels are stored within struct attr -> struct extra -> struct bgp_labels but not in the struct bgp_adj_in. Reference the bgp_labels pointer in struct bgp_adj_in and use its values when doing a soft reconfiguration of the BGP table. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
After modifying the "label vpn export value", the vpn label information of the VRF is not updated to the peers. For example, the 192.168.0.0/24 prefix is announced to the peer with a label value of 222. > router bgp 65500 > [..] > neighbor 192.0.2.2 remote-as 65501 > address-family ipv4-vpn > neighbor 192.0.2.2 activate > exit-address-family > exit > router bgp 65500 vrf vrf2 > address-family ipv4 unicast > network 192.168.0.0/24 > label vpn export 222 > rd vpn export 444:444 > rt vpn both 53:100 > export vpn > import vpn > exit-address-family Changing the label with "label vpn export" does not update the label value to the peer unless the BGP sessions is re-established. No labels are stored are stored struct bgp_adj_out so that it is impossible to compare the current value with the previous value in adj-RIB-out. Reference the bgp_labels pointer in struct bgp_adj_out and compare the values when updating adj-RIB-out. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of #15307. #15307 is the first step : doing a preliminary rework and this pull-request is solving the following issue.
The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.
bgp_path_info stores the MPLS labels, as shown below:
To solve those issues, the solution is to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. Store labels in a new bgp_labels structure.
Reference bgp_labels pointer in
bgp_adj_inandbgp_adj_outstructures in addition to thebgp_path_info_extrastructure and use a bgp_label hash list to save memory.