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

ISP modeling: Support Border-ISP peering over unnumbered #5641

Merged
merged 5 commits into from
Mar 21, 2020
Merged

Conversation

ratulm
Copy link
Member

@ratulm ratulm commented Mar 21, 2020

And move the AWS border (IGW) to this version.

@ratulm ratulm requested a review from haverma March 21, 2020 00:54
@batfish-bot
Copy link

This change is Reviewable

@ratulm ratulm requested a review from dhalperi March 21, 2020 00:54
@codecov
Copy link

codecov bot commented Mar 21, 2020

Codecov Report

Merging #5641 into master will decrease coverage by <.01%.
The diff coverage is 87.8%.

@@             Coverage Diff              @@
##             master    #5641      +/-   ##
============================================
- Coverage     73.77%   73.77%   -0.01%     
- Complexity    32887    32889       +2     
============================================
  Files          2667     2667              
  Lines        131605   131603       -2     
  Branches      15693    15699       +6     
============================================
- Hits          97093    97088       -5     
- Misses        26845    26851       +6     
+ Partials       7667     7664       -3
Impacted Files Coverage Δ Complexity Δ
...rg/batfish/representation/aws/InternetGateway.java 89.65% <100%> (-0.27%) 18 <0> (ø)
...rc/main/java/org/batfish/common/util/IspModel.java 100% <100%> (ø) 18 <0> (ø) ⬇️
...ish/representation/aws/ConvertedConfiguration.java 100% <100%> (+10%) 7 <3> (-2) ⬇️
...java/org/batfish/common/util/IspModelingUtils.java 94.57% <84.12%> (-0.49%) 54 <9> (+5)
...src/main/java/org/batfish/coordinator/PoolMgr.java 64.04% <0%> (-1.13%) 15% <0%> (-1%)
...batfish/src/main/java/org/batfish/main/Driver.java 49.03% <0%> (-0.33%) 29% <0%> (ø)
...tfish/representation/cisco/CiscoConfiguration.java 86.09% <0%> (-0.12%) 548% <0%> (-1%)
...ava/org/batfish/datamodel/BgpActivePeerConfig.java 90.32% <0%> (+6.45%) 9% <0%> (+1%) ⬆️

@dhalperi dhalperi requested review from progwriter and removed request for dhalperi and haverma March 21, 2020 01:11
Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ratulm)


projects/batfish-common-protocol/src/main/java/org/batfish/common/util/IspModel.java, line 36 at r1 (raw file):

        String remoteHostname,
        String remoteIfaceName,
        InterfaceAddress ispIfaceAdddress,
  1. typo in field name/setters/params

projects/batfish-common-protocol/src/main/java/org/batfish/common/util/IspModelingUtils.java, line 701 at r1 (raw file):

   */
  @VisibleForTesting
  static BgpPeerConfig.Builder<?, ?> getBgpPeerOnIsp(

why is return type a builder now? Seems like a recipe for callers to mess up the configuration.


projects/batfish-common-protocol/src/test/java/org/batfish/common/util/IspModelingUtilsTest.java, line 322 at r1 (raw file):

                        hasActiveNeighbor(
                            Prefix.parse("1.1.1.1/32"),
                            allOf(hasRemoteAs(2L), hasLocalAs(1L))))))));

what happened with the AS flip here?

Copy link
Member Author

@ratulm ratulm 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: all files reviewed, 1 unresolved discussion (waiting on @progwriter)


projects/batfish-common-protocol/src/test/java/org/batfish/common/util/IspModelingUtilsTest.java, line 322 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

what happened with the AS flip here?

Earlier we caching the reverse config in IspModel and then cloning when creating the node. Now, we are storing the border config in IspModel and reversing it when creating the node. This reversal led to this flip.

Copy link
Contributor

@progwriter progwriter 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 r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ratulm ratulm merged commit 79c97b5 into master Mar 21, 2020
@ratulm ratulm deleted the unnumbered-bgp branch March 21, 2020 04:20
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