SR app now assigns internal vlans per subnet and passes this

information in a filtering objective to drivers. OF-DPA driver uses this information to assign vlans to untagged packets. Change-Id: Ibf33bdcedf5f83992f362dfb91c12575c65da3b4

SR app now assigns internal vlans per subnet and passes this
information in a filtering objective to drivers. OF-DPA driver uses this information to assign vlans to untagged packets. Change-Id: Ibf33bdcedf5f83992f362dfb91c12575c65da3b4
Saurav Das · Gerrit Code Review
Commit 0e99e2ba4082099e560939faa821aa3513092993 0e99e2ba 1 parent 94872a1d
Showing 5 changed files with 267 additions and 77 deletions
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SubnetAssignedVidStoreKey.java
drivers/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
View file @0e99e2b
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
View file @0e99e2b
@@ -38,6 +38,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 /**
  * Segment Routing configuration component that reads the
@@ -51,7 +52,8 @@ public class DeviceConfiguration implements DeviceProperties {
     private static final Logger log = LoggerFactory
             .getLogger(DeviceConfiguration.class);
     private final List<Integer> allSegmentIds = new ArrayList<>();
-    private final HashMap<DeviceId, SegmentRouterInfo> deviceConfigMap = new HashMap<>();
+    private final ConcurrentHashMap<DeviceId, SegmentRouterInfo> deviceConfigMap
+        = new ConcurrentHashMap<>();
     private class SegmentRouterInfo {
         int nodeSid;
@@ -133,11 +135,10 @@ public class DeviceConfiguration implements DeviceProperties {
      */
     @Override
     public int getSegmentId(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("getSegmentId for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("getSegmentId for device{} is {}", deviceId, srinfo.nodeSid);
-                    deviceConfigMap.get(deviceId).nodeSid);
+            return srinfo.nodeSid;
-            return deviceConfigMap.get(deviceId).nodeSid;
         } else {
             log.warn("getSegmentId for device {} "
                     + "throwing IllegalStateException "
@@ -150,7 +151,7 @@ public class DeviceConfiguration implements DeviceProperties {
      * Returns the Node segment id of a segment router given its Router mac address.
      *
      * @param routerMac router mac address
-     * @return segment id
+     * @return node segment id, or -1 if not found in config
      */
     public int getSegmentId(MacAddress routerMac) {
         for (Map.Entry<DeviceId, SegmentRouterInfo> entry:
@@ -167,7 +168,7 @@ public class DeviceConfiguration implements DeviceProperties {
      * Returns the Node segment id of a segment router given its Router ip address.
      *
      * @param routerAddress router ip address
-     * @return segment id
+     * @return node segment id, or -1 if not found in config
      */
     public int getSegmentId(Ip4Address routerAddress) {
         for (Map.Entry<DeviceId, SegmentRouterInfo> entry:
@@ -188,11 +189,10 @@ public class DeviceConfiguration implements DeviceProperties {
      */
     @Override
     public MacAddress getDeviceMac(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("getDeviceMac for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("getDeviceMac for device{} is {}", deviceId, srinfo.mac);
-                    deviceConfigMap.get(deviceId).mac);
+            return srinfo.mac;
-            return deviceConfigMap.get(deviceId).mac;
         } else {
             log.warn("getDeviceMac for device {} "
                     + "throwing IllegalStateException "
@@ -208,11 +208,10 @@ public class DeviceConfiguration implements DeviceProperties {
      * @return router ip address
      */
     public Ip4Address getRouterIp(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("getDeviceIp for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("getDeviceIp for device{} is {}", deviceId, srinfo.ip);
-                    deviceConfigMap.get(deviceId).ip);
+            return srinfo.ip;
-            return deviceConfigMap.get(deviceId).ip;
         } else {
             log.warn("getRouterIp for device {} "
                     + "throwing IllegalStateException "
@@ -230,11 +229,10 @@ public class DeviceConfiguration implements DeviceProperties {
      */
     @Override
     public boolean isEdgeDevice(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("isEdgeDevice for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("isEdgeDevice for device{} is {}", deviceId, srinfo.isEdge);
-                    deviceConfigMap.get(deviceId).isEdge);
+            return srinfo.isEdge;
-            return deviceConfigMap.get(deviceId).isEdge;
         } else {
             log.warn("isEdgeDevice for device {} "
                     + "throwing IllegalStateException "
@@ -295,17 +293,16 @@ public class DeviceConfiguration implements DeviceProperties {
      * on those ports.
      *
      * @param deviceId device identifier
-     * @return list of ip addresses configured on the ports
+     * @return list of ip addresses configured on the ports or null if not found
      */
     public List<Ip4Address> getPortIPs(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("getSubnetGatewayIps for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("getSubnetGatewayIps for device{} is {}", deviceId,
-                    deviceConfigMap.get(deviceId).gatewayIps.values());
+                      srinfo.gatewayIps.values());
-            return new ArrayList<>(deviceConfigMap.get(deviceId).gatewayIps.values());
+            return new ArrayList<>(srinfo.gatewayIps.values());
-        } else {
-            return null;
         }
+        return null;
     }
     /**
@@ -313,11 +310,12 @@ public class DeviceConfiguration implements DeviceProperties {
      * for a segment router.
      *
      * @param deviceId device identifier
-     * @return map of port to gateway IP addresses
+     * @return map of port to gateway IP addresses or null if not found
      */
     public Map<PortNumber, Ip4Address> getPortIPMap(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            return deviceConfigMap.get(deviceId).gatewayIps;
+        if (srinfo != null) {
+            return srinfo.gatewayIps;
         }
         return null;
     }
@@ -326,17 +324,32 @@ public class DeviceConfiguration implements DeviceProperties {
      * Returns the configured subnet prefixes for a segment router.
      *
      * @param deviceId device identifier
-     * @return list of ip prefixes
+     * @return list of ip prefixes or null if not found
      */
     public List<Ip4Prefix> getSubnets(DeviceId deviceId) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            log.trace("getSubnets for device{} is {}",
+        if (srinfo != null) {
-                    deviceId,
+            log.trace("getSubnets for device{} is {}", deviceId,
-                    deviceConfigMap.get(deviceId).subnets.values());
+                      srinfo.subnets.values());
-            return new ArrayList<>(deviceConfigMap.get(deviceId).subnets.values());
+            return new ArrayList<>(srinfo.subnets.values());
-        } else {
+        }
         return null;
     }
+
+    /**
+     *  Returns the configured subnet on the given port, or null if no
+     *  subnet has been configured on the port.
+     *
+     *  @param deviceId device identifier
+     *  @param pnum  port identifier
+     *  @return configured subnet on port, or null
+     */
+    public Ip4Prefix getPortSubnet(DeviceId deviceId, PortNumber pnum) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
+        if (srinfo != null) {
+            return srinfo.subnets.get(pnum);
+        }
+        return null;
     }
     /**
@@ -365,7 +378,7 @@ public class DeviceConfiguration implements DeviceProperties {
      * specified ip address as one of its subnet gateway ip address.
      *
      * @param gatewayIpAddress router gateway ip address
-     * @return router mac address
+     * @return router mac address or null if not found
      */
     public MacAddress getRouterMacForAGatewayIp(Ip4Address gatewayIpAddress) {
         for (Map.Entry<DeviceId, SegmentRouterInfo> entry:
@@ -415,8 +428,9 @@ public class DeviceConfiguration implements DeviceProperties {
      * @return list of port numbers
      */
     public List<Integer> getPortsForAdjacencySid(DeviceId deviceId, int sid) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            for (AdjacencySid asid : deviceConfigMap.get(deviceId).adjacencySids) {
+        if (srinfo != null) {
+            for (AdjacencySid asid : srinfo.adjacencySids) {
                 if (asid.getAsid() == sid) {
                     return asid.getPorts();
                 }
@@ -435,12 +449,13 @@ public class DeviceConfiguration implements DeviceProperties {
      * otherwise false
      */
     public boolean isAdjacencySid(DeviceId deviceId, int sid) {
-        if (deviceConfigMap.get(deviceId) != null) {
+        SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
-            if (deviceConfigMap.get(deviceId).adjacencySids.isEmpty()) {
+        if (srinfo != null) {
+            if (srinfo.adjacencySids.isEmpty()) {
                 return false;
             } else {
                 for (AdjacencySid asid:
-                        deviceConfigMap.get(deviceId).adjacencySids) {
+                        srinfo.adjacencySids) {
                     if (asid.getAsid() == sid) {
                         return true;
                     }
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
View file @0e99e2b
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
View file @0e99e2b
@@ -25,6 +25,7 @@ import org.onlab.packet.VlanId;
 import org.onosproject.segmentrouting.grouphandler.NeighborSet;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.Link;
+import org.onosproject.net.Port;
 import org.onosproject.net.PortNumber;
 import org.onosproject.net.flow.DefaultTrafficSelector;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
@@ -60,6 +61,8 @@ public class RoutingRulePopulator {
     private DeviceConfiguration config;
     private static final int HIGHEST_PRIORITY = 0xffff;
+    private static final long OFPP_MAX = 0xffffff00L;
+
     /**
      * Creates a RoutingRulePopulator object.
@@ -355,26 +358,38 @@ public class RoutingRulePopulator {
     /**
      * Creates a filtering objective to permit all untagged packets with a
-     * dstMac corresponding to the router's MAC address.
+     * dstMac corresponding to the router's MAC address. For those pipelines
+     * that need to internally assign vlans to untagged packets, this method
+     * provides per-subnet vlan-ids as metadata.
      *
      * @param deviceId  the switch dpid for the router
      */
     public void populateRouterMacVlanFilters(DeviceId deviceId) {
+        log.debug("Installing per-port filtering objective for untagged "
+                + "packets in device {}", deviceId);
+        for (Port port : srManager.deviceService.getPorts(deviceId)) {
+            if (port.number().toLong() > 0 && port.number().toLong() < OFPP_MAX) {
+                Ip4Prefix portSubnet = config.getPortSubnet(deviceId, port.number());
+                VlanId assignedVlan = (portSubnet == null)
+                        ? VlanId.vlanId(SegmentRoutingManager.ASSIGNED_VLAN_NO_SUBNET)
+                        : srManager.getSubnetAssignedVlanId(deviceId, portSubnet);
+                TrafficTreatment tt = DefaultTrafficTreatment.builder()
+                        .pushVlan().setVlanId(assignedVlan).build();
                 FilteringObjective.Builder fob = DefaultFilteringObjective.builder();
-        fob.withKey(Criteria.matchInPort(PortNumber.ALL))
+                fob.withKey(Criteria.matchInPort(port.number()))
                 .addCondition(Criteria.matchEthDst(config.getDeviceMac(deviceId)))
                 .addCondition(Criteria.matchVlanId(VlanId.NONE))
-                .addCondition(Criteria.matchIPDst(
+                .setMeta(tt)
-                                  IpPrefix.valueOf(config.getRouterIp(deviceId),
+                .addCondition(Criteria.matchIPDst(IpPrefix.valueOf(
+                                                      config.getRouterIp(deviceId),
                                                       IpPrefix.MAX_INET_MASK_LENGTH)));
-
                 fob.permit().fromApp(srManager.appId);
-        log.debug("Installing filtering objective for untagged packets");
                 srManager.flowObjectiveService.
-            filter(deviceId,
+                filter(deviceId, fob.add(new SRObjectiveContext(deviceId,
-                   fob.add(new SRObjectiveContext(deviceId,
                                       SRObjectiveContext.ObjectiveType.FILTER)));
             }
+        }
+    }
     /**
      * Creates a forwarding objective to punt all IP packets, destined to the
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
View file @0e99e2b
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
View file @0e99e2b
@@ -23,6 +23,8 @@ import org.apache.felix.scr.annotations.ReferenceCardinality;
 import org.apache.felix.scr.annotations.Service;
 import org.onlab.packet.Ethernet;
 import org.onlab.packet.IPv4;
+import org.onlab.packet.Ip4Prefix;
+import org.onlab.packet.VlanId;
 import org.onlab.util.KryoNamespace;
 import org.onosproject.core.ApplicationId;
 import org.onosproject.core.CoreService;
@@ -45,7 +47,6 @@ import org.onosproject.net.device.DeviceEvent;
 import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
 import org.onosproject.net.flowobjective.FlowObjectiveService;
-import org.onosproject.net.group.GroupKey;
 import org.onosproject.net.host.HostService;
 import org.onosproject.net.intent.IntentService;
 import org.onosproject.net.link.LinkEvent;
@@ -64,9 +65,11 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.net.URI;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.Executors;
@@ -135,6 +138,10 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         Integer> nsNextObjStore = null;
     private EventuallyConsistentMap<String, Tunnel> tunnelStore = null;
     private EventuallyConsistentMap<String, Policy> policyStore = null;
+    // Per device, per-subnet assigned-vlans store, with (device id + subnet
+    // IPv4 prefix) as key
+    private EventuallyConsistentMap<SubnetAssignedVidStoreKey, VlanId>
+        subnetVidStore = null;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected StorageService storageService;
@@ -163,6 +170,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     private KryoNamespace.Builder kryoBuilder = null;
+    private static final short ASSIGNED_VLAN_START = 4093;
+    public static final short ASSIGNED_VLAN_NO_SUBNET = 4094;
+
     @Activate
     protected void activate() {
         appId = coreService
@@ -180,7 +190,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                     DefaultTunnel.class,
                     Policy.class,
                     TunnelPolicy.class,
-                    Policy.Type.class
+                    Policy.Type.class,
+                    SubnetAssignedVidStoreKey.class,
+                    VlanId.class
             );
         log.debug("Creating EC map nsnextobjectivestore");
@@ -212,6 +224,15 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                 .withTimestampProvider((k, v) -> new WallClockTimestamp())
                 .build();
+        EventuallyConsistentMapBuilder<SubnetAssignedVidStoreKey, VlanId>
+            subnetVidStoreMapBuilder = storageService.eventuallyConsistentMapBuilder();
+
+        subnetVidStore = subnetVidStoreMapBuilder
+                .withName("subnetvidstore")
+                .withSerializer(kryoBuilder)
+                .withTimestampProvider((k, v) -> new WallClockTimestamp())
+                .build();
+
         cfgService.addListener(cfgListener);
         cfgService.registerConfigFactory(cfgFactory);
@@ -296,23 +317,72 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     }
     /**
-     * Returns the GroupKey object for the device and the NeighborSet given.
+     * Returns the vlan-id assigned to the subnet configured for a device.
-     * XXX is this called
+     * If no vlan-id has been assigned, a new one is assigned out of a pool of ids,
+     * if and only if this controller instance is the master for the device.
+     * <p>
+     * USAGE: The assigned vlans are meant to be applied to untagged packets on those
+     * switches/pipelines that need this functionality. These vids are meant
+     * to be used internally within a switch, and thus need to be unique only
+     * on a switch level. Note that packets never go out on the wire with these
+     * vlans. Currently, vlan ids are assigned from value 4093 down.
+     * Vlan id 4094 expected to be used for all ports that are not assigned subnets.
+     * Vlan id 4095 is reserved and unused. Only a single vlan id is assigned
+     * per subnet.
+     * XXX This method should avoid any vlans configured on the ports, but
+     *     currently the app works only on untagged packets and as a result
+     *     ignores any vlan configuration.
      *
-     * @param ns NeightborSet object for the GroupKey
+     * @param deviceId switch dpid
-     * @return GroupKey object for the NeighborSet
+     * @param subnet IPv4 prefix for which assigned vlan is desired
+     * @return VlanId assigned for the subnet on the device, or
+     *         null if no vlan assignment was found and this instance is not
+     *         the master for the device.
      */
-    public GroupKey getGroupKey(NeighborSet ns) {
+    public VlanId getSubnetAssignedVlanId(DeviceId deviceId, Ip4Prefix subnet) {
-        for (DefaultGroupHandler groupHandler : groupHandlerMap.values()) {
+        VlanId assignedVid = subnetVidStore.get(new SubnetAssignedVidStoreKey(
-            return groupHandler.getGroupKey(ns);
+                                                        deviceId, subnet));
+        if (assignedVid != null) {
+            log.debug("Query for subnet:{} on device:{} returned assigned-vlan "
+                    + "{}", subnet, deviceId, assignedVid);
+            return assignedVid;
+        }
+        //check mastership for the right to assign a vlan
+        if (!mastershipService.isLocalMaster(deviceId)) {
+            log.warn("This controller instance is not the master for device {}. "
+                    + "Cannot assign vlan-id for subnet {}", deviceId, subnet);
+            return null;
+        }
+        // vlan assignment is expensive but done only once
+        List<Ip4Prefix> configuredSubnets = deviceConfiguration.getSubnets(deviceId);
+        Set<Short> assignedVlans = new HashSet<>();
+        Set<Ip4Prefix> unassignedSubnets = new HashSet<>();
+        for (Ip4Prefix sub : configuredSubnets) {
+            VlanId v = subnetVidStore.get(new SubnetAssignedVidStoreKey(deviceId,
+                                                                        sub));
+            if (v != null) {
+                assignedVlans.add(v.toShort());
+            } else {
+                unassignedSubnets.add(sub);
+            }
+        }
+        short nextAssignedVlan = ASSIGNED_VLAN_START;
+        if (!assignedVlans.isEmpty()) {
+            nextAssignedVlan = (short) (Collections.min(assignedVlans) - 1);
+        }
+        for (Ip4Prefix unsub : unassignedSubnets) {
+            subnetVidStore.put(new SubnetAssignedVidStoreKey(deviceId, unsub),
+                               VlanId.vlanId(nextAssignedVlan--));
+            log.info("Assigned vlan: {} to subnet: {} on device: {}",
+                      nextAssignedVlan + 1, unsub, deviceId);
         }
-        return null;
+        return subnetVidStore.get(new SubnetAssignedVidStoreKey(deviceId, subnet));
     }
     /**
-     * Returns the next objective ID for the NeighborSet given. If the nextObjectiveID does not exist,
+     * Returns the next objective ID for the given NeighborSet.
-     * a new one is created and returned.
+     * If the nextObjectiveID does not exist, a new one is created and returned.
      *
      * @param deviceId Device ID
      * @param ns NegighborSet
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SubnetAssignedVidStoreKey.java 0 → 100644
View file @0e99e2b
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SubnetAssignedVidStoreKey.java 0 → 100644
View file @0e99e2b
+package org.onosproject.segmentrouting;
+
+import java.util.Objects;
+
+import org.onlab.packet.Ip4Prefix;
+import org.onosproject.net.DeviceId;
+
+/**
+ * Class definition for key used to map per device subnets to assigned Vlan ids.
+ *
+ */
+public class SubnetAssignedVidStoreKey {
+    private final DeviceId deviceId;
+    private final Ip4Prefix subnet;
+
+    public SubnetAssignedVidStoreKey(DeviceId deviceId, Ip4Prefix subnet) {
+        this.deviceId = deviceId;
+        this.subnet = subnet;
+    }
+
+    /**
+     * Returns the device identification used to create this key.
+     *
+     * @return the device identifier
+     */
+    public DeviceId deviceId() {
+        return deviceId;
+    }
+
+    /**
+     * Returns the subnet information used to create this key.
+     *
+     * @return the subnet
+     */
+    public Ip4Prefix subnet() {
+        return subnet;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (!(o instanceof SubnetAssignedVidStoreKey)) {
+            return false;
+        }
+        SubnetAssignedVidStoreKey that =
+                (SubnetAssignedVidStoreKey) o;
+        return (Objects.equals(this.deviceId, that.deviceId) &&
+                Objects.equals(this.subnet, that.subnet));
+    }
+
+    @Override
+    public int hashCode() {
+        int result = 17;
+        result = 31 * result + Objects.hashCode(deviceId)
+                + Objects.hashCode(subnet);
+        return result;
+    }
+
+    @Override
+    public String toString() {
+        return "Device: " + deviceId + " Subnet: " + subnet;
+    }
+
+}
--- a/drivers/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
View file @0e99e2b
+++ b/drivers/src/main/java/org/onosproject/driver/pipeline/OFDPA2Pipeline.java
View file @0e99e2b
@@ -148,12 +148,6 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
     //private static final int MPLSINTERFACEMASK = 0x90000000;
     private static final int L3ECMPMASK = 0x70000000;
-    /*
-     * This driver assigns all incoming untagged packets the same VLAN ID
-     */
-    private static final short UNTAGGED_ASSIGNED_VLAN = 0xffa; // 4090
-
-
     private final Logger log = getLogger(getClass());
     private ServiceDirectory serviceDirectory;
     protected FlowRuleService flowRuleService;
@@ -351,11 +345,35 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
             }
         }
+        VlanId assignedVlan = null;
+        if (vidCriterion != null && vidCriterion.vlanId() == VlanId.NONE) {
+            // untagged packets are assigned vlans in OF-DPA
+            if (filt.meta() == null) {
+                log.error("Missing metadata in filtering objective required "
+                        + "for vlan assignment in dev {}", deviceId);
+                fail(filt, ObjectiveError.BADPARAMS);
+                return;
+            }
+            for (Instruction i : filt.meta().allInstructions()) {
+                if (i instanceof ModVlanIdInstruction) {
+                    assignedVlan = ((ModVlanIdInstruction) i).vlanId();
+                }
+            }
+            if (assignedVlan == null) {
+                log.error("Driver requires an assigned vlan-id to tag incoming "
+                        + "untagged packets. Not processing vlan filters on "
+                        + "device {}", deviceId);
+                fail(filt, ObjectiveError.BADPARAMS);
+                return;
+            }
+        }
+
         if (ethCriterion == null) {
             log.debug("filtering objective missing dstMac, cannot program TMAC table");
         } else {
             for (FlowRule tmacRule : processEthDstFilter(portCriterion, ethCriterion,
-                                                         vidCriterion, applicationId)) {
+                                                         vidCriterion, assignedVlan,
+                                                         applicationId)) {
                 log.debug("adding MAC filtering rules in TMAC table: {} for dev: {}",
                           tmacRule, deviceId);
                 ops = install ? ops.add(tmacRule) : ops.remove(tmacRule);
@@ -367,6 +385,7 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
                     + "Vlan Table");
         } else {
             for (FlowRule vlanRule : processVlanIdFilter(portCriterion, vidCriterion,
+                                                         assignedVlan,
                                                          applicationId)) {
                 log.debug("adding VLAN filtering rule in VLAN table: {} for dev: {}",
                           vlanRule, deviceId);
@@ -419,14 +438,18 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
     /**
      * Allows untagged packets into pipeline by assigning a vlan id.
+     * Vlan assignment is done by the application.
      * Allows tagged packets into pipeline as per configured port-vlan info.
+     *
      * @param portCriterion   port on device for which this filter is programmed
      * @param vidCriterion   vlan assigned to port, or NONE for untagged
+     * @param assignedVlan   assigned vlan-id for untagged packets
      * @param applicationId  for application programming this filter
      * @return list of FlowRule for port-vlan filters
      */
     protected List<FlowRule> processVlanIdFilter(PortCriterion portCriterion,
                                                  VlanIdCriterion vidCriterion,
+                                                 VlanId assignedVlan,
                                                  ApplicationId applicationId) {
         List<FlowRule> rules = new ArrayList<FlowRule>();
         TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
@@ -434,7 +457,7 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
         selector.matchVlanId(vidCriterion.vlanId());
         if (vidCriterion.vlanId() == VlanId.NONE) {
             // untagged packets are assigned vlans
-            treatment.pushVlan().setVlanId(VlanId.vlanId(UNTAGGED_ASSIGNED_VLAN));
+            treatment.pushVlan().setVlanId(assignedVlan);
             // XXX ofdpa may require an additional vlan match on the assigned vlan
             // and it may not require the push.
         }
@@ -475,6 +498,7 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
      * @param portCriterion  port on device for which this filter is programmed
      * @param ethCriterion   dstMac of device for which is filter is programmed
      * @param vidCriterion   vlan assigned to port, or NONE for untagged
+     * @param assignedVlan   assigned vlan-id for untagged packets
      * @param applicationId  for application programming this filter
      * @return list of FlowRule for port-vlan filters
@@ -482,11 +506,11 @@ public class OFDPA2Pipeline extends AbstractHandlerBehaviour implements Pipeline
     protected List<FlowRule> processEthDstFilter(PortCriterion portCriterion,
                                                  EthCriterion ethCriterion,
                                                  VlanIdCriterion vidCriterion,
+                                                 VlanId assignedVlan,
                                                  ApplicationId applicationId) {
         //handling untagged packets via assigned VLAN
         if (vidCriterion.vlanId() == VlanId.NONE) {
-            vidCriterion = (VlanIdCriterion) Criteria
+            vidCriterion = (VlanIdCriterion) Criteria.matchVlanId(assignedVlan);
-                                .matchVlanId(VlanId.vlanId(UNTAGGED_ASSIGNED_VLAN));
         }
         // ofdpa cannot match on ALL portnumber, so we need to use separate
         // rules for each port.