Adding reachability check for IPsec tunnels #3931

haverma · 2019-05-23T10:05:42Z

Now checking connectivity in the underlay network to properly verify the establishment of the IPsec based tunnels. This check is employed while we compute the fixed point IPsec topology in BDP engine.

More detailed intuition behind this change can be found here.

batfish-bot · 2019-05-23T10:05:49Z

This change is

codecov · 2019-05-23T10:17:15Z

Codecov Report

Merging #3931 into master will increase coverage by <.01%.
The diff coverage is 86.04%.

@@             Coverage Diff              @@
##             master    #3931      +/-   ##
============================================
+ Coverage     74.86%   74.86%   +<.01%     
- Complexity    24089    24106      +17     
============================================
  Files          2027     2027              
  Lines         96668    96755      +87     
  Branches      11509    11526      +17     
============================================
+ Hits          72367    72438      +71     
- Misses        19044    19053       +9     
- Partials       5257     5264       +7

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/org/batfish/datamodel/IpsecSession.java	`74.5% <ø> (ø)`	`8 <0> (ø)`	⬇️
...g/batfish/dataplane/ibdp/IncrementalBdpEngine.java	`90.32% <100%> (+0.07%)`	`85 <0> (ø)`	⬇️
...c/main/java/org/batfish/common/util/IpsecUtil.java	`85.53% <84.61%> (-0.31%)`	`72 <15> (+15)`
.../src/main/java/org/batfish/datamodel/flow/Hop.java	`53.33% <0%> (-6.67%)`	`5% <0%> (-1%)`
...java/org/batfish/symbolic/state/EdgeStateExpr.java	`80.95% <0%> (-4.77%)`	`10% <0%> (-1%)`
...org/batfish/datamodel/flow/BidirectionalTrace.java	`79.54% <0%> (-4.55%)`	`13% <0%> (-2%)`
...rc/main/java/org/batfish/datamodel/flow/Trace.java	`87.09% <0%> (-3.23%)`	`11% <0%> (-2%)`
...ava/org/batfish/datamodel/bgp/Layer2VniConfig.java	`85.71% <0%> (-2.86%)`	`15% <0%> (-1%)`
...col/src/main/java/org/batfish/role/InferRoles.java	`91.24% <0%> (-0.37%)`	`68% <0%> (-1%)`
...tfish/representation/cisco/CiscoConfiguration.java	`83.8% <0%> (-0.13%)`	`530% <0%> (-1%)`
... and 5 more

progwriter

Reviewed 3 of 4 files at r1.
Reviewable status: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @haverma and @progwriter)

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 140 at r1 (raw file):

                  initialTopologyContext.getIpsecTopology(),
                  configurations,
                  new TracerouteEngineImpl(

time to factor out the engine into a variable? why are we re-creating it three times here?

projects/batfish/src/test/java/org/batfish/dataplane/ibdp/FixedPointTopologyTest.java, line 407 at r1 (raw file):

            .build();
    IncrementalBdpEngine engine =
        new IncrementalBdpEngine(

Using org.batfish.main.BatfishTestUtils#getBatfish and do batfish.computeDataplane() seems much easier and in line with other tests. (applies elsewhere in the file)

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

      }

      Optional<IpsecSession> ipsecSession = ipsecTopology.getGraph().edgeValue(peerIdU, peerIdV);

I personally like to do the .orElse(null) at the end, that way your var type is not optional, you don't get to get() a bunch of times below, and you can still do if equal to null, continue.

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

      }

      if (ipsecSession.get().isCloud()

Comment why we're doing this -- it's a workaround until we're confident ISP modeling will let us do traceroutes properly.

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

   * IPsec encrypted data
   */
  private static boolean reachableIpsecEdge(

We should think for a better name. edges aren't reachable by themselves. nor does this return an edge (as the name would imply). perhaps isIpsecPeerReachable?

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

    List<Trace> traces =
        tracerouteEngine
            .computeTraces(ImmutableSet.of(flowForIpsecNegotiation), false)

should we use bidir traceroute here?

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

        .noneMatch(
            trace -> {
              List<Hop> hops = trace.getHops();

factor into a small static function: boolean isSuccessfulTraceroute(trace, hostname)

progwriter

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @haverma)

haverma

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @progwriter)

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 140 at r1 (raw file):