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.

Sign up for GitHub

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

Jump to bottom

bgpd: add bgp ipv6-auto-ra command #16354

Merged
merged 1 commit into from
Oct 28, 2024
Merged

bgpd: add bgp ipv6-auto-ra command #16354

merged 1 commit into from
Oct 28, 2024

Conversation

Sokolmish
Copy link
Contributor

Introduce a command to completely disable sending IPv6 router advertisement messages on an interface. Before, there were cases where it could be enabled by other daemons, namely bgpd and vrrpd. Particularly, it happens when BGP extended-nexthop capability is used: bgpd tells zebra to enable RA in this case, though it wasn't intended.

Fixes #7738.

@frrbot frrbot bot added the zebra label Jul 7, 2024
@Sokolmish
Copy link
Contributor Author

I apologize. I work with an older version of FRR, so I have messed up a bit with syncing changes between it and this PR. Fixed it now.

ton31337
ton31337 reviewed Jul 8, 2024
View reviewed changes
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.

Maybe shouldn't allow sending RAs or at least giving a warning (zlog_warn) to the operator that for example BGP unnumbered won't work if because you have RAs disabled?

@Sokolmish
Copy link
Contributor Author

Maybe shouldn't allow sending RAs or at least giving a warning (zlog_warn) to the operator that for example BGP unnumbered won't work if because you have RAs disabled?

Hello. Do you mean that the message should be printed in bgpd in an attempt to add an unnumbered peer? Or on any request to zebra (in zebra_interface_radv_set())?

The first case is similar to what I wanted to do initially: point out all calls from bgpd that enable RA (bgp_zebra_initiate_radv()/zclient_send_interface_radv_req()) and are not related to unnumbered peers, but I wasn't able to determine which calls are related to it and which are not.

The second case is easy, but it seems that it might create some misleading warnings in the case of RA not being intended. E.g., we use the extended-nexthop capability for SRv6 and don't need RA but will still get the warnings. Maybe reduce the level to, say, info?

@ton31337
Copy link
Member

ton31337 commented Jul 9, 2024

Do you mean that the message should be printed in bgpd in an attempt to add an unnumbered peer?

I'm trying to understand why we should allow sending RAs by any other protocol if we have it explicitly disabled per-interface level?

@Sokolmish
Copy link
Contributor Author

As far as I understood, before this PR there were 2 states: RA explicitly enabled (no suppress-ra) and default state (suppress-ra) overridable by other daemons. So I've added the explicitly disabled state (disable-ra).

Other daemons don't look at the state from vty and just send ZEBRA_INTERFACE_ENABLE_RADV to zebra. Bgpd enables RA in case of extended-nexthop capability being enabled in several places (e.g., on initialization or when new nexthop is got). VRRP enables RA when the router becomes active.

I don't think we should fail these operations because of disabled RA because it isn't the primary thing there: extended-nexthop (and VRRP, I guess) can work without RA. Unnumbered peers most probably won't (I'm not quite familiar with it), and it might be better to fail a command or print a warning in such case, but it will require determining which calls to radv_enable() in bgpd are purely for unnumbered peers, aside from getting the zebra's radv flags from bgpd.

@Jafaral Jafaral requested a review from eqvinox July 9, 2024 15:28
@eqvinox
Copy link
Contributor

eqvinox commented Jul 10, 2024 •
edited
Loading

I agree we need this feature, I've been burned by this myself before.

However, I'm not sure zebra is the correct place for this, especially after looking at the vrrpd use case.

  • bgpd: enables RA for use by BGP to find the neighbor router. Can be very unexpected and is an abuse of the protocol (should never have been designed like this.) It will be common to have no RA configuration, so the RAs are sent with default parameters.

  • vrrpd: enables RA for the RA function itself, i.e. telling clients. It just limits RA based on the VRRP role, so that only the primary router sends RAs. In this case the RAs will normally be configured by the admin and have correct settings, and they are expected and wanted.

The switch in zebra tries to fix the problem without distinguishing these two cases. It theoretically breaks VRRP use of this function.

⇒ I think this needs to be switched in bgpd, not zebra.

eqvinox
eqvinox previously requested changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@eqvinox eqvinox left a comment

Choose a reason for hiding this comment

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

I strongly believe this switch should be moved into bgpd instead.

@frrbot frrbot bot added the bgp label Jul 11, 2024
@github-actions github-actions bot added size/M and removed size/L labels Jul 11, 2024
@Sokolmish Sokolmish changed the title zebra: add ipv6 nd disable-ra command bgpd: add bgp ipv6-auto-ra command Jul 11, 2024
@Sokolmish
Copy link
Contributor Author

Sokolmish commented Jul 11, 2024 •
edited
Loading

I've added a new option to bgpd. Used the preserve-fw-state option as a template for VTY command and flag handling.

ton31337
ton31337 requested changes Jul 14, 2024
View reviewed changes
bgpd/bgp_zebra.c Outdated
@@ -2358,6 +2361,11 @@ void bgp_zebra_initiate_radv(struct bgp *bgp, struct peer *peer)

void bgp_zebra_terminate_radv(struct bgp *bgp, struct peer *peer)
{
/*
Copy link
Member

Choose a reason for hiding this comment

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

Abandoned comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

bgpd/bgp_vty.c Outdated
return CMD_SUCCESS;
}

DEFUN (bgp_ipv6_auto_ra,
Copy link
Member

Choose a reason for hiding this comment

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

DEFPY please, and please combine into a single DEFPY for both forms ([no] bgp ipv6-auto-ra).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bgpd/bgp_vty.c Show resolved Hide resolved
@Sokolmish Sokolmish force-pushed the zebra-no-ra branch 2 times, most recently from d5aa081 to 02a6900 Compare July 15, 2024 18:28
@Sokolmish
Copy link
Contributor Author

I keep getting random topotests failed in modules I haven't touched in the PR (now it's MSDP, before it was OSPF, PIM). Is it the infrastructure problem, or am I missing something?

riw777
riw777 approved these changes Jul 16, 2024
View reviewed changes
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 comments to be cleared and running failed test

ton31337
ton31337 reviewed Jul 16, 2024
View reviewed changes
bgpd/bgp_nht.c Outdated
@@ -1515,6 +1516,10 @@ void bgp_nht_reg_enhe_cap_intfs(struct peer *peer)
return;

bgp = peer->bgp;

/* Shouldn't enable RA if they are disabled */
assert(!CHECK_FLAG(bgp->flags, BGP_FLAG_IPV6_NO_AUTO_RA));
Copy link
Member

Choose a reason for hiding this comment

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

I doubt on this assert, why is this needed? Can't be handle it more gracefully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called from only one place that is already covered by checking the flag, so I thought about making an assert there. Now I've changed it to a regular check with early return.

@eqvinox eqvinox self-requested a review July 17, 2024 16:39
ton31337
ton31337 approved these changes Jul 22, 2024
View reviewed changes
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.

LGTM

@donaldsharp donaldsharp self-requested a review July 23, 2024 15:22
@riw777
Copy link
Member

riw777 commented Aug 6, 2024

I strongly believe this switch should be moved into bgpd instead.

I think this is done now (?) ... @eqvinox can you take a look to see if this okay?

@riw777
Copy link
Member

riw777 commented Aug 27, 2024

@eqvinox ping :-)

1 similar comment
@riw777
Copy link
Member

riw777 commented Sep 24, 2024

@eqvinox ping :-)

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Sokolmish
bgpd: add bgp ipv6-auto-ra command
7b1c0c2 
Introduce a command to stop bgpd from enabling IPv6 router advertisement
messages sending on interfaces.

Signed-off-by: Mikhail Sokolovskiy <sokolmish@gmail.com>
@ruben-herold ruben-herold mentioned this pull request Oct 10, 2024
Closed
2 tasks
@eqvinox eqvinox dismissed their stale review October 28, 2024 10:21

outdated

@eqvinox eqvinox merged commit e4df480 into FRRouting:master Oct 28, 2024
11 checks passed
@eqvinox
Copy link
Contributor

eqvinox commented Oct 28, 2024

@Mergifyio backport dev/10.2 stable/10.1 stable/10.0 stable/9.1 stable/9.0 stable/8.5 stable/8.4

@mergify Mergify
Copy link

mergify bot commented Oct 28, 2024 •
edited
Loading

backport dev/10.2 stable/10.1 stable/10.0 stable/9.1 stable/9.0 stable/8.5 stable/8.4

✅ Backports have been created

This was referenced Oct 28, 2024
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Jafaral added a commit that referenced this pull request Oct 28, 2024
@Jafaral
Merge pull request #17265 from FRRouting/mergify/bp/dev/10.2/pr-16354
e144cc3 
bgpd: add bgp ipv6-auto-ra command (backport #16354)
This was referenced Oct 29, 2024
Merged
Merged
Merged
Merged
Jafaral added a commit that referenced this pull request Nov 12, 2024
@Jafaral
FRR Release 10.2
22746b8 
New Features Highlight:

- PIM candidate BSR/RP [#16438]
- Static IGMP join without an IGMP report [1#6450]
- PIM AutoRP discovery/announcements [#16634]
- IGMP proxy [#16861]
- SRv6 SID Manager [#15604]
- Add `bgp ipv6-auto-ra` command [#16354]
- Implement `neighbor x remote-as auto` for BGP [#16345]
- Implement `bgp dual-as` for BGP [#16816]
- Implement BGP-wide configuration for graceful restart [#16099]
- Handle kernel routes appropriately (should fix recent NOPREFIXROUTE issue) [#16300]
- Add `cisco-authentication` password support for NHRP [#16172]

Signed-off-by: Jafar Al-Gharaibeh <jafar@atcorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ton31337 ton31337 ton31337 approved these changes

@eqvinox eqvinox eqvinox approved these changes

@riw777 riw777 riw777 approved these changes

@donaldsharp donaldsharp Awaiting requested review from donaldsharp

Assignees
No one assigned
Labels
backport bgp master size/M zebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipv6 nd suppress-ra does not take for OS-configured interface
4 participants
@Sokolmish @ton31337 @eqvinox @riw777