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

Cumulus FRR assign LLAs to point-to-point OSPF interfaces without addresses #7038

Merged
merged 4 commits into from Jun 2, 2021

Conversation

arifogel
Copy link
Member

@arifogel arifogel commented Jun 2, 2021

  • enables forwarding across unnumbered links

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #7038 (48f99ce) into master (9053587) will increase coverage by 0.01%.
The diff coverage is 96.42%.

@@             Coverage Diff              @@
##             master    #7038      +/-   ##
============================================
+ Coverage     71.89%   71.91%   +0.01%     
- Complexity    37699    37711      +12     
============================================
  Files          3036     3036              
  Lines        154740   154761      +21     
  Branches      18564    18570       +6     
============================================
+ Hits         111258   111290      +32     
+ Misses        34592    34585       -7     
+ Partials       8890     8886       -4     
Impacted Files Coverage Δ
...tion/cumulus/CumulusConcatenatedConfiguration.java 73.63% <96.42%> (+1.08%) ⬆️
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 78.23% <0.00%> (+0.12%) ⬆️
...ain/java/org/batfish/storage/FileBasedStorage.java 84.76% <0.00%> (+0.26%) ⬆️
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.81% <0.00%> (+0.38%) ⬆️
...rg/batfish/datamodel/ConcreteInterfaceAddress.java 96.87% <0.00%> (+6.25%) ⬆️
...n/java/org/batfish/datamodel/LinkLocalAddress.java 89.28% <0.00%> (+7.14%) ⬆️

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @corinaminer)


projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConcatenatedConfiguration.java, line 226 at r1 (raw file):

        .forEach(
            (iname, iface) -> {
              if (iface.getAllAddresses().size() == 0

It feels like there is a missing && <some vs property> that indicates this interface was configured for unnumbered.

Above, the function was labeled something like isUsedForBgpUnnumbered(ifacename, bgp VS model). I imagine something generic there.


projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConcatenatedConfiguration.java, line 226 at r1 (raw file):

        .forEach(
            (iname, iface) -> {
              if (iface.getAllAddresses().size() == 0

From discussion: seems like we should add an LLA unconditionally? Otherwise there might not be an L3 edge.

Also from discussion: it seems like there should not even be able to be a concrete address assigned. Unnumbered should be mutually exclusive with concrete.

Copy link
Member Author

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, and @dhalperi)

a discussion (no related file):
After #7039


Copy link
Member Author

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, and @dhalperi)


projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConcatenatedConfiguration.java, line 226 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

From discussion: seems like we should add an LLA unconditionally? Otherwise there might not be an L3 edge.

Also from discussion: it seems like there should not even be able to be a concrete address assigned. Unnumbered should be mutually exclusive with concrete.

I don't believe we said they should be mutually exclusive. An interface could win one unnumbered and address and lose another. In that case it would have a concrete address, but still may need the LLA for L3 edge.

In any case, now adding the LLA even for winning interface.


projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusConcatenatedConfiguration.java, line 226 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

It feels like there is a missing && <some vs property> that indicates this interface was configured for unnumbered.

Above, the function was labeled something like isUsedForBgpUnnumbered(ifacename, bgp VS model). I imagine something generic there.

Now relying on ospfAddresses being non-empty. This happens when at least 1 address was assigned in VS interfaces or frr file.

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @corinaminer)

@arifogel arifogel merged commit fdf4e80 into batfish:master Jun 2, 2021
@arifogel arifogel deleted the ari-frr-ospf-lla branch June 2, 2021 21:33
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

3 participants