NETVIRT-1536: Stale flows in ACL tables

Stale flows in ACL tables specific to security rules are observed when
delete events received by neutronvpn and ACL are as specified below:
(1) Neutron Port Event
(2) Neutron SG Event
(3) ACL Interface/State Event
(4) ACL Event (accesslist-acl)

In order to fix this issue, introduced 'deleted' in ACE yang
definition. When SG rule is deleted, ACE would not be deleted,
instead 'deleted' would be set. ACL Event listener after
processing add/delete ACE entries, deletes all the marked ACE
entries of the ACL being updated.

Change-Id: I47ef5328bf05bbed02c5492967005cd4c41f99d6
Signed-off-by: Shashidhar Raja <shashidharr@altencalsoftlabs.com>

diff --git a/aclservice/api/src/main/yang/aclservice.yang b/aclservice/api/src/main/yang/aclservice.yang
index c6cbe4403d72a7953c023bc08cb8766749b52416..0ab2b6262000a27e952ee6cc48f57b010edce903 100644 (file)
--- a/aclservice/api/src/main/yang/aclservice.yang
+++ b/aclservice/api/src/main/yang/aclservice.yang
@@ -96,6 +96,11 @@ module aclservice {
                 applied to incoming (ingress) traffic for that instance.
                 An egress rule is applied to traffic leaving the instance.";
         }
+        leaf deleted {
+            type boolean;
+            default false;
+            description "True when ACE is deleted; false otherwise.";
+        }
     }
 
     augment "/ietf-if:interfaces/ietf-if:interface" {

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 6017c44e6c6b9a019fdfc7900d495bece856b989..771922214798cbd25a0b0c177cf4d4106cd4754d 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
@@ -378,6 +378,10 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
     protected void programAceRule(List<FlowEntity> flowEntries, AclInterface port, String aclName, Ace ace,
             int addOrRemove) {
         SecurityRuleAttr aceAttr = AclServiceUtils.getAccessListAttributes(ace);
+        if (addOrRemove == NwConstants.ADD_FLOW && aceAttr.isDeleted()) {
+            LOG.trace("Ignoring {} rule which is already deleted", ace.getRuleName());
+            return;
+        }
         if (!isValidDirection(aceAttr.getDirection())) {
             LOG.trace("Ignoring {} direction while processing for {} ACE Rule {}", aceAttr.getDirection(),
                     this.directionString, ace.getRuleName());

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
index 32326ac859c7e7447c3d68d044d38cfcd0fc2e7c..80be48ee179c899cd72a3db35c96c8be032256b1 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
@@ -123,11 +123,14 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase<Acl, AclEv
         List<Ace> addedAceRules = getChangedAceList(aclAfter, aclBefore);
 
         // Find and update deleted ace rules in acl
-        List<Ace> deletedAceRules = getChangedAceList(aclBefore, aclAfter);
+        List<Ace> deletedAceRules = getDeletedAceList(aclAfter);
 
         if (aclClusterUtil.isEntityOwner()) {
             LOG.debug("On update event, remove Ace rules: {} for ACL: {}", deletedAceRules, aclName);
             updateAceRules(interfacesBefore, aclName, deletedAceRules, AclServiceManager.Action.REMOVE);
+            if (null != deletedAceRules && !deletedAceRules.isEmpty()) {
+                aclServiceUtils.deleteAcesFromConfigDS(aclName, deletedAceRules);
+            }
         }
         updateAclCaches(aclBefore, aclAfter, interfacesBefore);
 
@@ -281,4 +284,18 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase<Acl, AclEv
         }
         return updatedAceList;
     }
+
+    private List<Ace> getDeletedAceList(Acl acl) {
+        if (acl == null || acl.getAccessListEntries() == null || acl.getAccessListEntries().getAce() == null) {
+            return null;
+        }
+        List<Ace> aceList = acl.getAccessListEntries().getAce();
+        List<Ace> deletedAceList = new ArrayList<>();
+        for (Ace ace: aceList) {
+            if (ace.augmentation(SecurityRuleAttr.class).isDeleted()) {
+                deletedAceList.add(ace);
+            }
+        }
+        return deletedAceList;
+    }
 }

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclConstants.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclConstants.java
index 5abb085ae0d2b9b08794accc42b534be91c2ebdb..2ea0ad90f8d1f6838a6d4f00f836af6a4b40c597 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclConstants.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclConstants.java
@@ -122,7 +122,9 @@ public interface AclConstants {
 
     String ACL_SYNC_KEY_EXT = "-acl";
     int JOB_MAX_RETRIES = 3;
+    int ACEDELETE_MAX_RETRIES = 3;
     int FLOWS_PER_TRANSACTION = 30;
+    int ACES_PER_TRANSACTION = 30;
 
     String ACL_JOB_KEY_PREFIX = "ACL-";
 

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 451ca998b9413075ed23ff0170273e18d491c755..b5144dea1f107e20c575b5666525ff0819de66f4 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
@@ -9,6 +9,7 @@
 package org.opendaylight.netvirt.aclservice.utils;
 
 import static org.opendaylight.controller.md.sal.binding.api.WriteTransaction.CREATE_MISSING_PARENTS;
+import static org.opendaylight.genius.infra.Datastore.CONFIGURATION;
 import static org.opendaylight.genius.infra.Datastore.OPERATIONAL;
 
 import com.google.common.base.Optional;
@@ -70,7 +71,10 @@ import org.opendaylight.infrautils.jobcoordinator.JobCoordinator;
 import org.opendaylight.infrautils.utils.concurrent.ListenableFutures;
 import org.opendaylight.netvirt.aclservice.api.AclServiceManager.MatchCriteria;
 import org.opendaylight.netvirt.aclservice.api.utils.AclInterface;
+import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.AccessLists;
+import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.Ipv4Acl;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.access.lists.Acl;
+import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.access.lists.AclKey;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.access.lists.acl.AccessListEntries;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.access.lists.acl.access.list.entries.Ace;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.access.control.list.rev160218.access.lists.acl.access.list.entries.ace.Matches;
@@ -752,6 +756,22 @@ public final class AclServiceUtils {
                 OPERATIONAL, tx -> tx.delete(id)), LOG, "Failed to delete subnet info for port: " + portId);
     }
 
+    public void deleteAcesFromConfigDS(String aclName, List<Ace> deletedAceRules) {
+        List<List<Ace>> acesParts = Lists.partition(deletedAceRules, AclConstants.ACES_PER_TRANSACTION);
+        for (List<Ace> acePart : acesParts) {
+            jobCoordinator.enqueueJob(aclName,
+                () -> Collections.singletonList(txRunner.callWithNewReadWriteTransactionAndSubmit(CONFIGURATION,
+                    tx -> {
+                        for (Ace ace: acePart) {
+                            InstanceIdentifier<Ace> id = InstanceIdentifier.builder(AccessLists.class)
+                                    .child(Acl.class, new AclKey(aclName, Ipv4Acl.class)).child(AccessListEntries.class)
+                                    .child(Ace.class, ace.key()).build();
+                            tx.delete(id);
+                        }
+                    })), AclConstants.ACEDELETE_MAX_RETRIES);
+        }
+    }
+
     public static Integer allocateId(IdManagerService idManager, String poolName, String idKey, Integer defaultId) {
         AllocateIdInput getIdInput = new AllocateIdInputBuilder().setPoolName(poolName).setIdKey(idKey).build();
         try {

diff --git a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronSecurityRuleListener.java b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronSecurityRuleListener.java
index 74339be8f64bcadfb138f324e00e6c6b598cbebb..edd3a0abebda42f87086458023f429b08f0a2027 100644 (file)
--- a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronSecurityRuleListener.java
+++ b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronSecurityRuleListener.java
@@ -91,7 +91,7 @@ public class NeutronSecurityRuleListener
     protected void add(InstanceIdentifier<SecurityRule> instanceIdentifier, SecurityRule securityRule) {
         LOG.trace("added securityRule: {}", securityRule);
         try {
-            Ace ace = toAceBuilder(securityRule).build();
+            Ace ace = toAceBuilder(securityRule, false).build();
             InstanceIdentifier<Ace> identifier = getAceInstanceIdentifier(securityRule);
             MDSALUtil.syncWrite(dataBroker, LogicalDatastoreType.CONFIGURATION, identifier, ace);
         } catch (Exception ex) {
@@ -110,7 +110,7 @@ public class NeutronSecurityRuleListener
                 .build();
     }
 
-    private AceBuilder toAceBuilder(SecurityRule securityRule) {
+    private AceBuilder toAceBuilder(SecurityRule securityRule, boolean isDeleted) {
         AceIpBuilder aceIpBuilder = new AceIpBuilder();
         SecurityRuleAttrBuilder securityRuleAttrBuilder = new SecurityRuleAttrBuilder();
         DestinationPortRangeBuilder destinationPortRangeBuilder = new DestinationPortRangeBuilder();
@@ -142,6 +142,7 @@ public class NeutronSecurityRuleListener
                 aceIpBuilder.setProtocol(PROTOCOL_MAP.get(protocol.getIdentityref()));
             }
         }
+        securityRuleAttrBuilder.setDeleted(isDeleted);
 
         MatchesBuilder matchesBuilder = new MatchesBuilder();
         matchesBuilder.setAceType(aceIpBuilder.build());
@@ -216,9 +217,10 @@ public class NeutronSecurityRuleListener
     @SuppressWarnings("checkstyle:IllegalCatch")
     protected void remove(InstanceIdentifier<SecurityRule> instanceIdentifier, SecurityRule securityRule) {
         LOG.trace("removed securityRule: {}", securityRule);
+        InstanceIdentifier<Ace> identifier = getAceInstanceIdentifier(securityRule);
         try {
-            InstanceIdentifier<Ace> identifier = getAceInstanceIdentifier(securityRule);
-            MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, identifier);
+            Ace ace = toAceBuilder(securityRule, true).build();
+            MDSALUtil.syncUpdate(dataBroker, LogicalDatastoreType.CONFIGURATION, identifier, ace);
         } catch (Exception ex) {
             LOG.error("Exception occured while removing acl for security rule: {}. ", securityRule, ex);
         }