ACL: Multicast Aap Port and Recover acl-interface

Below ACL issues are fixed:

(a) Issue: Port having Multicast AllowedAddressPair-Mac same as other
           AAP-mac was filtered out from being added for DHCP client traffic flows.

    Fix: Filtering of AAPs required only in case of Port-Update.
         added checks to remove Multicast AAPs before finding the duplicate mac.

(b) Issue: srm:recover-instance-ACL-INTERFACE removes the flows when the
           command is executed again`

    Fix : Removing redundant interface remove during recovery. Remove
          method is redundant, since we only need to add flows for AclInterface.
          Having remove is causing cache update issue, where-in object in
          AclInterfaceCache is getting removed after first add is successful. loss
          of interfaceState information from cache causing add to fail subsequent
          invocations.`

Change-Id: Ib966034fc395b3b501e1756fa6fbade0be9f0f13
Signed-off-by: kiranvasudeva <kirankumar.v@altencalsoftlabs.com>

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
index 8dd955ced5450b22845348d382c994e982f47902..3b78577f1c6f8cc046c461b53478508d0d3cdd2d 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
@@ -204,7 +204,7 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
                     NwConstants.ADD_FLOW);
         }
         if (portAfter.getSubnetInfo() != null && portBefore.getSubnetInfo() == null) {
-            programBroadcastRules(addFlowEntries, portAfter, NwConstants.ADD_FLOW);
+            programBroadcastRules(addFlowEntries, portAfter, Action.UPDATE, NwConstants.ADD_FLOW);
         }
         handleSubnetChange(portBefore, portAfter, addFlowEntries, deleteFlowEntries);
 
@@ -561,7 +561,8 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
      * @param port the Acl Interface port
      * @param addOrRemove whether to delete or add flow
      */
-    protected abstract void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, int addOrRemove);
+    protected abstract void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, Action action,
+            int addOrRemove);
 
     /**
      * Programs broadcast rules.

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/EgressAclServiceImpl.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/EgressAclServiceImpl.java
index cf582de0bdc729ee25da8fa1fe1b63800ec474f5..039a65a5a59d175724943c1098a350541bf0098d 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/EgressAclServiceImpl.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/EgressAclServiceImpl.java
@@ -129,10 +129,11 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
             egressAclIcmpv6AllowedList(flowEntries, dpid, lportTag, addOrRemove);
         }
         List<AllowedAddressPairs> filteredAAPs = AclServiceUtils.excludeMulticastAAPs(allowedAddresses);
-        programL2BroadcastAllowRule(flowEntries, port, filteredAAPs, addOrRemove);
-
-        egressAclDhcpAllowClientTraffic(flowEntries, port, filteredAAPs, lportTag, addOrRemove);
-        egressAclDhcpv6AllowClientTraffic(flowEntries, port, filteredAAPs, lportTag, addOrRemove);
+        if (!hasDuplicateMac(port.getAllowedAddressPairs(), filteredAAPs, action)) {
+            programL2BroadcastAllowRule(flowEntries, port, filteredAAPs, addOrRemove);
+            egressAclDhcpAllowClientTraffic(flowEntries, port, filteredAAPs, lportTag, addOrRemove);
+            egressAclDhcpv6AllowClientTraffic(flowEntries, port, filteredAAPs, lportTag, addOrRemove);
+        }
         programArpRule(flowEntries, dpid, filteredAAPs, lportTag, addOrRemove);
     }
 
@@ -222,10 +223,6 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
      */
     private void egressAclDhcpAllowClientTraffic(List<FlowEntity> flowEntries, AclInterface port,
             List<AllowedAddressPairs> allowedAddresses, int lportTag, int addOrRemove) {
-        // if there is a duplicate mac with different aap, do not delete the Dhcp Allow rule.
-        if (hasDuplicateMac(port.getAllowedAddressPairs(), allowedAddresses)) {
-            return;
-        }
         BigInteger dpId = port.getDpId();
         List<InstructionInfo> instructions = getDispatcherTableResubmitInstructions();
         for (AllowedAddressPairs aap : allowedAddresses) {
@@ -256,10 +253,6 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
      */
     private void egressAclDhcpv6AllowClientTraffic(List<FlowEntity> flowEntries, AclInterface port,
             List<AllowedAddressPairs> allowedAddresses, int lportTag, int addOrRemove) {
-        // if there is a duplicate mac with different aap, do not delete the Dhcp Allow rule.
-        if (hasDuplicateMac(port.getAllowedAddressPairs(), allowedAddresses)) {
-            return;
-        }
         BigInteger dpId = port.getDpId();
         List<InstructionInfo> instructions = getDispatcherTableResubmitInstructions();
         for (AllowedAddressPairs aap : allowedAddresses) {
@@ -330,9 +323,13 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
      * @param addOrRemove whether to delete or add flow
      */
     @Override
-    protected void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, int addOrRemove) {
-        programL2BroadcastAllowRule(flowEntries, port,
-                AclServiceUtils.excludeMulticastAAPs(port.getAllowedAddressPairs()), addOrRemove);
+    protected void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, Action action,
+             int addOrRemove) {
+        List<AllowedAddressPairs> allowedAddressPairs = port.getAllowedAddressPairs();
+        List<AllowedAddressPairs> filteredAAPs = AclServiceUtils.excludeMulticastAAPs(allowedAddressPairs);
+        if (!hasDuplicateMac(allowedAddressPairs, filteredAAPs, action)) {
+            programL2BroadcastAllowRule(flowEntries, port, filteredAAPs, addOrRemove);
+        }
     }
 
     /**
@@ -358,10 +355,6 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
      */
     private void programL2BroadcastAllowRule(List<FlowEntity> flowEntries, AclInterface port,
             List<AllowedAddressPairs> filteredAAPs, int addOrRemove) {
-        // if there is a duplicate mac with different aap, do not delete the Broadcast rule.
-        if (hasDuplicateMac(port.getAllowedAddressPairs(), filteredAAPs)) {
-            return;
-        }
         BigInteger dpId = port.getDpId();
         int lportTag = port.getLPortTag();
         Set<MacAddress> macs = filteredAAPs.stream().map(AllowedAddressPairs::getMacAddress)
@@ -381,18 +374,20 @@ public class EgressAclServiceImpl extends AbstractAclServiceImpl {
     }
 
     private boolean hasDuplicateMac(List<AllowedAddressPairs> allowedAddresses,
-            List<AllowedAddressPairs> filteredAAPs) {
-        // Do not proceed further if VM delete or Port down event.
-        if (allowedAddresses.size() == filteredAAPs.size()) {
+            List<AllowedAddressPairs> filteredAAPs, Action action) {
+        // Do not proceed further if Port Up OR down event.
+        if (action != Action.UPDATE) {
             return false;
         }
-        //exclude filteredAAP entries from port's AAP's before comparison
-        List<AllowedAddressPairs> filteredAllowedAddressed = allowedAddresses.stream().filter(
+        // exclude multicastAAPs from PortBefore/PortAfter AAPs
+        List<AllowedAddressPairs> excludeMulticastAAPs = AclServiceUtils.excludeMulticastAAPs(allowedAddresses);
+        // GET Delta of ADDED/DELETED aaps with PortBefore/PortAfter-AAPs
+        List<AllowedAddressPairs> filteredAllowedAddresses = excludeMulticastAAPs.stream().filter(
             aap -> !filteredAAPs.contains(aap)).collect(Collectors.toList());
-        Set<MacAddress> macs = filteredAAPs.stream().map(AllowedAddressPairs::getMacAddress)
-                .collect(Collectors.toSet());
-        List<AllowedAddressPairs> aapWithDuplicateMac = filteredAllowedAddressed.stream()
+        Set<MacAddress> macs = filteredAAPs.stream().map(aap -> aap.getMacAddress()).collect(Collectors.toSet());
+        List<AllowedAddressPairs> aapWithDuplicateMac = filteredAllowedAddresses.stream()
             .filter(entry -> macs.contains(entry.getMacAddress())).collect(Collectors.toList());
+
         return !aapWithDuplicateMac.isEmpty();
     }
 

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/IngressAclServiceImpl.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/IngressAclServiceImpl.java
index 669f8c422ba98e334381e64dd9dba61c3fc27f4e..be8c994e2e8e7e46776f07dcd98abd34a03faad4 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/IngressAclServiceImpl.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/IngressAclServiceImpl.java
@@ -333,7 +333,8 @@ public class IngressAclServiceImpl extends AbstractAclServiceImpl {
      * @param addOrRemove whether to delete or add flow
      */
     @Override
-    protected void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, int addOrRemove) {
+    protected void programBroadcastRules(List<FlowEntity> flowEntries, AclInterface port, Action action,
+            int addOrRemove) {
         programIpv4BroadcastRule(flowEntries, port, port.getSubnetInfo(), addOrRemove);
     }
 

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/recovery/AclInterfaceRecoveryHandler.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/recovery/AclInterfaceRecoveryHandler.java
index fad692ffe838d4e9863d58421069e217a16ebe28..3fdeccfa88a49e6d9b036cc91d48cf4c5ab0e243 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/recovery/AclInterfaceRecoveryHandler.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/recovery/AclInterfaceRecoveryHandler.java
@@ -46,7 +46,6 @@ public class AclInterfaceRecoveryHandler implements ServiceRecoveryInterface {
         if (interfaceOp.isPresent()) {
             Interface aclInterface = interfaceOp.get();
             InstanceIdentifier<Interface> interfaceIdentifier = AclServiceUtils.getInterfaceIdentifier(entityId);
-            aclInterfaceListener.remove(interfaceIdentifier, aclInterface);
             aclInterfaceListener.add(interfaceIdentifier, aclInterface);
         } else {
             LOG.warn("{} is not valid ACL interface", entityId);