Validate IP addresses when serializing

3 files changed, 44 insertions, 36 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index 442013b8a6a..0ce0c76665f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.provision;
 
 import com.google.common.collect.ImmutableSet;
-import com.google.common.net.InetAddresses;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterMembership;
 import com.yahoo.config.provision.Flavor;
@@ -67,7 +66,7 @@ public final class Node {
                 Flavor flavor, Status status, State state, Optional<Allocation> allocation,
                 History history, NodeType type) {
         Objects.requireNonNull(id, "A node must have an ID");
-        requireIpAddresses(ipAddresses, "A node must have at least one valid IP address");
+        requireNonEmpty(ipAddresses, "A node must have at least one valid IP address");
         requireNonEmptyString(hostname, "A node must have a hostname");
         requireNonEmptyString(parentHostname, "A parent host name must be a proper value");
         Objects.requireNonNull(flavor, "A node must have a flavor");
@@ -261,8 +260,7 @@ public final class Node {
 
     private static void requireNonEmptyString(Optional<String> value, String message) {
         Objects.requireNonNull(value, message);
-        if (value.isPresent())
-            requireNonEmptyString(value.get(), message);
+        value.ifPresent(v -> requireNonEmptyString(v, message));
     }
 
     private static void requireNonEmptyString(String value, String message) {
@@ -271,15 +269,10 @@ public final class Node {
             throw new IllegalArgumentException(message + ", but was '" + value + "'");
     }
 
-    private static void requireIpAddresses(Set<String> values, String message) {
-        if (values.isEmpty()) {
+    private static void requireNonEmpty(Set<String> values, String message) {
+        if (values == null || values.isEmpty()) {
             throw new IllegalArgumentException(message);
         }
-        try {
-            values.forEach(InetAddresses::forString);
-        } catch (IllegalArgumentException e) {
-            throw new IllegalArgumentException(message, e);
-        }
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
index fbc358893e1..14609809f54 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
@@ -59,7 +59,7 @@ public class IP {
 
         public AddressPool(Node owner, Set<String> addresses) {
             this.owner = Objects.requireNonNull(owner, "owner must be non-null");
-            this.addresses = ImmutableSet.copyOf(requireAddresses(addresses));
+            this.addresses = ImmutableSet.copyOf(Objects.requireNonNull(addresses, "addresses must be non-null"));
         }
 
         /**
@@ -112,25 +112,6 @@ public class IP {
             return Objects.hash(addresses);
         }
 
-        private static Set<String> requireAddresses(Set<String> addresses) {
-            Objects.requireNonNull(addresses, "ipAddressPool must be non-null");
-
-            long ipv6AddrCount = addresses.stream().filter(IP::isV6).count();
-            if (ipv6AddrCount == addresses.size()) {
-                return addresses; // IPv6-only pool is valid
-            }
-
-            long ipv4AddrCount = addresses.stream().filter(IP::isV4).count();
-            if (ipv4AddrCount == ipv6AddrCount) {
-                return addresses;
-            }
-
-            throw new IllegalArgumentException(String.format("Dual-stacked IP address pool must have an " +
-                                                             "equal number of addresses of each version " +
-                                                             "[IPv6 address count = %d, IPv4 address count = %d]",
-                                                             ipv6AddrCount, ipv4AddrCount));
-        }
-
     }
 
     /** An IP address allocation from a pool */
@@ -224,4 +205,36 @@ public class IP {
         return InetAddresses.forString(ipAddress) instanceof Inet6Address;
     }
 
+    /** Validates and returns the given set of IP addresses */
+    public static Set<String> requireAddresses(Set<String> addresses) {
+        String message = "A node must have at least one valid IP address";
+        if (addresses.isEmpty()) {
+            throw new IllegalArgumentException(message);
+        }
+        try {
+            addresses.forEach(InetAddresses::forString);
+        } catch (IllegalArgumentException e) {
+            throw new IllegalArgumentException(message, e);
+        }
+        return addresses;
+    }
+
+    /** Validates and returns the given IP address pool */
+    public static Set<String> requireAddressPool(Set<String> addresses) {
+        long ipv6AddrCount = addresses.stream().filter(IP::isV6).count();
+        if (ipv6AddrCount == addresses.size()) {
+            return addresses; // IPv6-only pool is valid
+        }
+
+        long ipv4AddrCount = addresses.stream().filter(IP::isV4).count();
+        if (ipv4AddrCount == ipv6AddrCount) {
+            return addresses;
+        }
+
+        throw new IllegalArgumentException(String.format("Dual-stacked IP address list must have an " +
+                                                         "equal number of addresses of each version " +
+                                                         "[IPv6 address count = %d, IPv4 address count = %d]",
+                                                         ipv6AddrCount, ipv4AddrCount));
+    }
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
index f96ecc0431a..6374ad9ce91 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
@@ -30,6 +30,7 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.UnaryOperator;
 
 /**
  * Serializes a node to/from JSON.
@@ -99,8 +100,8 @@ public class NodeSerializer {
 
     private void toSlime(Node node, Cursor object) {
         object.setString(hostnameKey, node.hostname());
-        toSlime(node.ipAddresses(), object.setArray(ipAddressesKey));
-        toSlime(node.ipAddressPool().asSet(), object.setArray(ipAddressPoolKey));
+        toSlime(node.ipAddresses(), object.setArray(ipAddressesKey), IP::requireAddresses);
+        toSlime(node.ipAddressPool().asSet(), object.setArray(ipAddressPoolKey), IP::requireAddressPool);
         object.setString(idKey, node.id());
         node.parentHostname().ifPresent(hostname -> object.setString(parentHostnameKey, hostname));
         object.setString(flavorKey, node.flavor().name());
@@ -141,8 +142,9 @@ public class NodeSerializer {
         object.setString(agentKey, toString(event.agent()));
     }
 
-    private void toSlime(Set<String> ipAddresses, Cursor array) {
-        ipAddresses.stream().sorted(IP.naturalOrder).forEach(array::addString);
+    private void toSlime(Set<String> ipAddresses, Cursor array, UnaryOperator<Set<String>> validator) {
+        // Validating IP address format expensive, so we do it at serialization time instead of Node construction time
+        validator.apply(ipAddresses).stream().sorted(IP.naturalOrder).forEach(array::addString);
     }
 
     // ---------------- Deserialization --------------------------------------------------
@@ -260,7 +262,7 @@ public class NodeSerializer {
         return Optional.empty();
     }
 
-    // Enum <-> string mappings
+    // ----------------- Enum <-> string mappings ----------------------------------------
     
     /** Returns the event type, or null if this event type should be ignored */
     private History.Event.Type eventTypeFromString(String eventTypeString) {