bgpd: Add multiple clusters for bgp route-reflectors#22038
bgpd: Add multiple clusters for bgp route-reflectors#22038PierreNeltner6WIND wants to merge 8 commits into
Conversation
PierreNeltner6WIND
force-pushed
the
multiple_clusters
branch
25 times, most recently
from
45eb1e1 to
4fbaccf
Compare
|
@greptile review |
|
@greptile In your review you said this : The group-member unset loop decrements the cluster refcount for members that may never have had the flag set, which can free the cluster object while live peers still reference it. The peer_delete path similarly decrements the refcount for inherited cluster references, but peer_group2peer_config_copy_af may re-add the reference afterward, leaving the refcount permanently inflated. However for the first flagged issue the members either have indeed inherited that attribute from their group and the refcnt decrease et justified or they don't that happens only when a per-neighbor cluster as been explicitly specified for them ib that case the peer->af_flags_override is set, and the refcnt is not decreased. I don't think there is any risk of the refcnt being falsely decreased. Also can you stop flagging this : contains multiple refcount issues in peer-group flows and group member loops, wrong-peer reset bugs in group leader loops (flagged in previous rounds) or be more specific about it because O can't find these problems. |
|
Your explanations on both points are correct:
On the vague "multiple refcount issues / wrong-peer reset bugs" language: you're right to push back on that. Looking at the thread history, the specific wrong-peer issues were flagged as individual inline comments (lines 6983 and 7041) and have since been resolved. I should not be referencing them in aggregate summaries without citing specific lines. I'll stop doing that — if I see a concrete problem I'll point to the exact code location. Apologies for the noise on these points. |
|
@greptile review |
|
@greptile here is your review: Issue 1: This is prevented by placed before the reset loop in bgp_per_neighbor_cluster_id_delete, this prevents teardown time problems. For the rest of cases one of the first things that peer_delete does is to delete the peer from the bgp->peer list, therefor there is no risk of the soft_clear being called mid-deletion. Issue 2: this has already been addressed and the check of whether the peer is a group now only prevents the clear_soft et does not affect flag update. Issue 3: peer-group AS-change doesn't need a loop as it is already called on all member peers. |
|
Thanks for the detailed responses. All three explanations are technically sound:
No further action needed from my side on these three points. |
|
@greptile review |
1 similar comment
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile review |
1 similar comment
|
@greptile review |
Greptile SummaryThis PR adds support for multiple cluster-ids per BGP route-reflector, enabling per-neighbor cluster identity assignment per AFI/SAFI. It introduces a new
Confidence Score: 3/5PR introduces substantial new feature with generally sound refcount/lifecycle management but contains a misleading redundant condition in the cluster-id stamping logic and an O(N) hot-path scan that may need profiling at scale before merging Core lifecycle (alloc/free/refcount), peer-group propagation, teardown guards, and update-group hashing are all correct. The redundant negation in bgp_attr.c case 2 is a P2 clarity issue but not a runtime bug. The O(N) cluster scan in bgp_cluster_filter is architecturally intentional (RFC 4456) but warrants a comment noting the complexity. No data-loss or security issues found. Score of 3 reflects the feature complexity and the attr.c logic clarity issue. bgpd/bgp_attr.c (redundant condition in cluster-id stamping chain), bgpd/bgp_route.c (O(N) scan in bgp_cluster_filter on inbound path) Important Files Changed
Reviews (26): Last reviewed commit: "doc: Add documentation to bgp client-to-..." | Re-trigger Greptile |
|
@greptile can you verify all your comments have been resolved? |
|
All of my (Greptile) comments are resolved. Here's the summary: My comments — all resolved (✅):
Unresolved threads (not mine — from @riw777):
You may want to address those two remaining threads from @riw777 before merging. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Add the command : neighbor <A.B.C.D|X:X::X:X|WORD> cluster-id CLUSTER_ID This command can be executed after router bgp AS, address-family AFI SAFI. This command assigns a per-neighbor cluster for that specific peer and afi safi combination. If the peer is not previously declared that command fails. For that purpose the peer struct has one new attribute per_neighbor_cluster that is an in_addr list the content of which can be accessed through afi and safi indexes. This list is associated with a new af_flag that indicates whether it is configured. Another new attribute is added: cluster_flags that is also accessed throug afi safi indexes and ensures that the checking of cluster_flags can be performed in O(1) in the hot-path. A new structure is added to represent the new per-neighbor clusters: struct cluster, it contains the cluster-id of the cluster, flags that define its behavior and a count of how many times a cluster is referenced in the configuration, when that count goes down to 0 the cluster is deleted. That count takes into account every peers and groups that are part of the cluster and any reflecetion policy for that group. The CLUSTER_FLAG_GLOBAL marks any per-neighbor cluster whose cluster-id is identical to the global cluster-id: the unique historical cluster-id of route-reflectors. Whenever a cluster is marked as global,its flags are ignored and the ones of the global cluster are used instead. bgp instances have a new attribute: per_neighbor_clusters: a list that contains cluster objects. A peer that is part of a per-neighbor cluster and is configured as a route-reflector-client, behaves as a classic route-reflector-client peer. However, prefixes received from that peer will be advertised to other client and non-client peers with the per-neighbor cluster-id of that peer in the prefix's cluster-list. In the same spirit prefix reflected to one such peer from a non-client peer will have the per-neighbor cluster-id of the client-peer added to the cluster list. When a prefix is reflected between two client peers part of different per-neighbor clusters only the cluster-id of the sender is added to the cluster-list. If not configured as route-reflector client, a peer with a per-neighbor cluster-id behaves as a normal peer. Because the per-neighbor cluster-ids have an influence on out-going prefixes, this commit also modifies update-groups more specificaly ```updgrp_hash_key_make``` and ```updgrp_hash_cmp``` in order to have peers from different per-neighbor clusters sorted in different update groups. In the spirit of the rfc4456 any incoming prefix that contains one of the cluster-ids of the route-reflector clients is dropped. This implies changes to the ```bgp_cluster_filter``` function in order to check all clusters and not only the global one. A cluster-id can be assigned to a peer that is not a route-reflector-client in that case that peer will behave normaly but the cluster-id will still be taken into account for filtering. This makes sure that commands do not depend on the execution order. Information about cluster-ids that are configured on peers can be found in the running config and in the display of that peer's information. Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
the topology of the test is the following : +------+ +------+ | R1--|- -|--R6 | | | \ R9 / | | C1 | | \ / \ / | | C3 | R2--|---R4--R5---|--R7 | +------+ / \ +------+ +------+ / \ +------+ C2 | R3--|- -|--R8 | C2 +------+ +------+ All routers are in AS 100 (IBGP). R4 and R5 are configured as a route reflector with their neighbors as clients. They form with r9 a full-mesh of non-client peers. They each oversee two clusters of clients Test that reflection in the default setup works as intended and that cluster-lists are correct, verifies that client-to-client configurations works well both in the per-neighbor and global clusters. Test that the non-client-to-client prefer-global-cluster-id and loose-cluster-list-check configuration work as intentded. These checks are achieved through verifying the prefixes in the bgp tables of each client router and on R9 as well as their cluster-lists. Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
Refactor the route-reflector reflection-check in bgp_route.c in order to prepare for next commit. Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
Add 2 new flags: CLUSTER_FLAG_CLIENT_TO_CLIENT_INTRA_CLUSTER and CLUSTER_FLAG_CLIENT_TO_CLIENT_INTRA_CLUSTER_CONFIGURED. These flags represent the three states that are possible for a cluster configuration and apply to the flags attribute of the cluster object: the cluster-to-cluster behavior is not configured, the cluster-to-cluster behavior is configured and enabled, the cluster-to-cluster behavior is configured and disabled Add 2 new flags: BGP_FLAG_CLIENT_TO_CLIENT_GLOBAL_CLUSTER and BGP_FLAG_CLIENT_TO_CLIENT_GLOBAL_CLUSTER_CONFIGURED. These flags have same role as the ones described before but for the global cluster, they apply to bgp->flags. The behavior of client-to-client reflection is now the following: reflection inside of a cluster is always allowed if it is configured as allowed for that cluster, it is always forbidden if it is configured as forbidden for that cluster. If nothing is configured for that cluster then the legacy client-to-client reflection configuration applies. Between two different clusters the legacy client-to-client reflection configuration applies. This allows any pattern of reflection to be easily set up. This configuration is achieved through the new command: ```[no] bgp cluster <per-neighbor X.X.X.X|global> \ client-to-client-reflection <always|never>``` That command configures client-to-client reflection on any per-neighbor cluster or the global cluster. In its negative form it unconfigures client-to-client reflection. If it is configured on a per-neighbor cluster then adds one to the reference of that cluster and if it doesn't exist creates it in a similar fashion to the neighbor X.X.X.X cluster-id command. These configurations are shown in the running config Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
add the command :
```Add command show bgp clusters [json]```
that shows information about all configured or referenced clusters
including the global cluster and per-neighbor clusters
this includes the cluster id, client-to-client reflection policies
inside of each cluster and number of times referenced in the
configuration as well as whether it is overwritten by the global
cluster.
the json format is the following:
{
"global":{
"cluster-id":"CLUSTER_ID",
"client-to-client-reflection":<"always"|"never"|"not-configured">
},
"per-neighbor":[
"cluster-id":"CLUSTER_ID",
"global":bool,
"client-to-client-reflection":<"always"|"never"|"not-configured">,
"refcnt":NUMBER_OF_REFERENCES
],
}
Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
Add the command : bgp cluster-id non-client-to-client prefer-global-cluster This command configures the new flag BGP_FLAG_PREFER_GLOBAL_CLUSTER When this flag is configured, the global cluster-id is added to the cluster list of prefixes reflected from non-clients toward client-peers, default behavior is to add the destination peer's cluster-id instead. When configured the behavior is similar to the cisco one and further from a classic cluster-id behavior. This feature modifies the attributes of outgoing routes and so modifies the ```bgp_packet_attribute``` function from bgp_attribute.c This flag can be seen in the running config. Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
add the command: bgp cluster-id loose-cluster-list-check This command configures the new bgp flag: BGP_FLAG_LOOSE_CLUSTER_LIST_CHECK When this is configured, prefixes that contain one or more of the cluster-ids of the per neighbor clusters of the router are no longer dropped when received, they are just not reflected to members of these clusters. This implies that the ```bgp_cluster_filter``` function from bgp_route.c is modified to no longer reject these and that ```subgroup_announce_check``` is modified to reject these prefixes only when advertising toward a member of that cluster. When configured this option shows a behavior closer to juniper behavior while still respecting rfc4456. This configuration can be seen in the running-config. Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
Signed-off-by: Pierre Neltner <pierre.neltner@6wind.com>
4 participants
Add several commands that grant the possibility
to configure cluster-id on a per-neighbor basis
for route reflectors.
This allows more possibility in client-to-client
reflection management.
these commands are the following:
neighbor X.X.X.X cluster-id CLUSTER_ID
bgp cluster-id <per neighbor CLUSTER_ID|global>
client-to-client-reflection <true|fase>
show bgp clusters
two other command can change the behavior to be
closer to either juniper or cisco:
bgp cluster-id non-client-to-client prefer-global-cluster-id
bgp cluster-id loose_cluster-list-check