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

AWS: fix routes for ngw in subnets #6263

Merged
merged 1 commit into from
Oct 1, 2020
Merged

AWS: fix routes for ngw in subnets #6263

merged 1 commit into from
Oct 1, 2020

Conversation

ratulm
Copy link
Member

@ratulm ratulm commented Oct 1, 2020

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@dhalperi
Copy link
Member

dhalperi commented Oct 1, 2020


projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 650 at r1 (raw file):

            // instead of ignoring it because it is the correct model and users expect routes in AWS
            // and Batfish to line up.
            nexthopInterfaceName = instancesInterfaceName(_subnetId);

Hmm. IIUC we usually send packets between two nodes communicating in the same subnet p2p -- e.g., instances send directly to each other instead of through subnet node, right? Didn't we run into problems with filtering otherwise?

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.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @dhalperi, @progwriter, and @ratulm)


projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 643 at r1 (raw file):

send it directly.

update comment?

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.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dhalperi, @progwriter, and @ratulm)


projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 650 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Hmm. IIUC we usually send packets between two nodes communicating in the same subnet p2p -- e.g., instances send directly to each other instead of through subnet node, right? Didn't we run into problems with filtering otherwise?

nm, I misunderstood the context. What was happening before? Did that next hop interface not exist?

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #6263 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #6263      +/-   ##
============================================
- Coverage     72.90%   72.89%   -0.02%     
+ Complexity    35006    34998       -8     
============================================
  Files          2832     2832              
  Lines        142258   142256       -2     
  Branches      17082    17082              
============================================
- Hits         103714   103695      -19     
- Misses        30319    30331      +12     
- Partials       8225     8230       +5     
Impacted Files Coverage Δ Complexity Δ
...in/java/org/batfish/representation/aws/Subnet.java 84.32% <100.00%> (+1.20%) 69.00 <0.00> (+1.00)
...g/batfish/datamodel/flow/IncomingSessionScope.java 84.61% <0.00%> (-15.39%) 7.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 54.32% <0.00%> (-6.18%) 13.00% <0.00%> (-3.00%)
...col/src/main/java/org/batfish/role/InferRoles.java 89.54% <0.00%> (-1.37%) 50.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.38% <0.00%> (-0.78%) 244.00% <0.00%> (-3.00%)
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.93% <0.00%> (-0.59%) 90.00% <0.00%> (-1.00%)
...a/org/batfish/representation/aws/LoadBalancer.java 82.18% <0.00%> (-0.32%) 70.00% <0.00%> (-1.00%)
...tfish/representation/cisco/CiscoConfiguration.java 85.91% <0.00%> (-0.14%) 492.00% <0.00%> (-1.00%)
...g/batfish/dataplane/ibdp/IncrementalBdpEngine.java 90.77% <0.00%> (-0.02%) 75.00% <0.00%> (ø%)
... and 3 more

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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @progwriter)


projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 650 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

nm, I misunderstood the context. What was happening before? Did that next hop interface not exist?

yeah, the wrong interface name was being used, so the route was not getting wiped out.

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.

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @progwriter)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @progwriter)


projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 650 at r1 (raw file):

Previously, ratulm wrote…

yeah, the wrong interface name was being used, so the route was not getting wiped out.

was getting wiped out

@ratulm ratulm merged commit c625183 into master Oct 1, 2020
@ratulm ratulm deleted the ngw-route branch October 1, 2020 05:41
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