Merge pull request #27483 from vespa-engine/jonmv/unique-IPv4-within-cloud-accounts

Check for IPv4 conflicts within each cloud account, not across

3 files changed, 60 insertions, 0 deletions

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 3ef1951c19c..549fce92a5c 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
@@ -19,12 +19,14 @@ import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.config.provision.NodeType.confighost;
 import static com.yahoo.config.provision.NodeType.controllerhost;
 import static com.yahoo.config.provision.NodeType.proxyhost;
+import static java.util.function.Predicate.not;
 
 /**
  * This handles IP address configuration and allocation.
@@ -112,6 +114,7 @@ public record IP() {
                 for (var other : sortedNodes) {
                     if (node.equals(other)) continue;
                     if (canAssignIpOf(other, node)) continue;
+                    Predicate<String> sharedIpSpace = other.cloudAccount().equals(node.cloudAccount()) ? __ -> true : IP::isPublic;
 
                     var addresses = new HashSet<>(node.ipConfig().primary());
                     var otherAddresses = new HashSet<>(other.ipConfig().primary());
@@ -119,6 +122,7 @@ public record IP() {
                         addresses.addAll(node.ipConfig().pool().asSet());
                         otherAddresses.addAll(other.ipConfig().pool().asSet());
                     }
+                    otherAddresses.removeIf(not(sharedIpSpace));
                     otherAddresses.retainAll(addresses);
                     if (!otherAddresses.isEmpty())
                         throw new IllegalArgumentException("Cannot assign " + addresses + " to " + node.hostname() +
@@ -428,4 +432,10 @@ public record IP() {
         return ipAddress.contains(":");
     }
 
+    /** Returns whether given string is a public IP address */
+    public static boolean isPublic(String ip) {
+        InetAddress address = parse(ip);
+        return ! address.isLoopbackAddress() && ! address.isLinkLocalAddress() && ! address.isSiteLocalAddress();
+    }
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
index f45a5cd1c5f..605bf514f03 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision;
 
+import com.yahoo.config.provision.CloudAccount;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.History;
@@ -18,6 +19,7 @@ import java.util.function.Predicate;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -50,6 +52,53 @@ public class NodeRepositoryTest {
     }
 
     @Test
+    public void test_ip_conflicts() {
+        NodeRepositoryTester tester = new NodeRepositoryTester();
+        IP.Config ipConfig = IP.Config.of(Set.of("1.2.3.4", "10.2.3.4"), Set.of("1.2.3.4", "10.2.3.4"));
+        IP.Config publicIpConfig = IP.Config.of(Set.of("1.2.3.4"), Set.of("1.2.3.4"));
+        IP.Config privateIpConfig = IP.Config.of(Set.of("10.2.3.4"), Set.of("10.2.3.4"));
+
+        Node host1 = Node.create("id1", ipConfig, "host1", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host)
+                               .build();
+        tester.nodeRepository().nodes().addNodes(List.of(host1), Agent.system);
+
+        Node publicHost2 = Node.create("id2", publicIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host)
+                               .build();
+
+        Node publicEnclaveHost2 = Node.create("id2", publicIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host)
+                                      .cloudAccount(CloudAccount.from("gcp:foo-bar-baz"))
+                                      .build();
+
+        // Public IP conflicts inside an account are not allowed
+        assertEquals("Cannot assign [1.2.3.4] to host2: [1.2.3.4] already assigned to host1",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> tester.nodeRepository().nodes().addNodes(List.of(publicHost2), Agent.system))
+                             .getMessage());
+
+        // Public IP conflicts across accounts are not allowed
+        assertEquals("Cannot assign [1.2.3.4] to host2: [1.2.3.4] already assigned to host1",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> tester.nodeRepository().nodes().addNodes(List.of(publicEnclaveHost2), Agent.system))
+                             .getMessage());
+
+        Node privateHost2 = Node.create("id2", privateIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host)
+                                .build();
+
+        Node privateEnclaveHost2 = Node.create("id2", privateIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host)
+                                       .cloudAccount(CloudAccount.from("gcp:foo-bar-baz"))
+                                       .build();
+
+        // Private IP conflicts inside accounts are not allowed
+        assertEquals("Cannot assign [10.2.3.4] to host2: [10.2.3.4] already assigned to host1",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> tester.nodeRepository().nodes().addNodes(List.of(privateHost2), Agent.system))
+                             .getMessage());
+
+        // Private IP conflicts across accounts are allowed
+        tester.nodeRepository().nodes().addNodes(List.of(privateEnclaveHost2), Agent.system);
+    }
+
+    @Test
     public void only_allow_docker_containers_remove_in_ready() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
         tester.addHost("id1", "host1", "docker", NodeType.tenant);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
index 4d0b3e75740..00c4d95b0da 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
@@ -55,6 +55,7 @@ public class NodeRepositoryTester {
     
     public NodeRepository nodeRepository() { return nodeRepository; }
     public MockCurator curator() { return curator; }
+    public NodeFlavors nodeFlavors() { return nodeFlavors; }
     
     public List<Node> getNodes(NodeType type, Node.State ... inState) {
         return nodeRepository.nodes().list(inState).nodeType(type).asList();