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

Draft: [NF] Support BFD #854

Closed
wants to merge 2 commits into from
Closed

Draft: [NF] Support BFD #854

wants to merge 2 commits into from

Conversation

rlaager
Copy link
Contributor

@rlaager rlaager commented May 27, 2023

This enables BFD in passive mode in the bird2 route server templates. Passive mode means that the route server will only speak BFD if the participant initiates it.

At this point, this is just an example. The following issues need to be resolved before this could be merged:

  • Is BFD going to be enabled everywhere? If not, it needs an option. Is that option on or off by default? If on, should it be set to off for existing Routers on migration?
  • Should the interval be customizable? Should the multiplier?

This builds on top of the commit from PR #852, which is also included here.

To do:

  • Add a per-VLAN Interface (where "Route Server Client" is now) setting to enable BFD. Default to on.
  • Add per-Router (or global in .env, but I like per-Router because, among other reasons, it allows someone to step into it one route server at a time) settings for whether BFD is enabled. Default to off.
  • Add global in .env (or per-Router, but that seems unnecessary) settings for interval and multipler. Default the interval to 1s and the multiplier to 5, per Euro-IX recommendation.
  • Write tests
  • Write documentation

In addition to the above, I have reviewed the following:

  • ensured all relevant template output is escaped to avoid XSS attached with <?= $t->ee( $data ) ?> or equivalent.
  • ensured appropriate checks against user privilege / resources accessed
  • API calls (particular for add/edit/delete/toggle) are not implemented with GET and use CSRF tokens to avoid CSRF attacks

When no MD5 password is set, there should not be extra whitespace.
At this point, this is just an example.  The following issues need to be
resolved before this could be merged:

* Is BFD going to be enabled everywhere?  If not, it needs an option.
  Is that option on or off by default?  If on, should it be set to off
  for existing Routers on migration?
* Should the interval be customizable?  Should the multiplier?
@rlaager rlaager marked this pull request as draft May 28, 2023 17:38
@rlaager rlaager changed the title WIP: [NF] Support BFD Draft: [NF] Support BFD May 28, 2023
@rlaager
Copy link
Contributor Author

rlaager commented Jun 16, 2023

@barryo Can you make a call on this design? In other words, if I implemented the above (in a reasonable and correct way), would you merge this? If the answer to that is no, then we can stop here.

If the answer to it is yes, then I'll probably put in a small amount of work to determine how easy it would be to add those options, and then make a decision as to whether I want to attempt the whole thing.

@barryo
Copy link
Member

barryo commented Jun 19, 2023

Hey @rlaager - I have time set aside in Q4 for a pretty large refactoring of route server code and would like to put a pin in this until then if you don't mind. There's been a lot of discussion around this also at euro-ix which I need to catch up on. I just don't have the time until after august. There's a few building blocks on this including #173 #174 and #351 amongst others in ideas + openbgpd.

@barryo barryo closed this Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants