Port update with no security groups

(a) Cache was updated after added ACLs were processed during port update.
This was casuing problems when port was updated from no security groups to
valid security groups; required flows were not getting added in remote
group tables. This issue is fixed by updating cache before processing of
added ACLs.

(b) Port update to no security groups was leaving stale flows in remote
group tables sometimes. This was because of cache update happening in
different thread than the thread which was processing deleted ACLs. This
issue is fixed by passing copy of required unchanged cache to the thread
which is processin deleted ACLs.

(c) Remote group flow was getting deleted even though it was part of other
rules (of the same or different SG) of the port. Validation is introduced
to check this condition now before remote group flow is deleted.

Change-Id: I15022652ddb36ade060c3c26149c5d68594e1593
Signed-off-by: Shashidhar Raja <shashidharr@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 42fc2bd26043cb294ebcf64323883ed25bde9626..b579086e2e38a08d3c5e4d05815f172ab93e3f8b 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
@@ -21,6 +21,7 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.SortedSet;
+import java.util.concurrent.ConcurrentMap;
 import java.util.stream.Collectors;
 import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
@@ -136,7 +137,7 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
             }
             programAclWithAllowedAddress(flowEntries, port, port.getAllowedAddressPairs(),
                     Action.ADD, NwConstants.ADD_FLOW);
-            updateRemoteAclFilterTable(flowEntries, port, NwConstants.ADD_FLOW);
+            updateRemoteAclFilterTable(port, NwConstants.ADD_FLOW);
         }
         programFlows(AclConstants.ACL_JOB_KEY_PREFIX + portId, flowEntries, NwConstants.ADD_FLOW);
         return true;
@@ -224,13 +225,11 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
         if (!deletedAaps.isEmpty()) {
             programAclWithAllowedAddress(deleteFlowEntries, portBefore, deletedAaps, Action.UPDATE,
                     NwConstants.DEL_FLOW);
-            updateRemoteAclFilterTable(deleteFlowEntries, portBefore, portBefore.getSecurityGroups(), deletedAaps,
-                    NwConstants.DEL_FLOW);
+            updateRemoteAclFilterTable(portBefore, portBefore.getSecurityGroups(), deletedAaps, NwConstants.DEL_FLOW);
         }
         if (!addedAaps.isEmpty()) {
             programAclWithAllowedAddress(addFlowEntries, portAfter, addedAaps, Action.UPDATE, NwConstants.ADD_FLOW);
-            updateRemoteAclFilterTable(addFlowEntries, portAfter, portAfter.getSecurityGroups(), addedAaps,
-                    NwConstants.ADD_FLOW);
+            updateRemoteAclFilterTable(portAfter, portAfter.getSecurityGroups(), addedAaps, NwConstants.ADD_FLOW);
         }
         if (portAfter.getSubnetInfo() != null && portBefore.getSubnetInfo() == null) {
             programBroadcastRules(addFlowEntries, portAfter, Action.UPDATE, NwConstants.ADD_FLOW);
@@ -273,7 +272,7 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
             int addOrRemove) {
         int operationForAclRules = addOrRemove == NwConstants.DEL_FLOW ? NwConstants.MOD_FLOW : addOrRemove;
         programAclRules(flowEntries, port, aclList, operationForAclRules);
-        updateRemoteAclFilterTable(flowEntries, port, aclList, port.getAllowedAddressPairs(), addOrRemove);
+        updateRemoteAclFilterTable(port, aclList, port.getAllowedAddressPairs(), addOrRemove);
         programAclDispatcherTable(flowEntries, port, addOrRemove);
     }
 
@@ -532,7 +531,7 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
         } else {
             programAclWithAllowedAddress(flowEntries, port, port.getAllowedAddressPairs(),
                     Action.REMOVE, NwConstants.DEL_FLOW);
-            updateRemoteAclFilterTable(flowEntries, port, NwConstants.DEL_FLOW);
+            updateRemoteAclFilterTable(port, NwConstants.DEL_FLOW);
         }
         programFlows(AclConstants.ACL_JOB_KEY_PREFIX + port.getInterfaceId(), flowEntries, NwConstants.DEL_FLOW);
         return true;
@@ -760,8 +759,8 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
         programRemoteAclTable(deleteFlowEntries, aclName, remoteAclsDeleted, dpns, NwConstants.DEL_FLOW);
         programRemoteAclTable(addFlowEntries, aclName, remoteAclsAdded, dpns, NwConstants.ADD_FLOW);
 
-        programFlows(aclName, deleteFlowEntries, NwConstants.DEL_FLOW);
-        programFlows(aclName, addFlowEntries, NwConstants.ADD_FLOW);
+        programFlows(AclConstants.ACL_JOB_KEY_PREFIX + aclName, deleteFlowEntries, NwConstants.DEL_FLOW);
+        programFlows(AclConstants.ACL_JOB_KEY_PREFIX + aclName, addFlowEntries, NwConstants.ADD_FLOW);
     }
 
     private void programRemoteAclTable(List<FlowEntity> flowEntries, String aclName, Set<Uuid> remoteAclIds,
@@ -807,13 +806,12 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
         }
     }
 
-    private void updateRemoteAclFilterTable(List<FlowEntity> flowEntries, AclInterface port, int addOrRemove) {
-        updateRemoteAclFilterTable(flowEntries, port, port.getSecurityGroups(), port.getAllowedAddressPairs(),
-                addOrRemove);
+    private void updateRemoteAclFilterTable(AclInterface port, int addOrRemove) {
+        updateRemoteAclFilterTable(port, port.getSecurityGroups(), port.getAllowedAddressPairs(), addOrRemove);
     }
 
-    private void updateRemoteAclFilterTable(List<FlowEntity> flowEntries, AclInterface port, List<Uuid> aclList,
-            List<AllowedAddressPairs> aaps, int addOrRemove) {
+    private void updateRemoteAclFilterTable(AclInterface port, List<Uuid> aclList, List<AllowedAddressPairs> aaps,
+            int addOrRemove) {
         if (aclList == null) {
             LOG.debug("Port {} without SGs", port.getInterfaceId());
             return;
@@ -821,32 +819,42 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
         String portId = port.getInterfaceId();
         LOG.trace("updateRemoteAclFilterTable for portId={}, aclList={}, aaps={}, addOrRemove={}", portId, aclList,
                 aaps, addOrRemove);
+
+        ConcurrentMap<Uuid, Map<String, Set<AclInterface>>> mapOfAclWithInterfacesList =
+                aclDataUtil.getRemoteAclInterfaces(aclList, this.direction);
         for (Uuid aclId : aclList) {
+            Map<String, Set<AclInterface>> mapAclWithPortSet = mapOfAclWithInterfacesList.get(aclId);
             if (aclDataUtil.getRemoteAcl(aclId, this.direction) != null) {
                 Integer aclTag = aclServiceUtils.getAclTag(aclId);
-                if (addOrRemove == NwConstants.ADD_FLOW) {
-                    syncRemoteAclTable(flowEntries, portId, aclId, aclTag, aaps, addOrRemove);
-                }
-                else if (addOrRemove == NwConstants.DEL_FLOW) {
-                    jobCoordinator.enqueueJob(aclId.getValue(), () -> {
-                        List<FlowEntity> remoteTableFlowEntries = new ArrayList<>();
-                        syncRemoteAclTable(remoteTableFlowEntries, portId, aclId, aclTag, aaps, addOrRemove);
-                        programFlows(AclConstants.ACL_JOB_KEY_PREFIX + aclId.getValue(),
-                                remoteTableFlowEntries, NwConstants.DEL_FLOW);
-                        return Collections.emptyList();
-                    });
-                }
+                jobCoordinator.enqueueJob(aclId.getValue().intern(), () -> {
+                    List<FlowEntity> flowEntries = new ArrayList<>();
+                    syncRemoteAclTable(flowEntries, portId, aclId, aclTag, aaps, mapAclWithPortSet, addOrRemove);
+                    programFlows(AclConstants.ACL_JOB_KEY_PREFIX + aclId.getValue(), flowEntries, addOrRemove);
+                    return Collections.emptyList();
+                });
             }
         }
         Set<Uuid> remoteAclIds = aclServiceUtils.getRemoteAclIdsByDirection(aclList, direction);
         for (Uuid remoteAclId : remoteAclIds) {
-            syncRemoteAclTableFromOtherDpns(flowEntries, port, remoteAclId, addOrRemove);
+            List<Uuid> aclIds = new ArrayList<Uuid>(port.getSecurityGroups());
+            aclIds.removeAll(aclList);
+            if (addOrRemove == NwConstants.DEL_FLOW && aclServiceUtils.doesRemoteAclIdExistsInAcls(aclIds, remoteAclId,
+                    this.direction)) {
+                LOG.debug("Skipping delete as remoteAclId {} is used with other ACE configured with port {}",
+                        remoteAclId, portId);
+                return;
+            }
+            jobCoordinator.enqueueJob(remoteAclId.getValue().intern(), () -> {
+                List<FlowEntity> flowEntries = new ArrayList<>();
+                syncRemoteAclTableFromOtherDpns(flowEntries, port, remoteAclId, addOrRemove);
+                programFlows(AclConstants.ACL_JOB_KEY_PREFIX + remoteAclId.getValue(), flowEntries, addOrRemove);
+                return Collections.emptyList();
+            });
         }
     }
 
     private void syncRemoteAclTable(List<FlowEntity> flowEntries, String portId, Uuid acl, Integer aclTag,
-            List<AllowedAddressPairs> aaps, int addOrRemove) {
-        Map<String, Set<AclInterface>> mapAclWithPortSet = aclDataUtil.getRemoteAclInterfaces(acl, this.direction);
+            List<AllowedAddressPairs> aaps, Map<String, Set<AclInterface>> mapAclWithPortSet, int addOrRemove) {
         Set<BigInteger> dpns = collectDpns(mapAclWithPortSet);
         for (AllowedAddressPairs aap : aaps) {
             if (!AclServiceUtils.isNotIpAllNetwork(aap)) {

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceListener.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceListener.java
index 7a033d2dd2dfcf53fba67f1087c5fe670077fafe..61021fc6dbadbd5253b751d57d924cd943011522 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceListener.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceListener.java
@@ -126,6 +126,7 @@ public class AclInterfaceListener extends AsyncDataTreeChangeListenerBase<Interf
             boolean isSgChanged = isSecurityGroupsChanged(sgsBefore, aclInPortAfter.getSecurityGroups());
             AclInterface aclInterfaceAfter =
                     addOrUpdateAclInterfaceCache(interfaceId, aclInPortAfter, isSgChanged, interfaceState);
+            updateCacheWithAddedAcls(aclInterfaceBefore, aclInterfaceAfter);
 
             if (aclClusterUtil.isEntityOwner()) {
                 // Handle bind/unbind service irrespective of interface state (up/down)
@@ -160,16 +161,11 @@ public class AclInterfaceListener extends AsyncDataTreeChangeListenerBase<Interf
     }
 
     private void updateCacheWithAclChange(AclInterface aclInterfaceBefore, AclInterface aclInterfaceAfter) {
-        List<Uuid> addedAcls = AclServiceUtils.getUpdatedAclList(aclInterfaceAfter.getSecurityGroups(),
-                aclInterfaceBefore.getSecurityGroups());
         List<Uuid> deletedAcls = AclServiceUtils.getUpdatedAclList(aclInterfaceBefore.getSecurityGroups(),
                 aclInterfaceAfter.getSecurityGroups());
         if (!deletedAcls.isEmpty()) {
             aclDataUtil.removeAclInterfaceMap(deletedAcls, aclInterfaceAfter);
         }
-        if (!addedAcls.isEmpty()) {
-            aclDataUtil.addOrUpdateAclInterfaceMap(addedAcls, aclInterfaceAfter);
-        }
         List<AllowedAddressPairs> addedAap = AclServiceUtils.getUpdatedAllowedAddressPairs(aclInterfaceAfter
                 .getAllowedAddressPairs(), aclInterfaceBefore.getAllowedAddressPairs());
         List<AllowedAddressPairs> deletedAap = AclServiceUtils.getUpdatedAllowedAddressPairs(aclInterfaceBefore
@@ -180,6 +176,15 @@ public class AclInterfaceListener extends AsyncDataTreeChangeListenerBase<Interf
         }
     }
 
+    private void updateCacheWithAddedAcls(AclInterface aclInterfaceBefore, AclInterface aclInterfaceAfter) {
+        List<Uuid> addedAcls = AclServiceUtils.getUpdatedAclList(aclInterfaceAfter.getSecurityGroups(),
+                aclInterfaceBefore.getSecurityGroups());
+        if (addedAcls != null && !addedAcls.isEmpty()) {
+            LOG.debug("Update cache by adding interface={}", aclInterfaceAfter.getInterfaceId());
+            aclDataUtil.addOrUpdateAclInterfaceMap(addedAcls, aclInterfaceAfter);
+        }
+    }
+
     private boolean isPortSecurityEnabledNow(InterfaceAcl aclInPortBefore, InterfaceAcl aclInPortAfter) {
         return aclInPortBefore != null && !aclInPortBefore.isPortSecurityEnabled() && aclInPortAfter != null
                 && aclInPortAfter.isPortSecurityEnabled();

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
index 2ba6eae7df7434b0196e7df4f62756dcba06047e..9b41c688f07570bedc537418b69277d57c9423c8 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
@@ -96,6 +96,27 @@ public class AclDataUtil implements AclDataCache {
         return interfaceMap != null ? interfaceMap.values() : Collections.emptySet();
     }
 
+    @SuppressWarnings("checkstyle:JavadocParagraph")
+    /**
+     * Gets set of ACL interfaces per ACL (in a map) for the specified remote ACL IDs.
+     *
+     * @param remoteAclIdList List of remote ACL Ids
+     * @param direction the direction
+     * @return set of ACL interfaces per ACL (in a map) for the specified remote ACL IDs.
+     *         Format: ConcurrentMap<<Remote-ACL-ID>, Map<<ACL-ID>, Set<AclInterface>>>
+     */
+    public ConcurrentMap<Uuid, Map<String, Set<AclInterface>>> getRemoteAclInterfaces(List<Uuid> remoteAclIdList,
+            Class<? extends DirectionBase> direction) {
+        ConcurrentMap<Uuid, Map<String, Set<AclInterface>>> mapOfAclWithInterfacesList = new ConcurrentHashMap<>();
+        for (Uuid remoteAclId : remoteAclIdList) {
+            Map<String, Set<AclInterface>> mapOfAclWithInterfaces = getRemoteAclInterfaces(remoteAclId, direction);
+            if (mapOfAclWithInterfaces != null) {
+                mapOfAclWithInterfacesList.put(remoteAclId, mapOfAclWithInterfaces);
+            }
+        }
+        return mapOfAclWithInterfacesList;
+    }
+
     /**
      * Gets the set of ACL interfaces per ACL (in a map) which has specified
      * remote ACL ID.

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
index c1e0f7b2c8b55a0b5c6df792d166aa954a8626c8..256c09fc89d8b7bb8abfefcd89dfcca92fd93d2c 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
@@ -1074,6 +1074,33 @@ public final class AclServiceUtils {
         return skipDelete;
     }
 
+    public boolean doesRemoteAclIdExistsInAcls(List<Uuid> aclIds, Uuid remoteAclId,
+            Class<? extends DirectionBase> direction) {
+        if (aclIds == null) {
+            return false;
+        }
+        for (Uuid aclId : aclIds) {
+            Acl acl = this.aclDataUtil.getAcl(aclId.getValue());
+            if (null == acl) {
+                LOG.warn("ACL {} not found in cache.", aclId.getValue());
+                continue;
+            }
+            AccessListEntries accessListEntries = acl.getAccessListEntries();
+            if (accessListEntries != null && accessListEntries.getAce() != null) {
+                for (Ace ace : accessListEntries.getAce()) {
+                    SecurityRuleAttr aceAttr = AclServiceUtils.getAccessListAttributes(ace);
+                    if (aceAttr != null && aceAttr.getDirection().equals(direction)
+                            && doesAceHaveRemoteGroupId(aceAttr)) {
+                        if (aceAttr.getRemoteGroupId().equals(remoteAclId)) {
+                            return true;
+                        }
+                    }
+                }
+            }
+        }
+        return false;
+    }
+
     public static InstanceIdentifier<AclPortsByIp> aclPortsByIpPath(String aclName) {
         return InstanceIdentifier.builder(AclPortsLookup.class)
                 .child(AclPortsByIp.class, new AclPortsByIpKey(aclName)).build();

diff --git a/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/tests/FlowEntryObjectsStateful.xtend b/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/tests/FlowEntryObjectsStateful.xtend
index 516920e9b5736e58033d5c013766613c0911046a..0971fc8889f2a7b0e19a6809f1393820af2f2cae 100644 (file)
--- a/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/tests/FlowEntryObjectsStateful.xtend
+++ b/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/tests/FlowEntryObjectsStateful.xtend
@@ -248,7 +248,9 @@ class FlowEntryObjectsStateful extends FlowEntryObjectsBase {
         + remoteEgressFlowsPort3(ip2, prefix)
         + tcpEgressFlowPort2WithRemoteIpSg
         + tcpIngressFlowPort1WithMultipleSG
+        + remoteEgressFlowsPort3(ip1, prefix)
     }
+
     protected def tcpEgressFlowPort2WithRemoteIpSg() {
         val theFlowId1 ="TCP_DESTINATION_80_65535Egress_123_987_85cc3048-abc3-43cc-89b3-377341426a21"
         #[