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

Set up session info for F5 to handle bi-directional reachability #3574

Merged
merged 10 commits into from
Apr 10, 2019

Conversation

yifeiyuan
Copy link
Contributor

Add session info to each interface for F5, assuming that each interface has its own session info

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member

@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.

Reviewed 1 of 5 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown, @arifogel, and @yifeiyuan)


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1115 at r2 (raw file):

  }

  private @Nonnull org.batfish.datamodel.Interface toInterface(Vlan vlan) {

Only Vlan has layer-3 information. I don't think the other two interface types should have any session info.

Copy link
Member

@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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @anothermattbrown, @arifogel, and @yifeiyuan)


projects/batfish/src/test/java/org/batfish/grammar/f5_bigip_structured/F5BigipStructuredGrammarTest.java, line 1468 at r2 (raw file):

                .setIpProtocol(IpProtocol.TCP)
                .setDstIp(Ip.parse("10.200.1.2"))
                // TODO:

TODO what? Link to an issue?

Copy link
Member

@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.

Reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @anothermattbrown and @yifeiyuan)

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yifeiyuan)


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1116 at r3 (raw file):

    newIface.setBandwidth(Interface.DEFAULT_BANDWIDTH);
    newIface.setVrf(_c.getDefaultVrf());
    // Assume each interface has its own session info

please clarify that this means specifically that sessions are not shared by interfaces, i.e. that session return flows can only enter the interface the forward flow exited.


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1118 at r3 (raw file):

    // Assume each interface has its own session info
    newIface.setFirewallSessionInterfaceInfo(
        new FirewallSessionInterfaceInfo(ImmutableList.of(newIface.getName()), null, null));

please comment why ingress and egress ACLs should always be null.

Copy link
Member

@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.

Reviewed 1 of 2 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yifeiyuan)

Copy link
Contributor Author

@yifeiyuan yifeiyuan 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 3 files reviewed, 2 unresolved discussions (waiting on @anothermattbrown and @arifogel)


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1115 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Only Vlan has layer-3 information. I don't think the other two interface types should have any session info.

done


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1116 at r3 (raw file):

Previously, anothermattbrown (Matt Brown) wrote…

please clarify that this means specifically that sessions are not shared by interfaces, i.e. that session return flows can only enter the interface the forward flow exited.

Done.


projects/batfish/src/main/java/org/batfish/representation/f5_bigip/F5BigipConfiguration.java, line 1118 at r3 (raw file):

Previously, anothermattbrown (Matt Brown) wrote…

please comment why ingress and egress ACLs should always be null.

Done.


projects/batfish/src/test/java/org/batfish/grammar/f5_bigip_structured/F5BigipStructuredGrammarTest.java, line 1468 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

TODO what? Link to an issue?

should not have had this. removed.

Copy link
Member

@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.

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @anothermattbrown)

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #3574 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3574      +/-   ##
============================================
- Coverage     73.42%    73.4%   -0.03%     
- Complexity    24442    24451       +9     
============================================
  Files          2102     2105       +3     
  Lines        101144   101253     +109     
  Branches      11984    12011      +27     
============================================
+ Hits          74270    74323      +53     
- Misses        21497    21540      +43     
- Partials       5377     5390      +13
Impacted Files Coverage Δ Complexity Δ
.../representation/f5_bigip/F5BigipConfiguration.java 93.29% <100%> (+0.02%) 196 <0> (ø) ⬇️
.../org/batfish/datamodel/OspfExternalType1Route.java 53.33% <0%> (-42.83%) 4% <0%> (-6%)
.../org/batfish/datamodel/OspfExternalType2Route.java 51.51% <0%> (-39.11%) 4% <0%> (-6%)
...g/batfish/datamodel/acl/IpAccessListLineIndex.java 33.33% <0%> (-5.56%) 4% <0%> (-1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.55% <0%> (-4.5%) 13% <0%> (-2%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 79.54% <0%> (-2.28%) 13% <0%> (-1%)
...n/java/org/batfish/specifier/parboiled/Parser.java 19.64% <0%> (-0.63%) 28% <0%> (ø)
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.84% <0%> (-0.59%) 90% <0%> (-1%)
...batfish/representation/cisco/CiscoConversions.java 89.66% <0%> (-0.36%) 135% <0%> (ø)
...src/main/java/org/batfish/coordinator/WorkMgr.java 69.11% <0%> (-0.28%) 266% <0%> (-1%)
... and 24 more

@yifeiyuan yifeiyuan merged commit cc46021 into master Apr 10, 2019
@yifeiyuan yifeiyuan deleted the yifei-nat-f5 branch April 10, 2019 18:52
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

4 participants