fix spurious circular references in routing policy called policy dete…

…ction (#701) * fix spurious circular references in routing policy called policy detection * add tests for circular routing policy references - add getters and setters for transient Warnings fields - compute ALL rp sources instead of just used ones - remove unnecessary iterator() calls for ImmutableSet builder addAll - add null check for WithEnvironmentExpr _expr sources traversal - add tests * use correct import * clean up rp source gathering * add missing null check
batfish · Dec 5, 2017 · d7061ce · d7061ce
1 parent 1aef28a
commit d7061ce
Show file tree

Hide file tree

Showing 16 changed files with 338 additions and 72 deletions.
diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/common/Warnings.java b/projects/batfish-common-protocol/src/main/java/org/batfish/common/Warnings.java
@@ -165,14 +165,42 @@ public Warnings(
     _unimplementedRecord = unimplementedRecord;
   }
 
+  public boolean getPedanticAsError() {
+    return _pedanticAsError;
+  }
+
+  public boolean getPedanticRecord() {
+    return _pedanticRecord;
+  }
+
   public List<Warning> getPedanticWarnings() {
     return _pedanticWarnings;
   }
 
+  public boolean getPrintParseTree() {
+    return _printParseTree;
+  }
+
+  public boolean getRedFlagAsError() {
+    return _redFlagAsError;
+  }
+
+  public boolean getRedFlagRecord() {
+    return _redFlagRecord;
+  }
+
   public List<Warning> getRedFlagWarnings() {
     return _redFlagWarnings;
   }
 
+  public boolean getUnimplementedAsError() {
+    return _unimplementedAsError;
+  }
+
+  public boolean getUnimplementedRecord() {
+    return _unimplementedRecord;
+  }
+
   public List<Warning> getUnimplementedWarnings() {
     return _unimplementedWarnings;
   }
@@ -208,6 +236,34 @@ public void redFlag(String msg, String tag) {
     }
   }
 
+  public void setPedanticAsError(boolean pedanticAsError) {
+    _pedanticAsError = pedanticAsError;
+  }
+
+  public void setPedanticRecord(boolean pedanticRecord) {
+    _pedanticRecord = pedanticRecord;
+  }
+
+  public void setPrintParseTree(boolean printParseTree) {
+    _printParseTree = printParseTree;
+  }
+
+  public void setRedFlagAsError(boolean redFlagAsError) {
+    _redFlagAsError = redFlagAsError;
+  }
+
+  public void setRedFlagRecord(boolean redFlagRecord) {
+    _redFlagRecord = redFlagRecord;
+  }
+
+  public void setUnimplementedAsError(boolean unimplementedAsError) {
+    _unimplementedAsError = unimplementedAsError;
+  }
+
+  public void setUnimplementedRecord(boolean unimplementedRecord) {
+    _unimplementedRecord = unimplementedRecord;
+  }
+
   public void todo(
       ParserRuleContext ctx, String feature, BatfishCombinedParser<?, ?> parser, String text) {
     if (!_unimplementedRecord && !_unimplementedAsError) {

diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java b/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java
@@ -10,12 +10,10 @@
 import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaDescription;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.NavigableMap;
 import java.util.NavigableSet;
-import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeMap;
 import java.util.TreeSet;
@@ -247,41 +245,36 @@ public Configuration(
     _zones = new TreeMap<>();
   }
 
-  private SortedSet<String> collectRoutingPolicySources(String routingPolicyName, Warnings w) {
+  private void computeRoutingPolicySources(String routingPolicyName, Warnings w) {
     if (routingPolicyName == null) {
-      return Collections.emptySortedSet();
+      return;
     }
     RoutingPolicy routingPolicy = _routingPolicies.get(routingPolicyName);
     if (routingPolicy == null) {
-      return Collections.emptySortedSet();
+      return;
     }
-    Set<String> sources = new LinkedHashSet<>();
-    routingPolicy.computeSources(sources, _routingPolicies, w);
-    return sources
-        .stream()
-        .filter(not(RoutingPolicy::isGenerated))
-        .collect(ImmutableSortedSet.toImmutableSortedSet(Comparator.naturalOrder()));
+    routingPolicy.computeSources(Collections.emptySet(), _routingPolicies, w);
   }
 
   public void computeRoutingPolicySources(Warnings w) {
+    for (String rpName : _routingPolicies.keySet()) {
+      computeRoutingPolicySources(rpName, w);
+    }
     for (Vrf vrf : _vrfs.values()) {
       BgpProcess bgpProcess = vrf.getBgpProcess();
       if (bgpProcess != null) {
         for (BgpNeighbor neighbor : bgpProcess.getNeighbors().values()) {
-          neighbor.setExportPolicySources(
-              collectRoutingPolicySources(neighbor.getExportPolicy(), w));
-          neighbor.setImportPolicySources(
-              collectRoutingPolicySources(neighbor.getImportPolicy(), w));
+          neighbor.setExportPolicySources(getRoutingPolicySources(neighbor.getExportPolicy()));
+          neighbor.setImportPolicySources(getRoutingPolicySources(neighbor.getImportPolicy()));
         }
       }
       OspfProcess ospfProcess = vrf.getOspfProcess();
       if (ospfProcess != null) {
-        ospfProcess.setExportPolicySources(
-            collectRoutingPolicySources(ospfProcess.getExportPolicy(), w));
+        ospfProcess.setExportPolicySources(getRoutingPolicySources(ospfProcess.getExportPolicy()));
       }
       for (GeneratedRoute gr : vrf.getGeneratedRoutes()) {
-        gr.setAttributePolicySources(collectRoutingPolicySources(gr.getAttributePolicy(), w));
-        gr.setGenerationPolicySources(collectRoutingPolicySources(gr.getGenerationPolicy(), w));
+        gr.setAttributePolicySources(getRoutingPolicySources(gr.getAttributePolicy()));
+        gr.setGenerationPolicySources(getRoutingPolicySources(gr.getGenerationPolicy()));
       }
     }
   }
@@ -489,6 +482,20 @@ public NavigableMap<String, RoutingPolicy> getRoutingPolicies() {
     return _routingPolicies;
   }
 
+  private SortedSet<String> getRoutingPolicySources(String routingPolicyName) {
+    if (routingPolicyName == null) {
+      return Collections.emptySortedSet();
+    }
+    RoutingPolicy rp = _routingPolicies.get(routingPolicyName);
+    if (rp == null) {
+      return Collections.emptySortedSet();
+    }
+    return rp.getSources()
+        .stream()
+        .filter(not(RoutingPolicy::isGenerated))
+        .collect(ImmutableSortedSet.toImmutableSortedSet(Comparator.naturalOrder()));
+  }
+
   @JsonIgnore
   public NavigableSet<BgpAdvertisement> getSentAdvertisements() {
     return _sentAdvertisements;

diff --git a/...ish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/RoutingPolicy.java b/...ish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/RoutingPolicy.java
@@ -4,10 +4,11 @@
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonPropertyDescription;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaDescription;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -109,16 +110,18 @@ public Result call(Environment environment) {
     return result;
   }
 
-  public void computeSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+  public Set<String> computeSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
     if (_sources == null) {
-      _sources = new LinkedHashSet<>();
-      _sources.add(_key);
+      Set<String> newParentSources = Sets.union(parentSources, ImmutableSet.of(_key));
+      ImmutableSet.Builder<String> childSources = ImmutableSet.<String>builder();
+      childSources.add(_key);
       for (Statement statement : _statements) {
-        statement.collectSources(_sources, routingPolicies, w);
+        childSources.addAll(statement.collectSources(newParentSources, routingPolicies, w));
       }
+      _sources = childSources.build();
     }
-    sources.addAll(_sources);
+    return _sources;
   }
 
   @Override
@@ -137,6 +140,11 @@ public Configuration getOwner() {
     return _owner;
   }
 
+  @JsonIgnore
+  public Set<String> getSources() {
+    return _sources;
+  }
+
   @JsonProperty(PROP_STATEMENTS)
   @JsonPropertyDescription("The list of routing-policy statements to execute")
   public List<Statement> getStatements() {

diff --git a/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/BooleanExpr.java b/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/BooleanExpr.java
@@ -5,6 +5,7 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.MoreObjects.ToStringHelper;
 import java.io.Serializable;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 import java.util.SortedSet;
@@ -28,9 +29,9 @@ public abstract class BooleanExpr implements Serializable {
    * @return A {@link SortedSet} containing the names of each {@link RoutingPolicy} directly or
    *     indirectly referenced by this expression
    */
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
-    return;
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    return Collections.emptySet();
   }
 
   @Override

diff --git a/...ish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/CallExpr.java b/...ish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/CallExpr.java
@@ -2,6 +2,7 @@
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 import org.batfish.common.Warnings;
@@ -26,22 +27,22 @@ public CallExpr(String includedPolicyName) {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
-    if (sources.contains(_calledPolicyName)) {
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    if (parentSources.contains(_calledPolicyName)) {
       w.redFlag(
           "Circular reference to routing policy: '"
               + _calledPolicyName
               + "' detected at expression: '"
               + toString()
               + "'");
-      return;
+      return Collections.emptySet();
     }
     RoutingPolicy calledPolicy = routingPolicies.get(_calledPolicyName);
     if (calledPolicy == null) {
-      return;
+      return Collections.emptySet();
     }
-    calledPolicy.computeSources(sources, routingPolicies, w);
+    return calledPolicy.computeSources(parentSources, routingPolicies, w);
   }
 
   @Override

diff --git a/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Conjunction.java b/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Conjunction.java
@@ -1,5 +1,6 @@
 package org.batfish.datamodel.routing_policy.expr;
 
+import com.google.common.collect.ImmutableSet;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -21,11 +22,13 @@ public Conjunction() {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    ImmutableSet.Builder<String> childSources = ImmutableSet.builder();
     for (BooleanExpr conjunct : _conjuncts) {
-      conjunct.collectSources(sources, routingPolicies, w);
+      childSources.addAll(conjunct.collectSources(parentSources, routingPolicies, w));
     }
+    return childSources.build();
   }
 
   @Override

diff --git a/...on-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/ConjunctionChain.java b/...on-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/ConjunctionChain.java
@@ -1,6 +1,7 @@
 package org.batfish.datamodel.routing_policy.expr;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.google.common.collect.ImmutableSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -26,11 +27,13 @@ public ConjunctionChain(List<BooleanExpr> subroutines) {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    ImmutableSet.Builder<String> childSources = ImmutableSet.builder();
     for (BooleanExpr conjunct : _subroutines) {
-      conjunct.collectSources(sources, routingPolicies, w);
+      childSources.addAll(conjunct.collectSources(parentSources, routingPolicies, w));
     }
+    return childSources.build();
   }
 
   @Override

diff --git a/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Disjunction.java b/...-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Disjunction.java
@@ -1,5 +1,6 @@
 package org.batfish.datamodel.routing_policy.expr;
 
+import com.google.common.collect.ImmutableSet;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -21,11 +22,13 @@ public Disjunction() {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    ImmutableSet.Builder<String> childSources = ImmutableSet.builder();
     for (BooleanExpr disjunct : _disjuncts) {
-      disjunct.collectSources(sources, routingPolicies, w);
+      childSources.addAll(disjunct.collectSources(parentSources, routingPolicies, w));
     }
+    return childSources.build();
   }
 
   @Override

diff --git a/...on-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/DisjunctionChain.java b/...on-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/DisjunctionChain.java
@@ -1,6 +1,7 @@
 package org.batfish.datamodel.routing_policy.expr;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.google.common.collect.ImmutableSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -26,11 +27,13 @@ public DisjunctionChain(List<BooleanExpr> subroutines) {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    ImmutableSet.Builder<String> childSources = ImmutableSet.builder();
     for (BooleanExpr disjunct : _subroutines) {
-      disjunct.collectSources(sources, routingPolicies, w);
+      childSources.addAll(disjunct.collectSources(parentSources, routingPolicies, w));
     }
+    return childSources.build();
   }
 
   @Override

diff --git a/.../batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Not.java b/.../batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/Not.java
@@ -23,9 +23,9 @@ public Not(BooleanExpr expr) {
   }
 
   @Override
-  public void collectSources(
-      Set<String> sources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
-    _expr.collectSources(sources, routingPolicies, w);
+  public Set<String> collectSources(
+      Set<String> parentSources, Map<String, RoutingPolicy> routingPolicies, Warnings w) {
+    return _expr.collectSources(parentSources, routingPolicies, w);
   }
 
   @Override