diff options
author | Valerij Fredriksen <freva@users.noreply.github.com> | 2019-02-12 09:44:13 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-02-12 09:44:13 +0100 |
commit | 556a06cf0e1769399cb311427bc421cd296cb141 (patch) | |
tree | f8cff950faf677c8284c1930fa0f2a91fc2d66bb /node-admin | |
parent | 8093256a133616d810bc583d64c6603a4117a504 (diff) | |
parent | ea8d06c9a1c302c8e500d91e01d6708cfcf53abe (diff) |
Merge pull request #8456 from vespa-engine/freva/remove-acl-thread
Node-Admin: Run ACL maintainer from NodeAgent
Diffstat (limited to 'node-admin')
21 files changed, 664 insertions, 563 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Acl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Acl.java index f377a603ab6..8f39fddfa1f 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Acl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Acl.java @@ -5,12 +5,12 @@ import com.google.common.net.InetAddresses; import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; import java.net.InetAddress; -import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -22,6 +22,8 @@ import java.util.stream.Collectors; */ public class Acl { + public static final Acl EMPTY = new Acl(Set.of(), Set.of(), Set.of()); + private final Set<Node> trustedNodes; private final Set<Integer> trustedPorts; private final Set<String> trustedNetworks; @@ -38,7 +40,7 @@ public class Acl { } public Acl(Set<Integer> trustedPorts, Set<Node> trustedNodes) { - this(trustedPorts, trustedNodes, Collections.emptySet()); + this(trustedPorts, trustedNodes, Set.of()); } public List<String> toRules(IPVersion ipVersion) { @@ -131,10 +133,7 @@ public class Acl { } private static <T> Set<T> copyOfNullable(Set<T> set) { - if (set == null) { - return Collections.emptySet(); - } - return Set.copyOf(set); + return Optional.ofNullable(set).map(Set::copyOf).orElseGet(Set::of); } public static class Node { @@ -213,7 +212,7 @@ public class Acl { } public Builder withTrustedPorts(Integer... ports) { - trustedPorts.addAll(Arrays.asList(ports)); + trustedPorts.addAll(List.of(ports)); return this; } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java index 27dc4e5237b..cedddc9b0b6 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java @@ -2,7 +2,6 @@ package com.yahoo.vespa.hosted.node.admin.docker; import com.yahoo.vespa.hosted.dockerapi.Container; -import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.ContainerStats; import com.yahoo.vespa.hosted.dockerapi.DockerImage; @@ -31,7 +30,7 @@ public interface DockerOperations { ProcessResult executeCommandInContainerAsRoot(NodeAgentContext context, Long timeoutSeconds, String... command); - ProcessResult executeCommandInNetworkNamespace(ContainerName containerName, String... command); + ProcessResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command); /** Resume node. Resuming a node means that it is ready to take on traffic. */ diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java index bdda155b2f3..d65bca89733 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java @@ -6,7 +6,6 @@ import com.yahoo.collections.Pair; import com.yahoo.config.provision.NodeType; import com.yahoo.system.ProcessExecuter; import com.yahoo.vespa.hosted.dockerapi.Container; -import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.ContainerStats; import com.yahoo.vespa.hosted.dockerapi.Docker; @@ -196,12 +195,12 @@ public class DockerOperationsImpl implements DockerOperations { } @Override - public ProcessResult executeCommandInNetworkNamespace(ContainerName containerName, String... command) { - final Integer containerPid = docker.getContainer(containerName) + public ProcessResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command) { + final int containerPid = docker.getContainer(context.containerName()) .filter(container -> container.state.isRunning()) - .map(container -> container.pid) - .orElseThrow(() -> new RuntimeException("PID not found for container with name: " + - containerName.asString())); + .orElseThrow(() -> new RuntimeException( + "Found no running container named " + context.containerName().asString())) + .pid; final String[] wrappedCommand = Stream.concat( Stream.of("nsenter", String.format("--net=/proc/%d/ns/net", containerPid), "--"), @@ -213,12 +212,12 @@ public class DockerOperationsImpl implements DockerOperations { if (result.getFirst() != 0) { throw new RuntimeException(String.format( "Failed to execute %s in network namespace for %s (PID = %d), exit code: %d, output: %s", - Arrays.toString(wrappedCommand), containerName.asString(), containerPid, result.getFirst(), result.getSecond())); + Arrays.toString(wrappedCommand), context.containerName().asString(), containerPid, result.getFirst(), result.getSecond())); } return new ProcessResult(0, result.getSecond(), ""); } catch (IOException e) { throw new RuntimeException(String.format("IOException while executing %s in network namespace for %s (PID = %d)", - Arrays.toString(wrappedCommand), containerName.asString(), containerPid), e); + Arrays.toString(wrappedCommand), context.containerName().asString(), containerPid), e); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java index a733a52ea10..a68eda7e39e 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java @@ -2,19 +2,29 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.acl; import com.google.common.net.InetAddresses; -import com.yahoo.config.provision.HostName; -import com.yahoo.vespa.hosted.dockerapi.Container; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; +import com.yahoo.log.LogLevel; +import com.yahoo.vespa.hosted.dockerapi.ProcessResult; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.task.util.file.Editor; +import com.yahoo.vespa.hosted.node.admin.task.util.file.LineEditor; import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddresses; import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; -import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger; +import java.io.IOException; import java.net.InetAddress; -import java.util.Map; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; +import java.util.function.Consumer; +import java.util.function.Supplier; +import java.util.logging.Logger; import java.util.stream.Collectors; +import static com.yahoo.yolean.Exceptions.uncheck; + /** * This class maintains the iptables (ipv4 and ipv6) for all running containers. * The filter table is synced with ACLs fetched from the Node repository while the nat table @@ -31,56 +41,100 @@ import java.util.stream.Collectors; * @author smorgrav */ public class AclMaintainer { - - private static final PrefixLogger log = PrefixLogger.getNodeAdminLogger(AclMaintainer.class); + private static final Logger logger = Logger.getLogger(AclMaintainer.class.getName()); private final DockerOperations dockerOperations; - private final NodeRepository nodeRepository; private final IPAddresses ipAddresses; - private final String hostHostname; - public AclMaintainer(DockerOperations dockerOperations, NodeRepository nodeRepository, - HostName hostHostname, IPAddresses ipAddresses) { + public AclMaintainer(DockerOperations dockerOperations, IPAddresses ipAddresses) { this.dockerOperations = dockerOperations; - this.nodeRepository = nodeRepository; this.ipAddresses = ipAddresses; - this.hostHostname = hostHostname.value(); } - private void applyRedirect(Container container, InetAddress address) { + public void converge(NodeAgentContext context) { + // Apply acl to the filter table + editFlushOnError(context, IPVersion.IPv4, "filter", FilterTableLineEditor.from(context.acl(), IPVersion.IPv4)); + editFlushOnError(context, IPVersion.IPv6, "filter", FilterTableLineEditor.from(context.acl(), IPVersion.IPv6)); + + ipAddresses.getAddress(context.hostname().value(), IPVersion.IPv4).ifPresent(addr -> applyRedirect(context, addr)); + ipAddresses.getAddress(context.hostname().value(), IPVersion.IPv6).ifPresent(addr -> applyRedirect(context, addr)); + } + + private void applyRedirect(NodeAgentContext context, InetAddress address) { IPVersion ipVersion = IPVersion.get(address); // Necessary to avoid the routing packets destined for the node's own public IP address // via the bridge, which is illegal. String redirectRule = "-A OUTPUT -d " + InetAddresses.toAddrString(address) + ipVersion.singleHostCidr() + " -j REDIRECT"; - IPTablesEditor.editLogOnError(dockerOperations, container.name, ipVersion, "nat", NatTableLineEditor.from(redirectRule)); + editLogOnError(context, ipVersion, "nat", NatTableLineEditor.from(redirectRule)); } - private void apply(Container container, Acl acl) { - // Apply acl to the filter table - IPTablesEditor.editFlushOnError(dockerOperations, container.name, IPVersion.IPv6, "filter", FilterTableLineEditor.from(acl, IPVersion.IPv6)); - IPTablesEditor.editFlushOnError(dockerOperations, container.name, IPVersion.IPv4, "filter", FilterTableLineEditor.from(acl, IPVersion.IPv4)); + private boolean editFlushOnError(NodeAgentContext context, IPVersion ipVersion, String table, LineEditor lineEditor) { + return edit(context, table, ipVersion, lineEditor, true); + } + + private boolean editLogOnError(NodeAgentContext context, IPVersion ipVersion, String table, LineEditor lineEditor) { + return edit(context, table, ipVersion, lineEditor, false); + } - ipAddresses.getAddress(container.hostname, IPVersion.IPv4).ifPresent(addr -> applyRedirect(container, addr)); - ipAddresses.getAddress(container.hostname, IPVersion.IPv6).ifPresent(addr -> applyRedirect(container, addr)); + private boolean edit(NodeAgentContext context, String table, IPVersion ipVersion, LineEditor lineEditor, boolean flush) { + Editor editor = new Editor( + ipVersion.iptablesCmd() + "-" + table, + listTable(context, table, ipVersion), + restoreTable(context, table, ipVersion, flush), + lineEditor); + return editor.edit(message -> context.log(logger, message)); } - private synchronized void configureAcls() { - log.info("Configuring ACLs"); // Needed to potentially nail down when ACL maintainer stopped working - Map<String, Container> runningContainers = dockerOperations - .getAllManagedContainers().stream() - .filter(container -> container.state.isRunning()) - .collect(Collectors.toMap(container -> container.hostname, container -> container)); + private Supplier<List<String>> listTable(NodeAgentContext context, String table, IPVersion ipVersion) { + return () -> { + ProcessResult currentRulesResult = + dockerOperations.executeCommandInNetworkNamespace(context, ipVersion.iptablesCmd(), "-S", "-t", table); + return Arrays.stream(currentRulesResult.getOutput().split("\n")) + .map(String::trim) + .collect(Collectors.toList()); + }; + } - nodeRepository.getAcls(hostHostname).entrySet().stream() - .filter(entry -> runningContainers.containsKey(entry.getKey())) - .forEach(entry -> apply(runningContainers.get(entry.getKey()), entry.getValue())); + private Consumer<List<String>> restoreTable(NodeAgentContext context, String table, IPVersion ipVersion, boolean flush) { + return list -> { + try (TemporaryIpTablesFileHandler fileHandler = new TemporaryIpTablesFileHandler(table)) { + String rules = String.join("\n", list); + String fileContent = "*" + table + "\n" + rules + "\nCOMMIT\n"; + fileHandler.writeUtf8Content(fileContent); + dockerOperations.executeCommandInNetworkNamespace(context, ipVersion.iptablesRestore(), fileHandler.absolutePath()); + } catch (Exception e) { + if (flush) { + context.log(logger, LogLevel.ERROR, "Exception occurred while syncing iptable " + table + ", attempting rollback", e); + try { + dockerOperations.executeCommandInNetworkNamespace(context, ipVersion.iptablesCmd(), "-F", "-t", table); + } catch (Exception ne) { + context.log(logger, LogLevel.ERROR, "Rollback of table " + table + " failed, giving up", ne); + } + } else { + context.log(logger, LogLevel.WARNING, "Unable to sync iptables for " + table, e); + } + } + }; } - public void converge() { - try { - configureAcls(); - } catch (Throwable t) { - log.error("Failed to configure ACLs", t); + private static class TemporaryIpTablesFileHandler implements AutoCloseable { + private final Path path; + + private TemporaryIpTablesFileHandler(String table) { + this.path = uncheck(() -> Files.createTempFile("iptables-restore", "." + table)); + } + + private void writeUtf8Content(String content) throws IOException { + Files.write(path, content.getBytes(StandardCharsets.UTF_8)); + } + + private String absolutePath() { + return path.toAbsolutePath().toString(); + } + + @Override + public void close() throws IOException { + Files.deleteIfExists(path); } } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditor.java deleted file mode 100644 index d34b41e3ae5..00000000000 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditor.java +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.node.admin.maintenance.acl; - -import com.yahoo.vespa.hosted.dockerapi.ContainerName; -import com.yahoo.vespa.hosted.dockerapi.ProcessResult; -import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; -import com.yahoo.vespa.hosted.node.admin.task.util.file.Editor; -import com.yahoo.vespa.hosted.node.admin.task.util.file.LineEditor; -import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; -import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger; - -import java.io.File; -import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.Arrays; -import java.util.List; -import java.util.function.Consumer; -import java.util.function.Supplier; -import java.util.stream.Collectors; - -/** - * Edit the iptables for docker containers. - */ -class IPTablesEditor { - - private final PrefixLogger log; - private final DockerOperations dockerOperations; - private final ContainerName containerName; - private final Consumer<String> testInterceptor; - - public IPTablesEditor(DockerOperations dockerOperations, ContainerName containerName) { - this(dockerOperations, containerName, (result) -> {}); - } - - IPTablesEditor(DockerOperations dockerOperations, ContainerName containerName, Consumer<String> testInterceptor) { - this.dockerOperations = dockerOperations; - this.containerName = containerName; - this.testInterceptor = testInterceptor; - this.log = PrefixLogger.getNodeAgentLogger(AclMaintainer.class, containerName); - } - - public static boolean editFlushOnError(DockerOperations dockerOperations, ContainerName containerName, IPVersion ipVersion, String table, LineEditor lineEditor) { - return new IPTablesEditor(dockerOperations, containerName).edit(table, ipVersion, lineEditor, true); - } - - public static boolean editLogOnError(DockerOperations dockerOperations, ContainerName containerName, IPVersion ipVersion, String table, LineEditor lineEditor) { - return new IPTablesEditor(dockerOperations, containerName).edit(table, ipVersion, lineEditor, false); - } - - public boolean edit(String table, IPVersion ipVersion, LineEditor lineEditor, boolean flush) { - String editId = ipVersion.iptablesCmd() + "-" + table; - Editor editor = new Editor(editId, listTable(table, ipVersion), restoreTable(table, ipVersion, flush), lineEditor); - return editor.edit(log::info); - } - - private Supplier<List<String>> listTable(String table, IPVersion ipVersion) { - return () -> { - ProcessResult currentRulesResult = - dockerOperations.executeCommandInNetworkNamespace(containerName, ipVersion.iptablesCmd(), "-S", "-t", table); - return Arrays.stream(currentRulesResult.getOutput().split("\n")) - .map(String::trim) - .collect(Collectors.toList()); - }; - } - - private Consumer<List<String>> restoreTable(String table, IPVersion ipVersion, boolean flush) { - return list -> { - File file = null; - try { - String rules = String.join("\n", list); - String fileContent = "*" + table + "\n" + rules + "\nCOMMIT\n"; - file = writeTempFile(table, fileContent); - dockerOperations.executeCommandInNetworkNamespace(containerName, ipVersion.iptablesRestore(), file.getAbsolutePath()); - testInterceptor.accept(fileContent); - } catch (Exception e) { - if (flush) { - log.error("Exception occurred while syncing iptable " + table + " for " + containerName.asString() + ", attempting rollback", e); - try { - dockerOperations.executeCommandInNetworkNamespace(containerName, ipVersion.iptablesCmd(), "-F", "-t", table); - } catch (Exception ne) { - log.error("Rollback of table " + table + " for " + containerName.asString() + " failed, giving up", ne); - } - } else { - log.warning("Unable to sync iptables for " + table, e); - } - } finally { - if (file != null) { - file.delete(); - } - } - }; - } - - private File writeTempFile(String table, String content) { - try { - Path path = Files.createTempFile("iptables-restore", "." + table); - File file = path.toFile(); - Files.write(path, content.getBytes(StandardCharsets.UTF_8)); - file.deleteOnExit(); - return file; - } catch (IOException e) { - throw new RuntimeException("Unable to write restore file for iptables.", e); - } - } -} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java index 456391c65c2..6002e7bcd89 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java @@ -1,10 +1,11 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.nodeadmin; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; import java.time.Duration; import java.util.List; +import java.util.Set; /** * NodeAdmin manages the life cycle of NodeAgents. @@ -12,11 +13,8 @@ import java.util.List; */ public interface NodeAdmin { - /** - * Calling this will cause NodeAdmin to move to the state containersToRun by adding or removing nodes. - * @param containersToRun this is the wanted state. - */ - void refreshContainersToRun(final List<NodeSpec> containersToRun); + /** Start/stop NodeAgents and schedule next NodeAgent ticks with the given NodeAgentContexts */ + void refreshContainersToRun(Set<NodeAgentContext> nodeAgentContexts); /** Gather node agent and its docker container metrics and forward them to the {@code MetricReceiverWrapper} */ void updateNodeAgentMetrics(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java index 2b37dcdf69c..e6a3d740273 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java @@ -1,20 +1,15 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.nodeadmin; -import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.vespa.hosted.dockerapi.metrics.CounterWrapper; import com.yahoo.vespa.hosted.dockerapi.metrics.Dimensions; import com.yahoo.vespa.hosted.dockerapi.metrics.GaugeWrapper; import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; -import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; -import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextFactory; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextManager; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentFactory; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentScheduler; -import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger; import java.time.Clock; import java.time.Duration; @@ -22,12 +17,9 @@ import java.time.Instant; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -36,16 +28,10 @@ import java.util.stream.Collectors; * @author stiankri */ public class NodeAdminImpl implements NodeAdmin { - private static final PrefixLogger logger = PrefixLogger.getNodeAdminLogger(NodeAdmin.class); private static final Duration NODE_AGENT_FREEZE_TIMEOUT = Duration.ofSeconds(5); private static final Duration NODE_AGENT_SPREAD = Duration.ofSeconds(3); - private final ScheduledExecutorService aclScheduler = - Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getDaemonThreadFactory("aclscheduler")); - private final NodeAgentWithSchedulerFactory nodeAgentWithSchedulerFactory; - private final NodeAgentContextFactory nodeAgentContextFactory; - private final Optional<AclMaintainer> aclMaintainer; private final Clock clock; private final Duration freezeTimeout; @@ -59,27 +45,20 @@ public class NodeAdminImpl implements NodeAdmin { private final GaugeWrapper numberOfContainersInLoadImageState; private final CounterWrapper numberOfUnhandledExceptionsInNodeAgent; - public NodeAdminImpl(NodeAgentFactory nodeAgentFactory, - NodeAgentContextFactory nodeAgentContextFactory, - Optional<AclMaintainer> aclMaintainer, - MetricReceiverWrapper metricReceiver, - Clock clock) { + public NodeAdminImpl(NodeAgentFactory nodeAgentFactory, MetricReceiverWrapper metricReceiver, Clock clock) { this((NodeAgentWithSchedulerFactory) nodeAgentContext -> create(clock, nodeAgentFactory, nodeAgentContext), - nodeAgentContextFactory, aclMaintainer, metricReceiver, clock, NODE_AGENT_FREEZE_TIMEOUT, NODE_AGENT_SPREAD); + metricReceiver, clock, NODE_AGENT_FREEZE_TIMEOUT, NODE_AGENT_SPREAD); } - public NodeAdminImpl(NodeAgentFactory nodeAgentFactory, NodeAgentContextFactory nodeAgentContextFactory, - Optional<AclMaintainer> aclMaintainer, MetricReceiverWrapper metricReceiver, Clock clock, Duration freezeTimeout, Duration spread) { + public NodeAdminImpl(NodeAgentFactory nodeAgentFactory, MetricReceiverWrapper metricReceiver, + Clock clock, Duration freezeTimeout, Duration spread) { this((NodeAgentWithSchedulerFactory) nodeAgentContext -> create(clock, nodeAgentFactory, nodeAgentContext), - nodeAgentContextFactory, aclMaintainer, metricReceiver, clock, freezeTimeout, spread); + metricReceiver, clock, freezeTimeout, spread); } NodeAdminImpl(NodeAgentWithSchedulerFactory nodeAgentWithSchedulerFactory, - NodeAgentContextFactory nodeAgentContextFactory, Optional<AclMaintainer> aclMaintainer, MetricReceiverWrapper metricReceiver, - Clock clock, Duration freezeTimeout, Duration spread) { + MetricReceiverWrapper metricReceiver, Clock clock, Duration freezeTimeout, Duration spread) { this.nodeAgentWithSchedulerFactory = nodeAgentWithSchedulerFactory; - this.nodeAgentContextFactory = nodeAgentContextFactory; - this.aclMaintainer = aclMaintainer; this.clock = clock; this.freezeTimeout = freezeTimeout; @@ -94,9 +73,9 @@ public class NodeAdminImpl implements NodeAdmin { } @Override - public void refreshContainersToRun(List<NodeSpec> containersToRun) { - final Map<String, NodeAgentContext> nodeAgentContextsByHostname = containersToRun.stream() - .collect(Collectors.toMap(NodeSpec::getHostname, nodeAgentContextFactory::create)); + public void refreshContainersToRun(Set<NodeAgentContext> nodeAgentContexts) { + Map<String, NodeAgentContext> nodeAgentContextsByHostname = nodeAgentContexts.stream() + .collect(Collectors.toMap(nac -> nac.hostname().value(), Function.identity())); // Stop and remove NodeAgents that should no longer be running diff(nodeAgentWithSchedulerByHostname.keySet(), nodeAgentContextsByHostname.keySet()) @@ -112,9 +91,8 @@ public class NodeAdminImpl implements NodeAdmin { Duration timeBetweenNodeAgents = spread.dividedBy(Math.max(nodeAgentContextsByHostname.size() - 1, 1)); Instant nextAgentStart = clock.instant(); // At this point, nodeAgentContextsByHostname and nodeAgentWithSchedulerByHostname should have the same keys - for (String hostname : nodeAgentContextsByHostname.keySet()) { - NodeAgentContext context = nodeAgentContextsByHostname.get(hostname); - nodeAgentWithSchedulerByHostname.get(hostname).scheduleTickWith(context, nextAgentStart); + for (Map.Entry<String, NodeAgentContext> entry : nodeAgentContextsByHostname.entrySet()) { + nodeAgentWithSchedulerByHostname.get(entry.getKey()).scheduleTickWith(entry.getValue(), nextAgentStart); nextAgentStart = nextAgentStart.plus(timeBetweenNodeAgents); } } @@ -186,28 +164,13 @@ public class NodeAdminImpl implements NodeAdmin { @Override public void start() { - aclMaintainer.ifPresent(maintainer -> { - int delay = 120; // WARNING: Reducing this will increase the load on config servers. - aclScheduler.scheduleWithFixedDelay(() -> { - if (!isFrozen()) maintainer.converge(); - }, 30, delay, TimeUnit.SECONDS); - }); + } @Override public void stop() { - aclScheduler.shutdown(); - // Stop all node-agents in parallel, will block until the last NodeAgent is stopped nodeAgentWithSchedulerByHostname.values().parallelStream().forEach(NodeAgent::stop); - - do { - try { - aclScheduler.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS); - } catch (InterruptedException e) { - logger.info("Was interrupted while waiting for metricsScheduler and aclScheduler to shutdown"); - } - } while (!aclScheduler.isTerminated()); } // Set-difference. Returns minuend minus subtrahend. diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java index 18c3a836e41..eb306036416 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java @@ -4,18 +4,28 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin; import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.config.provision.HostName; import com.yahoo.log.LogLevel; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.Orchestrator; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextFactory; import com.yahoo.vespa.hosted.provision.Node; +import java.time.Clock; import java.time.Duration; +import java.time.Instant; import java.util.ArrayList; import java.util.EnumSet; import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.function.Function; +import java.util.function.Supplier; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -37,6 +47,8 @@ public class NodeAdminStateUpdater { private final ScheduledExecutorService metricsScheduler = Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getDaemonThreadFactory("metricsscheduler")); + private final CachedSupplier<Map<String, Acl>> cachedAclSupplier; + private final NodeAgentContextFactory nodeAgentContextFactory; private final NodeRepository nodeRepository; private final Orchestrator orchestrator; private final NodeAdmin nodeAdmin; @@ -47,14 +59,18 @@ public class NodeAdminStateUpdater { private volatile State currentState = SUSPENDED_NODE_ADMIN; public NodeAdminStateUpdater( + NodeAgentContextFactory nodeAgentContextFactory, NodeRepository nodeRepository, Orchestrator orchestrator, NodeAdmin nodeAdmin, - HostName hostHostname) { + HostName hostHostname, + Clock clock) { + this.nodeAgentContextFactory = nodeAgentContextFactory; this.nodeRepository = nodeRepository; this.orchestrator = orchestrator; this.nodeAdmin = nodeAdmin; this.hostHostname = hostHostname.value(); + this.cachedAclSupplier = new CachedSupplier<>(clock, Duration.ofSeconds(115), () -> nodeRepository.getAcls(this.hostHostname)); } public void start() { @@ -145,11 +161,21 @@ public class NodeAdminStateUpdater { currentState = wantedState; } - private void adjustNodeAgentsToRunFromNodeRepository() { + void adjustNodeAgentsToRunFromNodeRepository() { try { - final List<NodeSpec> containersToRun = nodeRepository.getNodes(hostHostname); - nodeAdmin.refreshContainersToRun(containersToRun); - } catch (Exception e) { + Map<String, NodeSpec> nodeSpecByHostname = nodeRepository.getNodes(hostHostname).stream() + .collect(Collectors.toMap(NodeSpec::getHostname, Function.identity())); + Map<String, Acl> aclByHostname = Optional.of(cachedAclSupplier.get()) + .filter(acls -> acls.keySet().containsAll(nodeSpecByHostname.keySet())) + .orElseGet(cachedAclSupplier::invalidateAndGet); + + Set<NodeAgentContext> nodeAgentContexts = nodeSpecByHostname.keySet().stream() + .map(hostname -> nodeAgentContextFactory.create( + nodeSpecByHostname.get(hostname), + aclByHostname.getOrDefault(hostname, Acl.EMPTY))) + .collect(Collectors.toSet()); + nodeAdmin.refreshContainersToRun(nodeAgentContexts); + } catch (RuntimeException e) { log.log(LogLevel.WARNING, "Failed to update which containers should be running", e); } } @@ -161,4 +187,33 @@ public class NodeAdminStateUpdater { .map(NodeSpec::getHostname) .collect(Collectors.toList()); } + + private static class CachedSupplier<T> implements Supplier<T> { + private final Clock clock; + private final Duration expiration; + private final Supplier<T> supplier; + private Instant refreshAt; + private T cachedValue; + + private CachedSupplier(Clock clock, Duration expiration, Supplier<T> supplier) { + this.clock = clock; + this.expiration = expiration; + this.supplier = supplier; + this.refreshAt = Instant.MIN; + } + + @Override + public T get() { + if (! clock.instant().isBefore(refreshAt)) { + cachedValue = supplier.get(); + refreshAt = clock.instant().plus(expiration); + } + return cachedValue; + } + + private T invalidateAndGet() { + refreshAt = Instant.MIN; + return get(); + } + } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java index 2e9f58a2c31..496f4bd667d 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java @@ -6,6 +6,7 @@ import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.node.admin.component.TaskContext; import com.yahoo.vespa.hosted.node.admin.component.ZoneId; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerNetworking; @@ -14,10 +15,16 @@ import java.nio.file.Paths; public interface NodeAgentContext extends TaskContext { + /** @return node specification from node-repository */ NodeSpec node(); + /** @return node ACL from node-repository */ + Acl acl(); + + /** @return name of the docker container this context applies to */ ContainerName containerName(); + /** @return hostname of the docker container this context applies to */ default HostName hostname() { return HostName.from(node().getHostname()); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextFactory.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextFactory.java index 0cfafe34717..3e0991d4357 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextFactory.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextFactory.java @@ -1,6 +1,7 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.nodeagent; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; /** @@ -8,5 +9,5 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; */ @FunctionalInterface public interface NodeAgentContextFactory { - NodeAgentContext create(NodeSpec nodeSpec); + NodeAgentContext create(NodeSpec nodeSpec, Acl acl); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java index 58414ab55f4..804450f05ff 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java @@ -7,6 +7,7 @@ import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.node.admin.component.ZoneId; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerNetworking; import com.yahoo.vespa.hosted.provision.Node; @@ -27,6 +28,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { private final String logPrefix; private final NodeSpec node; + private final Acl acl; private final ContainerName containerName; private final AthenzService identity; private final DockerNetworking dockerNetworking; @@ -36,11 +38,12 @@ public class NodeAgentContextImpl implements NodeAgentContext { private final String vespaUser; private final String vespaUserOnHost; - public NodeAgentContextImpl(NodeSpec node, AthenzService identity, + public NodeAgentContextImpl(NodeSpec node, Acl acl, AthenzService identity, DockerNetworking dockerNetworking, ZoneId zoneId, Path pathToContainerStorage, Path pathToVespaHome, String vespaUser, String vespaUserOnHost) { this.node = Objects.requireNonNull(node); + this.acl = Objects.requireNonNull(acl); this.containerName = ContainerName.fromHostname(node.getHostname()); this.identity = Objects.requireNonNull(identity); this.dockerNetworking = Objects.requireNonNull(dockerNetworking); @@ -58,6 +61,11 @@ public class NodeAgentContextImpl implements NodeAgentContext { } @Override + public Acl acl() { + return acl; + } + + @Override public ContainerName containerName() { return containerName; } @@ -133,6 +141,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { public String toString() { return "NodeAgentContextImpl{" + "node=" + node + + ", acl=" + acl + ", containerName=" + containerName + ", identity=" + identity + ", dockerNetworking=" + dockerNetworking + @@ -147,6 +156,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { /** For testing only! */ public static class Builder { private NodeSpec.Builder nodeSpecBuilder = new NodeSpec.Builder(); + private Acl acl; private AthenzService identity; private DockerNetworking dockerNetworking; private ZoneId zoneId; @@ -177,6 +187,11 @@ public class NodeAgentContextImpl implements NodeAgentContext { return this; } + public Builder acl(Acl acl) { + this.acl = acl; + return this; + } + public Builder identity(AthenzService identity) { this.identity = identity; return this; @@ -219,6 +234,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { public NodeAgentContextImpl build() { return new NodeAgentContextImpl( nodeSpecBuilder.build(), + Optional.ofNullable(acl).orElse(Acl.EMPTY), Optional.ofNullable(identity).orElseGet(() -> new AthenzService("domain", "service")), Optional.ofNullable(dockerNetworking).orElse(DockerNetworking.HOST_NETWORK), Optional.ofNullable(zoneId).orElseGet(() -> new ZoneId(SystemName.dev, Environment.dev, RegionName.defaultName())), diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index e5e045c6013..d31a888ea44 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -468,11 +468,11 @@ public class NodeAgentImpl implements NodeAgent { containerState = STARTING; startContainer(context); containerState = UNKNOWN; - aclMaintainer.ifPresent(AclMaintainer::converge); } else { updateContainerIfNeeded(context, container.get()); } + aclMaintainer.ifPresent(maintainer -> maintainer.converge(context)); startServicesIfNeeded(context); resumeNodeIfNeeded(context); healthChecker.ifPresent(checker -> checker.verifyHealth(context)); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java index 99b3fde4eb1..db1d45b9449 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java @@ -75,12 +75,13 @@ public class DockerOperationsImplTest { @Test public void runsCommandInNetworkNamespace() throws IOException { - Container container = makeContainer("container-42", Container.State.RUNNING, 42); + NodeAgentContext context = new NodeAgentContextImpl.Builder("container-42.domain.tld").build(); + makeContainer("container-42", Container.State.RUNNING, 42); when(processExecuter.exec(aryEq(new String[]{"nsenter", "--net=/proc/42/ns/net", "--", "iptables", "-nvL"}))) .thenReturn(new Pair<>(0, "")); - dockerOperations.executeCommandInNetworkNamespace(container.name, "iptables", "-nvL"); + dockerOperations.executeCommandInNetworkNamespace(context, "iptables", "-nvL"); } private Container makeContainer(String name, Container.State state, int pid) { diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java index 0254f58e7eb..6ae373e8b55 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java @@ -96,11 +96,11 @@ public class DockerTester implements AutoCloseable { NodeAgentFactory nodeAgentFactory = contextSupplier -> new NodeAgentImpl( contextSupplier, nodeRepository, orchestrator, dockerOperations, storageMaintainer, flagSource, Optional.empty(), Optional.empty(), Optional.empty()); - NodeAgentContextFactory nodeAgentContextFactory = nodeSpec -> - new NodeAgentContextImpl.Builder(nodeSpec).fileSystem(fileSystem).build(); - nodeAdmin = new NodeAdminImpl(nodeAgentFactory, nodeAgentContextFactory, Optional.empty(), mr, Clock.systemUTC(), Duration.ofMillis(10), Duration.ZERO); - nodeAdminStateUpdater = new NodeAdminStateUpdater(nodeRepository, orchestrator, - nodeAdmin, HOST_HOSTNAME); + nodeAdmin = new NodeAdminImpl(nodeAgentFactory, mr, Clock.systemUTC(), Duration.ofMillis(10), Duration.ZERO); + NodeAgentContextFactory nodeAgentContextFactory = (nodeSpec, acl) -> + new NodeAgentContextImpl.Builder(nodeSpec).acl(acl).fileSystem(fileSystem).build(); + nodeAdminStateUpdater = new NodeAdminStateUpdater(nodeAgentContextFactory, nodeRepository, orchestrator, + nodeAdmin, HOST_HOSTNAME, Clock.systemUTC()); loopThread = new Thread(() -> { nodeAdminStateUpdater.start(); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java index 65e8ebf0743..8f455a08860 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java @@ -1,236 +1,263 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.maintenance.acl; -import com.yahoo.config.provision.HostName; -import com.yahoo.vespa.hosted.dockerapi.Container; -import com.yahoo.vespa.hosted.dockerapi.ContainerName; -import com.yahoo.vespa.hosted.dockerapi.DockerImage; import com.yahoo.vespa.hosted.dockerapi.ProcessResult; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl; +import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesMock; import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; +import com.yahoo.vespa.test.file.TestFileSystem; import org.junit.Before; import org.junit.Test; +import java.nio.file.FileSystem; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashMap; +import java.util.Collections; import java.util.List; -import java.util.Map; +import java.util.function.Function; +import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.endsWith; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; public class AclMaintainerTest { - private final HostName hostHostname = HostName.from("node-admin.region-1.yahoo.com"); - private final IPAddressesMock ipAddresses = new IPAddressesMock(); - private final DockerOperations dockerOperations = mock(DockerOperations.class); - private final NodeRepository nodeRepository = mock(NodeRepository.class); - private final Map<String, Container> containers = new HashMap<>(); - private final List<Container> containerList = new ArrayList<>(); - private final AclMaintainer aclMaintainer = - new AclMaintainer(dockerOperations, nodeRepository, hostHostname, ipAddresses); - - @Before - public void before() { - when(dockerOperations.getAllManagedContainers()).thenReturn(containerList); - } - - @Test - public void empty_trusted_ports_are_handled() { - Container container = addContainer("container1", "container1.host.com", Container.State.RUNNING); - Map<String, Acl> acls = makeAcl(container.hostname, "4321", "2001::1"); - - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); - - whenListRules(container.name, "filter", IPVersion.IPv6, ""); - whenListRules(container.name, "filter", IPVersion.IPv4, ""); - whenListRules(container.name, "nat", IPVersion.IPv4, ""); - whenListRules(container.name, "nat", IPVersion.IPv6, ""); - - aclMaintainer.converge(); - - verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(container.name), eq("iptables-restore"), any()); //we don;t have a ip4 address for the container so no redirect either - verify(dockerOperations, times(2)).executeCommandInNetworkNamespace(eq(container.name), eq("ip6tables-restore"), any()); - } - - @Test - public void configures_container_acl_when_iptables_differs() { - Container container = addContainer("container1", "container1.host.com", Container.State.RUNNING); - Map<String, Acl> acls = makeAcl(container.hostname, "4321", "2001::1"); - - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); - - whenListRules(container.name, "filter", IPVersion.IPv6, ""); - whenListRules(container.name, "filter", IPVersion.IPv4, ""); - whenListRules(container.name, "nat", IPVersion.IPv4, ""); - whenListRules(container.name, "nat", IPVersion.IPv6, ""); - - aclMaintainer.converge(); - - verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(container.name), eq("iptables-restore"), any()); //we don;t have a ip4 address for the container so no redirect either - verify(dockerOperations, times(2)).executeCommandInNetworkNamespace(eq(container.name), eq("ip6tables-restore"), any()); - } - - @Test - public void ignore_containers_not_running() { - Container container = addContainer("container1", "container1.host.com", Container.State.EXITED); - Map<String, Acl> acls = makeAcl(container.hostname, "4321", "2001::1"); + private static final String EMPTY_FILTER_TABLE = "-P INPUT ACCEPT\n-P FORWARD ACCEPT\n-P OUTPUT ACCEPT\n"; + private static final String EMPTY_NAT_TABLE = "-P PREROUTING ACCEPT\n-P INPUT ACCEPT\n-P OUTPUT ACCEPT\n-P POSTROUTING ACCEPT\n"; - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); + private final DockerOperations dockerOperations = mock(DockerOperations.class); + private final IPAddressesMock ipAddresses = new IPAddressesMock(); + private final AclMaintainer aclMaintainer = new AclMaintainer(dockerOperations, ipAddresses); - aclMaintainer.converge(); - - verify(dockerOperations, never()).executeCommandInNetworkNamespace(eq(container.name), any()); - } + private final FileSystem fileSystem = TestFileSystem.create(); + private final Function<Acl, NodeAgentContext> contextGenerator = + acl -> new NodeAgentContextImpl.Builder("container1.host.com").fileSystem(fileSystem).acl(acl).build(); + private final List<String> writtenFileContents = new ArrayList<>(); @Test - public void only_configure_iptables_for_ipversion_that_differs() { - Container container = addContainer("container1", "container1.host.com", Container.State.RUNNING); - Map<String, Acl> acls = makeAcl(container.hostname, "4321,2345,22", "2001::1", "fd01:1234::4321"); - - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); - - String IPV6 = "-P INPUT ACCEPT\n" + + public void configures_full_container_acl_from_empty() { + Acl acl = new Acl.Builder().withTrustedPorts(22, 4443) + .withTrustedNode("hostname1", "3001::abcd") + .withTrustedNode("hostname2", "3001::1234") + .withTrustedNode("hostname1", "192.168.0.5") + .withTrustedNode("hostname4", "172.16.5.234").build(); + NodeAgentContext context = contextGenerator.apply(acl); + + ipAddresses.addAddress(context.hostname().value(), "2001::1"); + ipAddresses.addAddress(context.hostname().value(), "10.0.0.1"); + + whenListRules(context, "filter", IPVersion.IPv4, EMPTY_FILTER_TABLE); + whenListRules(context, "filter", IPVersion.IPv6, EMPTY_FILTER_TABLE); + whenListRules(context, "nat", IPVersion.IPv4, EMPTY_NAT_TABLE); + whenListRules(context, "nat", IPVersion.IPv6, EMPTY_NAT_TABLE); + + aclMaintainer.converge(context); + + verify(dockerOperations, times(4)).executeCommandInNetworkNamespace(eq(context), any(), eq("-S"), eq("-t"), any()); + verify(dockerOperations, times(2)).executeCommandInNetworkNamespace(eq(context), eq("iptables-restore"), any()); + verify(dockerOperations, times(2)).executeCommandInNetworkNamespace(eq(context), eq("ip6tables-restore"), any()); + verifyNoMoreInteractions(dockerOperations); + + List<String> expected = Arrays.asList( + // IPv4 filter table restore + "*filter\n" + + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + + "-A INPUT -i lo -j ACCEPT\n" + + "-A INPUT -p icmp -j ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22,4443 -j ACCEPT\n" + + "-A INPUT -s 172.16.5.234/32 -j ACCEPT\n" + + "-A INPUT -s 192.168.0.5/32 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp-port-unreachable\n" + + "COMMIT\n", + + // IPv6 filter table restore + "*filter\n" + + "-P INPUT ACCEPT\n" + "-P FORWARD ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + "-A INPUT -i lo -j ACCEPT\n" + "-A INPUT -p ipv6-icmp -j ACCEPT\n" + - "-A INPUT -p tcp -m multiport --dports 22,2345,4321 -j ACCEPT\n" + - "-A INPUT -s 2001::1/128 -j ACCEPT\n" + - "-A INPUT -s fd01:1234::4321/128 -j ACCEPT\n" + - "-A INPUT -j REJECT --reject-with icmp6-port-unreachable"; - - String NATv6 = "-P PREROUTING ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22,4443 -j ACCEPT\n" + + "-A INPUT -s 3001::1234/128 -j ACCEPT\n" + + "-A INPUT -s 3001::abcd/128 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp6-port-unreachable\n" + + "COMMIT\n", + + // IPv4 nat table restore + "*nat\n" + + "-P PREROUTING ACCEPT\n" + "-P INPUT ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT"; + "-A OUTPUT -d 10.0.0.1/32 -j REDIRECT\n" + + "COMMIT\n", - whenListRules(container.name, "filter", IPVersion.IPv6, IPV6); - whenListRules(container.name, "filter", IPVersion.IPv4, ""); //IPv4 will then differ from wanted - whenListRules(container.name, "nat", IPVersion.IPv6, NATv6); - - aclMaintainer.converge(); - - verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(container.name), eq("iptables-restore"), any()); - verify(dockerOperations, never()).executeCommandInNetworkNamespace(eq(container.name), eq("ip6tables-restore"), any()); + // IPv6 nat table restore + "*nat\n" + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-A OUTPUT -d 2001::1/128 -j REDIRECT\n" + + "COMMIT\n"); + assertEquals(expected, writtenFileContents); } @Test - public void does_not_configure_acl_if_iptables_dualstack_are_ok() { - Container container = addContainer("container1", "container1.host.com", Container.State.RUNNING); - Map<String, Acl> acls = makeAcl(container.hostname, "22,4443,2222", "2001::1", "192.64.13.2"); - - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); - - String IPV4_FILTER = "-P INPUT ACCEPT\n" + + public void configures_minimal_container_acl_from_empty() { + // The ACL spec is empty and our this node's addresses do not resolve + Acl acl = new Acl.Builder().withTrustedPorts().build(); + NodeAgentContext context = contextGenerator.apply(acl); + + whenListRules(context, "filter", IPVersion.IPv4, EMPTY_FILTER_TABLE); + whenListRules(context, "filter", IPVersion.IPv6, EMPTY_FILTER_TABLE); + whenListRules(context, "nat", IPVersion.IPv4, EMPTY_NAT_TABLE); + whenListRules(context, "nat", IPVersion.IPv6, EMPTY_NAT_TABLE); + + aclMaintainer.converge(context); + + verify(dockerOperations, times(2)).executeCommandInNetworkNamespace(eq(context), any(), eq("-S"), eq("-t"), any()); + verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(context), eq("iptables-restore"), any()); + verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(context), eq("ip6tables-restore"), any()); + verifyNoMoreInteractions(dockerOperations); + + List<String> expected = Arrays.asList( + // IPv4 filter table restore + "*filter\n" + + "-P INPUT ACCEPT\n" + "-P FORWARD ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + "-A INPUT -i lo -j ACCEPT\n" + "-A INPUT -p icmp -j ACCEPT\n" + - "-A INPUT -p tcp -m multiport --dports 22,2222,4443 -j ACCEPT\n" + - "-A INPUT -s 192.64.13.2/32 -j ACCEPT\n" + - "-A INPUT -j REJECT --reject-with icmp-port-unreachable"; + "-A INPUT -j REJECT --reject-with icmp-port-unreachable\n" + + "COMMIT\n", - String IPV6_FILTER = "-P INPUT ACCEPT\n" + + // IPv6 filter table restore + "*filter\n" + + "-P INPUT ACCEPT\n" + "-P FORWARD ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + "-A INPUT -i lo -j ACCEPT\n" + "-A INPUT -p ipv6-icmp -j ACCEPT\n" + - "-A INPUT -p tcp -m multiport --dports 22,2222,4443 -j ACCEPT\n" + - "-A INPUT -s 2001::1/128 -j ACCEPT\n" + - "-A INPUT -j REJECT --reject-with icmp6-port-unreachable"; + "-A INPUT -j REJECT --reject-with icmp6-port-unreachable\n" + + "COMMIT\n"); + assertEquals(expected, writtenFileContents); + } - String IPV6_NAT = "-P PREROUTING ACCEPT\n" + + @Test + public void only_configure_iptables_for_ipversion_that_differs() { + Acl acl = new Acl.Builder().withTrustedPorts(22, 4443).withTrustedNode("hostname1", "3001::abcd").build(); + NodeAgentContext context = contextGenerator.apply(acl); + + ipAddresses.addAddress(context.hostname().value(), "2001::1"); + + whenListRules(context, "filter", IPVersion.IPv4, EMPTY_FILTER_TABLE); + whenListRules(context, "filter", IPVersion.IPv6, + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + + "-A INPUT -i lo -j ACCEPT\n" + + "-A INPUT -p ipv6-icmp -j ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22,4443 -j ACCEPT\n" + + "-A INPUT -s 3001::abcd/128 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp6-port-unreachable\n"); + whenListRules(context, "nat", IPVersion.IPv6, + "-P PREROUTING ACCEPT\n" + "-P INPUT ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT"; + "-A OUTPUT -d 2001::1/128 -j REDIRECT\n"); - whenListRules(container.name, "filter", IPVersion.IPv6, IPV6_FILTER); - whenListRules(container.name, "nat", IPVersion.IPv6, IPV6_NAT); - whenListRules(container.name, "filter", IPVersion.IPv4, IPV4_FILTER); + aclMaintainer.converge(context); - aclMaintainer.converge(); + verify(dockerOperations, times(3)).executeCommandInNetworkNamespace(eq(context), any(), eq("-S"), eq("-t"), any()); + verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(context), eq("iptables-restore"), any()); + verify(dockerOperations, never()).executeCommandInNetworkNamespace(eq(context), eq("ip6tables-restore"), any()); //we don't have a ip4 address for the container so no redirect + verifyNoMoreInteractions(dockerOperations); - verify(dockerOperations, never()).executeCommandInNetworkNamespace(any(), eq("ip6tables-restore"), any()); - verify(dockerOperations, never()).executeCommandInNetworkNamespace(any(), eq("iptables-restore"), any()); + List<String> expected = Collections.singletonList( + "*filter\n" + + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + + "-A INPUT -i lo -j ACCEPT\n" + + "-A INPUT -p icmp -j ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22,4443 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp-port-unreachable\n" + + "COMMIT\n"); + assertEquals(expected, writtenFileContents); } - @Test public void rollback_is_attempted_when_applying_acl_fail() { - Container container = addContainer("container1", "container1.host.com", Container.State.RUNNING); - Map<String, Acl> acls = makeAcl(container.hostname, "4321", "2001::1"); - when(nodeRepository.getAcls(hostHostname.value())).thenReturn(acls); + Acl acl = new Acl.Builder().withTrustedPorts(22, 4443).withTrustedNode("hostname1", "3001::abcd").build(); + NodeAgentContext context = contextGenerator.apply(acl); + + ipAddresses.addAddress(context.hostname().value(), "2001::1"); - String IPV6_NAT = "-P PREROUTING ACCEPT\n" + + whenListRules(context, "filter", IPVersion.IPv4, EMPTY_FILTER_TABLE); + whenListRules(context, "filter", IPVersion.IPv6, + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + + "-A INPUT -i lo -j ACCEPT\n" + + "-A INPUT -p ipv6-icmp -j ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22,4443 -j ACCEPT\n" + + "-A INPUT -s 3001::abcd/128 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp6-port-unreachable\n"); + whenListRules(context, "nat", IPVersion.IPv6, + "-P PREROUTING ACCEPT\n" + "-P INPUT ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT"; + "-A OUTPUT -d 2001::1/128 -j REDIRECT\n"); - whenListRules(container.name, "filter", IPVersion.IPv6, ""); - whenListRules(container.name, "filter", IPVersion.IPv4, ""); - whenListRules(container.name, "nat", IPVersion.IPv6, IPV6_NAT); + when(dockerOperations.executeCommandInNetworkNamespace(eq(context), eq("iptables-restore"), any())) + .thenThrow(new RuntimeException("iptables restore failed")); - when(dockerOperations.executeCommandInNetworkNamespace( - eq(container.name), - eq("ip6tables-restore"), any())).thenThrow(new RuntimeException("iptables restore failed")); + aclMaintainer.converge(context); - when(dockerOperations.executeCommandInNetworkNamespace( - eq(container.name), - eq("iptables-restore"), any())).thenThrow(new RuntimeException("iptables restore failed")); + verify(dockerOperations, times(3)).executeCommandInNetworkNamespace(eq(context), any(), eq("-S"), eq("-t"), any()); + verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(context), eq("iptables-restore"), any()); + verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(context), eq("iptables"), eq("-F"), eq("-t"), eq("filter")); + verifyNoMoreInteractions(dockerOperations); - aclMaintainer.converge(); + aclMaintainer.converge(context); + } - verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(container.name), - eq("ip6tables"), eq("-F"), eq("-t"), eq("filter")); - verify(dockerOperations, times(1)).executeCommandInNetworkNamespace(eq(container.name), - eq("iptables"), eq("-F"), eq("-t"), eq("filter")); + @Before + public void setup() { + doAnswer(invoc -> { + String path = invoc.getArgument(2); + writtenFileContents.add(new UnixPath(path).readUtf8File()); + return new ProcessResult(0, "", ""); + }).when(dockerOperations).executeCommandInNetworkNamespace(any(), endsWith("-restore"), any()); } - private void whenListRules(ContainerName name, String table, IPVersion ipVersion, String result) { + private void whenListRules(NodeAgentContext context, String table, IPVersion ipVersion, String result) { when(dockerOperations.executeCommandInNetworkNamespace( - eq(name), - eq(ipVersion.iptablesCmd()), eq("-S"), eq("-t"), eq(table))) + eq(context), eq(ipVersion.iptablesCmd()), eq("-S"), eq("-t"), eq(table))) .thenReturn(new ProcessResult(0, result, "")); } - - private Container addContainer(String name, String hostname, Container.State state) { - final ContainerName containerName = new ContainerName(name); - final Container container = new Container(hostname, new DockerImage("mock"), null, - containerName, state, 2); - containers.put(name, container); - containerList.add(container); - ipAddresses.addAddress(hostname, "3001::" + containers.size()); - return container; - } - - private Map<String, Acl> makeAcl(String containerHostname, String portsCommaSeparated, String... addresses) { - Acl.Builder aclBuilder = new Acl.Builder(); - - Arrays.stream(portsCommaSeparated.split(",")) - .map(Integer::valueOf) - .forEach(aclBuilder::withTrustedPorts); - - Arrays.stream(addresses) - .forEach(address -> aclBuilder.withTrustedNode(new Acl.Node("hostname", address))); - - Map<String, Acl> map = new HashMap<>(); - map.put(containerHostname, aclBuilder.build()); - return map; - } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/FilterTableLineEditorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/FilterTableLineEditorTest.java new file mode 100644 index 00000000000..f72cde92839 --- /dev/null +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/FilterTableLineEditorTest.java @@ -0,0 +1,50 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.node.admin.maintenance.acl; + +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; +import com.yahoo.vespa.hosted.node.admin.task.util.file.Editor; +import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; +import org.junit.Test; + +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; + +/** + * @author freva + */ +public class FilterTableLineEditorTest { + + @Test + public void filter_set_wanted_rules() { + Acl acl = new Acl.Builder().withTrustedPorts(22).withTrustedNode("hostname", "3001::1").build(); + + assertFilterTableLineEditorResult( + acl, IPVersion.IPv6, + + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n", + + "-P INPUT ACCEPT\n" + + "-P FORWARD ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + + "-A INPUT -i lo -j ACCEPT\n" + + "-A INPUT -p ipv6-icmp -j ACCEPT\n" + + "-A INPUT -p tcp -m multiport --dports 22 -j ACCEPT\n" + + "-A INPUT -s 3001::1/128 -j ACCEPT\n" + + "-A INPUT -j REJECT --reject-with icmp6-port-unreachable"); + } + + private static void assertFilterTableLineEditorResult( + Acl acl, IPVersion ipVersion, String currentFilterTable, String expectedRestoreFileContent) { + FilterTableLineEditor filterLineEditor = FilterTableLineEditor.from(acl, ipVersion); + Editor editor = new Editor( + "nat-table", + () -> Arrays.asList(currentFilterTable.split("\n")), + result -> assertEquals(expectedRestoreFileContent, String.join("\n", result)), + filterLineEditor); + editor.edit(m -> {}); + } +}
\ No newline at end of file diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditorTest.java deleted file mode 100644 index abcba44bb7a..00000000000 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/IPTablesEditorTest.java +++ /dev/null @@ -1,144 +0,0 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.node.admin.maintenance.acl; - -import com.yahoo.vespa.hosted.dockerapi.ContainerName; -import com.yahoo.vespa.hosted.dockerapi.ProcessResult; -import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; -import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; -import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion; -import org.junit.Assert; -import org.junit.Test; - -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -public class IPTablesEditorTest { - - private final DockerOperations dockerOperations = mock(DockerOperations.class); - private final ContainerName containerName = ContainerName.fromHostname("myhostname"); - private String actualRestoreContent = null; - private final IPTablesEditor editor = new IPTablesEditor(dockerOperations, containerName, (fileContent) -> actualRestoreContent = fileContent); - - @Test - public void filter_set_wanted_rules() { - Acl acl = new Acl.Builder().withTrustedPorts(22).withTrustedNode(new Acl.Node("hostname", "3001::1")).build(); - FilterTableLineEditor filterLineEditor = FilterTableLineEditor.from(acl, IPVersion.IPv6); - - String currentFilterTable = "-P INPUT ACCEPT\n" + - "-P FORWARD ACCEPT\n" + - "-P OUTPUT ACCEPT\n"; - - String expectedRestoreFileContent = "*filter\n" + - "-P INPUT ACCEPT\n" + - "-P FORWARD ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" + - "-A INPUT -i lo -j ACCEPT\n" + - "-A INPUT -p ipv6-icmp -j ACCEPT\n" + - "-A INPUT -p tcp -m multiport --dports 22 -j ACCEPT\n" + - "-A INPUT -s 3001::1/128 -j ACCEPT\n" + - "-A INPUT -j REJECT --reject-with icmp6-port-unreachable\n" + - "COMMIT\n"; - - whenListRules(containerName, "filter", IPVersion.IPv6, currentFilterTable); - - editor.edit("filter", IPVersion.IPv6, filterLineEditor, false); - - Assert.assertEquals(expectedRestoreFileContent, actualRestoreContent); - } - - @Test - public void nat_set_redirect_rule_without_touching_docker_rules() { - NatTableLineEditor natLineEditor = NatTableLineEditor.from("-A OUTPUT -d 3001::1/128 -j REDIRECT"); - - String currentNatTable = "-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-N DOCKER_OUTPUT\n" + - "-N DOCKER_POSTROUTING\n" + - "-A OUTPUT -d 127.0.0.11/32 -j DOCKER_OUTPUT\n" + - "-A POSTROUTING -d 127.0.0.11/32 -j DOCKER_POSTROUTING\n" + - "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p tcp -m tcp --dport 53 -j DNAT --to-destination 127.0.0.11:43500\n" + - "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p udp -m udp --dport 53 -j DNAT --to-destination 127.0.0.11:57392\n" + - "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p tcp -m tcp --sport 43500 -j SNAT --to-source :53\n" + - "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p udp -m udp --sport 57392 -j SNAT --to-source :53\n"; - - String expectedRestoreFileContent = "*nat\n-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-N DOCKER_OUTPUT\n" + - "-N DOCKER_POSTROUTING\n" + - "-A OUTPUT -d 127.0.0.11/32 -j DOCKER_OUTPUT\n" + - "-A POSTROUTING -d 127.0.0.11/32 -j DOCKER_POSTROUTING\n" + - "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p tcp -m tcp --dport 53 -j DNAT --to-destination 127.0.0.11:43500\n" + - "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p udp -m udp --dport 53 -j DNAT --to-destination 127.0.0.11:57392\n" + - "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p tcp -m tcp --sport 43500 -j SNAT --to-source :53\n" + - "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p udp -m udp --sport 57392 -j SNAT --to-source :53\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT\nCOMMIT\n"; - - whenListRules(containerName, "nat", IPVersion.IPv6, currentNatTable); - - editor.edit("nat", IPVersion.IPv6, natLineEditor, false); - - Assert.assertEquals(expectedRestoreFileContent, actualRestoreContent); - } - - @Test - public void nat_cleanup_wrong_redirect_rules() { - NatTableLineEditor natLineEditor = NatTableLineEditor.from("-A OUTPUT -d 3001::1/128 -j REDIRECT"); - - String currentNatTable = "-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::2/128 -j REDIRECT\n"; - - String expectedRestoreFileContent = "-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT\n"; - - whenListRules(containerName, "nat", IPVersion.IPv6, currentNatTable); - - editor.edit("nat", IPVersion.IPv6, natLineEditor, false); - - Assert.assertEquals(expectedRestoreFileContent, expectedRestoreFileContent); - } - - @Test - public void nat_delete_duplicate_rules() { - NatTableLineEditor natLineEditor = NatTableLineEditor.from("-A OUTPUT -d 3001::1/128 -j REDIRECT"); - - String currentNatTable = "-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::2/128 -j REDIRECT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT\n" + - "-A OUTPUT -d 3001::4/128 -j REDIRECT\n"; - - String expectedRestoreFileContent = "-P PREROUTING ACCEPT\n" + - "-P INPUT ACCEPT\n" + - "-P OUTPUT ACCEPT\n" + - "-P POSTROUTING ACCEPT\n" + - "-A OUTPUT -d 3001::1/128 -j REDIRECT\n"; - - whenListRules(containerName, "nat", IPVersion.IPv6, currentNatTable); - - editor.edit("nat", IPVersion.IPv6, natLineEditor, false); - - Assert.assertEquals(expectedRestoreFileContent, expectedRestoreFileContent); - } - - private void whenListRules(ContainerName name, String table, IPVersion ipVersion, String result) { - when(dockerOperations.executeCommandInNetworkNamespace( - eq(name), - eq(ipVersion.iptablesCmd()), eq("-S"), eq("-t"), eq(table))) - .thenReturn(new ProcessResult(0, result, "")); - } - -} diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/NatTableLineEditorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/NatTableLineEditorTest.java new file mode 100644 index 00000000000..63dc69a180c --- /dev/null +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/NatTableLineEditorTest.java @@ -0,0 +1,96 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.node.admin.maintenance.acl; + +import com.yahoo.vespa.hosted.node.admin.task.util.file.Editor; +import org.junit.Test; + +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; + +/** + * @author freva + */ +public class NatTableLineEditorTest { + + @Test + public void nat_set_redirect_rule_without_touching_docker_rules() { + assertNatTableLineEditorResult( + "-A OUTPUT -d 3001::1/128 -j REDIRECT", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-N DOCKER_OUTPUT\n" + + "-N DOCKER_POSTROUTING\n" + + "-A OUTPUT -d 127.0.0.11/32 -j DOCKER_OUTPUT\n" + + "-A POSTROUTING -d 127.0.0.11/32 -j DOCKER_POSTROUTING\n" + + "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p tcp -m tcp --dport 53 -j DNAT --to-destination 127.0.0.11:43500\n" + + "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p udp -m udp --dport 53 -j DNAT --to-destination 127.0.0.11:57392\n" + + "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p tcp -m tcp --sport 43500 -j SNAT --to-source :53\n" + + "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p udp -m udp --sport 57392 -j SNAT --to-source :53\n", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-N DOCKER_OUTPUT\n" + + "-N DOCKER_POSTROUTING\n" + + "-A OUTPUT -d 127.0.0.11/32 -j DOCKER_OUTPUT\n" + + "-A POSTROUTING -d 127.0.0.11/32 -j DOCKER_POSTROUTING\n" + + "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p tcp -m tcp --dport 53 -j DNAT --to-destination 127.0.0.11:43500\n" + + "-A DOCKER_OUTPUT -d 127.0.0.11/32 -p udp -m udp --dport 53 -j DNAT --to-destination 127.0.0.11:57392\n" + + "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p tcp -m tcp --sport 43500 -j SNAT --to-source :53\n" + + "-A DOCKER_POSTROUTING -s 127.0.0.11/32 -p udp -m udp --sport 57392 -j SNAT --to-source :53\n" + + "-A OUTPUT -d 3001::1/128 -j REDIRECT"); + } + + @Test + public void nat_cleanup_wrong_redirect_rules() { + assertNatTableLineEditorResult( + "-A OUTPUT -d 3001::1/128 -j REDIRECT", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-A OUTPUT -d 3001::2/128 -j REDIRECT\n", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-A OUTPUT -d 3001::1/128 -j REDIRECT"); + } + + @Test + public void nat_delete_duplicate_rules() { + assertNatTableLineEditorResult( + "-A OUTPUT -d 3001::1/128 -j REDIRECT", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-A OUTPUT -d 3001::2/128 -j REDIRECT\n" + + "-A OUTPUT -d 3001::1/128 -j REDIRECT\n" + + "-A OUTPUT -d 3001::4/128 -j REDIRECT\n", + + "-P PREROUTING ACCEPT\n" + + "-P INPUT ACCEPT\n" + + "-P OUTPUT ACCEPT\n" + + "-P POSTROUTING ACCEPT\n" + + "-A OUTPUT -d 3001::1/128 -j REDIRECT"); + } + + private static void assertNatTableLineEditorResult(String redirectRule, String currentNatTable, String expectedNatTable) { + NatTableLineEditor natLineEditor = NatTableLineEditor.from(redirectRule); + Editor editor = new Editor( + "nat-table", + () -> Arrays.asList(currentNatTable.split("\n")), + result -> assertEquals(expectedNatTable, String.join("\n", result)), + natLineEditor); + editor.edit(m -> {}); + } +}
\ No newline at end of file diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java index f8e87ccef53..2f835a535ff 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java @@ -15,9 +15,9 @@ import org.mockito.InOrder; import java.time.Duration; import java.util.ArrayList; -import java.util.Collections; +import java.util.HashSet; import java.util.List; -import java.util.Optional; +import java.util.Set; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -44,40 +44,40 @@ public class NodeAdminImplTest { private final NodeAgentContextFactory nodeAgentContextFactory = mock(NodeAgentContextFactory.class); private final ManualClock clock = new ManualClock(); - private final NodeAdminImpl nodeAdmin = new NodeAdminImpl(nodeAgentWithSchedulerFactory, nodeAgentContextFactory, - Optional.empty(), new MetricReceiverWrapper(MetricReceiver.nullImplementation), clock, Duration.ZERO, Duration.ZERO); + private final NodeAdminImpl nodeAdmin = new NodeAdminImpl(nodeAgentWithSchedulerFactory, + new MetricReceiverWrapper(MetricReceiver.nullImplementation), clock, Duration.ZERO, Duration.ZERO); @Test public void nodeAgentsAreProperlyLifeCycleManaged() { - final NodeSpec nodeSpec1 = createNodeSpec("host1.test.yahoo.com"); - final NodeSpec nodeSpec2 = createNodeSpec("host2.test.yahoo.com"); - final NodeAgentWithScheduler nodeAgent1 = mockNodeAgentWithSchedulerFactory(nodeSpec1); - final NodeAgentWithScheduler nodeAgent2 = mockNodeAgentWithSchedulerFactory(nodeSpec2); + final NodeAgentContext context1 = createNodeAgentContext("host1.test.yahoo.com"); + final NodeAgentContext context2 = createNodeAgentContext("host2.test.yahoo.com"); + final NodeAgentWithScheduler nodeAgent1 = mockNodeAgentWithSchedulerFactory(context1); + final NodeAgentWithScheduler nodeAgent2 = mockNodeAgentWithSchedulerFactory(context2); final InOrder inOrder = inOrder(nodeAgentWithSchedulerFactory, nodeAgent1, nodeAgent2); - nodeAdmin.refreshContainersToRun(Collections.emptyList()); + nodeAdmin.refreshContainersToRun(Set.of()); verifyNoMoreInteractions(nodeAgentWithSchedulerFactory); - nodeAdmin.refreshContainersToRun(Collections.singletonList(nodeSpec1)); + nodeAdmin.refreshContainersToRun(Set.of(context1)); inOrder.verify(nodeAgent1).start(); inOrder.verify(nodeAgent2, never()).start(); inOrder.verify(nodeAgent1, never()).stop(); - nodeAdmin.refreshContainersToRun(Collections.singletonList(nodeSpec1)); + nodeAdmin.refreshContainersToRun(Set.of(context1)); inOrder.verify(nodeAgentWithSchedulerFactory, never()).create(any()); inOrder.verify(nodeAgent1, never()).start(); inOrder.verify(nodeAgent1, never()).stop(); - nodeAdmin.refreshContainersToRun(Collections.emptyList()); + nodeAdmin.refreshContainersToRun(Set.of()); inOrder.verify(nodeAgentWithSchedulerFactory, never()).create(any()); verify(nodeAgent1).stop(); - nodeAdmin.refreshContainersToRun(Collections.singletonList(nodeSpec2)); + nodeAdmin.refreshContainersToRun(Set.of(context2)); inOrder.verify(nodeAgent2).start(); inOrder.verify(nodeAgent2, never()).stop(); inOrder.verify(nodeAgent1, never()).stop(); - nodeAdmin.refreshContainersToRun(Collections.emptyList()); + nodeAdmin.refreshContainersToRun(Set.of()); inOrder.verify(nodeAgentWithSchedulerFactory, never()).create(any()); inOrder.verify(nodeAgent2, never()).start(); inOrder.verify(nodeAgent2).stop(); @@ -87,17 +87,17 @@ public class NodeAdminImplTest { @Test public void testSetFrozen() { - List<NodeSpec> nodeSpecs = new ArrayList<>(); + Set<NodeAgentContext> contexts = new HashSet<>(); List<NodeAgentWithScheduler> nodeAgents = new ArrayList<>(); for (int i = 0; i < 3; i++) { - NodeSpec nodeSpec = createNodeSpec("host" + i + ".test.yahoo.com"); - NodeAgentWithScheduler nodeAgent = mockNodeAgentWithSchedulerFactory(nodeSpec); + NodeAgentContext context = createNodeAgentContext("host" + i + ".test.yahoo.com"); + NodeAgentWithScheduler nodeAgent = mockNodeAgentWithSchedulerFactory(context); - nodeSpecs.add(nodeSpec); + contexts.add(context); nodeAgents.add(nodeAgent); } - nodeAdmin.refreshContainersToRun(nodeSpecs); + nodeAdmin.refreshContainersToRun(contexts); assertTrue(nodeAdmin.isFrozen()); // Initially everything is frozen to force convergence mockNodeAgentSetFrozenResponse(nodeAgents, true, true, true); @@ -160,19 +160,18 @@ public class NodeAdminImplTest { } } - private NodeSpec createNodeSpec(String hostname) { - return new NodeSpec.Builder() + private NodeAgentContext createNodeAgentContext(String hostname) { + NodeSpec nodeSpec = new NodeSpec.Builder() .hostname(hostname) .state(Node.State.active) .nodeType(NodeType.tenant) .flavor("default") .build(); - } - private NodeAgentWithScheduler mockNodeAgentWithSchedulerFactory(NodeSpec nodeSpec) { - NodeAgentContext context = new NodeAgentContextImpl.Builder(nodeSpec).build(); - when(nodeAgentContextFactory.create(eq(nodeSpec))).thenReturn(context); + return new NodeAgentContextImpl.Builder(nodeSpec).build(); + } + private NodeAgentWithScheduler mockNodeAgentWithSchedulerFactory(NodeAgentContext context) { NodeAgentWithScheduler nodeAgentWithScheduler = mock(NodeAgentWithScheduler.class); when(nodeAgentWithSchedulerFactory.create(eq(context))).thenReturn(nodeAgentWithScheduler); return nodeAgentWithScheduler; diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java index 74ba5561c8e..2387c279672 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java @@ -3,15 +3,21 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeType; +import com.yahoo.test.ManualClock; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.Orchestrator; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextFactory; import com.yahoo.vespa.hosted.provision.Node; import org.junit.Test; import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import java.util.Map; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -22,6 +28,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -37,13 +44,15 @@ import static org.mockito.Mockito.when; * @author freva */ public class NodeAdminStateUpdaterTest { + private final NodeAgentContextFactory nodeAgentContextFactory = mock(NodeAgentContextFactory.class); private final NodeRepository nodeRepository = mock(NodeRepository.class); private final Orchestrator orchestrator = mock(Orchestrator.class); private final NodeAdmin nodeAdmin = mock(NodeAdmin.class); private final HostName hostHostname = HostName.from("basehost1.test.yahoo.com"); + private final ManualClock clock = new ManualClock(); private final NodeAdminStateUpdater updater = spy(new NodeAdminStateUpdater( - nodeRepository, orchestrator, nodeAdmin, hostHostname)); + nodeAgentContextFactory, nodeRepository, orchestrator, nodeAdmin, hostHostname, clock)); @Test @@ -167,6 +176,77 @@ public class NodeAdminStateUpdaterTest { verify(orchestrator, times(1)).suspend(eq(hostHostname.value()), eq(activeHostnames)); } + @Test + public void uses_cached_acl() { + mockNodeRepo(Node.State.active, 1); + mockAcl(Acl.EMPTY, 1); + + updater.adjustNodeAgentsToRunFromNodeRepository(); + verify(nodeRepository, times(1)).getAcls(any()); + clock.advance(Duration.ofSeconds(30)); + + updater.adjustNodeAgentsToRunFromNodeRepository(); + clock.advance(Duration.ofSeconds(30)); + updater.adjustNodeAgentsToRunFromNodeRepository(); + clock.advance(Duration.ofSeconds(30)); + verify(nodeRepository, times(1)).getAcls(any()); + + clock.advance(Duration.ofSeconds(30)); + updater.adjustNodeAgentsToRunFromNodeRepository(); + verify(nodeRepository, times(2)).getAcls(any()); + } + + @Test + public void node_spec_and_acl_aligned() { + Acl acl = new Acl.Builder().withTrustedPorts(22).build(); + mockNodeRepo(Node.State.active, 3); + mockAcl(acl, 1, 2, 3); + + updater.adjustNodeAgentsToRunFromNodeRepository(); + updater.adjustNodeAgentsToRunFromNodeRepository(); + updater.adjustNodeAgentsToRunFromNodeRepository(); + + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host1.yahoo.com")), eq(acl)); + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host2.yahoo.com")), eq(acl)); + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host3.yahoo.com")), eq(acl)); + verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value())); + verify(nodeRepository, times(1)).getAcls(eq(hostHostname.value())); + } + + @Test + public void node_spec_and_acl_mismatch_missing_one_acl() { + Acl acl = new Acl.Builder().withTrustedPorts(22).build(); + mockNodeRepo(Node.State.active, 3); + mockAcl(acl, 1, 2); // Acl for 3 is missing + + updater.adjustNodeAgentsToRunFromNodeRepository(); + mockNodeRepo(Node.State.active, 2); // Next tick, the spec for 3 is no longer returned + updater.adjustNodeAgentsToRunFromNodeRepository(); + updater.adjustNodeAgentsToRunFromNodeRepository(); + + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host1.yahoo.com")), eq(acl)); + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host2.yahoo.com")), eq(acl)); + verify(nodeAgentContextFactory, times(1)).create(argThat(spec -> spec.getHostname().equals("host3.yahoo.com")), eq(Acl.EMPTY)); + verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value())); + verify(nodeRepository, times(2)).getAcls(eq(hostHostname.value())); // During the first tick, the cache is invalidated and retried + } + + @Test + public void node_spec_and_acl_mismatch_additional_acl() { + Acl acl = new Acl.Builder().withTrustedPorts(22).build(); + mockNodeRepo(Node.State.active, 2); + mockAcl(acl, 1, 2, 3); // Acl for 3 is extra + + updater.adjustNodeAgentsToRunFromNodeRepository(); + updater.adjustNodeAgentsToRunFromNodeRepository(); + updater.adjustNodeAgentsToRunFromNodeRepository(); + + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host1.yahoo.com")), eq(acl)); + verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.getHostname().equals("host2.yahoo.com")), eq(acl)); + verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value())); + verify(nodeRepository, times(1)).getAcls(eq(hostHostname.value())); + } + private void assertConvergeError(NodeAdminStateUpdater.State targetState, String reason) { try { updater.converge(targetState); @@ -177,9 +257,9 @@ public class NodeAdminStateUpdaterTest { } private void mockNodeRepo(Node.State hostState, int numberOfNodes) { - List<NodeSpec> containersToRun = IntStream.range(0, numberOfNodes) + List<NodeSpec> containersToRun = IntStream.range(1, numberOfNodes + 1) .mapToObj(i -> new NodeSpec.Builder() - .hostname("host" + i + ".test.yahoo.com") + .hostname("host" + i + ".yahoo.com") .state(Node.State.active) .nodeType(NodeType.tenant) .flavor("docker") @@ -201,4 +281,12 @@ public class NodeAdminStateUpdaterTest { .minDiskAvailableGb(1) .build()); } + + private void mockAcl(Acl acl, int... nodeIds) { + Map<String, Acl> aclByHostname = Arrays.stream(nodeIds) + .mapToObj(i -> "host" + i + ".yahoo.com") + .collect(Collectors.toMap(Function.identity(), h -> acl)); + + when(nodeRepository.getAcls(eq(hostHostname.value()))).thenReturn(aclByHostname); + } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java index 60c36ad4412..35494b375f6 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java @@ -199,7 +199,7 @@ public class NodeAgentImplTest { inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage)); inOrder.verify(dockerOperations, times(1)).createContainer(eq(context), any()); inOrder.verify(dockerOperations, times(1)).startContainer(eq(context)); - inOrder.verify(aclMaintainer, times(1)).converge(); + inOrder.verify(aclMaintainer, times(1)).converge(eq(context)); inOrder.verify(dockerOperations, times(1)).resumeNode(eq(context)); inOrder.verify(healthChecker, times(1)).verifyHealth(eq(context)); inOrder.verify(nodeRepository).updateNodeAttributes( @@ -742,7 +742,7 @@ public class NodeAgentImplTest { inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage)); inOrder.verify(dockerOperations, times(1)).createContainer(eq(context), any()); inOrder.verify(dockerOperations, times(1)).startContainer(eq(context)); - inOrder.verify(aclMaintainer, times(1)).converge(); + inOrder.verify(aclMaintainer, times(1)).converge(eq(context)); inOrder.verify(dockerOperations, times(1)).resumeNode(eq(context)); inOrder.verify(nodeRepository).updateNodeAttributes( hostName, new NodeAttributes().withDockerImage(dockerImage)); |