[VOL-4695] Remove only EAPOL flow on port disable
Change-Id: I4b4921eeea7d407fdca4cb2b86b457ec6a93258a
diff --git a/impl/src/main/java/org/opencord/olt/impl/DiscoveredSubscriber.java b/impl/src/main/java/org/opencord/olt/impl/DiscoveredSubscriber.java
index 4280eaf..226f1b5 100644
--- a/impl/src/main/java/org/opencord/olt/impl/DiscoveredSubscriber.java
+++ b/impl/src/main/java/org/opencord/olt/impl/DiscoveredSubscriber.java
@@ -35,6 +35,8 @@
public enum Status {
ADDED,
REMOVED,
+ // Used for the remove subscriber calls from REST/CLI
+ ADMIN_REMOVED,
}
public Port port;
diff --git a/impl/src/main/java/org/opencord/olt/impl/Olt.java b/impl/src/main/java/org/opencord/olt/impl/Olt.java
index ace976b..7b9dc75 100644
--- a/impl/src/main/java/org/opencord/olt/impl/Olt.java
+++ b/impl/src/main/java/org/opencord/olt/impl/Olt.java
@@ -376,7 +376,7 @@
return false;
}
DiscoveredSubscriber sub = new DiscoveredSubscriber(device, port,
- DiscoveredSubscriber.Status.REMOVED, true, si);
+ DiscoveredSubscriber.Status.ADMIN_REMOVED, true, si);
// NOTE we need to keep a list of the subscribers that are provisioned on a port,
// regardless of the flow status
@@ -456,7 +456,7 @@
uniTagInformationList.add(specificService);
si.setUniTagList(uniTagInformationList);
DiscoveredSubscriber sub = new DiscoveredSubscriber(device, port,
- DiscoveredSubscriber.Status.REMOVED, true, si);
+ DiscoveredSubscriber.Status.ADMIN_REMOVED, true, si);
// NOTE we need to keep a list of the subscribers that are provisioned on a port,
// regardless of the flow status
diff --git a/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java b/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
index 10be314..3a2c818 100644
--- a/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
+++ b/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
@@ -525,7 +525,8 @@
// NOTE that we are taking defaultBandwithProfile as a parameter as that can be configured in the Olt component
if (sub.status == DiscoveredSubscriber.Status.ADDED) {
return addSubscriberFlows(sub, defaultBandwidthProfile, multicastServiceName);
- } else if (sub.status == DiscoveredSubscriber.Status.REMOVED) {
+ } else if (sub.status == DiscoveredSubscriber.Status.REMOVED ||
+ sub.status == DiscoveredSubscriber.Status.ADMIN_REMOVED) {
return removeSubscriberFlows(sub, defaultBandwidthProfile, multicastServiceName);
} else {
log.error("don't know how to handle {}", sub);
@@ -596,44 +597,63 @@
portWithName(sub.port));
}
SubscriberAndDeviceInformation si = sub.subscriberAndDeviceInformation;
-
- handleSubscriberDhcpFlows(sub.device.id(), sub.port, FlowOperation.REMOVE, si);
-
- handleSubscriberPppoeFlows(sub.device.id(), sub.port, FlowOperation.REMOVE, sub.subscriberAndDeviceInformation);
-
+ //If the port has been removed the device service will return null, while it will be true if it's just disabled
+ boolean isPortPresent = deviceService.getPort(new ConnectPoint(sub.device.id(), sub.port.number())) != null;
+ if (log.isTraceEnabled()) {
+ log.trace("Port {} present: ", portWithName(sub.port), isPortPresent);
+ }
+ // Always remove the EAPOL flow in case of port disable,remove or subscriber remove.
if (enableEapol) {
// remove the tagged eapol
handleSubscriberEapolFlows(sub, FlowOperation.REMOVE, si);
+ log.info("Removal of eapol flow for subscriber on {} completed", portWithName(sub.port));
+
}
- handleSubscriberDataFlows(sub.device, sub.port, FlowOperation.REMOVE, si, multicastServiceName);
+ // If the port is gone entirely (onu delete) or it's enabled (subscriber remove request) remove all the flows
+ // In the case the port is just disabled we remove only the EAPOL flow because the ONU disable only represents
+ // the UNI side of the ONU going down, either for RG cable detach or for an administrative decision, but the PON
+ // side is still up as nothing changed, so no need to add/remove flows, when and if the UNI comes up
+ // we will re-push the EAPOL flow to require the subscriber to auth again.
+ // When the subscriber is admin removed from REST or CLI we ignore the port status.
+ if (!isPortPresent || sub.port.isEnabled() || sub.status == DiscoveredSubscriber.Status.ADMIN_REMOVED) {
- handleSubscriberIgmpFlows(sub, FlowOperation.REMOVE);
+ handleSubscriberDhcpFlows(sub.device.id(), sub.port, FlowOperation.REMOVE, si);
- if (enableEapol) {
+ handleSubscriberPppoeFlows(sub.device.id(), sub.port, FlowOperation.REMOVE,
+ sub.subscriberAndDeviceInformation);
- // if any of the services still has flows, return false
- Iterator<UniTagInformation> iter = sub.subscriberAndDeviceInformation.uniTagList().iterator();
- while (iter.hasNext()) {
- UniTagInformation entry = iter.next();
- if (areSubscriberFlowsPendingRemoval(sub.port, entry, enableEapol)) {
- log.info("Subscriber {} still have flows on service {}, postpone default EAPOL installation.",
- portWithName(sub.port), entry.getServiceName());
- return false;
+ handleSubscriberDataFlows(sub.device, sub.port, FlowOperation.REMOVE, si, multicastServiceName);
+
+ handleSubscriberIgmpFlows(sub, FlowOperation.REMOVE);
+
+
+ if (enableEapol) {
+
+ // if any of the services still has flows, return false
+ Iterator<UniTagInformation> iter = sub.subscriberAndDeviceInformation.uniTagList().iterator();
+ while (iter.hasNext()) {
+ UniTagInformation entry = iter.next();
+ if (areSubscriberFlowsPendingRemoval(sub.port, entry, enableEapol)) {
+ log.info("Subscriber {} still have flows on service {}, postpone default EAPOL installation.",
+ portWithName(sub.port), entry.getServiceName());
+ return false;
+ }
+ }
+
+ // once the flows are removed add the default one back
+ // (only if the port is ENABLED and still present on the device)
+ if (sub.port.isEnabled() && deviceService.getPort(sub.device.id(), sub.port.number()) != null) {
+
+ // NOTE we remove the subscriber when the port goes down
+ // but in that case we don't need to add default eapol
+ handleEapolFlow(sub, defaultBandwithProfile, defaultBandwithProfile,
+ FlowOperation.ADD, VlanId.vlanId(EAPOL_DEFAULT_VLAN));
}
}
-
- // once the flows are removed add the default one back
- // (only if the port is ENABLED and still present on the device)
- if (sub.port.isEnabled() && deviceService.getPort(sub.device.id(), sub.port.number()) != null) {
-
- // NOTE we remove the subscriber when the port goes down
- // but in that case we don't need to add default eapol
- handleEapolFlow(sub, defaultBandwithProfile, defaultBandwithProfile,
- FlowOperation.ADD, VlanId.vlanId(EAPOL_DEFAULT_VLAN));
- }
+ // FIXME check the return status of the flow and return accordingly
+ log.info("Removal of subscriber on {} completed", portWithName(sub.port));
+ return true;
}
- // FIXME check the return status of the flow and return accordingly
- log.info("Removal of subscriber on {} completed", portWithName(sub.port));
return true;
}
@@ -910,17 +930,7 @@
// TODO verify we need an EAPOL flow for EACH service
AtomicBoolean success = new AtomicBoolean(true);
si.uniTagList().forEach(u -> {
- // we assume that if subscriber flows are installed, tagged EAPOL is installed as well
- boolean hasFlows = hasSubscriberFlows(sub.port, u);
-
- // if the subscriber flows are present the EAPOL flow is present thus we don't need to install it,
- // if the subscriber does not have flows the EAPOL flow is not present thus we don't need to remove it
- if (action == FlowOperation.ADD && hasFlows ||
- action == FlowOperation.REMOVE && !hasFlows) {
- log.debug("Not {} EAPOL on {}({}) as subscriber flow status is {}", flowOpToString(action),
- portWithName(sub.port), u.getServiceName(), hasFlows);
- return;
- }
+ //Always act on the eapol flow
log.info("{} EAPOL flows for subscriber {} on {} and service {}",
flowOpToString(action), si.id(), portWithName(sub.port), u.getServiceName());
if (!handleEapolFlow(sub, u.getUpstreamBandwidthProfile(),
@@ -1672,7 +1682,7 @@
if (log.isTraceEnabled()) {
log.trace("update subscriberEapolStatus {} on {}", status, sk);
}
- updateConnectPointStatus(sk, null, status, null, status, null);
+ updateConnectPointStatus(sk, null, status, null, null, null);
} else if (OltFlowServiceUtils.isDhcpFlow(flowRule)) {
ServiceKey sk = getSubscriberKeyFromFlowRule(flowRule, port);
if (sk == null) {
diff --git a/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java b/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
index 1525bcd..4196f57 100644
--- a/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
+++ b/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
@@ -113,6 +113,8 @@
public static final String PORT_3 = "port-3";
private OltFlowService component;
private OltFlowService oltFlowService;
+
+ private DeviceService deviceService;
OltFlowService.InternalFlowListener internalFlowListener;
private final ApplicationId testAppId = new DefaultApplicationId(1, "org.opencord.olt.test");
private final short eapolDefaultVlan = 4091;
@@ -192,6 +194,7 @@
internalFlowListener = spy(component.internalFlowListener);
oltFlowService = spy(component);
+ deviceService = Mockito.mock(DeviceService.class);
}
@After
@@ -908,6 +911,7 @@
// first test that when we remove the EAPOL flow we return false so that the
// subscriber is not removed from the queue
+ doReturn(null).when(deviceService).getPort(any());
doReturn(true).when(oltFlowService).areSubscriberFlowsPendingRemoval(any(), any(), eq(true));
boolean res = oltFlowService.removeSubscriberFlows(sub, DEFAULT_BP_ID_DEFAULT, DEFAULT_MCAST_SERVICE_NAME);
verify(oltFlowService, times(1))