From 8fd3d66d777a875789ac2f40058b639d00db57be Mon Sep 17 00:00:00 2001 From: jonmv Date: Mon, 19 Jun 2023 15:07:55 +0200 Subject: Detect public IP conflicts across accounts --- .../com/yahoo/vespa/hosted/provision/node/IP.java | 16 ++++++-- .../vespa/hosted/provision/NodeRepositoryTest.java | 47 +++++++++++++--------- 2 files changed, 41 insertions(+), 22 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 f2dd14bbf6d..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 @@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.node; import com.google.common.net.InetAddresses; import com.google.common.primitives.UnsignedBytes; -import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.HostName; import com.yahoo.vespa.hosted.provision.LockedNodeList; import com.yahoo.vespa.hosted.provision.Node; @@ -17,16 +16,17 @@ import java.util.Comparator; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; -import java.util.Map; 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. @@ -109,11 +109,12 @@ public record IP() { * @throws IllegalArgumentException if there are IP conflicts with existing nodes */ public static List verify(List nodes, LockedNodeList allNodes) { - Map sortedNodes = allNodes.sortedBy(Comparator.comparing(Node::hostname)).groupingBy(Node::cloudAccount); + NodeList sortedNodes = allNodes.sortedBy(Comparator.comparing(Node::hostname)); for (var node : nodes) { - for (var other : sortedNodes.getOrDefault(node.cloudAccount(), NodeList.of())) { + for (var other : sortedNodes) { if (node.equals(other)) continue; if (canAssignIpOf(other, node)) continue; + Predicate sharedIpSpace = other.cloudAccount().equals(node.cloudAccount()) ? __ -> true : IP::isPublic; var addresses = new HashSet<>(node.ipConfig().primary()); var otherAddresses = new HashSet<>(other.ipConfig().primary()); @@ -121,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() + @@ -430,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 867509e1ae3..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 @@ -8,7 +8,6 @@ import com.yahoo.vespa.hosted.provision.node.History; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.node.Report; import com.yahoo.vespa.hosted.provision.node.Reports; -import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver; import org.junit.Test; import java.time.Duration; @@ -55,36 +54,48 @@ public class NodeRepositoryTest { @Test public void test_ip_conflicts() { NodeRepositoryTester tester = new NodeRepositoryTester(); - ((MockNameResolver) tester.nodeRepository().nameResolver()).addRecord("host1", "1.2.3.4") - .addRecord("host2", "1.2.3.4") - .addRecord("host3", "1.2.3.4"); - tester.addHost("id1", "host1", "default", NodeType.host); + 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")); - IP.Config ipConfig = IP.Config.of(Set.of("1.2.3.4"), Set.of("1.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 host2 = Node.create("id2", ipConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host) - .build(); + Node publicHost2 = Node.create("id2", publicIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host) + .build(); - Node enclaveHost2 = Node.create("id2", ipConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host) - .cloudAccount(CloudAccount.from("gcp:foo-bar-baz")) - .build(); + Node publicEnclaveHost2 = Node.create("id2", publicIpConfig, "host2", tester.nodeFlavors().getFlavorOrThrow("default"), NodeType.host) + .cloudAccount(CloudAccount.from("gcp:foo-bar-baz")) + .build(); - Node enclaveHost3 = Node.create("id3", ipConfig, "host3", 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(host2), Agent.system)) + () -> tester.nodeRepository().nodes().addNodes(List.of(publicEnclaveHost2), Agent.system)) .getMessage()); - tester.nodeRepository().nodes().addNodes(List.of(enclaveHost2), Agent.system); + 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(); - assertEquals("Cannot assign [1.2.3.4] to host3: [1.2.3.4] already assigned to host2", + // 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(enclaveHost3), Agent.system)) + () -> 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 -- cgit v1.2.3