From e55b8242a4c52ac0935a74146adb98ccbddc17a3 Mon Sep 17 00:00:00 2001 From: Shashidhar Raja Date: Fri, 10 Jan 2020 15:04:24 +0530 Subject: [PATCH] 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 --- .../aclservice/AbstractAclServiceImpl.java | 66 +++++++++++-------- .../listeners/AclInterfaceListener.java | 15 +++-- .../netvirt/aclservice/utils/AclDataUtil.java | 21 ++++++ .../aclservice/utils/AclServiceUtils.java | 27 ++++++++ .../tests/FlowEntryObjectsStateful.xtend | 2 + 5 files changed, 97 insertions(+), 34 deletions(-) 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 42fc2bd260..b579086e2e 100644 --- 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 flowEntries, String aclName, Set remoteAclIds, @@ -807,13 +806,12 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener { } } - private void updateRemoteAclFilterTable(List 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 flowEntries, AclInterface port, List aclList, - List aaps, int addOrRemove) { + private void updateRemoteAclFilterTable(AclInterface port, List aclList, List 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>> mapOfAclWithInterfacesList = + aclDataUtil.getRemoteAclInterfaces(aclList, this.direction); for (Uuid aclId : aclList) { + Map> 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 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 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 remoteAclIds = aclServiceUtils.getRemoteAclIdsByDirection(aclList, direction); for (Uuid remoteAclId : remoteAclIds) { - syncRemoteAclTableFromOtherDpns(flowEntries, port, remoteAclId, addOrRemove); + List aclIds = new ArrayList(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 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 flowEntries, String portId, Uuid acl, Integer aclTag, - List aaps, int addOrRemove) { - Map> mapAclWithPortSet = aclDataUtil.getRemoteAclInterfaces(acl, this.direction); + List aaps, Map> mapAclWithPortSet, int addOrRemove) { Set 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 7a033d2dd2..61021fc6db 100644 --- 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 addedAcls = AclServiceUtils.getUpdatedAclList(aclInterfaceAfter.getSecurityGroups(), - aclInterfaceBefore.getSecurityGroups()); List deletedAcls = AclServiceUtils.getUpdatedAclList(aclInterfaceBefore.getSecurityGroups(), aclInterfaceAfter.getSecurityGroups()); if (!deletedAcls.isEmpty()) { aclDataUtil.removeAclInterfaceMap(deletedAcls, aclInterfaceAfter); } - if (!addedAcls.isEmpty()) { - aclDataUtil.addOrUpdateAclInterfaceMap(addedAcls, aclInterfaceAfter); - } List addedAap = AclServiceUtils.getUpdatedAllowedAddressPairs(aclInterfaceAfter .getAllowedAddressPairs(), aclInterfaceBefore.getAllowedAddressPairs()); List deletedAap = AclServiceUtils.getUpdatedAllowedAddressPairs(aclInterfaceBefore @@ -180,6 +176,15 @@ public class AclInterfaceListener extends AsyncDataTreeChangeListenerBase 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 2ba6eae7df..9b41c688f0 100644 --- 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<, Map<, Set>> + */ + public ConcurrentMap>> getRemoteAclInterfaces(List remoteAclIdList, + Class direction) { + ConcurrentMap>> mapOfAclWithInterfacesList = new ConcurrentHashMap<>(); + for (Uuid remoteAclId : remoteAclIdList) { + Map> 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 c1e0f7b2c8..256c09fc89 100644 --- 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 aclIds, Uuid remoteAclId, + Class 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 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 516920e9b5..0971fc8889 100644 --- 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" #[ -- 2.36.6