[CORD-1898] Fix ignore rule bug of DHCP relay
Change-Id: I773f0f2e640d3b06fcbe326877f4ceb0e473cd94
diff --git a/apps/dhcprelay/src/main/java/org/onosproject/dhcprelay/DhcpRelayManager.java b/apps/dhcprelay/src/main/java/org/onosproject/dhcprelay/DhcpRelayManager.java
index 98f6d3c..2d76639 100644
--- a/apps/dhcprelay/src/main/java/org/onosproject/dhcprelay/DhcpRelayManager.java
+++ b/apps/dhcprelay/src/main/java/org/onosproject/dhcprelay/DhcpRelayManager.java
@@ -23,6 +23,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.apache.felix.scr.annotations.Activate;
@@ -58,6 +59,7 @@
import org.onosproject.dhcprelay.store.DhcpRecord;
import org.onosproject.dhcprelay.store.DhcpRelayStore;
import org.onosproject.net.ConnectPoint;
+import org.onosproject.net.Device;
import org.onosproject.net.DeviceId;
import org.onosproject.net.Host;
import org.onosproject.net.HostId;
@@ -70,6 +72,10 @@
import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.TrafficSelector;
import org.onosproject.net.flow.TrafficTreatment;
+import org.onosproject.net.behaviour.Pipeliner;
+import org.onosproject.net.device.DeviceEvent;
+import org.onosproject.net.device.DeviceListener;
+import org.onosproject.net.device.DeviceService;
import org.onosproject.net.flow.criteria.Criterion;
import org.onosproject.net.flow.criteria.UdpPortCriterion;
import org.onosproject.net.flowobjective.DefaultForwardingObjective;
@@ -98,6 +104,9 @@
import com.google.common.collect.Multimap;
import static org.onosproject.net.config.basics.SubjectFactories.APP_SUBJECT_FACTORY;
+import static org.onosproject.net.flowobjective.Objective.Operation.ADD;
+import static org.onosproject.net.flowobjective.Objective.Operation.REMOVE;
+
/**
* DHCP Relay Agent Application Component.
*/
@@ -197,19 +206,23 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected FlowObjectiveService flowObjectiveService;
+ @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+ protected DeviceService deviceService;
+
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY,
- target = "(version=4)")
+ target = "(version=4)")
protected DhcpHandler v4Handler;
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY,
- target = "(version=6)")
+ target = "(version=6)")
protected DhcpHandler v6Handler;
@Property(name = "arpEnabled", boolValue = true,
- label = "Enable Address resolution protocol")
+ label = "Enable Address resolution protocol")
protected boolean arpEnabled = true;
protected Multimap<DeviceId, VlanId> ignoredVlans = HashMultimap.create();
+ protected DeviceListener deviceListener = new InternalDeviceListener();
private DhcpRelayPacketProcessor dhcpRelayPacketProcessor = new DhcpRelayPacketProcessor();
private ApplicationId appId;
@@ -237,6 +250,8 @@
compCfgService.preSetProperty(ROUTE_STORE_IMPL,
"distributed", Boolean.TRUE.toString());
compCfgService.registerProperties(getClass());
+
+ deviceService.addListener(deviceListener);
log.info("DHCP-RELAY Started");
}
@@ -247,8 +262,8 @@
packetService.removeProcessor(dhcpRelayPacketProcessor);
cancelDhcpPackets();
cancelArpPackets();
-
compCfgService.unregisterProperties(getClass(), false);
+ deviceService.removeListener(deviceListener);
log.info("DHCP-RELAY Stopped");
}
@@ -309,7 +324,7 @@
v6Handler.setDefaultDhcpServerConfigs(defaultConfig.dhcpServerConfigs());
}
if (config instanceof IgnoreDhcpConfig) {
- addIgnoreVlanRules((IgnoreDhcpConfig) config);
+ updateIgnoreVlanRules((IgnoreDhcpConfig) config);
}
}
@@ -322,93 +337,103 @@
v6Handler.setDefaultDhcpServerConfigs(Collections.emptyList());
}
if (config instanceof IgnoreDhcpConfig) {
- ignoredVlans.forEach(this::removeIgnoreVlanRule);
- ignoredVlans.clear();
+ ignoredVlans.forEach(((deviceId, vlanId) -> {
+ processIgnoreVlanRule(deviceId, vlanId, REMOVE);
+ }));
}
}
- private void addIgnoreVlanRules(IgnoreDhcpConfig config) {
+ private void updateIgnoreVlanRules(IgnoreDhcpConfig config) {
config.ignoredVlans().forEach((deviceId, vlanId) -> {
if (ignoredVlans.get(deviceId).contains(vlanId)) {
// don't need to process if it already ignored
return;
}
- installIgnoreVlanRule(deviceId, vlanId);
- ignoredVlans.put(deviceId, vlanId);
+ processIgnoreVlanRule(deviceId, vlanId, ADD);
});
- Multimap<DeviceId, VlanId> removedVlans = HashMultimap.create();
ignoredVlans.forEach((deviceId, vlanId) -> {
if (!config.ignoredVlans().get(deviceId).contains(vlanId)) {
// not contains in new config, remove it
- removeIgnoreVlanRule(deviceId, vlanId);
- removedVlans.put(deviceId, vlanId);
-
+ processIgnoreVlanRule(deviceId, vlanId, REMOVE);
}
});
- removedVlans.forEach(ignoredVlans::remove);
}
- private void installIgnoreVlanRule(DeviceId deviceId, VlanId vlanId) {
+ /**
+ * Process the ignore rules.
+ *
+ * @param deviceId the device id
+ * @param vlanId the vlan to be ignored
+ * @param op the operation, ADD to install; REMOVE to uninstall rules
+ */
+ private void processIgnoreVlanRule(DeviceId deviceId, VlanId vlanId, Objective.Operation op) {
TrafficTreatment dropTreatment = DefaultTrafficTreatment.emptyTreatment();
dropTreatment.clearedDeferred();
+ AtomicInteger installedCount = new AtomicInteger(DHCP_SELECTORS.size());
DHCP_SELECTORS.forEach(trafficSelector -> {
UdpPortCriterion udpDst = (UdpPortCriterion) trafficSelector.getCriterion(Criterion.Type.UDP_DST);
+ int udpDstPort = udpDst.udpPort().toInt();
TrafficSelector selector = DefaultTrafficSelector.builder(trafficSelector)
.matchVlanId(vlanId)
.build();
- ForwardingObjective fwd = DefaultForwardingObjective.builder()
+ ForwardingObjective.Builder builder = DefaultForwardingObjective.builder()
.withFlag(ForwardingObjective.Flag.VERSATILE)
.withSelector(selector)
.withPriority(IGNORE_CONTROL_PRIORITY)
.withTreatment(dropTreatment)
- .fromApp(appId)
- .add(new ObjectiveContext() {
- @Override
- public void onSuccess(Objective objective) {
- log.info("Vlan id {} from device {} ignored (UDP port {})",
- vlanId, deviceId, udpDst.udpPort().toInt());
- }
+ .fromApp(appId);
- @Override
- public void onError(Objective objective, ObjectiveError error) {
- log.warn("Can't ignore vlan id {} from device {} due to {}",
- vlanId, deviceId, error);
- }
- });
- flowObjectiveService.apply(deviceId, fwd);
- });
- }
- private void removeIgnoreVlanRule(DeviceId deviceId, VlanId vlanId) {
- TrafficTreatment dropTreatment = DefaultTrafficTreatment.emptyTreatment();
- dropTreatment.clearedDeferred();
- DHCP_SELECTORS.forEach(trafficSelector -> {
- UdpPortCriterion udpDst = (UdpPortCriterion) trafficSelector.getCriterion(Criterion.Type.UDP_DST);
- TrafficSelector selector = DefaultTrafficSelector.builder(trafficSelector)
- .matchVlanId(vlanId)
- .build();
+ ObjectiveContext objectiveContext = new ObjectiveContext() {
+ @Override
+ public void onSuccess(Objective objective) {
+ log.info("Ignore rule {} (Vlan id {}, device {}, UDP dst {})",
+ op, vlanId, deviceId, udpDstPort);
+ int countDown = installedCount.decrementAndGet();
+ if (countDown != 0) {
+ return;
+ }
+ switch (op) {
+ case ADD:
- ForwardingObjective fwd = DefaultForwardingObjective.builder()
- .withFlag(ForwardingObjective.Flag.VERSATILE)
- .withSelector(selector)
- .withPriority(IGNORE_CONTROL_PRIORITY)
- .withTreatment(dropTreatment)
- .fromApp(appId)
- .remove(new ObjectiveContext() {
- @Override
- public void onSuccess(Objective objective) {
- log.info("Vlan id {} from device {} ignore rule removed (UDP port {})",
- vlanId, deviceId, udpDst.udpPort().toInt());
- }
+ ignoredVlans.put(deviceId, vlanId);
+ break;
+ case REMOVE:
+ ignoredVlans.remove(deviceId, vlanId);
+ break;
+ default:
+ log.warn("Unsupported objective operation {}", op);
+ break;
+ }
+ }
- @Override
- public void onError(Objective objective, ObjectiveError error) {
- log.warn("Can't remove ignore rule of vlan id {} from device {} due to {}",
- vlanId, deviceId, error);
- }
- });
+ @Override
+ public void onError(Objective objective, ObjectiveError error) {
+ log.warn("Can't {} ignore rule (vlan id {}, udp dst {}, device {}) due to {}",
+ op, vlanId, udpDstPort, deviceId, error);
+ }
+ };
+
+ ForwardingObjective fwd;
+ switch (op) {
+ case ADD:
+ fwd = builder.add(objectiveContext);
+ break;
+ case REMOVE:
+ fwd = builder.remove(objectiveContext);
+ break;
+ default:
+ log.warn("Unsupported objective operation {}", op);
+ return;
+ }
+
+ Device device = deviceService.getDevice(deviceId);
+ if (device == null || !device.is(Pipeliner.class)) {
+ log.warn("Device {} is not available now, wait until device is available", deviceId);
+ return;
+ }
flowObjectiveService.apply(deviceId, fwd);
});
}
@@ -620,4 +645,41 @@
}
}
+
+ private class InternalDeviceListener implements DeviceListener {
+
+ @Override
+ public void event(DeviceEvent event) {
+ Device device = event.subject();
+ switch (event.type()) {
+ case DEVICE_ADDED:
+ deviceAdd(device.id());
+ break;
+ case DEVICE_REMOVED:
+ ignoredVlans.removeAll(device.id());
+ break;
+ case DEVICE_AVAILABILITY_CHANGED:
+ deviceAvailabilityChanged(device);
+
+ default:
+ break;
+ }
+ }
+
+ private void deviceAvailabilityChanged(Device device) {
+ if (deviceService.isAvailable(device.id())) {
+ deviceAdd(device.id());
+ } else {
+ ignoredVlans.removeAll(device.id());
+ }
+ }
+
+ private void deviceAdd(DeviceId deviceId) {
+ IgnoreDhcpConfig config = cfgService.getConfig(appId, IgnoreDhcpConfig.class);
+ Collection<VlanId> vlanIds = config.ignoredVlans().get(deviceId);
+ vlanIds.forEach(vlanId -> {
+ processIgnoreVlanRule(deviceId, vlanId, ADD);
+ });
+ }
+ }
}
diff --git a/apps/dhcprelay/src/test/java/org/onosproject/dhcprelay/DhcpRelayManagerTest.java b/apps/dhcprelay/src/test/java/org/onosproject/dhcprelay/DhcpRelayManagerTest.java
index 3d89d76..a179e63 100644
--- a/apps/dhcprelay/src/test/java/org/onosproject/dhcprelay/DhcpRelayManagerTest.java
+++ b/apps/dhcprelay/src/test/java/org/onosproject/dhcprelay/DhcpRelayManagerTest.java
@@ -64,7 +64,11 @@
import org.onosproject.dhcprelay.store.DhcpRecord;
import org.onosproject.dhcprelay.store.DhcpRelayStore;
import org.onosproject.dhcprelay.store.DhcpRelayStoreEvent;
+import org.onosproject.net.Device;
import org.onosproject.net.DeviceId;
+import org.onosproject.net.behaviour.Pipeliner;
+import org.onosproject.net.device.DeviceEvent;
+import org.onosproject.net.device.DeviceService;
import org.onosproject.net.flow.DefaultTrafficSelector;
import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.TrafficSelector;
@@ -100,6 +104,7 @@
import org.onlab.packet.DHCP6;
import org.onlab.packet.IPv6;
+import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.Dictionary;
@@ -113,6 +118,7 @@
import static org.easymock.EasyMock.*;
import static org.junit.Assert.*;
import static org.onosproject.dhcprelay.DhcpRelayManager.DHCP_RELAY_APP;
+import static org.onosproject.dhcprelay.DhcpRelayManager.DHCP_SELECTORS;
public class DhcpRelayManagerTest {
private static final String CONFIG_FILE_PATH = "dhcp-relay.json";
@@ -239,6 +245,14 @@
packetService = new MockPacketService();
manager.packetService = packetService;
manager.compCfgService = createNiceMock(ComponentConfigService.class);
+ manager.deviceService = createNiceMock(DeviceService.class);
+
+ Device device = createNiceMock(Device.class);
+ expect(device.is(Pipeliner.class)).andReturn(true).anyTimes();
+
+ expect(manager.deviceService.getDevice(DEV_1_ID)).andReturn(device).anyTimes();
+ expect(manager.deviceService.getDevice(DEV_2_ID)).andReturn(device).anyTimes();
+ replay(manager.deviceService, device);
mockHostStore = new MockHostStore();
mockRouteStore = new MockRouteStore();
@@ -248,6 +262,8 @@
manager.interfaceService = new MockInterfaceService();
manager.flowObjectiveService = EasyMock.niceMock(FlowObjectiveService.class);
+
+
Dhcp4HandlerImpl v4Handler = new Dhcp4HandlerImpl();
v4Handler.dhcpRelayStore = mockDhcpRelayStore;
v4Handler.hostService = manager.hostService;
@@ -400,10 +416,10 @@
Capture<Objective> capturedFromDev1 = newCapture(CaptureType.ALL);
manager.flowObjectiveService.apply(eq(DEV_1_ID), capture(capturedFromDev1));
- expectLastCall().times(2);
+ expectLastCall().times(DHCP_SELECTORS.size());
Capture<Objective> capturedFromDev2 = newCapture(CaptureType.ALL);
manager.flowObjectiveService.apply(eq(DEV_2_ID), capture(capturedFromDev2));
- expectLastCall().times(2);
+ expectLastCall().times(DHCP_SELECTORS.size());
replay(manager.flowObjectiveService);
manager.updateConfig(config);
verify(manager.flowObjectiveService);
@@ -427,8 +443,11 @@
assertEquals(IGNORE_CONTROL_PRIORITY, fwd.priority());
assertEquals(ForwardingObjective.Flag.VERSATILE, fwd.flag());
assertEquals(Objective.Operation.ADD, fwd.op());
+ fwd.context().ifPresent(ctx -> {
+ ctx.onSuccess(fwd);
+ });
}
-
+ objectivesFromDev2.forEach(obj -> obj.context().ifPresent(ctx -> ctx.onSuccess(obj)));
assertEquals(2, manager.ignoredVlans.size());
}
@@ -443,10 +462,10 @@
Capture<Objective> capturedFromDev1 = newCapture(CaptureType.ALL);
manager.flowObjectiveService.apply(eq(DEV_1_ID), capture(capturedFromDev1));
- expectLastCall().times(2);
+ expectLastCall().times(DHCP_SELECTORS.size());
Capture<Objective> capturedFromDev2 = newCapture(CaptureType.ALL);
manager.flowObjectiveService.apply(eq(DEV_2_ID), capture(capturedFromDev2));
- expectLastCall().times(2);
+ expectLastCall().times(DHCP_SELECTORS.size());
replay(manager.flowObjectiveService);
manager.removeConfig(config);
verify(manager.flowObjectiveService);
@@ -470,11 +489,74 @@
assertEquals(IGNORE_CONTROL_PRIORITY, fwd.priority());
assertEquals(ForwardingObjective.Flag.VERSATILE, fwd.flag());
assertEquals(Objective.Operation.REMOVE, fwd.op());
+ fwd.context().ifPresent(ctx -> {
+ ctx.onSuccess(fwd);
+ });
}
+ objectivesFromDev2.forEach(obj -> obj.context().ifPresent(ctx -> ctx.onSuccess(obj)));
assertEquals(0, manager.ignoredVlans.size());
}
/**
+ * Should ignore ignore rules installation when device not available.
+ */
+ @Test
+ public void testIgnoreUnknownDevice() throws IOException {
+ reset(manager.deviceService);
+ Device device = createNiceMock(Device.class);
+ expect(device.is(Pipeliner.class)).andReturn(true).anyTimes();
+
+ expect(manager.deviceService.getDevice(DEV_1_ID)).andReturn(device).anyTimes();
+ expect(manager.deviceService.getDevice(DEV_2_ID)).andReturn(null).anyTimes();
+
+ ObjectMapper om = new ObjectMapper();
+ JsonNode json = om.readTree(Resources.getResource(CONFIG_FILE_PATH));
+ IgnoreDhcpConfig config = new IgnoreDhcpConfig();
+ json = json.path("apps").path(DHCP_RELAY_APP).path(IgnoreDhcpConfig.KEY);
+ config.init(APP_ID, IgnoreDhcpConfig.KEY, json, om, null);
+
+ Capture<Objective> capturedFromDev1 = newCapture(CaptureType.ALL);
+ manager.flowObjectiveService.apply(eq(DEV_1_ID), capture(capturedFromDev1));
+ expectLastCall().times(DHCP_SELECTORS.size());
+ replay(manager.flowObjectiveService, manager.deviceService, device);
+
+ manager.updateConfig(config);
+ capturedFromDev1.getValues().forEach(obj -> obj.context().ifPresent(ctx -> ctx.onSuccess(obj)));
+
+ assertEquals(1, manager.ignoredVlans.size());
+ }
+
+ /**
+ * Should try install ignore rules when device comes up.
+ */
+ @Test
+ public void testInstallIgnoreRuleWhenDeviceComesUp() throws IOException {
+ ObjectMapper om = new ObjectMapper();
+ JsonNode json = om.readTree(Resources.getResource(CONFIG_FILE_PATH));
+ IgnoreDhcpConfig config = new IgnoreDhcpConfig();
+ json = json.path("apps").path(DHCP_RELAY_APP).path(IgnoreDhcpConfig.KEY);
+ config.init(APP_ID, IgnoreDhcpConfig.KEY, json, om, null);
+
+ reset(manager.cfgService, manager.flowObjectiveService, manager.deviceService);
+ expect(manager.cfgService.getConfig(APP_ID, IgnoreDhcpConfig.class))
+ .andReturn(config).anyTimes();
+
+ Device device = createNiceMock(Device.class);
+ expect(device.is(Pipeliner.class)).andReturn(true).anyTimes();
+ expect(device.id()).andReturn(DEV_1_ID).anyTimes();
+ expect(manager.deviceService.getDevice(DEV_1_ID)).andReturn(device).anyTimes();
+ DeviceEvent event = new DeviceEvent(DeviceEvent.Type.DEVICE_ADDED, device);
+ Capture<Objective> capturedFromDev1 = newCapture(CaptureType.ALL);
+ manager.flowObjectiveService.apply(eq(DEV_1_ID), capture(capturedFromDev1));
+ expectLastCall().times(DHCP_SELECTORS.size());
+ replay(manager.cfgService, manager.flowObjectiveService, manager.deviceService, device);
+ assertEquals(0, manager.ignoredVlans.size());
+ manager.deviceListener.event(event);
+ capturedFromDev1.getValues().forEach(obj -> obj.context().ifPresent(ctx -> ctx.onSuccess(obj)));
+ assertEquals(1, manager.ignoredVlans.size());
+ }
+
+ /**
* Relay a DHCP6 packet without relay option
* Note: Should add new host to host store after dhcp ack.
*/