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

IOS: Convert route-map-based NAT rules #6570

Merged
merged 5 commits into from Jan 15, 2021
Merged

Conversation

corinaminer
Copy link
Contributor

Same basic idea as ACL-based NAT rules. So far, only limited support for content of route-maps that are used in NAT rules. The rule will be ignored if the route-map has:

  • deny clauses
  • set lines
  • match lines other than matching IPv4 ACLs
  • references to undefined ACLs

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #6570 (a369cac) into master (8322769) will decrease coverage by 0.00%.
The diff coverage is 74.56%.

@@             Coverage Diff              @@
##             master    #6570      +/-   ##
============================================
- Coverage     73.40%   73.39%   -0.01%     
- Complexity    35720    35739      +19     
============================================
  Files          2837     2837              
  Lines        144171   144244      +73     
  Branches      17429    17446      +17     
============================================
+ Hits         105822   105871      +49     
- Misses        29967    29980      +13     
- Partials       8382     8393      +11     
Impacted Files Coverage Δ Complexity Δ
.../org/batfish/representation/cisco/CiscoIosNat.java 88.00% <ø> (ø) 8.00 <0.00> (ø)
...atfish/representation/cisco/CiscoIosStaticNat.java 65.21% <ø> (ø) 13.00 <0.00> (ø)
...tfish/representation/cisco/CiscoIosDynamicNat.java 72.22% <59.72%> (-10.69%) 54.00 <18.00> (+9.00) ⬇️
...tfish/representation/cisco/CiscoConfiguration.java 86.59% <100.00%> (ø) 504.00 <0.00> (ø)
.../batfish/representation/cisco/CiscoIosNatUtil.java 93.24% <100.00%> (+7.94%) 18.00 <9.00> (+9.00)
...g/batfish/datamodel/flow/IncomingSessionScope.java 84.61% <0.00%> (-15.39%) 7.00% <0.00%> (-1.00%)
...a/org/batfish/representation/aws/LoadBalancer.java 82.18% <0.00%> (-0.32%) 70.00% <0.00%> (-1.00%)
...fish/grammar/cisco/CiscoControlPlaneExtractor.java 66.81% <0.00%> (-0.02%) 1286.00% <0.00%> (+1.00%) ⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java 60.71% <0.00%> (+1.19%) 16.00% <0.00%> (+1.00%)
...g/batfish/representation/cisco/RouteMapClause.java 81.81% <0.00%> (+4.54%) 9.00% <0.00%> (+1.00%)

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 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @corinaminer)


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoIosNatUtil.java, line 66 at r1 (raw file):

    for (RouteMapClause clause : routeMap.getClauses().values()) {
      if (clause.getAction() != LineAction.PERMIT) {
        // TODO Support NAT rules referencing route-maps with deny clauses

can we surface these unsupported features somehow? if not in convert warnings, at least with error logs so we devs know when we encounter this?

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

Copy link
Contributor Author

@corinaminer corinaminer 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: 7 of 8 files reviewed, all discussions resolved (waiting on @progwriter)


projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoIosNatUtil.java, line 66 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

can we surface these unsupported features somehow? if not in convert warnings, at least with error logs so we devs know when we encounter this?

yes, surfaced a bunch of warnings that cause NAT rules to be ignored.

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

@corinaminer corinaminer merged commit 1c14bbb into master Jan 15, 2021
@corinaminer corinaminer deleted the ios-convert-nat-rm branch January 15, 2021 23:51
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