Ensuring that dhcp and igmp trap flows for UNI ports are solely determined by sadis config.

The flows on the NNI ports are still controlled by component configs in the app. Renaming them
so it's more explicit - enableDhcpOnNNI and enableIgmpOnNNI. Also igmp on NNI was blocked earlier -
removing that restriction. Finally improved unit tests to check if flowObjective calls actually happen.

Change-Id: I28e3a0dafb043391ddf8c397f3096d23acb86452
diff --git a/app/src/main/java/org/opencord/olt/impl/OltFlowService.java b/app/src/main/java/org/opencord/olt/impl/OltFlowService.java
index a53e019..d932638 100644
--- a/app/src/main/java/org/opencord/olt/impl/OltFlowService.java
+++ b/app/src/main/java/org/opencord/olt/impl/OltFlowService.java
@@ -15,7 +15,28 @@
  */
 package org.opencord.olt.impl;
 
-import com.google.common.collect.Sets;
+import static com.google.common.base.Strings.isNullOrEmpty;
+import static org.onlab.util.Tools.get;
+import static org.opencord.olt.impl.OsgiPropertyConstants.DEFAULT_TP_ID;
+import static org.opencord.olt.impl.OsgiPropertyConstants.DEFAULT_TP_ID_DEFAULT;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_ON_NNI;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_ON_NNI_DEFAULT;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_V4;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_V4_DEFAULT;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_V6;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_DHCP_V6_DEFAULT;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_EAPOL;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_EAPOL_DEFAULT;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_IGMP_ON_NNI;
+import static org.opencord.olt.impl.OsgiPropertyConstants.ENABLE_IGMP_ON_NNI_DEFAULT;
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.util.Dictionary;
+import java.util.Iterator;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+
 import org.onlab.packet.EthType;
 import org.onlab.packet.IPv4;
 import org.onlab.packet.IPv6;
@@ -63,25 +84,16 @@
 import org.osgi.service.component.annotations.ReferenceCardinality;
 import org.slf4j.Logger;
 
-import java.util.Dictionary;
-import java.util.Iterator;
-import java.util.Properties;
-import java.util.Set;
-import java.util.concurrent.CompletableFuture;
-
-import static com.google.common.base.Strings.isNullOrEmpty;
-import static org.onlab.util.Tools.get;
-import static org.opencord.olt.impl.OsgiPropertyConstants.*;
-import static org.slf4j.LoggerFactory.getLogger;
+import com.google.common.collect.Sets;
 
 /**
  * Provisions flow rules on access devices.
  */
 @Component(immediate = true, property = {
-        ENABLE_DHCP_ON_PROVISIONING + ":Boolean=" + ENABLE_DHCP_ON_PROVISIONING_DEFAULT,
+        ENABLE_DHCP_ON_NNI + ":Boolean=" + ENABLE_DHCP_ON_NNI_DEFAULT,
         ENABLE_DHCP_V4 + ":Boolean=" + ENABLE_DHCP_V4_DEFAULT,
         ENABLE_DHCP_V6 + ":Boolean=" + ENABLE_DHCP_V6_DEFAULT,
-        ENABLE_IGMP_ON_PROVISIONING + ":Boolean=" + ENABLE_IGMP_ON_PROVISIONING_DEFAULT,
+        ENABLE_IGMP_ON_NNI + ":Boolean=" + ENABLE_IGMP_ON_NNI_DEFAULT,
         ENABLE_EAPOL + ":Boolean=" + ENABLE_EAPOL_DEFAULT,
         DEFAULT_TP_ID + ":Integer=" + DEFAULT_TP_ID_DEFAULT
 })
@@ -123,24 +135,24 @@
     protected ComponentConfigService componentConfigService;
 
     /**
-     * Create the DHCP Flow rules when a subscriber is provisioned.
-     **/
-    protected boolean enableDhcpOnProvisioning = ENABLE_DHCP_ON_PROVISIONING_DEFAULT;
+     * Create DHCP trap flow on NNI port(s).
+     */
+    protected boolean enableDhcpOnNni = ENABLE_DHCP_ON_NNI_DEFAULT;
 
     /**
-     * Enable flows for DHCP v4.
+     * Enable flows for DHCP v4 if dhcp is required in sadis config.
      **/
     protected boolean enableDhcpV4 = ENABLE_DHCP_V4_DEFAULT;
 
     /**
-     * Enable flows for DHCP v6.
+     * Enable flows for DHCP v6 if dhcp is required in sadis config.
      **/
     protected boolean enableDhcpV6 = ENABLE_DHCP_V6_DEFAULT;
 
     /**
-     * Create IGMP Flow rules when a subscriber is provisioned.
+     * Create IGMP trap flow on NNI port(s).
      **/
-    protected boolean enableIgmpOnProvisioning = ENABLE_IGMP_ON_PROVISIONING_DEFAULT;
+    protected boolean enableIgmpOnNni = ENABLE_IGMP_ON_NNI_DEFAULT;
 
     /**
      * Send EAPOL authentication trap flows before subscriber provisioning.
@@ -179,9 +191,9 @@
 
         Dictionary<?, ?> properties = context != null ? context.getProperties() : new Properties();
 
-        Boolean o = Tools.isPropertyEnabled(properties, ENABLE_DHCP_ON_PROVISIONING);
+        Boolean o = Tools.isPropertyEnabled(properties, ENABLE_DHCP_ON_NNI);
         if (o != null) {
-            enableDhcpOnProvisioning = o;
+            enableDhcpOnNni = o;
         }
 
         Boolean v4 = Tools.isPropertyEnabled(properties, ENABLE_DHCP_V4);
@@ -194,9 +206,9 @@
             enableDhcpV6 = v6;
         }
 
-        Boolean p = Tools.isPropertyEnabled(properties, ENABLE_IGMP_ON_PROVISIONING);
+        Boolean p = Tools.isPropertyEnabled(properties, ENABLE_IGMP_ON_NNI);
         if (p != null) {
-            enableIgmpOnProvisioning = p;
+            enableIgmpOnNni = p;
         }
 
         Boolean eap = Tools.isPropertyEnabled(properties, ENABLE_EAPOL);
@@ -207,11 +219,11 @@
         String tpId = get(properties, DEFAULT_TP_ID);
         defaultTechProfileId = isNullOrEmpty(tpId) ? DEFAULT_TP_ID_DEFAULT : Integer.parseInt(tpId.trim());
 
-        log.info("modified. Values = enableDhcpOnProvisioning: {}, enableDhcpV4: {}, " +
-                         "enableDhcpV6:{}, enableIgmpOnProvisioning:{}, " +
+        log.info("modified. Values = enableDhcpOnNni: {}, enableDhcpV4: {}, " +
+                         "enableDhcpV6:{}, enableIgmpOnNni:{}, " +
                          "enableEapol{}, defaultTechProfileId: {}",
-                 enableDhcpOnProvisioning, enableDhcpV4, enableDhcpV6,
-                 enableIgmpOnProvisioning, enableEapol, defaultTechProfileId);
+                 enableDhcpOnNni, enableDhcpV4, enableDhcpV6,
+                 enableIgmpOnNni, enableEapol, defaultTechProfileId);
 
     }
 
@@ -221,14 +233,22 @@
                                                UniTagInformation tagInformation,
                                                boolean install,
                                                boolean upstream) {
-        //tagInformation can be none in case of NNI port.
-        //in that case relying on global flag
-        if (!enableDhcpOnProvisioning || (tagInformation != null
-                && !tagInformation.getIsDhcpRequired())) {
-            log.debug("Dhcp provisioning is disabled for port {} on device {}", devId, port);
-            return;
+        if (upstream) {
+            // for UNI ports
+            if (tagInformation != null && !tagInformation.getIsDhcpRequired()) {
+                log.debug("Dhcp provisioning is disabled for UNI port {} on "
+                        + "device {} for service {}", port, devId,
+                        tagInformation.getServiceName());
+                return;
+            }
+        } else {
+            // for NNI ports
+            if (!enableDhcpOnNni) {
+                log.debug("Dhcp provisioning is disabled for NNI port {} on "
+                        + "device {}", port, devId);
+                return;
+            }
         }
-
         int techProfileId = tagInformation != null ? tagInformation.getTechnologyProfileId() : NONE_TP_ID;
         VlanId cTag = tagInformation != null ? tagInformation.getPonCTag() : VlanId.NONE;
         VlanId unitagMatch = tagInformation != null ? tagInformation.getUniTagMatch() : VlanId.ANY;
@@ -302,7 +322,6 @@
                         error);
             }
         });
-
         flowObjectiveService.filter(devId, dhcpUpstream);
 
     }
@@ -313,18 +332,21 @@
                                                UniTagInformation tagInformation,
                                                boolean install,
                                                boolean upstream) {
-        //tagInformation can be none in case of NNI port.
-        //in that case relying on global flag
-        if (!enableIgmpOnProvisioning || (tagInformation != null
-                && !tagInformation.getIsIgmpRequired())) {
-            log.debug("Igmp provisioning is disabled for port {} on device {}", devId, port);
-            return;
-        }
-
-        if (!upstream) {
-            log.debug("Direction on port {} for device {} " +
-                              "is not Upstream, ignoring Igmp request", port, devId);
-            return;
+        if (upstream) {
+            // for UNI ports
+            if (tagInformation != null && !tagInformation.getIsIgmpRequired()) {
+                log.debug("Igmp provisioning is disabled for UNI port {} on "
+                        + "device {} for service {}", port, devId,
+                          tagInformation.getServiceName());
+                return;
+            }
+        } else {
+            // for NNI ports
+            if (!enableIgmpOnNni) {
+                log.debug("Igmp provisioning is disabled for NNI port {} on device {}",
+                          port, devId);
+                return;
+            }
         }
 
         log.debug("{} IGMP flows on {}:{}", (install) ?
@@ -551,7 +573,8 @@
      */
     @Override
     public void processNniFilteringObjectives(DeviceId devId, PortNumber port, boolean install) {
-        log.info("Sending flows for NNI port {} of the device {}", port, devId);
+        log.info("{} flows for NNI port {} on device {}",
+                 install ? "Adding" : "Removing", port, devId);
         processLldpFilteringObjective(devId, port, install);
         processDhcpFilteringObjectives(devId, port, null, null, install, false);
         processIgmpFilteringObjectives(devId, port, null, null, install, false);
diff --git a/app/src/main/java/org/opencord/olt/impl/OltMeterService.java b/app/src/main/java/org/opencord/olt/impl/OltMeterService.java
index fc56066..7616141 100644
--- a/app/src/main/java/org/opencord/olt/impl/OltMeterService.java
+++ b/app/src/main/java/org/opencord/olt/impl/OltMeterService.java
@@ -15,9 +15,32 @@
  */
 package org.opencord.olt.impl;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
+import static java.util.stream.Collectors.collectingAndThen;
+import static java.util.stream.Collectors.groupingBy;
+import static java.util.stream.Collectors.mapping;
+import static java.util.stream.Collectors.toSet;
+import static org.onlab.util.Tools.groupedThreads;
+import static org.opencord.olt.impl.OsgiPropertyConstants.DELETE_METERS;
+import static org.opencord.olt.impl.OsgiPropertyConstants.DELETE_METERS_DEFAULT;
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Dictionary;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+
 import org.onlab.util.KryoNamespace;
 import org.onlab.util.Tools;
 import org.onosproject.cfg.ComponentConfigService;
@@ -52,31 +75,9 @@
 import org.osgi.service.component.annotations.ReferenceCardinality;
 import org.slf4j.Logger;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Dictionary;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Properties;
-import java.util.Set;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
-import java.util.stream.Collectors;
-
-import static java.util.stream.Collectors.collectingAndThen;
-import static java.util.stream.Collectors.groupingBy;
-import static java.util.stream.Collectors.mapping;
-import static java.util.stream.Collectors.toSet;
-import static org.onlab.util.Tools.groupedThreads;
-import static org.opencord.olt.impl.OsgiPropertyConstants.DELETE_METERS;
-import static org.opencord.olt.impl.OsgiPropertyConstants.DELETE_METERS_DEFAULT;
-import static org.slf4j.LoggerFactory.getLogger;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 
 /**
  * Provisions Meters on access devices.
@@ -98,7 +99,10 @@
     @Reference(cardinality = ReferenceCardinality.MANDATORY)
     protected StorageService storageService;
 
-    protected boolean deleteMeters = true;
+    /**
+     * Delete meters when reference count drops to zero.
+     */
+    protected boolean deleteMeters = DELETE_METERS_DEFAULT;
 
     private ApplicationId appId;
     private static final String APP_NAME = "org.opencord.olt";
@@ -201,7 +205,7 @@
     @Override
     public MeterId createMeter(DeviceId deviceId, BandwidthProfileInformation bpInfo,
                                CompletableFuture<Object> meterFuture) {
-        log.debug("Installing meter on {} for {}", deviceId, bpInfo);
+        log.debug("Creating meter on {} for {}", deviceId, bpInfo);
         if (bpInfo == null) {
             log.warn("Requested bandwidth profile information is NULL");
             meterFuture.complete(ObjectiveError.BADPARAMS);
@@ -246,7 +250,8 @@
 
         Meter meter = meterService.submit(meterRequest);
         meterIdRef.set(meter.id());
-        log.info("Meter is created. Meter Id {}", meter.id());
+        log.info("Meter {} created and sent for installation on {} for {}",
+                 meter.id(), deviceId, bpInfo);
         return meter.id();
     }
 
diff --git a/app/src/main/java/org/opencord/olt/impl/OsgiPropertyConstants.java b/app/src/main/java/org/opencord/olt/impl/OsgiPropertyConstants.java
index 67bd5fa..c4ea7c9 100644
--- a/app/src/main/java/org/opencord/olt/impl/OsgiPropertyConstants.java
+++ b/app/src/main/java/org/opencord/olt/impl/OsgiPropertyConstants.java
@@ -27,8 +27,8 @@
     public static final String DEFAULT_MCAST_SERVICE_NAME = "multicastServiceName";
     public static final String DEFAULT_MCAST_SERVICE_NAME_DEFAULT = "MC";
 
-    public static final String ENABLE_DHCP_ON_PROVISIONING = "enableDhcpOnProvisioning";
-    public static final boolean ENABLE_DHCP_ON_PROVISIONING_DEFAULT = false;
+    public static final String ENABLE_DHCP_ON_NNI = "enableDhcpOnNni";
+    public static final boolean ENABLE_DHCP_ON_NNI_DEFAULT = false;
 
     public static final String ENABLE_DHCP_V4 = "enableDhcpV4";
     public static final boolean ENABLE_DHCP_V4_DEFAULT = true;
@@ -36,8 +36,8 @@
     public static final String ENABLE_DHCP_V6 = "enableDhcpV6";
     public static final boolean ENABLE_DHCP_V6_DEFAULT = false;
 
-    public static final String ENABLE_IGMP_ON_PROVISIONING = "enableIgmpOnProvisioning";
-    public static final boolean ENABLE_IGMP_ON_PROVISIONING_DEFAULT = false;
+    public static final String ENABLE_IGMP_ON_NNI = "enableIgmpOnNni";
+    public static final boolean ENABLE_IGMP_ON_NNI_DEFAULT = false;
 
     public static final String DELETE_METERS = "deleteMeters";
     public static final boolean DELETE_METERS_DEFAULT = true;
diff --git a/app/src/test/java/org/opencord/olt/impl/OltFlowTest.java b/app/src/test/java/org/opencord/olt/impl/OltFlowTest.java
index 25bc87a..4a86703 100644
--- a/app/src/test/java/org/opencord/olt/impl/OltFlowTest.java
+++ b/app/src/test/java/org/opencord/olt/impl/OltFlowTest.java
@@ -15,9 +15,14 @@
  */
 package org.opencord.olt.impl;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ListMultimap;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+
 import org.apache.commons.lang3.tuple.Pair;
 import org.junit.Before;
 import org.junit.Test;
@@ -51,16 +56,13 @@
 import org.opencord.sadis.BandwidthProfileInformation;
 import org.opencord.sadis.UniTagInformation;
 
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
-import java.util.concurrent.CompletableFuture;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ListMultimap;
 
 public class OltFlowTest extends TestBase {
     private OltFlowService oltFlowService;
-
     PortNumber uniPortNumber = PortNumber.portNumber(1);
     PortNumber nniPortNumber = PortNumber.portNumber(65535);
 
@@ -73,6 +75,20 @@
             .setTechnologyProfileId(64)
             .setDownstreamBandwidthProfile(dsBpId)
             .setUpstreamBandwidthProfile(usBpId)
+            .setIsDhcpRequired(true)
+            .setIsIgmpRequired(true)
+            .build();
+
+    UniTagInformation.Builder tagInfoBuilder2 = new UniTagInformation.Builder();
+    UniTagInformation uniTagInfoNoDhcpNoIgmp = tagInfoBuilder2
+            .setUniTagMatch(VlanId.vlanId((short) 35))
+            .setPonCTag(VlanId.vlanId((short) 33))
+            .setPonSTag(VlanId.vlanId((short) 7))
+            .setDsPonCTagPriority(0)
+            .setUsPonSTagPriority(0)
+            .setTechnologyProfileId(64)
+            .setDownstreamBandwidthProfile(dsBpId)
+            .setUpstreamBandwidthProfile(usBpId)
             .build();
 
     @Before
@@ -88,22 +104,95 @@
 
     @Test
     public void testDhcpFiltering() {
-        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId, uniTagInfo,
-                true, true);
-        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId, uniTagInfo,
-                false, true);
-        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber, dsMeterId, uniTagInfo,
-                true, false);
-        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId, uniTagInfo,
-                false, false);
+        oltFlowService.flowObjectiveService.clearQueue();
+        // ensure upstream dhcp traps can be added and removed
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfo,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 1;
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfo,
+                                                      false, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+        // ensure upstream flows are not added if uniTagInfo is missing dhcp requirement
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfoNoDhcpNoIgmp,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+
+        // ensure downstream traps don't succeed without global config for nni ports
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      true, false);
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      false, false);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+        // do global config for nni ports and now it should succeed
+        oltFlowService.enableDhcpOnNni = true;
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      true, false);
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      false, false);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 4;
+
+        // turn on DHCPv6 and we should get 2 flows
+        oltFlowService.enableDhcpV6 = true;
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfo,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 6;
+
+        // turn off DHCPv4 and it's only v6
+        oltFlowService.enableDhcpV4 = false;
+        oltFlowService.processDhcpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfo,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 7;
+
+        // cleanup
+        oltFlowService.flowObjectiveService.clearQueue();
+        oltFlowService.enableDhcpV4 = true;
+        oltFlowService.enableDhcpV6 = false;
     }
 
     @Test
     public void testIgmpFiltering() {
-        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId, uniTagInfo,
-                true, true);
-        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId, uniTagInfo,
-                false, true);
+        oltFlowService.flowObjectiveService.clearQueue();
+
+        // ensure igmp flows can be added and removed
+        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfo,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 1;
+        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, uniPortNumber, usMeterId,
+                                                      uniTagInfo,
+                                                      false, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+
+        // ensure igmp flow is not added if uniTag has no igmp requirement
+        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, uniPortNumber,
+                                                      usMeterId, uniTagInfoNoDhcpNoIgmp,
+                                                      true, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+
+        //ensure igmp flow on NNI fails without global setting
+        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      true, false);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 2;
+
+        // igmp trap on NNI should succeed with global config
+        oltFlowService.enableIgmpOnNni = true;
+        oltFlowService.processIgmpFilteringObjectives(DEVICE_ID_1, nniPortNumber,
+                                                      null, null,
+                                                      true, false);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives().size() == 3;
+        // cleanup
+        oltFlowService.flowObjectiveService.clearQueue();
+
     }
 
     @Test
@@ -130,10 +219,16 @@
 
     @Test
     public void testNniFiltering() {
-        oltFlowService.enableDhcpOnProvisioning = true;
-        oltFlowService.enableIgmpOnProvisioning = true;
+        oltFlowService.flowObjectiveService.clearQueue();
+        oltFlowService.enableDhcpOnNni = true;
+        oltFlowService.enableIgmpOnNni = true;
         oltFlowService.processNniFilteringObjectives(DEVICE_ID_1, nniPortNumber, true);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives()
+                .size() == 3;
         oltFlowService.processNniFilteringObjectives(DEVICE_ID_1, nniPortNumber, false);
+        assert oltFlowService.flowObjectiveService.getPendingFlowObjectives()
+                .size() == 6;
+        oltFlowService.flowObjectiveService.clearQueue();
     }
 
     @Test
@@ -231,9 +326,11 @@
     }
 
     private class MockOltFlowObjectiveService implements org.onosproject.net.flowobjective.FlowObjectiveService {
+        List<String> flowObjectives = new ArrayList<>();
+
         @Override
         public void filter(DeviceId deviceId, FilteringObjective filteringObjective) {
-
+            flowObjectives.add(filteringObjective.toString());
             EthTypeCriterion ethType = (EthTypeCriterion)
                     filterForCriterion(filteringObjective.conditions(), Criterion.Type.ETH_TYPE);
 
@@ -294,7 +391,7 @@
 
         @Override
         public List<String> getPendingFlowObjectives() {
-            return null;
+            return ImmutableList.copyOf(flowObjectives);
         }
 
         @Override
@@ -329,7 +426,7 @@
 
         @Override
         public void clearQueue() {
-
+            flowObjectives.clear();
         }
 
         private Criterion filterForCriterion(Collection<Criterion> criteria, Criterion.Type type) {