Improve SegmentRoutingAppConfig

- Separate host and subnet suppression
- Use port instead of interface to specify which SR should ignore

Change-Id: Ie6491950cddf0860924565f081504b4f4d788179

@@ -473,7 +473,7 @@ public class RoutingRulePopulator {
         for (Port port : srManager.deviceService.getPorts(deviceId)) {
             ConnectPoint cp = new ConnectPoint(deviceId, port.number());
             // TODO: Handles dynamic port events when we are ready for dynamic config
-            if (!srManager.deviceConfiguration.excludedPorts().contains(cp) &&
+            if (!srManager.deviceConfiguration.suppressSubnet().contains(cp) &&
                     port.isEnabled()) {
                 Ip4Prefix portSubnet = config.getPortSubnet(deviceId, port.number());
                 VlanId assignedVlan = (portSubnet == null)

@@ -991,7 +991,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             Set<IpAddress> ips = event.subject().ipAddresses();
             log.info("Host {}/{} is added at {}:{}", mac, vlanId, deviceId, port);
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(deviceId, port))) {
                 // Populate bridging table entry
                 log.debug("Populate L2 table entry for host {} at {}:{}",
@@ -1020,7 +1020,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             Set<IpAddress> ips = event.subject().ipAddresses();
             log.debug("Host {}/{} is removed from {}:{}", mac, vlanId, deviceId, port);
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(deviceId, port))) {
                 // Revoke bridging table entry
                 ForwardingObjective.Builder fob =
@@ -1051,7 +1051,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             log.debug("Host {}/{} is moved from {}:{} to {}:{}",
                     mac, vlanId, prevDeviceId, prevPort, newDeviceId, newPort);
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(prevDeviceId, prevPort))) {
                 // Revoke previous bridging table entry
                 ForwardingObjective.Builder prevFob =
@@ -1069,7 +1069,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                 });
             }
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(newDeviceId, newPort))) {
                 // Populate new bridging table entry
                 ForwardingObjective.Builder newFob =
@@ -1099,7 +1099,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             Set<IpAddress> newIps = event.subject().ipAddresses();
             log.debug("Host {}/{} is updated", mac, vlanId);
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(prevDeviceId, prevPort))) {
                 // Revoke previous IP table entry
                 prevIps.forEach(ip -> {
@@ -1110,7 +1110,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                 });
             }
 
-            if (!deviceConfiguration.excludedPorts()
+            if (!deviceConfiguration.suppressHost()
                     .contains(new ConnectPoint(newDeviceId, newPort))) {
                 // Populate new IP table entry
                 newIps.forEach(ip -> {

@@ -18,7 +18,6 @@ package org.onosproject.segmentrouting.config;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.SetMultimap;
-import com.google.common.collect.Sets;
 import org.onlab.packet.Ip4Address;
 import org.onlab.packet.Ip4Prefix;
 import org.onlab.packet.IpPrefix;
@@ -30,6 +29,7 @@ import org.onosproject.incubator.net.config.basics.InterfaceConfig;
 import org.onosproject.incubator.net.intf.Interface;
 import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.config.NetworkConfigRegistry;
+import org.onosproject.net.config.NetworkConfigService;
 import org.onosproject.net.host.InterfaceIpAddress;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.PortNumber;
@@ -57,7 +57,8 @@ public class DeviceConfiguration implements DeviceProperties {
     private final List<Integer> allSegmentIds = new ArrayList<>();
     private final Map<DeviceId, SegmentRouterInfo> deviceConfigMap = new ConcurrentHashMap<>();
     private final Map<VlanId, List<ConnectPoint>> xConnects = new ConcurrentHashMap<>();
-    private final Set<ConnectPoint> excludedPorts = Sets.newConcurrentHashSet();
+    private ApplicationId appId;
+    private NetworkConfigService cfgService;
     private SegmentRoutingAppConfig appConfig;
 
     private class SegmentRouterInfo {
@@ -85,6 +86,9 @@ public class DeviceConfiguration implements DeviceProperties {
      */
     public DeviceConfiguration(ApplicationId appId,
             NetworkConfigRegistry cfgService) {
+        this.appId = appId;
+        this.cfgService = cfgService;
+
         // Read config from device subject, excluding gatewayIps and subnets.
         Set<DeviceId> deviceSubjects =
                 cfgService.getSubjects(DeviceId.class, SegmentRoutingDeviceConfig.class);
@@ -103,15 +107,18 @@ public class DeviceConfiguration implements DeviceProperties {
             allSegmentIds.add(info.nodeSid);
         });
 
-        // Read excluded port names from config
+        // Read SegmentRoutingAppConfig
         appConfig = cfgService.getConfig(appId, SegmentRoutingAppConfig.class);
-        Set<String> excludePorts = (appConfig != null) ?
-                appConfig.excludePorts() : ImmutableSet.of();
 
         // Read gatewayIps and subnets from port subject.
         Set<ConnectPoint> portSubjects =
             cfgService.getSubjects(ConnectPoint.class, InterfaceConfig.class);
         portSubjects.forEach(subject -> {
+            // Do not process excluded ports
+            if (suppressSubnet().contains(subject)) {
+                return;
+            }
+
             InterfaceConfig config =
                     cfgService.getConfig(subject, InterfaceConfig.class);
             Set<Interface> networkInterfaces;
@@ -122,12 +129,6 @@ public class DeviceConfiguration implements DeviceProperties {
                 return;
             }
             networkInterfaces.forEach(networkInterface -> {
-                // Do not process excluded ports
-                if (excludePorts.contains(networkInterface.name())) {
-                    excludedPorts.add(subject);
-                    return;
-                }
-
                 VlanId vlanId = networkInterface.vlan();
                 ConnectPoint connectPoint = networkInterface.connectPoint();
                 DeviceId dpid = connectPoint.deviceId();
@@ -366,7 +367,7 @@ public class DeviceConfiguration implements DeviceProperties {
 
             ImmutableSet.Builder<Ip4Prefix> builder = ImmutableSet.builder();
             builder.addAll(srinfo.subnets.values());
-            if (deviceId.equals(appConfig.vRouterId())) {
+            if (appConfig != null && deviceId.equals(appConfig.vRouterId())) {
                 builder.add(Ip4Prefix.valueOf("0.0.0.0/0"));
             }
             return builder.build();
@@ -491,12 +492,11 @@ public class DeviceConfiguration implements DeviceProperties {
         return srinfo != null && srinfo.adjacencySids.containsKey(sid);
     }
 
-    /**
-     * Returns a set of excluded ports.
-     *
-     * @return excluded ports
-     */
-    public Set<ConnectPoint> excludedPorts() {
-        return excludedPorts;
+    public Set<ConnectPoint> suppressSubnet() {
+        return (appConfig != null) ? appConfig.suppressSubnet() : ImmutableSet.of();
+    }
+
+    public Set<ConnectPoint> suppressHost() {
+        return (appConfig != null) ? appConfig.suppressHost() : ImmutableSet.of();
     }
 }

@@ -21,8 +21,11 @@ import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.google.common.collect.ImmutableSet;
 import org.onlab.packet.MacAddress;
 import org.onosproject.core.ApplicationId;
+import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.config.Config;
+
+import java.util.Optional;
 import java.util.Set;
 
 import static com.google.common.base.MoreObjects.toStringHelper;
@@ -33,23 +36,25 @@ import static com.google.common.base.MoreObjects.toStringHelper;
 public class SegmentRoutingAppConfig extends Config<ApplicationId> {
     private static final String VROUTER_MACS = "vRouterMacs";
     private static final String VROUTER_ID = "vRouterId";
-    private static final String EXCLUDE_PORTS = "excludePorts";
+    private static final String SUPPRESS_SUBNET = "suppressSubnet";
+    private static final String SUPPRESS_HOST = "suppressHost";
 
     @Override
     public boolean isValid() {
-        return hasOnlyFields(VROUTER_MACS, VROUTER_ID, EXCLUDE_PORTS) &&
+        return hasOnlyFields(VROUTER_MACS, VROUTER_ID, SUPPRESS_SUBNET, SUPPRESS_HOST) &&
                 vRouterMacs() != null && vRouterId() != null &&
-                excludePorts() != null;
+                suppressSubnet() != null && suppressHost() != null;
     }
 
     /**
      * Gets vRouters from the config.
      *
-     * @return a set of vRouter MAC addresses
+     * @return Set of vRouter MAC addresses, empty is not specified,
+     *         or null if not valid
      */
     public Set<MacAddress> vRouterMacs() {
         if (!object.has(VROUTER_MACS)) {
-            return null;
+            return ImmutableSet.of();
         }
 
         ImmutableSet.Builder<MacAddress> builder = ImmutableSet.builder();
@@ -96,15 +101,16 @@ public class SegmentRoutingAppConfig extends Config<ApplicationId> {
     /**
      * Gets vRouter device ID.
      *
-     * @return vRouter device ID, or null if not valid
+     * @return Optional vRouter device ID,
+     *         empty is not specified or null if not valid
      */
-    public DeviceId vRouterId() {
+    public Optional<DeviceId> vRouterId() {
         if (!object.has(VROUTER_ID)) {
-            return null;
+            return Optional.empty();
         }
 
         try {
-            return DeviceId.deviceId(object.path(VROUTER_ID).asText());
+            return Optional.of(DeviceId.deviceId(object.path(VROUTER_ID).asText()));
         } catch (IllegalArgumentException e) {
             return null;
         }
@@ -126,42 +132,95 @@ public class SegmentRoutingAppConfig extends Config<ApplicationId> {
     }
 
     /**
-     * Gets names of ports that are ignored by SegmentRouting.
+     * Gets names of ports to which SegmentRouting does not push subnet rules.
      *
-     * @return set of port names
+     * @return Set of port names, empty if not specified, or null
+     *         if not valid
      */
-    public Set<String> excludePorts() {
-        if (!object.has(EXCLUDE_PORTS)) {
-            return null;
+    public Set<ConnectPoint> suppressSubnet() {
+        if (!object.has(SUPPRESS_SUBNET)) {
+            return ImmutableSet.of();
         }
 
-        ImmutableSet.Builder<String> builder = ImmutableSet.builder();
-        ArrayNode arrayNode = (ArrayNode) object.path(EXCLUDE_PORTS);
+        ImmutableSet.Builder<ConnectPoint> builder = ImmutableSet.builder();
+        ArrayNode arrayNode = (ArrayNode) object.path(SUPPRESS_SUBNET);
         for (JsonNode jsonNode : arrayNode) {
             String portName = jsonNode.asText(null);
             if (portName == null) {
                 return null;
             }
-            builder.add(portName);
+            try {
+                builder.add(ConnectPoint.deviceConnectPoint(portName));
+            } catch (IllegalArgumentException e) {
+                return null;
+            }
+        }
+        return builder.build();
+    }
+
+    /**
+     * Sets names of ports to which SegmentRouting does not push subnet rules.
+     *
+     * @param suppressSubnet names of ports to which SegmentRouting does not push
+     *                     subnet rules
+     * @return this {@link SegmentRoutingAppConfig}
+     */
+    public SegmentRoutingAppConfig setSuppressSubnet(Set<ConnectPoint> suppressSubnet) {
+        if (suppressSubnet == null) {
+            object.remove(SUPPRESS_SUBNET);
+        } else {
+            ArrayNode arrayNode = mapper.createArrayNode();
+            suppressSubnet.forEach(connectPoint -> {
+                arrayNode.add(connectPoint.deviceId() + "/" + connectPoint.port());
+            });
+            object.set(SUPPRESS_SUBNET, arrayNode);
+        }
+        return this;
+    }
+
+    /**
+     * Gets names of ports to which SegmentRouting does not push host rules.
+     *
+     * @return Set of port names, empty if not specified, or null
+     *         if not valid
+     */
+    public Set<ConnectPoint> suppressHost() {
+        if (!object.has(SUPPRESS_HOST)) {
+            return ImmutableSet.of();
+        }
+
+        ImmutableSet.Builder<ConnectPoint> builder = ImmutableSet.builder();
+        ArrayNode arrayNode = (ArrayNode) object.path(SUPPRESS_HOST);
+        for (JsonNode jsonNode : arrayNode) {
+            String portName = jsonNode.asText(null);
+            if (portName == null) {
+                return null;
+            }
+            try {
+                builder.add(ConnectPoint.deviceConnectPoint(portName));
+            } catch (IllegalArgumentException e) {
+                return null;
+            }
         }
         return builder.build();
     }
 
     /**
-     * Sets names of ports that are ignored by SegmentRouting.
+     * Sets names of ports to which SegmentRouting does not push host rules.
      *
-     * @param excludePorts names of ports that are ignored by SegmentRouting
+     * @param suppressHost names of ports to which SegmentRouting does not push
+     *                     host rules
      * @return this {@link SegmentRoutingAppConfig}
      */
-    public SegmentRoutingAppConfig setExcludePorts(Set<String> excludePorts) {
-        if (excludePorts == null) {
-            object.remove(EXCLUDE_PORTS);
+    public SegmentRoutingAppConfig setSuppressHost(Set<ConnectPoint> suppressHost) {
+        if (suppressHost == null) {
+            object.remove(SUPPRESS_HOST);
         } else {
             ArrayNode arrayNode = mapper.createArrayNode();
-            excludePorts.forEach(portName -> {
-                arrayNode.add(portName);
+            suppressHost.forEach(connectPoint -> {
+                arrayNode.add(connectPoint.deviceId() + "/" + connectPoint.port());
             });
-            object.set(EXCLUDE_PORTS, arrayNode);
+            object.set(SUPPRESS_HOST, arrayNode);
         }
         return this;
     }
@@ -170,7 +229,9 @@ public class SegmentRoutingAppConfig extends Config<ApplicationId> {
     public String toString() {
         return toStringHelper(this)
                 .add("vRouterMacs", vRouterMacs())
-                .add("excludePorts", excludePorts())
+                .add("vRouterId", vRouterId())
+                .add("suppressSubnet", suppressSubnet())
+                .add("suppressHost", suppressHost())
                 .toString();
     }
 }

@@ -24,16 +24,17 @@ import org.junit.Test;
 import org.onlab.packet.MacAddress;
 import org.onosproject.core.ApplicationId;
 import org.onosproject.TestApplicationId;
+import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.config.Config;
 import org.onosproject.net.config.ConfigApplyDelegate;
 import org.onosproject.segmentrouting.SegmentRoutingManager;
+
+import java.util.Optional;
 import java.util.Set;
 
 import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.*;
 
 /**
  * Tests for class {@link SegmentRoutingAppConfig}.
@@ -50,25 +51,29 @@ public class SegmentRoutingAppConfigTest {
             "    \"00:00:00:00:00:02\"" +
             "]," +
             "\"vRouterId\" : \"of:1\"," +
-            "\"excludePorts\" : [" +
-            "    \"port1\"," +
-            "    \"port2\"" +
+            "\"suppressSubnet\" : [" +
+            "    \"of:1/1\"," +
+            "    \"of:1/2\"" +
+            "]," +
+            "\"suppressHost\" : [" +
+            "    \"of:1/1\"," +
+            "    \"of:1/2\"" +
             "]}";
     private static final String INVALID_JSON_STRING = "{" +
             "\"vRouterMacs\" : [" +
             "    \"00:00:00:00:00:01\"," +
             "    \"00:00:00:00:00:02\"" +
             "]," +
-            "\"excludePorts\" : [" +
-            "    \"port1\"," +
-            "    \"port2\"" +
+            "\"suppressSubnet\" : [" +
+            "    \"of:1/1\"," +
+            "    \"wrongport\"" +
             "]}";
     private static final MacAddress ROUTER_MAC_1 = MacAddress.valueOf("00:00:00:00:00:01");
     private static final MacAddress ROUTER_MAC_2 = MacAddress.valueOf("00:00:00:00:00:02");
     private static final MacAddress ROUTER_MAC_3 = MacAddress.valueOf("00:00:00:00:00:03");
-    private static final String PORT_NAME_1 = "port1";
-    private static final String PORT_NAME_2 = "port2";
-    private static final String PORT_NAME_3 = "port3";
+    private static final ConnectPoint PORT_1 = ConnectPoint.deviceConnectPoint("of:1/1");
+    private static final ConnectPoint PORT_2 = ConnectPoint.deviceConnectPoint("of:1/2");
+    private static final ConnectPoint PORT_3 = ConnectPoint.deviceConnectPoint("of:1/3");
     private static final DeviceId VROUTER_ID_1 = DeviceId.deviceId("of:1");
     private static final DeviceId VROUTER_ID_2 = DeviceId.deviceId("of:2");
 
@@ -109,11 +114,12 @@ public class SegmentRoutingAppConfigTest {
      * @throws Exception
      */
     @Test
-    public void testVRouters() throws Exception {
-        Set<MacAddress> vRouters = config.vRouterMacs();
-        assertThat(vRouters.size(), is(2));
-        assertTrue(vRouters.contains(ROUTER_MAC_1));
-        assertTrue(vRouters.contains(ROUTER_MAC_2));
+    public void testVRouterMacs() throws Exception {
+        Set<MacAddress> vRouterMacs = config.vRouterMacs();
+        assertNotNull("vRouterMacs should not be null", vRouterMacs);
+        assertThat(vRouterMacs.size(), is(2));
+        assertTrue(vRouterMacs.contains(ROUTER_MAC_1));
+        assertTrue(vRouterMacs.contains(ROUTER_MAC_2));
     }
 
     /**
@@ -122,14 +128,14 @@ public class SegmentRoutingAppConfigTest {
      * @throws Exception
      */
     @Test
-    public void testSetVRouters() throws Exception {
+    public void testSetVRouterMacs() throws Exception {
         ImmutableSet.Builder<MacAddress> builder = ImmutableSet.builder();
         builder.add(ROUTER_MAC_3);
         config.setVRouterMacs(builder.build());
 
-        Set<MacAddress> macs = config.vRouterMacs();
-        assertThat(macs.size(), is(1));
-        assertTrue(macs.contains(ROUTER_MAC_3));
+        Set<MacAddress> vRouterMacs = config.vRouterMacs();
+        assertThat(vRouterMacs.size(), is(1));
+        assertTrue(vRouterMacs.contains(ROUTER_MAC_3));
     }
 
     /**
@@ -139,7 +145,9 @@ public class SegmentRoutingAppConfigTest {
      */
     @Test
     public void testVRouterId() throws Exception {
-        assertThat(config.vRouterId(), is(VROUTER_ID_1));
+        Optional<DeviceId> vRouterId = config.vRouterId();
+        assertTrue(vRouterId.isPresent());
+        assertThat(vRouterId.get(), is(VROUTER_ID_1));
     }
 
     /**
@@ -150,36 +158,72 @@ public class SegmentRoutingAppConfigTest {
     @Test
     public void testSetVRouterId() throws Exception {
         config.setVRouterId(VROUTER_ID_2);
-        assertThat(config.vRouterId(), is(VROUTER_ID_2));
+
+        Optional<DeviceId> vRouterId = config.vRouterId();
+        assertTrue(vRouterId.isPresent());
+        assertThat(vRouterId.get(), is(VROUTER_ID_2));
+    }
+
+    /**
+     * Tests suppressSubnet getter.
+     *
+     * @throws Exception
+     */
+    @Test
+    public void testSuppressSubnet() throws Exception {
+        Set<ConnectPoint> suppressSubnet = config.suppressSubnet();
+        assertNotNull("suppressSubnet should not be null", suppressSubnet);
+        assertThat(suppressSubnet.size(), is(2));
+        assertTrue(suppressSubnet.contains(PORT_1));
+        assertTrue(suppressSubnet.contains(PORT_2));
+    }
+
+    /**
+     * Tests suppressSubnet setter.
+     *
+     * @throws Exception
+     */
+    @Test
+    public void testSetSuppressSubnet() throws Exception {
+        ImmutableSet.Builder<ConnectPoint> builder = ImmutableSet.builder();
+        builder.add(PORT_3);
+        config.setSuppressSubnet(builder.build());
+
+        Set<ConnectPoint> suppressSubnet = config.suppressSubnet();
+        assertNotNull("suppressSubnet should not be null", suppressSubnet);
+        assertThat(suppressSubnet.size(), is(1));
+        assertTrue(suppressSubnet.contains(PORT_3));
     }
 
     /**
-     * Tests excludePort getter.
+     * Tests suppressHost getter.
      *
      * @throws Exception
      */
     @Test
-    public void testExcludePorts() throws Exception {
-        Set<String> excludePorts = config.excludePorts();
-        assertThat(excludePorts.size(), is(2));
-        assertTrue(excludePorts.contains(PORT_NAME_1));
-        assertTrue(excludePorts.contains(PORT_NAME_2));
+    public void testSuppressHost() throws Exception {
+        Set<ConnectPoint> suppressHost = config.suppressHost();
+        assertNotNull("suppressHost should not be null", suppressHost);
+        assertThat(suppressHost.size(), is(2));
+        assertTrue(suppressHost.contains(PORT_1));
+        assertTrue(suppressHost.contains(PORT_2));
     }
 
     /**
-     * Tests excludePort setter.
+     * Tests suppressHost setter.
      *
      * @throws Exception
      */
     @Test
-    public void testSetExcludePorts() throws Exception {
-        ImmutableSet.Builder<String> builder = ImmutableSet.builder();
-        builder.add(PORT_NAME_3);
-        config.setExcludePorts(builder.build());
-
-        Set<String> excludePorts = config.excludePorts();
-        assertThat(excludePorts.size(), is(1));
-        assertTrue(excludePorts.contains(PORT_NAME_3));
+    public void testSetSuppressHost() throws Exception {
+        ImmutableSet.Builder<ConnectPoint> builder = ImmutableSet.builder();
+        builder.add(PORT_3);
+        config.setSuppressHost(builder.build());
+
+        Set<ConnectPoint> suppressHost = config.suppressHost();
+        assertNotNull("suppressHost should not be null", suppressHost);
+        assertThat(suppressHost.size(), is(1));
+        assertTrue(suppressHost.contains(PORT_3));
     }
 
     private class MockDelegate implements ConfigApplyDelegate {

@@ -35,7 +35,6 @@
     "of:0000000000000002/5" : {
         "interfaces" : [
             {
-                "name" : "2/5/1",
                 "ips" : [ "100.0.0.0/24" ],
                 "vlan" : "-1"
             }
@@ -123,8 +122,11 @@
                     "00:00:00:aa:00:02"
                 ],
                 "vRouterId" : "of:0000000000000002",
-                "excludePorts" : [
-                    "2/5/1"
+                "suppressSubnet" : [
+                    "of:0000000000000002/5"
+                ],
+                "suppressHost" : [
+                    "of:0000000000000002/5"
                 ]
             }
         }