ONOS-3387 Adding ability for network configurations to be validated before being accepted into the system.
Change-Id: I26a7e2adb20318cf17a35081ff753b3448105e31
diff --git a/apps/dhcp/app/src/main/java/org/onosproject/dhcp/impl/DhcpConfig.java b/apps/dhcp/app/src/main/java/org/onosproject/dhcp/impl/DhcpConfig.java
index 4353d62..1efdd08 100644
--- a/apps/dhcp/app/src/main/java/org/onosproject/dhcp/impl/DhcpConfig.java
+++ b/apps/dhcp/app/src/main/java/org/onosproject/dhcp/impl/DhcpConfig.java
@@ -21,6 +21,9 @@
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 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.
*
diff --git a/core/api/src/main/java/org/onosproject/net/config/Config.java b/core/api/src/main/java/org/onosproject/net/config/Config.java
index 5cdc0c1..3757d32 100644
--- a/core/api/src/main/java/org/onosproject/net/config/Config.java
+++ b/core/api/src/main/java/org/onosproject/net/config/Config.java
@@ -20,10 +20,15 @@
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 @@
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 @@
}
/**
+ * 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 @@
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;
+ }
+
}
diff --git a/core/api/src/main/java/org/onosproject/net/config/NetworkConfigService.java b/core/api/src/main/java/org/onosproject/net/config/NetworkConfigService.java
index 8eb69a45..f1b22c4 100644
--- a/core/api/src/main/java/org/onosproject/net/config/NetworkConfigService.java
+++ b/core/api/src/main/java/org/onosproject/net/config/NetworkConfigService.java
@@ -119,7 +119,7 @@
/**
* 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 @@
* @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);
diff --git a/core/api/src/main/java/org/onosproject/net/config/NetworkConfigStore.java b/core/api/src/main/java/org/onosproject/net/config/NetworkConfigStore.java
index 9dd66e8..9be4b12 100644
--- a/core/api/src/main/java/org/onosproject/net/config/NetworkConfigStore.java
+++ b/core/api/src/main/java/org/onosproject/net/config/NetworkConfigStore.java
@@ -113,6 +113,8 @@
* @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);
diff --git a/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java b/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java
new file mode 100644
index 0000000..34b0fe7
--- /dev/null
+++ b/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java
@@ -0,0 +1,141 @@
+/*
+ * 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
diff --git a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
index 3e73d8f..ca8eea3 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
@@ -60,6 +60,7 @@
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 @@
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 @@
@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