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

[RFC v3] OSPFv3 LSA Extensibility RFC 8362 #17343

Closed
wants to merge 80 commits into from

Conversation

acooks-at-bda
Copy link
Contributor

This adds support for RFC 8362 OSPFv3 LSA Extensibility.

The PR is for review and comment on Work In Progress. It is a continuation of the work in PR #16532

Compared to the previously posted work, this PR includes:

  • making callback structs static const so that they are placed in .rodata section, in response to a previous review comment
  • E-Network-LSA origination
  • E-Inter-Area-Prefix and E-Inter-Area-Router LSA origination
  • updated SPF calculation based on E-LSAs
  • additional topotests
  • bug fixes, of course

Known deficiencies include:

  • Routes are not installed for OSPF6_PATH_TYPE_EXTERNAL2

New E-LSAs can be enabled by adding ospf6 extended-lsa-support elsa or ospf6 extended-lsa-support both to the router ospf6 config. The default is to only send the legacy LSAs.

I understand that the proposed changes are not easily reviewable or acceptable as is. This is the progress we've made, and we seek suggestions for improvement, and guidance for what might be upstreamable.

Thank you for your consideration.

Add a generic iterator for the descriptors in an LSA, with
specializations for the descriptor type.

This introduces a callback mechanism to decouple the LSA type from the
function that operates on each of the descriptors.

It prepares for further specialization of the iterators for LSAs that
contain TLVs, while reusing the callback that operate on the descriptors
contained in the TLVs.

Signed-off-by: Andrew Cooks <[email protected]>
Signed-off-by: Andrew Cooks <[email protected]>
Add utility function to find the Nth TLV in an E-LSA.

Signed-off-by: Andrew Cooks <[email protected]>
Signed-off-by: Andrew Cooks <[email protected]>
Gotcha:
The E-Entra-Area-Prefix LSA has removed the field containing the number
of prefixes from the LSA, even though RFC 8362 says: "all LSA Header
fields are the same as defined for the Intra-Area-Prefix-LSA".

In order to report the number of prefixes, the iterator has to keep count.

Signed-off-by: Andrew Cooks <[email protected]>
to appease checkpatch and the CI system.

Signed-off-by: Andrew Cooks <[email protected]>
This adds a handler and .lh_get_prefix_str callback for E-Router LSAs,
similar to ospf6_router_lsa_get_nbr_id().

Signed-off-by: Andrew Cooks <[email protected]>
This adds a handler and .lh_get_prefix_str callback for E-Network LSAs,
similar to ospf6_network_lsa_get_ar_id().

Signed-off-by: Andrew Cooks <[email protected]>
This adds a handler and .lh_get_prefix_str callback for E-Link LSAs,
similar to ospf6_link_lsa_get_prefix_str()

Signed-off-by: Andrew Cooks <[email protected]>
Add a handler and .lh_get_prefix_str callback for E-Intra-Area-Prefix
LSAs, similar to ospf6_intra_prefix_lsa_get_prefix_str().

Signed-off-by: Andrew Cooks <[email protected]>
acooks-at-bda and others added 19 commits October 30, 2024 15:01
remove magical caddr_t + 4 pointer
and subsequent call of FOO_LSDESC_GET_BAR macro on said pointer.

Signed-off-by: Andrew Cooks <[email protected]>
The pattern where a macro receives a pointer, immediately casts it,
accesses a struct member and also converts byte order is surely not a
recipe for success...

Signed-off-by: Andrew Cooks <[email protected]>
Signed-off-by: Andrew Cooks <[email protected]>
Use indent.py to apply coding style changes and selectively commit the
changes to logging calls.

Not only is the newer style easier to read, but committing the style
changes separately from surrounding functional changes will make functional
changes easier to stage and review.

Signed-off-by: Andrew Cooks <[email protected]>
When calculating inter-area routes, consider TLV-based E-Intra-Area-Prefix
LSA in addition to Intra-Area-Prefix LSA.

Signed-off-by: Andrew Cooks <[email protected]>
When calculating inter-area routes, consider TLV-based
E-Inter-Area-Router LSA in addition to Inter-Area-Router LSA.

Signed-off-by: Andrew Cooks <[email protected]>
Attached Routers TLV contains multiple Router IDs. Print all of them.

In RFC8362, only a single Attached-Routers TLV is applicable to
E-Network-LSA, so for now, the end of the TLV is the end of the LSA.

Signed-off-by: Andrew Cooks <[email protected]>
previous code was only checking lsdb for legacy inter lsa to
determine lsid

recursively check for free lsid for IAP and EIAP

find a free link state id values across both extended and legacy LSA's

Signed-off-by: Jacob Lodge <[email protected]>
Signed-off-by: Andrew Cooks <[email protected]>
@frrbot frrbot bot added bugfix ospfv3 tests Topotests, make check, etc labels Nov 5, 2024
@github-actions github-actions bot added master rebase PR needs rebase labels Nov 5, 2024
@riw777 riw777 self-requested a review November 5, 2024 15:53
@eqvinox eqvinox self-requested a review November 12, 2024 16:42
@eqvinox
Copy link
Contributor

eqvinox commented Nov 12, 2024

@acooks-at-bda — we just discussed this on the weekly community call and unfortunately the consensus (primarily @aceelindem , @riw777 and myself) is that the cost of making everything be callbacks with void pointers is too high. "Cost" here is first and foremost about code maintainability concerns and safety against bugs.

(i.e. all patterns similar to https://github.com/FRRouting/frr/pull/17343/files#diff-160d7849a335ef56a04caa4e090e4a2cea7272f14fde626613c1c93dfe62d95dR290 )

The best path to continue here is to have a discussion and/or brainstorming about how to structure the code in a good way…

@acooks-at-bda
Copy link
Contributor Author

@acooks-at-bda — we just discussed this on the weekly community call and unfortunately the consensus (primarily @aceelindem , @riw777 and myself) is that the cost of making everything be callbacks with void pointers is too high. "Cost" here is first and foremost about code maintainability concerns and safety against bugs.

(i.e. all patterns similar to https://github.com/FRRouting/frr/pull/17343/files#diff-160d7849a335ef56a04caa4e090e4a2cea7272f14fde626613c1c93dfe62d95dR290 )

The best path to continue here is to have a discussion and/or brainstorming about how to structure the code in a good way…

Thanks, I understand that this is the TSC position. It is consistent with the feedback provided on previous PRs.

In opening this PR, I had hoped to:

  • demonstrate that the pattern is needed and hoped that it would be obvious when considering the magnitude of change required to implement this feature;
  • get insightful suggestions for specific improvements;
  • get support for any subset of this work that could be made ready for merge;
  • share the work we've done so far and show that we're getting somewhere.

I can understand the maintainers' position, really. I would also reject this change if I was the maintainer, for these reasons: There doesn't appear to be any pressing need for it. It's a large, risky change. The changed code flow obscures the core algorithm. It's probably riddled with bugs. It's coming from new contributors with no previous engagement or announcement or experience in this area. It's not clear who these people are or they will stick around to support it. Etcetera.

We did this work, because we need it in order to implement other things like:

We are now continuing with these other things, which will take our attention away from making this change upstreamable.

Without any specific advice like "go and look at how it was done in XYZ" or "follow this pattern instead:...", we honestly don't know how to make this change any more acceptable. We don't see the alternative you see. For months we've been looking for a way to do this without using callbacks and dreading being told nothing more than "don't use callbacks". Again.

Frankly, the existing ospf6d code is not in great shape. The existing style of very long functions with deep nesting is not easy to read or change and is not a style that can be built on for E-LSAs. Function pointers are already used for a few other things in the ospf6d code - and they are not the problem.

The ospf6d implementation is years behind the OSPFv3 RFCs. It seems unloved, and unless you can find someone to care about it and trust them to breathe some life into it, perhaps the next stage of its lifecycle should be to rewrite it in Rust.

@acooks-at-bda acooks-at-bda deleted the ELSA-devel-v6 branch November 14, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix master ospfv3 rebase PR needs rebase tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants