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

WIP: BIER extension for OSPFv3 #16

Merged
merged 13 commits into from
Sep 10, 2024
Merged

Conversation

nrybowski
Copy link
Contributor

@nrybowski nrybowski commented Apr 8, 2024

This PR adds BIER extensions for OSPFv3 as defined by draft-ietf-bier-ospfv3-extensions-07. The TLV format is the same as for OSPFv2 hence implementing RFC8444 on the basis laid down by this PR should be straightforward.

It relies on #10 for the addition of the BIER YANG model.

TODO

  • Add non-mpls encapsulation sub-sub-TLV as defined by draft-ietf-bier-lsr-non-mpls-extensions-03
  • Encode BIER encapsulation sub-sub-TLV
  • Handle events
    • BierEnableChange
    • BierCfgUpdate
    • BierCfgEncapUpdate
  • Enforce constraints on incoming BIER TLVs
  • Integration tests
  • Implement BIER control-plane (BIRT computation, ...)
  • Add unit test for BIER TLVs

@nrybowski
Copy link
Contributor Author

Rebase on 9806948

@nrybowski nrybowski force-pushed the bier_ospfv3 branch 2 times, most recently from b147a94 to bd3277e Compare August 14, 2024 08:15
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 40.70796% with 134 lines in your changes missing coverage. Please review.

Project coverage is 63.07%. Comparing base (c370052) to head (81a0ab1).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
holo-ospf/src/ospfv3/lsdb.rs 4.87% 39 Missing ⚠️
holo-ospf/src/northbound/configuration.rs 16.66% 35 Missing ⚠️
holo-ospf/src/bier.rs 0.00% 21 Missing ⚠️
holo-ospf/src/southbound/tx.rs 12.50% 14 Missing ⚠️
holo-ospf/src/events.rs 0.00% 8 Missing ⚠️
holo-ospf/src/packet/tlv.rs 89.33% 8 Missing ⚠️
holo-ospf/src/instance.rs 0.00% 6 Missing ⚠️
holo-ospf/src/route.rs 60.00% 2 Missing ⚠️
holo-ospf/src/lsdb.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   63.40%   63.07%   -0.34%     
==========================================
  Files         178      179       +1     
  Lines       29704    30018     +314     
==========================================
+ Hits        18833    18933     +100     
- Misses      10871    11085     +214     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nrybowski
Copy link
Contributor Author

I'm not sure to understand why the BGP test suite is failing (https://github.com/holo-routing/holo/actions/runs/10468201712/job/28988566615?pr=16), I can't reproduce locally ..

@nrybowski
Copy link
Contributor Author

The PR is still missing some integration tests and sanity check upon BIER TLV reception but it should be ready for a first review.

@nrybowski nrybowski marked this pull request as ready for review August 20, 2024 08:49
Copy link
Member

@rwestphal rwestphal left a comment

Choose a reason for hiding this comment

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

@nrybowski Amazing work! I'm really happy seeing progress on this front (SR ❤️ BIER).

I've left a few review comments below, all of which should be easy to address.

The TLV format is the same as for OSPFv2 hence implementing RFC8444 on the basis laid down by this PR should be straightforward.

That's great. I can see the OSPFv3-specific changes are pretty small, and most of the OSPF BIER code being added is version-agnostic (including the TLV code). Hoping we can get BIER support for OSPFv2 in the future too :)

I'm not sure to understand why the BGP test suite is failing (https://github.com/holo-routing/holo/actions/runs/10468201712/job/28988566615?pr=16), I can't reproduce locally ..

This was fixed on the master branch. Please rebase to solve the problem.

The PR is still missing some integration tests and sanity check upon BIER TLV reception but it should be ready for a first review.

I think this PR is already a huge step forward. I wouldn't mind seeing those tests and sanity checks implemented in a separate PR later on.

holo-ospf/src/route.rs Show resolved Hide resolved
holo-yang/modules/augmentations/holo-routing.yang Outdated Show resolved Hide resolved
holo-yang/modules/augmentations/holo-ospf.yang Outdated Show resolved Hide resolved
description
"Enables bier protocol extensions.";
}
leaf advertise {
Copy link
Member

Choose a reason for hiding this comment

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

Question: is there a case where one might want to enable the "advertise" leaf but not the "receive" leaf, or vice-versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Draft-09 does not really detail the goal of such switches.

I guess that they control the behavior of the BFR, e.g. a BFR that only advertises its prefixes could be an egress-only (i.e., a leaf in multicast trees). But this seems error prone as other BFRs must be aware that this specific BFR is not able to forward multicast traffic. A BFR configured with "receive" could be an ingress-only or a "passive" probe that collects the BIRTs for monitoring purpose.

Furthermore, draft-08 added those values in an additional sub-domain container, which is not reflected in the current implementation ..

The "sub-domain" container controls the routing-protocol instance's
advertisement of the relevant BIER information of the BIER subdomain
and the processing of received the relevant BIER information of the
BIER subdomain.

holo-utils/src/bier.rs Outdated Show resolved Hide resolved
holo-ospf/src/northbound/configuration.rs Show resolved Hide resolved
- Add YANG augmentations from draft-ietf-bier-bier-yang-07 OSPF-side
- Partial implementation of draft-ietf-bier-ospfv3-extensions-07

Signed-off-by: Nicolas Rybowski <[email protected]>
Signed-off-by: Nicolas Rybowski <[email protected]>
- Handle BIER enable / disable by re-installing routes in the BIRT /
purging the BIRT, and re-originating self-LSA to advertise the update
- Handle BIER sub-domain configuration events by re-originating self-LSA
- Handle BIER encapsulation configuration events by re-originating
self-LSA

Signed-off-by: Nicolas Rybowski <[email protected]>
@nrybowski
Copy link
Contributor Author

Rebase on c370052 to fix CI failure on BGP tests.

@nrybowski nrybowski requested a review from rwestphal September 9, 2024 10:56
Copy link
Member

@rwestphal rwestphal left a comment

Choose a reason for hiding this comment

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

@nrybowski Thank you for the updates! Merging...

@rwestphal rwestphal merged commit 1d3461c into holo-routing:master Sep 10, 2024
5 checks passed
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.

2 participants