Fix TODOs

3 files changed, 39 insertions, 38 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index 945ed4198f5..417f0f27bf0 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -6,13 +6,13 @@ import com.google.inject.Inject;
 import com.yahoo.collections.ListMap;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.Flavor;
+import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.curator.Curator;
-import com.yahoo.config.provision.Flavor;
-import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.vespa.hosted.provision.node.NodeAcl;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeFilter;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter;
@@ -31,6 +31,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
 
@@ -147,13 +148,16 @@ public class NodeRepository extends AbstractComponent {
     public List<Node> getFailed() { return zkClient.getNodes(Node.State.failed); }
 
     /**
-     * Returns a list of nodes that should be trusted by the given node.
+     * Returns a set of nodes that should be trusted by the given node.
      */
-    private List<Node> getTrustedNodes(Node node) {
-        final List<Node> trustedNodes = new ArrayList<>();
+    private Set<Node> getTrustedNodes(Node node) {
+        final Set<Node> trustedNodes = new TreeSet<>(Comparator.comparing(Node::hostname));
 
-        // For all cases below: Trust nodes in same application (if node is part of an application)
+        // For all cases below:
+        // - Trust nodes in same application
+        // - All config servers
         node.allocation().ifPresent(allocation -> trustedNodes.addAll(getNodes(allocation.owner())));
+        trustedNodes.addAll(getConfigNodes());
 
         switch (node.type()) {
             case tenant:
@@ -162,28 +166,16 @@ public class NodeRepository extends AbstractComponent {
                 // as it may be NATed traffic from trusted Docker containers
                 trustedNodes.addAll(getDockerHosts(trustedNodes)); // TODO: Remove when we no longer have IPv4-only nodes
                 trustedNodes.addAll(getNodes(NodeType.proxy));
-                trustedNodes.addAll(getConfigNodes());
                 break;
 
             case config:
-                // Config servers trust each other and all nodes in the zone
-                trustedNodes.addAll(getConfigNodes());
+                // Config servers trust all nodes in the zone
                 trustedNodes.addAll(getNodes());
                 break;
 
             case proxy:
-                // Proxy nodes trust nodes in same application and config servers. They also trust any traffic to ports
-                // 4080/4443, but these static rules are configured by the node itself
-                trustedNodes.addAll(getConfigNodes());
-                if ( ! node.allocation().isPresent()) // TODO: Remove when proxy nodes are in zone app everywhere
-                    trustedNodes.addAll(getNodes(NodeType.proxy));
-                break;
-
             case host:
-                // Docker hosts trust nodes in same application and config servers
-                trustedNodes.addAll(getConfigNodes());
-                if ( ! node.allocation().isPresent()) // TODO: Remove when Docker hosts are in zone app everywhere
-                    trustedNodes.addAll(getNodes(NodeType.host));
+                // No special rules for proxies and Docker hosts
                 break;
 
             default:
@@ -192,10 +184,7 @@ public class NodeRepository extends AbstractComponent {
                                 node.hostname(), node.type()));
         }
 
-        // Sort by hostname so that the resulting list is always the same if trusted nodes don't change
-        trustedNodes.sort(Comparator.comparing(Node::hostname));
-
-        return Collections.unmodifiableList(trustedNodes);
+        return Collections.unmodifiableSet(trustedNodes);
     }
 
     /**
@@ -516,7 +505,7 @@ public class NodeRepository extends AbstractComponent {
                 .collect(Collectors.toList());
     }
 
-    private List<Node> getDockerHosts(List<Node> nodes) {
+    private List<Node> getDockerHosts(Set<Node> nodes) {
         return nodes.stream()
                 .map(Node::parentHostname)
                 .filter(Optional::isPresent)
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
index 25000cd6574..b54f28b02e5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
@@ -1,31 +1,31 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.node;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.yahoo.vespa.hosted.provision.Node;
 
-import java.util.List;
+import java.util.Set;
 
 /**
- * A node ACL. The ACL contains the node which the ACL is valid for, and a list of nodes that the node should trust.
+ * A node ACL. The ACL contains the node which the ACL is valid for, and a set of nodes that the node should trust.
  *
  * @author mpolden
  */
 public class NodeAcl {
 
     private final Node node;
-    private final List<Node> trustedNodes;
+    private final Set<Node> trustedNodes;
 
-    public NodeAcl(Node node, List<Node> trustedNodes) {
+    public NodeAcl(Node node, Set<Node> trustedNodes) {
         this.node = node;
-        this.trustedNodes = ImmutableList.copyOf(trustedNodes);
+        this.trustedNodes = ImmutableSet.copyOf(trustedNodes);
     }
 
     public Node node() {
         return node;
     }
 
-    public List<Node> trustedNodes() {
+    public Set<Node> trustedNodes() {
         return trustedNodes;
     }
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
index 27b321b0673..a02a5803f43 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
@@ -19,6 +19,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -109,9 +110,14 @@ public class AclProvisioningTest {
 
         // Populate repo
         tester.makeReadyNodes(10, "default");
-        List<Node> proxyNodes = tester.makeReadyNodes(3, "default", NodeType.proxy);
+        tester.makeReadyNodes(3, "default", NodeType.proxy);
+
+        // Deploy zone application
+        ApplicationId zoneApplication = tester.makeApplicationId();
+        allocateNodes(Capacity.fromRequiredNodeType(NodeType.proxy), zoneApplication);
 
         // Get trusted nodes for first proxy node
+        List<Node> proxyNodes = tester.nodeRepository().getNodes(zoneApplication);
         Node node = proxyNodes.get(0);
         List<NodeAcl> nodeAcls = tester.nodeRepository().getNodeAcls(node, false);
 
@@ -124,8 +130,13 @@ public class AclProvisioningTest {
         List<Node> configServers = setConfigServers("cfg1:1234,cfg2:1234,cfg3:1234");
 
         // Populate repo
-        List<Node> dockerHostNodes = tester.makeReadyNodes(2, "default", NodeType.host);
+        tester.makeReadyNodes(2, "default", NodeType.host);
+
+        // Deploy zone application
+        ApplicationId zoneApplication = tester.makeApplicationId();
+        allocateNodes(Capacity.fromRequiredNodeType(NodeType.host), zoneApplication);
 
+        List<Node> dockerHostNodes = tester.nodeRepository().getNodes(zoneApplication);
         List<NodeAcl> acls = tester.nodeRepository().getNodeAcls(dockerHostNodes.get(0), false);
 
         // Trusted nodes is all Docker hosts and all config servers
@@ -178,7 +189,7 @@ public class AclProvisioningTest {
                     .findFirst()
                     .orElseThrow(() -> new RuntimeException("Expected to find ACL for node " + dockerNode.hostname()));
             assertEquals(dockerHostNodeUnderTest.hostname(), dockerNode.parentHostname().get());
-            // Since the containers are unallocated, they only trust config servers
+            // Since the containers are unallocated, they only trust Docker hosts config servers
             assertAcls(Collections.singletonList(configServers), nodeAcl);
         }
     }
@@ -194,9 +205,10 @@ public class AclProvisioningTest {
         List<NodeAcl> nodeAcls = tester.nodeRepository().getNodeAcls(readyNodes.get(0), false);
 
         assertEquals(3, nodeAcls.get(0).trustedNodes().size());
-        assertEquals(singleton("127.0.0.1"), nodeAcls.get(0).trustedNodes().get(0).ipAddresses());
-        assertEquals(singleton("127.0.0.2"), nodeAcls.get(0).trustedNodes().get(1).ipAddresses());
-        assertEquals(singleton("127.0.0.3"), nodeAcls.get(0).trustedNodes().get(2).ipAddresses());
+        Iterator<Node> trustedNodes = nodeAcls.get(0).trustedNodes().iterator();
+        assertEquals(singleton("127.0.0.1"), trustedNodes.next().ipAddresses());
+        assertEquals(singleton("127.0.0.2"), trustedNodes.next().ipAddresses());
+        assertEquals(singleton("127.0.0.3"), trustedNodes.next().ipAddresses());
     }
 
     private List<Node> allocateNodes(int nodeCount) {