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

frr: 9.1 -> 10.0 #304232

Merged
merged 2 commits into from
May 22, 2024
Merged

frr: 9.1 -> 10.0 #304232

merged 2 commits into from
May 22, 2024

Conversation

thillux
Copy link
Contributor

@thillux thillux commented Apr 15, 2024

Description of changes

Release notes: https://github.com/FRRouting/frr/releases/tag/frr-10.0

Breaking changes:

  • per-daemon config files no longer supported -> only when writing, not possible with configs in read-only store paths
  • noprefixroute flag for interface prefixes with NetworkManager -> routes with noprefixroute set no longer inserted into advertised set, should be no real issue
  • -n is required for mgmtd -> currently netns-based VRFs are not supported in the module. I'd ignore this until someone needs it explicitely.
  • Enable enforce-first-as by default for BGP -> likely breaks Route Reflector/Route Server Setups -> mention in release notes?
  • Deprecate ConfD -> confd is deprecated by its developing company tail-f

Currently WIP, opened for possible discussion.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@woffs
Copy link
Contributor

woffs commented Apr 15, 2024

The "breaking changes" have to be understood. #274425 has to be reviewed/merged.

@thillux thillux marked this pull request as ready for review April 18, 2024 16:26
@thillux
Copy link
Contributor Author

thillux commented Apr 18, 2024

@woffs What's your opinion regarding the breaking changes list?

@risicle
Copy link
Contributor

risicle commented Apr 26, 2024

Probably need some movement on this because of

CVE-2024-31948
CVE-2024-31949
CVE-2024-31950
CVE-2024-31951

@risicle
Copy link
Contributor

risicle commented Apr 26, 2024

Actually I notice the respective PRs for two of those CVEs haven't even been merged yet..

@woffs
Copy link
Contributor

woffs commented Apr 28, 2024

@woffs What's your opinion regarding the breaking changes list?

per-daemon conf has to be replaced by unified config. this has to be reworked in the module, but seems to simplify things.

@thillux
Copy link
Contributor Author

thillux commented Apr 29, 2024

@woffs What's your opinion regarding the breaking changes list?

per-daemon conf has to be replaced by unified config. this has to be reworked in the module, but seems to simplify things.

FRR 10.0 also works fine with per-daemon config files in the NixOS setting. It is just not able to write them back to disk after changes in vtysh/mgmtd (which is nevertheless not possible on NixOS due to read-only config out of the store). I'd not change anything in the module for 10.0. Unifying the config to a single frr.conf will at least break user configs with the same route-map or prefix list names in different per-daemon config.

Let's justs state multiple config files as deprecated and unify them in the future otherwise it IMHO breaks config without necessity.

@woffs
Copy link
Contributor

woffs commented Apr 29, 2024

You're right, @thillux .
Changes from #274425 (still pending) and this frr-10 PR apply cleanly, test (OSPF) succeeds. The new wip #193981 test fails (BGP), but it fails in frr-9, too.
Maybe someone wants to test a bit more or wants to review the upstream changes.
IMO this looks good so far.

@nbraud
Copy link
Contributor

nbraud commented May 3, 2024

Seems OK to me, but needs a release note if enforce-first-as is now the default for BGP. The note can then refer to the upstream changelog for other breaking changes, stating they do not impact users of the NixOS module (?)

@thillux
Copy link
Contributor Author

thillux commented May 13, 2024

Seems OK to me, but needs a release note if enforce-first-as is now the default for BGP. The note can then refer to the upstream changelog for other breaking changes, stating they do not impact users of the NixOS module (?)

Done

Release notes:
https://github.com/FRRouting/frr/releases/tag/frr-10.0

Breaking changes relevant for NixOS:
- bgpd: Enable enforce-first-as by default for BGP -> may disable for RR

Some Notable changes:
- BGP RPKI VRF support
- Introduce local host routes

Notable fixes:
- Fix crash in OSPF TE parsing

Signed-off-by: Markus Theil <[email protected]>
@nbraud
Copy link
Contributor

nbraud commented May 22, 2024

Rebased to address merge conflict in release notes

@nbraud nbraud merged commit a39290d into NixOS:master May 22, 2024
7 of 8 checks passed
@nbraud
Copy link
Contributor

nbraud commented May 22, 2024

@thillux What's the plan to address the security issues in 23.11?

@thillux
Copy link
Contributor Author

thillux commented May 23, 2024

@thillux What's the plan to address the security issues in 23.11?

The plan is to address the security issues in 23.11 and unstable/24.05 when they have landed upstream. See e.g. FRRouting/frr#15674 which is an unmerged fix for CVE-2024-31951.

};

patches = [
# fixes crash in OSPF TE parsing
(fetchpatch {
Copy link
Member

Choose a reason for hiding this comment

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

Not a pressing issue but it was forgotten to remove fetchpatch from the inputs.

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.

5 participants