Skip to content

Commit

Permalink
ComparePeerGroupPolicies: deduplicate context differences.
Browse files Browse the repository at this point in the history
  • Loading branch information
@nickgian
nickgian committed Apr 29, 2024
1 parent eb8c824 commit bf2558f
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 116 deletions.
3 changes: 1 addition & 2 deletions .../batfish/minesweeper/question/comparepeergrouppolicies/ComparePeerGroupPoliciesUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ private static void trackDifference(
SyntacticDifference d =
new SyntacticDifference(
syntacticCompare.getCurrentConfig().getRoutingPolicies().get(currentPolicy),
syntacticCompare.getReferenceConfig().getRoutingPolicies().get(referencePolicy),
syntacticCompare.getContextDiff());
syntacticCompare.getReferenceConfig().getRoutingPolicies().get(referencePolicy));
SortedSet<String> routers = differences.computeIfAbsent(d, k -> new TreeSet<>());
routers.add(router);
}
Expand Down
93 changes: 37 additions & 56 deletions ...n/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,9 @@
import static org.batfish.question.TracingHintsStripper.TRACING_HINTS_STRIPPER;

import com.google.common.collect.ImmutableList;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.batfish.datamodel.routing_policy.RoutingPolicy;
import org.batfish.datamodel.routing_policy.statement.Statement;
import org.batfish.minesweeper.collectors.RoutePolicyStatementCallCollector;
import org.batfish.minesweeper.utils.Tuple;

/**
* Captures pairs of current/reference routing policies that *may* differ, based on syntactic
Expand All @@ -24,16 +19,9 @@ public class SyntacticDifference implements Comparable<SyntacticDifference> {
/** The policy for the reference snapshot */
private final RoutingPolicy _referencePolicy;

/** The (potential) context-diff under which the two policies differ */
private final RoutingPolicyContextDiff _context;

public SyntacticDifference(
RoutingPolicy currentPolicy,
RoutingPolicy referencePolicy,
RoutingPolicyContextDiff context) {
public SyntacticDifference(RoutingPolicy currentPolicy, RoutingPolicy referencePolicy) {
this._currentPolicy = currentPolicy;
this._referencePolicy = referencePolicy;
this._context = context;
}

public RoutingPolicy getCurrentPolicy() {
Expand All @@ -44,18 +32,14 @@ public RoutingPolicy getReferencePolicy() {
return _referencePolicy;
}

public RoutingPolicyContextDiff getContext() {
return _context;
}

/**
* Strips tracing hints from a list of statements. Useful for comparing routing policies across
* different files.
*
* @param stmts A list of statements
* @return The same list of statements but with tracing information stripped.
*/
private List<Statement> stripStatements(List<Statement> stmts) {
private static List<Statement> stripStatements(List<Statement> stmts) {
return stmts.stream()
.map(st -> st.accept(TRACING_HINTS_STRIPPER, null))
.collect(ImmutableList.toImmutableList());
Expand All @@ -64,54 +48,51 @@ private List<Statement> stripStatements(List<Statement> stmts) {
/**
* @param p1 the first policy
* @param p2 the second policy
* @return true if the two policies are syntactically equal, including any sub-policies they call.
* @return the result of comparing the strings of the statements of two routing policies. Note
* that this does not recursively look into called statements; it assumes that if the
* top-level statements are equal (e.g., the route-map names), any call statement is also
* equal. This allows for more aggressive deduplication, especially in cases where there might
* be small differences in prefix-lists or community-lists.
*/
private boolean routingPolicySyntaxEq(RoutingPolicy p1, RoutingPolicy p2) {
private static int routingPolicySyntaxComparison(RoutingPolicy p1, RoutingPolicy p2) {

List<Statement> p1Stripped = stripStatements(p1.getStatements());
if (p1Stripped.equals(stripStatements(p2.getStatements()))) {
Set<String> callees =
new RoutePolicyStatementCallCollector()
.visitAll(p1Stripped, new Tuple<>(new HashSet<>(), p1.getOwner()));
for (String callee : callees) {
if (!routingPolicySyntaxEq(
p1.getOwner().getRoutingPolicies().get(callee),
p2.getOwner().getRoutingPolicies().get(callee))) {
return false;
}
}
return true;
} else {
return false;
}
List<Statement> p2Stripped = stripStatements(p2.getStatements());
return p1Stripped.toString().compareTo(p2Stripped.toString());
}

/**
* @param o the object to be compared.
* @return This comparison is a bit strange, because Statements are not comparable. To determine
* the ordering of two potential differences we first do a syntactic comparison between the
* policy in the current snapshot, the policy in the reference snapshot, the routing contexts
* and the names of the route-maps. If all of these are equal, the two differences are equal.
* We require the names to be equal too, otherwise it would be difficult for a user to track
* down all the route-maps with the same content but different names. Otherwise if these are
* not equal, we simply order the differences by name and hostname. Note that we consider the
* full routing context of the device, even if some definitions are not used in this routing
* policy. This might lead to two seemingly equal policies being considered different, but
* this can only cause a performance issue by running CRP more than necessary. The results are
* anyway deduplicated.
* @return Compares the statements of the current routing policies, and if they are equal of the
* reference routing policies.
*/
@Override
public int compareTo(SyntacticDifference o) {

if (this.getCurrentPolicy().getName().equals(o.getCurrentPolicy().getName())
&& this.getReferencePolicy().getName().equals(o.getReferencePolicy().getName())
&& routingPolicySyntaxEq(this.getCurrentPolicy(), o.getCurrentPolicy())
&& routingPolicySyntaxEq(this.getReferencePolicy(), o.getReferencePolicy())
&& this.getContext().equals(o.getContext())) {
return 0;
int current = routingPolicySyntaxComparison(this.getCurrentPolicy(), o.getCurrentPolicy());
if (current != 0) {
return current;
} else {
Comparator<RoutingPolicy> policyName = Comparator.comparing(RoutingPolicy::getName);
Comparator<RoutingPolicy> ownerName = Comparator.comparing(r -> r.getOwner().getHostname());
return policyName.thenComparing(ownerName).compare(this._currentPolicy, o._currentPolicy);
return routingPolicySyntaxComparison(this.getReferencePolicy(), o.getReferencePolicy());
}
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}

SyntacticDifference that = (SyntacticDifference) o;
return this.compareTo(that) == 0;
}

@Override
public int hashCode() {
int result = stripStatements(_currentPolicy.getStatements()).hashCode();
result = 31 * result + stripStatements(_referencePolicy.getStatements()).hashCode();
return result;
}
}
8 changes: 4 additions & 4 deletions ...h/minesweeper/question/comparepeergrouppolicies/ComparePeerGroupPoliciesAnswererTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -679,17 +679,17 @@ public void testTableStitching() throws IOException {
Matchers.contains(
allOf(
hasColumn(COL_NODE, equalTo(new Node(HOSTNAME)), Schema.NODE),
hasColumn(COL_POLICY_NAME, equalTo(POLICY_NAME), Schema.STRING),
hasColumn(COL_REFERENCE_POLICY_NAME, equalTo(POLICY_NAME), Schema.STRING),
hasColumn(COL_POLICY_NAME, equalTo("RM2"), Schema.STRING),
hasColumn(COL_REFERENCE_POLICY_NAME, equalTo("RM2"), Schema.STRING),
hasColumn(COL_INPUT_ROUTE, equalTo(inputRoute), Schema.BGP_ROUTE),
hasColumn(baseColumnName(COL_ACTION), equalTo(DENY.toString()), Schema.STRING),
hasColumn(deltaColumnName(COL_ACTION), equalTo(PERMIT.toString()), Schema.STRING),
hasColumn(baseColumnName(COL_OUTPUT_ROUTE), anything(), Schema.BGP_ROUTE),
hasColumn(COL_DIFF, equalTo(diff), Schema.BGP_ROUTE_DIFFS)),
allOf(
hasColumn(COL_NODE, equalTo(new Node(HOSTNAME)), Schema.NODE),
hasColumn(COL_POLICY_NAME, equalTo("RM2"), Schema.STRING),
hasColumn(COL_REFERENCE_POLICY_NAME, equalTo("RM2"), Schema.STRING),
hasColumn(COL_POLICY_NAME, equalTo(POLICY_NAME), Schema.STRING),
hasColumn(COL_REFERENCE_POLICY_NAME, equalTo(POLICY_NAME), Schema.STRING),
hasColumn(COL_INPUT_ROUTE, equalTo(inputRoute), Schema.BGP_ROUTE),
hasColumn(baseColumnName(COL_ACTION), equalTo(DENY.toString()), Schema.STRING),
hasColumn(deltaColumnName(COL_ACTION), equalTo(PERMIT.toString()), Schema.STRING),
Expand Down
72 changes: 18 additions & 54 deletions ...va/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifferenceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import projects.minesweeper.src.main.java.org.batfish.minesweeper.question.comparepeergrouppolicies.RoutingPolicyContextDiff;
import projects.minesweeper.src.main.java.org.batfish.minesweeper.question.comparepeergrouppolicies.SyntacticDifference;

public class SyntacticDifferenceTest {
Expand Down Expand Up @@ -125,14 +124,8 @@ public void testSameDifferenceBothDevices() {
.addStatement(new Statements.StaticStatement(Statements.ExitReject))
.build();

SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertEquals(0, d1.compareTo(d2));
}

Expand All @@ -156,15 +149,9 @@ public void testSameDifferenceDifferentName() {
.addStatement(new Statements.StaticStatement(Statements.ExitReject))
.build();

SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
assertNotEquals(0, d1.compareTo(d2));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertEquals(0, d1.compareTo(d2));
}

@Test
Expand All @@ -187,14 +174,8 @@ public void testDifferentDifference_current() {
.addStatement(new Statements.StaticStatement(Statements.ExitReject))
.build();

SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertNotEquals(0, d1.compareTo(d2));
}

Expand All @@ -218,14 +199,8 @@ public void testDifferentDifference_reference() {
.addStatement(new Statements.StaticStatement(Statements.ExitReject))
.build();

SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertNotEquals(0, d1.compareTo(d2));
}

Expand All @@ -251,17 +226,12 @@ public void testDifferentDifference_context() {

base.getOwner().setRouteFilterLists(ImmutableMap.of());

SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
assertNotEquals(0, d1.compareTo(d2));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertEquals(0, d1.compareTo(d2));
}

/* Differences in nested calls do not matter. */
@Test
public void testDifference_recursive() {
RoutingPolicy calledPolicyBase =
Expand Down Expand Up @@ -305,23 +275,17 @@ public void testDifference_recursive() {
deltaOther.getOwner().getRoutingPolicies().put("RM2", calledPolicyDeltaOther);

// Compare the differences between the two callers.
SyntacticDifference d1 =
new SyntacticDifference(
base, delta, new RoutingPolicyContextDiff(base.getOwner(), delta.getOwner()));
SyntacticDifference d2 =
new SyntacticDifference(
baseOther,
deltaOther,
new RoutingPolicyContextDiff(baseOther.getOwner(), deltaOther.getOwner()));
SyntacticDifference d1 = new SyntacticDifference(base, delta);
SyntacticDifference d2 = new SyntacticDifference(baseOther, deltaOther);
assertEquals(0, d1.compareTo(d2));

// If we change the called reference policy on one of the two devices, then the two differences
// will be different.
// remain unchanged.
calledPolicyDeltaOther.setStatements(
ImmutableList.of(
new Statements.StaticStatement(Statements.ExitAccept),
new Statements.StaticStatement(Statements.ExitAccept)));
assertNotEquals(0, d1.compareTo(d2));
assertEquals(0, d1.compareTo(d2));

// Likewise, if we change the called base policy. (first restore the delta policy)
calledPolicyDeltaOther.setStatements(
Expand All @@ -331,6 +295,6 @@ public void testDifference_recursive() {
new Statements.StaticStatement(Statements.ExitAccept),
new Statements.StaticStatement(Statements.ExitAccept)));

assertNotEquals(0, d1.compareTo(d2));
assertEquals(0, d1.compareTo(d2));
}
}

0 comments on commit bf2558f

Please sign in to comment.