Implement vlan-untagged, vlan-tagged, vlan-native in interface config

Existing vlan config is ambiguous and different apps could have different interpretation.
In this commit, we introduce three more vlan configs and hope this will eventually replace the original one.

Change-Id: If8dd985cc3a420601073797eb617ffd1adf90d1d

cli/src/main/java/org/onosproject/cli/net/InterfacesListCommand.java

@@ -38,6 +38,9 @@ public class InterfacesListCommand extends AbstractShellCommand {
     private static final String IP_FORMAT = " ips=";
     private static final String MAC_FORMAT = " mac=";
     private static final String VLAN_FORMAT = " vlan=";
+    private static final String VLAN_UNTAGGED = " vlanUntagged=";
+    private static final String VLAN_TAGGED = " vlanTagged=";
+    private static final String VLAN_NATIVE = " vlanNative=";
 
     private static final String NO_NAME = "(unamed)";
 
@@ -70,6 +73,21 @@ public class InterfacesListCommand extends AbstractShellCommand {
             formatStringBuilder.append(intf.vlan().toString());
         }
 
+        if (!intf.vlanUntagged().equals(VlanId.NONE)) {
+            formatStringBuilder.append(VLAN_UNTAGGED);
+            formatStringBuilder.append(intf.vlanUntagged().toString());
+        }
+
+        if (!intf.vlanTagged().isEmpty()) {
+            formatStringBuilder.append(VLAN_TAGGED);
+            formatStringBuilder.append(intf.vlanTagged().toString());
+        }
+
+        if (!intf.vlanNative().equals(VlanId.NONE)) {
+            formatStringBuilder.append(VLAN_NATIVE);
+            formatStringBuilder.append(intf.vlanNative().toString());
+        }
+
         String name = (intf.name().equals(Interface.NO_INTERFACE_NAME)) ?
                       NO_NAME : intf.name();
 

incubator/api/src/main/java/org/onosproject/incubator/net/config/basics/InterfaceConfig.java

@@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.google.common.annotations.Beta;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import org.onlab.packet.IpPrefix;
@@ -29,6 +30,8 @@ import org.onosproject.incubator.net.intf.Interface;
 import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.config.Config;
 import org.onosproject.net.host.InterfaceIpAddress;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Iterator;
 import java.util.List;
@@ -42,10 +45,15 @@ import static com.google.common.base.Preconditions.checkNotNull;
  */
 @Beta
 public class InterfaceConfig extends Config<ConnectPoint> {
+    private static Logger log = LoggerFactory.getLogger(InterfaceConfig.class);
+
     public static final String NAME = "name";
     public static final String IPS = "ips";
     public static final String MAC = "mac";
     public static final String VLAN = "vlan";
+    public static final String VLAN_UNTAGGED = "vlan-untagged";
+    public static final String VLAN_TAGGED = "vlan-tagged";
+    public static final String VLAN_NATIVE = "vlan-native";
 
     private static final String CONFIG_VALUE_ERROR = "Error parsing config value";
     private static final String INTF_NULL_ERROR = "Interface cannot be null";
@@ -54,7 +62,8 @@ public class InterfaceConfig extends Config<ConnectPoint> {
     @Override
     public boolean isValid() {
         for (JsonNode node : array) {
-            if (!hasOnlyFields((ObjectNode) node, NAME, IPS, MAC, VLAN)) {
+            if (!hasOnlyFields((ObjectNode) node, NAME, IPS, MAC, VLAN,
+                    VLAN_UNTAGGED, VLAN_TAGGED, VLAN_NATIVE)) {
                 return false;
             }
 
@@ -62,16 +71,41 @@ public class InterfaceConfig extends Config<ConnectPoint> {
 
             if (!(isString(obj, NAME, FieldPresence.OPTIONAL) &&
                     isMacAddress(obj, MAC, FieldPresence.OPTIONAL) &&
-                    isIntegralNumber(obj, VLAN, FieldPresence.OPTIONAL, 0, VlanId.MAX_VLAN))) {
+                    isIntegralNumber(obj, VLAN, FieldPresence.OPTIONAL, 0, VlanId.MAX_VLAN) &&
+                    isIntegralNumber(obj, VLAN_UNTAGGED, FieldPresence.OPTIONAL, 0, VlanId.MAX_VLAN) &&
+                    isIntegralNumber(obj, VLAN_NATIVE, FieldPresence.OPTIONAL, 0, VlanId.MAX_VLAN))) {
                 return false;
             }
 
-
             for (JsonNode ipNode : node.path(IPS)) {
                 if (!ipNode.isTextual() || IpPrefix.valueOf(ipNode.asText()) == null) {
                     return false;
                 }
             }
+
+            checkArgument(!hasField(obj, VLAN_TAGGED) ||
+                            (node.path(VLAN_TAGGED).isArray() && node.path(VLAN_TAGGED).size() >= 1),
+                    "%s must be an array with at least one element", VLAN_TAGGED);
+
+            for (JsonNode vlanNode : node.path(VLAN_TAGGED)) {
+                checkArgument(vlanNode.isInt() &&
+                        vlanNode.intValue() >= 0 &&  vlanNode.intValue() <= VlanId.MAX_VLAN,
+                        "Invalid VLAN ID %s in %s", vlanNode.intValue(), VLAN_TAGGED);
+            }
+
+            checkArgument(!hasField(obj, VLAN_UNTAGGED) ||
+                    !(hasField(obj, VLAN_TAGGED) || hasField(obj, VLAN_NATIVE)),
+                    "%s and %s should not be used when %s is set", VLAN_TAGGED, VLAN_NATIVE, VLAN_UNTAGGED);
+
+            checkArgument(!hasField(obj, VLAN_TAGGED) || !hasField(obj, VLAN_UNTAGGED),
+                    "%s should not be used when %s is set", VLAN_UNTAGGED, VLAN_TAGGED);
+
+            checkArgument(!hasField(obj, VLAN_NATIVE) || hasField(obj, VLAN_TAGGED),
+                    "%s should not be used alone without %s", VLAN_NATIVE, VLAN_TAGGED);
+
+            checkArgument(!hasField(obj, VLAN_NATIVE) || !hasField(obj, VLAN_TAGGED) ||
+                    !getVlans(obj, VLAN_TAGGED).contains(getVlan(obj, VLAN_NATIVE)),
+                    "%s cannot be one of the VLANs configured in %s", VLAN_NATIVE, VLAN_TAGGED);
         }
         return true;
     }
@@ -94,9 +128,13 @@ public class InterfaceConfig extends Config<ConnectPoint> {
                 String mac = intfNode.path(MAC).asText();
                 MacAddress macAddr = mac.isEmpty() ? null : MacAddress.valueOf(mac);
 
-                VlanId vlan = getVlan(intfNode);
+                VlanId vlan = getVlan(intfNode, VLAN);
+                VlanId vlanUntagged = getVlan(intfNode, VLAN_UNTAGGED);
+                Set<VlanId> vlanTagged = getVlans(intfNode, VLAN_TAGGED);
+                VlanId vlanNative = getVlan(intfNode, VLAN_NATIVE);
 
-                interfaces.add(new Interface(name, subject, ips, macAddr, vlan));
+                interfaces.add(new Interface(name, subject, ips, macAddr, vlan,
+                        vlanUntagged, vlanTagged, vlanNative));
             }
         } catch (IllegalArgumentException e) {
             throw new ConfigException(CONFIG_VALUE_ERROR, e);
@@ -125,13 +163,25 @@ public class InterfaceConfig extends Config<ConnectPoint> {
             intfNode.put(MAC, intf.mac().toString());
         }
 
-        if (!intf.ipAddresses().isEmpty()) {
+        if (!intf.ipAddressesList().isEmpty()) {
             intfNode.set(IPS, putIps(intf.ipAddressesList()));
         }
 
         if (!intf.vlan().equals(VlanId.NONE)) {
             intfNode.put(VLAN, intf.vlan().toString());
         }
+
+        if (!intf.vlanUntagged().equals(VlanId.NONE)) {
+            intfNode.put(VLAN_UNTAGGED, intf.vlanUntagged().toString());
+        }
+
+        if (!intf.vlanTagged().isEmpty()) {
+            intfNode.set(VLAN_UNTAGGED, putVlans(intf.vlanTagged()));
+        }
+
+        if (!intf.vlanNative().equals(VlanId.NONE)) {
+            intfNode.put(VLAN_NATIVE, intf.vlanNative().toString());
+        }
     }
 
     /**
@@ -153,14 +203,48 @@ public class InterfaceConfig extends Config<ConnectPoint> {
         }
     }
 
-    private VlanId getVlan(JsonNode node) {
+    /**
+     * Extracts VLAN ID from given path of given json node.
+     *
+     * @param node JSON node
+     * @param path path
+     * @return VLAN ID
+     */
+    private VlanId getVlan(JsonNode node, String path) {
         VlanId vlan = VlanId.NONE;
-        if (!node.path(VLAN).isMissingNode()) {
+        if (!node.path(path).isMissingNode()) {
-            vlan = VlanId.vlanId(Short.valueOf(node.path(VLAN).asText()));
+            vlan = VlanId.vlanId(Short.valueOf(node.path(path).asText()));
         }
         return vlan;
     }
 
+    /**
+     * Extracts a set of VLAN ID from given path of given json node.
+     *
+     * @param node JSON node
+     * @param path path
+     * @return a set of VLAN ID
+     */
+    private Set<VlanId> getVlans(JsonNode node, String path) {
+        ImmutableSet.Builder<VlanId> vlanIdBuilder = ImmutableSet.builder();
+
+        JsonNode vlansNode = node.get(path);
+        if (vlansNode != null) {
+            vlansNode.forEach(vlanNode ->
+                    vlanIdBuilder.add(VlanId.vlanId(vlanNode.shortValue())));
+        }
+
+        return vlanIdBuilder.build();
+    }
+
+    private ArrayNode putVlans(Set<VlanId> vlans) {
+        ArrayNode vlanArray = mapper.createArrayNode();
+
+        vlans.forEach(vlan -> vlanArray.add(vlan.toShort()));
+
+        return vlanArray;
+    }
+
     private List<InterfaceIpAddress> getIps(JsonNode node) {
         List<InterfaceIpAddress> ips = Lists.newArrayList();
 

incubator/api/src/main/java/org/onosproject/incubator/net/intf/Interface.java

@@ -17,6 +17,7 @@ package org.onosproject.incubator.net.intf;
 
 import com.google.common.annotations.Beta;
 import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.VlanId;
@@ -42,7 +43,11 @@ public class Interface {
     private final ConnectPoint connectPoint;
     private final List<InterfaceIpAddress> ipAddresses;
     private final MacAddress macAddress;
+    // TODO: Deprecate this due to ambiguity
     private final VlanId vlan;
+    private final VlanId vlanUntagged;
+    private final Set<VlanId> vlanTagged;
+    private final VlanId vlanNative;
 
     /**
      * Creates new Interface with the provided configuration.
@@ -56,11 +61,36 @@ public class Interface {
     public Interface(String name, ConnectPoint connectPoint,
                      List<InterfaceIpAddress> ipAddresses,
                      MacAddress macAddress, VlanId vlan) {
+        this(name, connectPoint, ipAddresses, macAddress, vlan, null, null, null);
+    }
+
+    /**
+     * Creates new Interface with the provided configuration.
+     *
+     * @param name name of the interface
+     * @param connectPoint the connect point this interface maps to
+     * @param ipAddresses list of IP addresses
+     * @param macAddress MAC address
+     * @param vlan VLAN ID
+     * @param vlanUntagged untagged VLAN.
+     *                     Cannot be used with vlanTagged or vlanNative.
+     * @param vlanTagged   tagged VLANs.
+     *                     Cannot be used with vlanUntagged.
+     * @param vlanNative   native vLAN. Optional.
+     *                     Can only be used when vlanTagged is specified. Cannot be used with vlanUntagged.
+     */
+    public Interface(String name, ConnectPoint connectPoint,
+                     List<InterfaceIpAddress> ipAddresses,
+                     MacAddress macAddress, VlanId vlan,
+                     VlanId vlanUntagged, Set<VlanId> vlanTagged, VlanId vlanNative) {
         this.name = name == null ? NO_INTERFACE_NAME : name;
         this.connectPoint = checkNotNull(connectPoint);
         this.ipAddresses = ipAddresses == null ? Lists.newArrayList() : ipAddresses;
         this.macAddress = macAddress == null ? MacAddress.NONE : macAddress;
         this.vlan = vlan == null ? VlanId.NONE : vlan;
+        this.vlanUntagged = vlanUntagged == null ? VlanId.NONE : vlanUntagged;
+        this.vlanTagged = vlanTagged == null ? ImmutableSet.of() : ImmutableSet.copyOf(vlanTagged);
+        this.vlanNative = vlanNative == null ? VlanId.NONE : vlanNative;
     }
 
     /**
@@ -120,6 +150,33 @@ public class Interface {
         return vlan;
     }
 
+    /**
+     * Retrieves the VLAN ID that will be assign to untagged packets.
+     *
+     * @return the VLAN ID
+     */
+    public VlanId vlanUntagged() {
+        return vlanUntagged;
+    }
+
+    /**
+     * Retrieves the set of VLAN IDs that will be allowed on this interface.
+     *
+     * @return the VLAN ID
+     */
+    public Set<VlanId> vlanTagged() {
+        return vlanTagged;
+    }
+
+    /**
+     * Retrieves the set of VLAN IDs that will be allowed on this interface.
+     *
+     * @return the VLAN ID
+     */
+    public VlanId vlanNative() {
+        return vlanNative;
+    }
+
     @Override
     public boolean equals(Object other) {
         if (!(other instanceof Interface)) {
@@ -132,12 +189,16 @@ public class Interface {
                 Objects.equals(connectPoint, otherInterface.connectPoint) &&
                 Objects.equals(ipAddresses, otherInterface.ipAddresses) &&
                 Objects.equals(macAddress, otherInterface.macAddress) &&
-                Objects.equals(vlan, otherInterface.vlan);
+                Objects.equals(vlan, otherInterface.vlan) &&
+                Objects.equals(vlanUntagged, otherInterface.vlanUntagged) &&
+                Objects.equals(vlanTagged, otherInterface.vlanTagged) &&
+                Objects.equals(vlanNative, otherInterface.vlanNative);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(connectPoint, name, ipAddresses, macAddress, vlan);
+        return Objects.hash(connectPoint, name, ipAddresses, macAddress, vlan,
+                vlanUntagged, vlanTagged, vlanNative);
     }
 
     @Override
@@ -148,6 +209,9 @@ public class Interface {
                 .add("ipAddresses", ipAddresses)
                 .add("macAddress", macAddress)
                 .add("vlan", vlan)
+                .add("vlanUntagged", vlanUntagged)
+                .add("vlanTagged", vlanTagged)
+                .add("vlanNative", vlanNative)
                 .toString();
     }
 }