Skip to content
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 : Prevent deletion of BGP peer groups associated to listen range #15670

Merged

Conversation

poojarathore30
Copy link

Description:

Deleting a peer group also deletes its associated BGP listen range. This behaviour is undesired as it could cause unintended configuration changes.

Fix :

-Do not allow peer group deletion until they are no longer associated with any listen range.
-Check the count of listen ranges attached to the group. If any listen ranges are found, returns a configuration warning, preventing the deletion.

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix styling/formatting also.

bgpd/bgp_vty.c Outdated Show resolved Hide resolved
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good ... waiting on other's comments

@github-actions github-actions bot added size/XS rebase PR needs rebase and removed size/S labels Apr 15, 2024
@ton31337
Copy link
Member

Still... styling issues + signed-off is missing.

@poojarathore30 poojarathore30 deleted the pg-deletion-with-listen-range branch April 18, 2024 10:29
@poojarathore30 poojarathore30 restored the pg-deletion-with-listen-range branch April 18, 2024 11:26
@ton31337
Copy link
Member

Still, styling issues, see:

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 1d50e1946..88dc2303b 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -5220,10 +5220,12 @@ DEFUN (no_neighbor_peer_group,
 		for (afi = AFI_IP; afi < AFI_MAX; afi++) {
 			int lr_count = listcount(group->listen_range[afi]);
 			if (lr_count) {
-				vty_out(vty, "%%Peer-group %s is attached to %d listen-range(s), delete them first\n", group->name, lr_count);
+				vty_out(vty,
+					"%%Peer-group %s is attached to %d listen-range(s), delete them first\n",
+					group->name, lr_count);
 				return CMD_WARNING_CONFIG_FAILED;
-				}
 			}
+		}
 		peer_group_notify_unconfig(group);
 		peer_group_delete(group);
 	} else {

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have one question. If we do no neighbor PG should it also be prevented from deleting it or only for no neighbor PG peer-group?

@poojarathore30
Copy link
Author

I still have one question. If we do no neighbor PG should it also be prevented from deleting it or only for no neighbor PG peer-group?

Yeah true
Ideally if a listen-range is attached to any peer-group, we should prevent it from being deleted whether it's done with
no neighbor PG peer-group or simply no neighbor PG.

Thanks for bringing it up! I'll add the fix for the same.

Description:
-----
Deleting a peer group also deletes its associated BGP listen range.
This behaviour is undesired as it could cause unintended configuration changes.

Fix :
-----
-Do not allow peer group deletion until they are no longer associated with any listen range.
-Check the count of listen ranges attached to the group.
If any listen ranges are found, returns a configuration warning, preventing the deletion.

Signed-off-by: Pooja Rathore <rathorepo@vmware.com>
@ton31337 ton31337 merged commit 2187b82 into FRRouting:master Apr 29, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants