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

zebra: Fix non-notification of better admin won #14795

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

donaldsharp
Copy link
Member

If there happens to be a entry in the zebra rib
that has a lower admin distance then a newly received re, zebra would not notify the upper level protocol about this happening. Imagine a case where there
is a connected route for say a /32 and bgp receives a route from a peer that is the same route as the
connected. Since BGP has no network statement and perceives the route as being good bgp will install the route into zebra. Zebra will look at the new
bgp re and correctly identify that the re is not
something that it will use and do nothing. This
change notices this and sends up a BETTER_ADMIN_WON route notification.

@donaldsharp
Copy link
Member Author

@backport stable/9.1 stable/9.0 stable/8.5

@github-actions github-actions bot added the rebase PR needs rebase label Nov 14, 2023
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

Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

Looks like the change here makes sense - but I am a little uncertain about the need for this?
No daemon can assume that zebra will install its contributions to the rib - zebra notifies daemons if it does install their routes, and that seems like a reasonable expectation. Shouldn't bgp be following that pattern - it will get a positive acknowledgement if zebra installs (or uninstalls) one of its routes?

@donaldsharp
Copy link
Member Author

Zebra is never going to install this route. BGP does not know about the prefix at all that is already installed. It has just received a route from a peer with a valid nexthop to that peer so it installs it. Prior to this patch BGP is never notified about the route not being installed as that rib_process() notices that a better path exists.. Leaving BGP in a state where it never receives an answer about the route. This is broken from my perspective.

@mjstapp
Copy link
Contributor

mjstapp commented Nov 15, 2023

I'll just capture what we talked about online today: a daemon offers zebra a route as a candidate for installation. Until zebra notifies the daemon that the route was installed ... it's not installed. There are only two states: not installed, and installed. And zebra's show output does/should indicate which daemons have offered candidates, and which was selected for installation. If bgp doesn't understand what it should and shouldn't do before it hears from zebra, then ... that may be a bgp problem that could be fixed? Something may well be broken, but ... it's not zebra or the rib processing code that's broken, imo.

But that said, I also understand that it may be easier to debug or diagnose problems if bgp has heard something, even when zebra does not select and install bgp's route contribution, so it's ok with me to have this change merged.

Zebra is never going to install this route. BGP does not know about the prefix at all that is already installed. It has just received a route from a peer with a valid nexthop to that peer so it installs it. Prior to this patch BGP is never notified about the route not being installed as that rib_process() notices that a better path exists.. Leaving BGP in a state where it never receives an answer about the route. This is broken from my perspective.

@donaldsharp donaldsharp force-pushed the zebra_notify_admin_lost branch from 582740b to f12783b Compare November 20, 2023 15:52
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

Thanks, looks good

@mjstapp
Copy link
Contributor

mjstapp commented Nov 20, 2023

Looks like the CI didn't actually run here? Let's try again

@mjstapp
Copy link
Contributor

mjstapp commented Nov 20, 2023

CI:rerun

1 similar comment
@mwinter-osr
Copy link
Member

CI:rerun

@donaldsharp
Copy link
Member Author

ci:rerun ci has gone belly up

@donaldsharp
Copy link
Member Author

ci:rerun

If there happens to be a entry in the zebra rib
that has a lower admin distance then a newly received
re, zebra would not notify the upper level protocol
about this happening.  Imagine a case where there
is a connected route for say a /32 and bgp receives
a route from a peer that is the same route as the
connected.  Since BGP has no network statement and
perceives the route as being `good` bgp will install
the route into zebra.  Zebra will look at the new
bgp re and correctly identify that the re is not
something that it will use and do nothing.  This
change notices this and sends up a BETTER_ADMIN_WON
route notification.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp donaldsharp force-pushed the zebra_notify_admin_lost branch from f12783b to 2d30050 Compare November 29, 2023 18:48
@mjstapp mjstapp merged commit 2648661 into FRRouting:master Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants