ONOS-3387 Adding ability for network configurations to be validated before being…

… accepted into the system.

Change-Id: I26a7e2adb20318cf17a35081ff753b3448105e31

@@ -21,6 +21,9 @@ import org.onosproject.core.ApplicationId;
 import org.onosproject.net.config.Config;
 import org.onosproject.net.config.basics.BasicElementConfig;
 
+import static org.onosproject.net.config.Config.FieldPresence.MANDATORY;
+import static org.onosproject.net.config.Config.FieldPresence.OPTIONAL;
+
 /**
  * DHCP Config class.
  */
@@ -43,6 +46,20 @@ public class DhcpConfig extends Config<ApplicationId> {
 
     public static final int DEFAULT = -1;
 
+    @Override
+    public boolean isValid() {
+        // FIXME: Sweep through and revisit the validation assertions
+        // For now, this is just a demonstration of potential uses
+        return hasOnlyFields(MY_IP, MY_MAC, SUBNET_MASK, BROADCAST_ADDRESS,
+                             ROUTER_ADDRESS, DOMAIN_SERVER, TTL, LEASE_TIME,
+                             RENEW_TIME, REBIND_TIME, TIMER_DELAY, DEFAULT_TIMEOUT,
+                             START_IP, END_IP) &&
+                isIpAddress(MY_IP, MANDATORY) && isMacAddress(MY_MAC, MANDATORY) &&
+                isIpAddress(START_IP, MANDATORY) && isIpAddress(END_IP, MANDATORY) &&
+                isNumber(LEASE_TIME, OPTIONAL, 1) && isNumber(REBIND_TIME, OPTIONAL, 1) &&
+                isNumber(DEFAULT_TIMEOUT, OPTIONAL, 1, 3600);
+    }
+
     /**
      * Returns the dhcp server ip.
      *

@@ -20,10 +20,15 @@ import com.fasterxml.jackson.databind.ObjectMapper;
 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.Iterators;
 import com.google.common.collect.Lists;
+import org.onlab.packet.IpAddress;
+import org.onlab.packet.MacAddress;
 
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 import java.util.function.Function;
 
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -51,6 +56,21 @@ public abstract class Config<S> {
     protected ConfigApplyDelegate delegate;
 
     /**
+     * Indicator of whether a configuration JSON field is required.
+     */
+    public enum FieldPresence {
+        /**
+         * Signifies that config field is an optional one.
+         */
+        OPTIONAL,
+
+        /**
+         * Signifies that config field is mandatory.
+         */
+        MANDATORY
+    }
+
+    /**
      * Initializes the configuration behaviour with necessary context.
      *
      * @param subject  configuration subject
@@ -71,6 +91,29 @@ public abstract class Config<S> {
     }
 
     /**
+     * Indicates whether or not the backing JSON node contains valid data.
+     * <p>
+     * Default implementation returns true.
+     * Subclasses are expected to override this with their own validation.
+     * </p>
+     *
+     * @return true if the data is valid; false otherwise
+     */
+    public boolean isValid() {
+        // TODO: figure out what assertions could be made in the base class
+        // NOTE: The thought is to have none, but instead to provide a set
+        // of predicates to allow configs to test validity of present fields,
+        // e.g.:
+        //      isString(path)
+        //      isBoolean(path)
+        //      isNumber(path, [min, max])
+        //      isDecimal(path, [min, max])
+        //      isMacAddress(path)
+        //      isIpAddress(path)
+        return true;
+    }
+
+    /**
      * Returns the specific subject to which this configuration pertains.
      *
      * @return configuration subject
@@ -309,4 +352,104 @@ public abstract class Config<S> {
         return this;
     }
 
+    /**
+     * Indicates whether only the specified fields are present in the backing JSON.
+     *
+     * @param allowedFields allowed field names
+     * @return true if all allowedFields are present; false otherwise
+     */
+    protected boolean hasOnlyFields(String... allowedFields) {
+        Set<String> fields = ImmutableSet.copyOf(allowedFields);
+        return !Iterators.any(object.fieldNames(), f -> !fields.contains(f));
+    }
+
+    /**
+     * Indicates whether the specified field holds a valid MAC address.
+     *
+     * @param field    JSON field name
+     * @param presence specifies if field is optional or mandatory
+     * @return true if valid; false otherwise
+     * @throws IllegalArgumentException if field is present, but not valid MAC
+     */
+    protected boolean isMacAddress(String field, FieldPresence presence) {
+        JsonNode node = object.path(field);
+        return isValid(node, presence, node.isTextual() &&
+                MacAddress.valueOf(node.asText()) != null);
+    }
+
+    /**
+     * Indicates whether the specified field holds a valid IP address.
+     *
+     * @param field    JSON field name
+     * @param presence specifies if field is optional or mandatory
+     * @return true if valid; false otherwise
+     * @throws IllegalArgumentException if field is present, but not valid IP
+     */
+    protected boolean isIpAddress(String field, FieldPresence presence) {
+        JsonNode node = object.path(field);
+        return isValid(node, presence, node.isTextual() &&
+                IpAddress.valueOf(node.asText()) != null);
+    }
+
+    /**
+     * Indicates whether the specified field holds a valid string value.
+     *
+     * @param field    JSON field name
+     * @param presence specifies if field is optional or mandatory
+     * @param pattern  optional regex pattern
+     * @return true if valid; false otherwise
+     * @throws IllegalArgumentException if field is present, but not valid MAC
+     */
+    protected boolean isString(String field, FieldPresence presence, String... pattern) {
+        JsonNode node = object.path(field);
+        return isValid(node, presence, node.isTextual() &&
+                (pattern.length > 0 && node.asText().matches(pattern[0]) || pattern.length < 1));
+    }
+
+    /**
+     * Indicates whether the specified field holds a valid number.
+     *
+     * @param field    JSON field name
+     * @param presence specifies if field is optional or mandatory
+     * @param minMax   optional min/max values
+     * @return true if valid; false otherwise
+     * @throws IllegalArgumentException if field is present, but not valid
+     */
+    protected boolean isNumber(String field, FieldPresence presence, long... minMax) {
+        JsonNode node = object.path(field);
+        return isValid(node, presence, (node.isLong() || node.isInt()) &&
+                (minMax.length > 0 && minMax[0] <= node.asLong() || minMax.length < 1) &&
+                (minMax.length > 1 && minMax[1] > node.asLong() || minMax.length < 2));
+    }
+
+    /**
+     * Indicates whether the specified field holds a valid decimal number.
+     *
+     * @param field    JSON field name
+     * @param presence specifies if field is optional or mandatory
+     * @param minMax   optional min/max values
+     * @return true if valid; false otherwise
+     * @throws IllegalArgumentException if field is present, but not valid
+     */
+    protected boolean isDecimal(String field, FieldPresence presence, double... minMax) {
+        JsonNode node = object.path(field);
+        return isValid(node, presence, (node.isDouble() || node.isFloat()) &&
+                (minMax.length > 0 && minMax[0] <= node.asDouble() || minMax.length < 1) &&
+                (minMax.length > 1 && minMax[1] > node.asDouble() || minMax.length < 2));
+    }
+
+    /**
+     * Indicates whether the node is present and of correct value or not
+     * mandatory and absent.
+     *
+     * @param node         JSON node
+     * @param presence     specifies if field is optional or mandatory
+     * @param correctValue true if the value is correct
+     * @return true if the field is as expected
+     */
+    private boolean isValid(JsonNode node, FieldPresence presence, boolean correctValue) {
+        boolean isMandatory = presence == FieldPresence.MANDATORY;
+        return isMandatory && correctValue || !isMandatory && !node.isNull() || correctValue;
+    }
+
 }

@@ -119,7 +119,7 @@ public interface NetworkConfigService
 
     /**
      * Applies configuration for the specified subject and configuration
-     * class using the raw JSON object. If configuration already exists, it
+     * class using the raw JSON node. If configuration already exists, it
      * will be updated.
      *
      * @param subject     configuration subject
@@ -128,6 +128,8 @@ public interface NetworkConfigService
      * @param <S>         type of subject
      * @param <C>         type of configuration
      * @return configuration or null if one is not available
+     * @throws IllegalArgumentException if the supplied JSON node contains
+     *                                  invalid data
      */
     <S, C extends Config<S>> C applyConfig(S subject, Class<C> configClass,
                                            JsonNode json);

@@ -113,6 +113,8 @@ public interface NetworkConfigStore extends Store<NetworkConfigEvent, NetworkCon
      * @param <S>         type of subject
      * @param <C>         type of configuration
      * @return configuration object
+     * @throws IllegalArgumentException if the supplied JSON node contains
+     *                                  invalid data
      */
     <S, C extends Config<S>> C applyConfig(S subject, Class<C> configClass,
                                            JsonNode json);

+/*
+ * Copyright 2014-2015 Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.net.config;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.onosproject.net.config.Config.FieldPresence.MANDATORY;
+import static org.onosproject.net.config.Config.FieldPresence.OPTIONAL;
+
+/**
+ * Test of the base network config class.
+ */
+public class ConfigTest {
+
+    private static final String SUBJECT = "subject";
+    private static final String KEY = "key";
+
+    private static final String TEXT = "text";
+    private static final String LONG = "long";
+    private static final String DOUBLE = "double";
+    private static final String MAC = "mac";
+    private static final String IP = "ip";
+
+    private final ObjectMapper mapper = new ObjectMapper();
+    private final ConfigApplyDelegate delegate = new TestDelegate();
+
+    private Config<String> cfg;
+    private JsonNode json;
+
+    @Before
+    public void setUp() {
+        json = new ObjectMapper().createObjectNode()
+                .put(TEXT, "foo").put(LONG, 5).put(DOUBLE, 0.5)
+                .put(MAC, "ab:cd:ef:ca:fe:ed").put(IP, "12.34.56.78");
+        cfg = new TestConfig();
+        cfg.init(SUBJECT, KEY, json, mapper, delegate);
+    }
+
+    @Test
+    public void hasOnlyFields() {
+        assertTrue("has unexpected fields", cfg.hasOnlyFields(TEXT, LONG, DOUBLE, MAC, IP));
+        assertFalse("did not detect unexpected fields", cfg.hasOnlyFields(TEXT, LONG, DOUBLE, MAC));
+        assertTrue("is not proper text", cfg.isString(TEXT, MANDATORY));
+    }
+
+    @Test
+    public void isString() {
+        assertTrue("is not proper text", cfg.isString(TEXT, MANDATORY));
+        assertTrue("is not proper text", cfg.isString(TEXT, MANDATORY, "^f.*"));
+        assertTrue("is not proper text", cfg.isString(TEXT, OPTIONAL, "^f.*"));
+        assertTrue("is not proper text", cfg.isString(TEXT, OPTIONAL));
+        assertTrue("is not proper text", cfg.isString("none", OPTIONAL));
+        assertFalse("did not detect missing field", cfg.isString("none", MANDATORY));
+    }
+
+    @Test
+    public void isNumber() {
+        assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY));
+        assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 0));
+        assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 0, 10));
+        assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 5, 6));
+        assertFalse("is not in range", cfg.isNumber(LONG, MANDATORY, 6, 10));
+        assertFalse("is not in range", cfg.isNumber(LONG, MANDATORY, 4, 5));
+        assertTrue("is not proper number", cfg.isNumber(LONG, OPTIONAL, 0, 10));
+        assertTrue("is not proper number", cfg.isNumber(LONG, OPTIONAL));
+        assertTrue("is not proper number", cfg.isNumber("none", OPTIONAL));
+        assertFalse("did not detect missing field", cfg.isNumber("none", MANDATORY));
+    }
+
+    @Test
+    public void isDecimal() {
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY));
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.0));
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.0, 1.0));
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.5, 0.6));
+        assertFalse("is not in range", cfg.isDecimal(DOUBLE, MANDATORY, 0.6, 1.0));
+        assertFalse("is not in range", cfg.isDecimal(DOUBLE, MANDATORY, 0.4, 0.5));
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, OPTIONAL, 0.0, 1.0));
+        assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, OPTIONAL));
+        assertTrue("is not proper decimal", cfg.isDecimal("none", OPTIONAL));
+        assertFalse("did not detect missing field", cfg.isDecimal("none", MANDATORY));
+    }
+
+    @Test
+    public void isMacAddress() {
+        assertTrue("is not proper mac", cfg.isMacAddress(MAC, MANDATORY));
+        assertTrue("is not proper mac", cfg.isMacAddress(MAC, OPTIONAL));
+        assertTrue("is not proper mac", cfg.isMacAddress("none", OPTIONAL));
+        assertFalse("did not detect missing field", cfg.isMacAddress("none", MANDATORY));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void badMacAddress() {
+        assertTrue("is not proper mac", cfg.isMacAddress(TEXT, MANDATORY));
+    }
+
+
+    @Test
+    public void isIpAddress() {
+        assertTrue("is not proper ip", cfg.isIpAddress(IP, MANDATORY));
+        assertTrue("is not proper ip", cfg.isIpAddress(IP, OPTIONAL));
+        assertTrue("is not proper ip", cfg.isIpAddress("none", OPTIONAL));
+        assertFalse("did not detect missing field", cfg.isMacAddress("none", MANDATORY));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void badIpAddress() {
+        assertTrue("is not proper ip", cfg.isIpAddress(TEXT, MANDATORY));
+    }
+
+
+    // TODO: Add tests for other helper methods
+
+    private class TestConfig extends Config<String> {
+    }
+
+    private class TestDelegate implements ConfigApplyDelegate {
+        @Override
+        public void onApply(Config config) {
+        }
+    }
+}
\ No newline at end of file

@@ -60,6 +60,7 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static org.onosproject.net.config.NetworkConfigEvent.Type.*;
 
 /**
@@ -71,10 +72,12 @@ public class DistributedNetworkConfigStore
         extends AbstractStore<NetworkConfigEvent, NetworkConfigStoreDelegate>
         implements NetworkConfigStore {
 
-    private static final int MAX_BACKOFF = 10;
-
     private final Logger log = LoggerFactory.getLogger(getClass());
 
+    private static final int MAX_BACKOFF = 10;
+    private static final String INVALID_CONFIG_JSON =
+            "JSON node does not contain valid configuration";
+
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected StorageService storageService;
 
@@ -187,8 +190,17 @@ public class DistributedNetworkConfigStore
 
     @Override
     public <S, C extends Config<S>> C applyConfig(S subject, Class<C> configClass, JsonNode json) {
-        return createConfig(subject, configClass,
-                            configs.putAndGet(key(subject, configClass), json).value());
+        // Create the configuration and validate it.
+        C config = createConfig(subject, configClass, json);
+        checkArgument(config.isValid(), INVALID_CONFIG_JSON);
+
+        // Insert the validated configuration and get it back.
+        Versioned<JsonNode> versioned = configs.putAndGet(key(subject, configClass), json);
+
+        // Re-create the config if for some reason what we attempted to put
+        // was supplanted by someone else already.
+        return versioned.value() == json ? config :
+                createConfig(subject, configClass, versioned.value());
     }
 
     @Override