FortiOS: service conversion update (#6721)

batfish · Mar 12, 2021 · 75e21f9 · 75e21f9
1 parent a1015b1
commit 75e21f9
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 38 deletions.
diff --git a/projects/batfish/src/main/java/org/batfish/representation/fortios/FortiosConfiguration.java b/projects/batfish/src/main/java/org/batfish/representation/fortios/FortiosConfiguration.java
@@ -2,6 +2,7 @@
 
 import static org.batfish.datamodel.acl.AclLineMatchExprs.and;
 import static org.batfish.datamodel.acl.AclLineMatchExprs.or;
+import static org.batfish.representation.fortios.FortiosTraceElementCreators.matchServiceTraceElement;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
@@ -26,7 +27,9 @@
 import org.batfish.datamodel.LineAction;
 import org.batfish.datamodel.TraceElement;
 import org.batfish.datamodel.acl.AclLineMatchExpr;
+import org.batfish.datamodel.acl.AclLineMatchExprs;
 import org.batfish.datamodel.acl.MatchHeaderSpace;
+import org.batfish.datamodel.acl.OrMatchExpr;
 import org.batfish.vendor.VendorConfiguration;
 import org.batfish.vendor.VendorStructureId;
 
@@ -112,7 +115,7 @@ public List<Configuration> toVendorIndependentConfigurations() throws VendorConv
     // Convert policies. Must happen after c._ipSpaces is populated (addresses are converted)
     Map<String, AclLineMatchExpr> convertedServices =
         _services.values().stream()
-            .collect(ImmutableMap.toImmutableMap(Service::getName, svc -> svc.toMatchExpr(_w)));
+            .collect(ImmutableMap.toImmutableMap(Service::getName, this::toMatchExpr));
     _policies.values().forEach(policy -> convertPolicy(policy, c, convertedServices));
 
     // Count structure references
@@ -122,6 +125,22 @@ public List<Configuration> toVendorIndependentConfigurations() throws VendorConv
     return c;
   }
 
+  /** Convert specified {@link Service} into its corresponding {@link AclLineMatchExpr}. */
+  @VisibleForTesting
+  @Nonnull
+  AclLineMatchExpr toMatchExpr(Service service) {
+    List<AclLineMatchExpr> matchExprs =
+        service
+            .toHeaderSpaces()
+            .map(MatchHeaderSpace::new)
+            .collect(ImmutableList.toImmutableList());
+    if (matchExprs.isEmpty()) {
+      _w.redFlag(String.format("Service %s does not match any packets", service.getName()));
+      return AclLineMatchExprs.FALSE;
+    }
+    return new OrMatchExpr(matchExprs, matchServiceTraceElement(service, _filename));
+  }
+
   private void convertPolicy(
       Policy policy, Configuration c, Map<String, AclLineMatchExpr> convertedServices) {
     if (policy.getStatusEffective() != Policy.Status.ENABLE) {

diff --git a/...batfish/src/main/java/org/batfish/representation/fortios/FortiosTraceElementCreators.java b/...batfish/src/main/java/org/batfish/representation/fortios/FortiosTraceElementCreators.java
@@ -0,0 +1,27 @@
+package org.batfish.representation.fortios;
+
+import org.batfish.datamodel.TraceElement;
+import org.batfish.vendor.VendorStructureId;
+
+/** Collection of methods to create {@link TraceElement trace elements} for FortiOS ACL lines. */
+public final class FortiosTraceElementCreators {
+
+  private FortiosTraceElementCreators() {}
+
+  /** Creates {@link TraceElement} for specified {@link Service}. */
+  static TraceElement matchServiceTraceElement(Service service, String filename) {
+    TraceElement.Builder te =
+        TraceElement.builder()
+            .add("Matched service ")
+            .add(
+                service.getName(),
+                new VendorStructureId(
+                    filename,
+                    FortiosStructureType.SERVICE_CUSTOM.getDescription(),
+                    service.getName()));
+    if (service.getComment() != null) {
+      te.add(String.format("(%s)", service.getComment()));
+    }
+    return te.build();
+  }
+}
diff --git a/projects/batfish/src/main/java/org/batfish/representation/fortios/Service.java b/projects/batfish/src/main/java/org/batfish/representation/fortios/Service.java
@@ -1,24 +1,17 @@
 package org.batfish.representation.fortios;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Range;
 import java.io.Serializable;
-import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Stream;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import org.batfish.common.Warnings;
 import org.batfish.datamodel.HeaderSpace;
 import org.batfish.datamodel.IntegerSpace;
 import org.batfish.datamodel.IpProtocol;
 import org.batfish.datamodel.IpRange;
-import org.batfish.datamodel.acl.AclLineMatchExpr;
-import org.batfish.datamodel.acl.AclLineMatchExprs;
-import org.batfish.datamodel.acl.MatchHeaderSpace;
-import org.batfish.datamodel.acl.OrMatchExpr;
 
 /** FortiOS datamodel component containing firewall service configuration */
 public final class Service implements FortiosRenameableObject, Serializable {
@@ -223,17 +216,8 @@ public Service(String name, BatfishUUID uuid) {
   @Nullable private IntegerSpace _sctpPortRangeDst;
   @Nullable private IntegerSpace _sctpPortRangeSrc;
 
-  public @Nonnull AclLineMatchExpr toMatchExpr(Warnings w) {
-    List<AclLineMatchExpr> matchExprs =
-        toHeaderSpaces().map(MatchHeaderSpace::new).collect(ImmutableList.toImmutableList());
-    if (matchExprs.isEmpty()) {
-      w.redFlag(String.format("Service %s does not match any packets", _name));
-      return AclLineMatchExprs.FALSE;
-    }
-    return new OrMatchExpr(matchExprs, getTraceElement());
-  }
-
-  private @Nonnull Stream<HeaderSpace> toHeaderSpaces() {
+  @Nonnull
+  Stream<HeaderSpace> toHeaderSpaces() {
     switch (getProtocolEffective()) {
       case TCP_UDP_SCTP:
         return Stream.of(
@@ -286,9 +270,4 @@ private static HeaderSpace buildIcmpHeaderSpace(
     Optional.ofNullable(icmpType).ifPresent(headerSpace::setIcmpTypes);
     return headerSpace.build();
   }
-
-  private @Nonnull String getTraceElement() {
-    String baseTrace = "Matched service " + _name;
-    return _comment == null ? baseTrace : String.format("%s: %s", baseTrace, _comment);
-  }
 }
diff --git a/...h/representation/fortios/ServiceTest.java → ...ion/fortios/FortiosConfigurationTest.java b/...h/representation/fortios/ServiceTest.java → ...ion/fortios/FortiosConfigurationTest.java
@@ -1,6 +1,7 @@
 package org.batfish.representation.fortios;
 
 import static org.batfish.common.matchers.WarningMatchers.hasText;
+import static org.batfish.representation.fortios.FortiosTraceElementCreators.matchServiceTraceElement;
 import static org.batfish.representation.fortios.Service.DEFAULT_SOURCE_PORT_RANGE;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
@@ -16,10 +17,9 @@
 import org.batfish.datamodel.HeaderSpace;
 import org.batfish.datamodel.IntegerSpace;
 import org.batfish.datamodel.IpProtocol;
-import org.batfish.datamodel.TraceElement;
 import org.junit.Test;
 
-public class ServiceTest {
+public class FortiosConfigurationTest {
 
   private static final BddTestbed BDD_TESTBED;
   private static final BDD ZERO;
@@ -39,16 +39,18 @@ public class ServiceTest {
   public void testToMatchExpr_tcpUdpSctp_defaults() {
     // Default service with no dst ports specified matches nothing and files warning
     String svcName = "name";
+    FortiosConfiguration c = new FortiosConfiguration();
     Service service = new Service(svcName, new BatfishUUID(1));
     Warnings w = new Warnings(true, true, true);
-    assertThat(ACL_TO_BDD.toBdd(service.toMatchExpr(w)), equalTo(ZERO));
+    c.setWarnings(w);
+    assertThat(ACL_TO_BDD.toBdd(c.toMatchExpr(service)), equalTo(ZERO));
     assertThat(
         w.getRedFlagWarnings(),
         contains(hasText(String.format("Service %s does not match any packets", svcName))));
 
     // behavior is the same if protocol is explicit
     service.setProtocol(Service.Protocol.TCP_UDP_SCTP);
-    assertThat(ACL_TO_BDD.toBdd(service.toMatchExpr(w)), equalTo(ZERO));
+    assertThat(ACL_TO_BDD.toBdd(c.toMatchExpr(service)), equalTo(ZERO));
   }
 
   @Test
@@ -180,18 +182,15 @@ public void testToMatchExpr_ip_custom() {
   @Test
   public void testToMatchExpr_traceElement() {
     String svcName = "name";
+    String filename = "filename";
     Service service = new Service(svcName, new BatfishUUID(1));
+    FortiosConfiguration c = new FortiosConfiguration();
+    c.setWarnings(new Warnings());
+    c.setFilename(filename);
     service.setProtocol(Service.Protocol.ICMP);
     assertThat(
-        service.toMatchExpr(new Warnings()).getTraceElement(),
-        equalTo(TraceElement.of("Matched service " + svcName)));
-
-    // With comment
-    String comment = "you can't go there";
-    service.setComment(comment);
-    assertThat(
-        service.toMatchExpr(new Warnings()).getTraceElement(),
-        equalTo(TraceElement.of(String.format("Matched service %s: %s", svcName, comment))));
+        c.toMatchExpr(service).getTraceElement(),
+        equalTo(matchServiceTraceElement(service, filename)));
   }
 
   /**
@@ -200,7 +199,9 @@ public void testToMatchExpr_traceElement() {
    */
   private void assertConvertsWithoutWarnings(Service service, BDD expected) {
     Warnings w = new Warnings();
-    assertThat(ACL_TO_BDD.toBdd(service.toMatchExpr(w)), equalTo(expected));
+    FortiosConfiguration c = new FortiosConfiguration();
+    c.setWarnings(w);
+    assertThat(ACL_TO_BDD.toBdd(c.toMatchExpr(service)), equalTo(expected));
     assertThat(w.getRedFlagWarnings(), empty());
   }
 }