Improve SegmentRoutingAppConfig
- Separate host and subnet suppression
- Use port instead of interface to specify which SR should ignore
Change-Id: Ie6491950cddf0860924565f081504b4f4d788179
diff --git a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index f77804f..da8faa7 100644
--- a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -473,7 +473,7 @@
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)
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index ac9b291..ec2ea06 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -991,7 +991,7 @@
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 @@
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 @@
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 @@
});
}
- if (!deviceConfiguration.excludedPorts()
+ if (!deviceConfiguration.suppressHost()
.contains(new ConnectPoint(newDeviceId, newPort))) {
// Populate new bridging table entry
ForwardingObjective.Builder newFob =
@@ -1099,7 +1099,7 @@
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 @@
});
}
- if (!deviceConfiguration.excludedPorts()
+ if (!deviceConfiguration.suppressHost()
.contains(new ConnectPoint(newDeviceId, newPort))) {
// Populate new IP table entry
newIps.forEach(ip -> {
diff --git a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
index 932b8ad..1beed32 100644
--- a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
+++ b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
@@ -18,7 +18,6 @@
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.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 @@
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 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 @@
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 @@
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 @@
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 @@
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();
}
}
diff --git a/src/main/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfig.java b/src/main/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfig.java
index 62b31b9..2932800 100644
--- a/src/main/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfig.java
+++ b/src/main/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfig.java
@@ -21,8 +21,11 @@
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 @@
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 @@
/**
* 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 @@
}
/**
- * 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 that are ignored by SegmentRouting.
+ * Sets names of ports to which SegmentRouting does not push subnet rules.
*
- * @param excludePorts names of ports that are ignored by SegmentRouting
+ * @param suppressSubnet names of ports to which SegmentRouting does not push
+ * subnet rules
* @return this {@link SegmentRoutingAppConfig}
*/
- public SegmentRoutingAppConfig setExcludePorts(Set<String> excludePorts) {
- if (excludePorts == null) {
- object.remove(EXCLUDE_PORTS);
+ public SegmentRoutingAppConfig setSuppressSubnet(Set<ConnectPoint> suppressSubnet) {
+ if (suppressSubnet == null) {
+ object.remove(SUPPRESS_SUBNET);
} else {
ArrayNode arrayNode = mapper.createArrayNode();
- excludePorts.forEach(portName -> {
- arrayNode.add(portName);
+ suppressSubnet.forEach(connectPoint -> {
+ arrayNode.add(connectPoint.deviceId() + "/" + connectPoint.port());
});
- object.set(EXCLUDE_PORTS, arrayNode);
+ 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 to which SegmentRouting does not push host rules.
+ *
+ * @param suppressHost names of ports to which SegmentRouting does not push
+ * host rules
+ * @return this {@link SegmentRoutingAppConfig}
+ */
+ public SegmentRoutingAppConfig setSuppressHost(Set<ConnectPoint> suppressHost) {
+ if (suppressHost == null) {
+ object.remove(SUPPRESS_HOST);
+ } else {
+ ArrayNode arrayNode = mapper.createArrayNode();
+ suppressHost.forEach(connectPoint -> {
+ arrayNode.add(connectPoint.deviceId() + "/" + connectPoint.port());
+ });
+ object.set(SUPPRESS_HOST, arrayNode);
}
return this;
}
@@ -170,7 +229,9 @@
public String toString() {
return toStringHelper(this)
.add("vRouterMacs", vRouterMacs())
- .add("excludePorts", excludePorts())
+ .add("vRouterId", vRouterId())
+ .add("suppressSubnet", suppressSubnet())
+ .add("suppressHost", suppressHost())
.toString();
}
}
diff --git a/src/test/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfigTest.java b/src/test/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfigTest.java
index a7799a0..a43c2ba 100644
--- a/src/test/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfigTest.java
+++ b/src/test/java/org/onosproject/segmentrouting/config/SegmentRoutingAppConfigTest.java
@@ -24,16 +24,17 @@
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 @@
" \"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 @@
* @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 @@
* @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 @@
*/
@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 @@
@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 excludePort getter.
+ * Tests suppressSubnet 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 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 excludePort setter.
+ * Tests suppressSubnet setter.
*
* @throws Exception
*/
@Test
- public void testSetExcludePorts() throws Exception {
- ImmutableSet.Builder<String> builder = ImmutableSet.builder();
- builder.add(PORT_NAME_3);
- config.setExcludePorts(builder.build());
+ public void testSetSuppressSubnet() throws Exception {
+ ImmutableSet.Builder<ConnectPoint> builder = ImmutableSet.builder();
+ builder.add(PORT_3);
+ config.setSuppressSubnet(builder.build());
- Set<String> excludePorts = config.excludePorts();
- assertThat(excludePorts.size(), is(1));
- assertTrue(excludePorts.contains(PORT_NAME_3));
+ Set<ConnectPoint> suppressSubnet = config.suppressSubnet();
+ assertNotNull("suppressSubnet should not be null", suppressSubnet);
+ assertThat(suppressSubnet.size(), is(1));
+ assertTrue(suppressSubnet.contains(PORT_3));
+ }
+
+ /**
+ * Tests suppressHost getter.
+ *
+ * @throws Exception
+ */
+ @Test
+ 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 suppressHost setter.
+ *
+ * @throws Exception
+ */
+ @Test
+ 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 {