Merge remote-tracking branch 'origin/master'

16 files changed, 303 insertions, 90 deletions

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 3f9f0686d82..0785e292a79 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
@@ -28,7 +28,7 @@ public interface DockerOperations {
 
     ProcessResult executeCommandInContainerAsRoot(ContainerName containerName, String[] command);
 
-    void executeCommandInNetworkNamespace(ContainerName containerName, String[] command);
+    String executeCommandInNetworkNamespace(ContainerName containerName, String[] command);
 
     void resumeNode(ContainerName containerName);
 
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 aa4e2b892be..1474b66fae7 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
@@ -30,7 +30,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
-import java.util.function.Consumer;
+import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -78,14 +78,14 @@ public class DockerOperationsImpl implements DockerOperations {
 
     private final Docker docker;
     private final Environment environment;
-    private final Consumer<List<String>> commandExecutor;
+    private final Function<List<String>, String> commandExecutor;
     private GaugeWrapper numberOfRunningContainersGauge;
 
     public DockerOperationsImpl(Docker docker, Environment environment, MetricReceiverWrapper metricReceiver) {
         this(docker, environment, metricReceiver, DockerOperationsImpl::runCommand);
     }
 
-    DockerOperationsImpl(Docker docker, Environment environment, MetricReceiverWrapper metricReceiver, Consumer<List<String>> commandExecutor) {
+    DockerOperationsImpl(Docker docker, Environment environment, MetricReceiverWrapper metricReceiver, Function<List<String>, String> commandExecutor) {
         this.docker = docker;
         this.environment = environment;
         setMetrics(metricReceiver);
@@ -278,7 +278,7 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
-    public void executeCommandInNetworkNamespace(ContainerName containerName, String[] command) {
+    public String executeCommandInNetworkNamespace(ContainerName containerName, String[] command) {
         final PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
         final Docker.ContainerInfo containerInfo = docker.inspectContainer(containerName)
                 .orElseThrow(() -> new RuntimeException("Container " + containerName + " does not exist"));
@@ -294,7 +294,7 @@ public class DockerOperationsImpl implements DockerOperations {
         wrappedCommand.addAll(Arrays.asList(command));
 
         try {
-            commandExecutor.accept(wrappedCommand);
+            return commandExecutor.apply(wrappedCommand);
         } catch (Exception e) {
             logger.error(String.format("Failed to execute %s in network namespace for %s (PID = %d)",
                     Arrays.toString(command), containerName.asString(), containerPid));
@@ -343,16 +343,17 @@ public class DockerOperationsImpl implements DockerOperations {
         numberOfRunningContainersGauge.sample(getAllManagedContainers().size());
     }
 
-    private static void runCommand(final List<String> command) {
-        ProcessBuilder builder = new ProcessBuilder(command);
-        builder.redirectErrorStream(true);
+    private static String runCommand(List<String> command) {
         try {
-            Process process = builder.start();
-            String output = CharStreams.toString(new InputStreamReader(process.getInputStream()));
-            int resultCode = process.waitFor();
+            final Process process = new ProcessBuilder(command)
+                    .redirectErrorStream(true)
+                    .start();
+            final String output = CharStreams.toString(new InputStreamReader(process.getInputStream()));
+            final int resultCode = process.waitFor();
             if (resultCode != 0) {
                 throw new RuntimeException("Command " + Joiner.on(' ').join(command) + " failed: " + output);
             }
+            return output;
         } catch (IOException|InterruptedException e) {
             throw new RuntimeException(e);
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/AclMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
index 8ecd0716958..73e50705fc4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/AclMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
@@ -1,4 +1,4 @@
-package com.yahoo.vespa.hosted.node.admin.maintenance;
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl;
 
 import com.google.common.net.InetAddresses;
 import com.yahoo.net.HostName;
@@ -6,9 +6,15 @@ import com.yahoo.vespa.hosted.dockerapi.Container;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.ContainerAclSpec;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.Action;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.Chain;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.FilterCommand;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.FlushCommand;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.ListCommand;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.PolicyCommand;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.Command;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables.CommandList;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
-import com.yahoo.vespa.hosted.node.admin.util.IpTables;
-import com.yahoo.vespa.hosted.node.admin.util.IpTables.Action;
 import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger;
 
 import java.net.Inet6Address;
@@ -23,7 +29,7 @@ import java.util.stream.Collectors;
  * repository. Based on those ACLs, iptables commands are created and then executed in each of the containers network
  * namespace.
  *
- * If a rule cannot be configured (e.g. iptables process execution fails), a rollback is attempted by setting the
+ * If an ACL cannot be configured (e.g. iptables process execution fails), a rollback is attempted by setting the
  * default policy to ACCEPT which will allow any traffic. The configuration will be retried the next time the
  * maintainer runs.
  *
@@ -36,6 +42,8 @@ public class AclMaintainer implements Runnable {
 
     private static final PrefixLogger log = PrefixLogger.getNodeAdminLogger(AclMaintainer.class);
 
+    private static final String IPTABLES_COMMAND = "ip6tables";
+
     private final DockerOperations dockerOperations;
     private final NodeRepository nodeRepository;
     private final Supplier<String> nodeAdminHostnameSupplier;
@@ -51,22 +59,27 @@ public class AclMaintainer implements Runnable {
         this.nodeAdminHostnameSupplier = nodeAdminHostnameSupplier;
     }
 
+    private boolean shouldApplyAcl(ContainerName containerName, CommandList commandList) {
+        final String currentRules = dockerOperations.executeCommandInNetworkNamespace(containerName,
+                new ListCommand().asArray(IPTABLES_COMMAND));
+        return !commandList.asString().equals(currentRules.trim());
+    }
+
     private void applyAcl(ContainerName containerName, List<ContainerAclSpec> aclSpecs) {
+        final CommandList commandList = commandListFrom(aclSpecs);
+        final Command flush = new FlushCommand(Chain.INPUT);
+        final Command rollback = new PolicyCommand(Chain.INPUT, Action.ACCEPT);
+        if (!shouldApplyAcl(containerName, commandList)) {
+            return;
+        }
         try {
-            dockerOperations.executeCommandInNetworkNamespace(containerName, IpTables.flushChain());
-            dockerOperations.executeCommandInNetworkNamespace(containerName, IpTables.allowAssociatedConnections());
-            // ICMPv6 packets are always accepted as they are required for PMTU discovery to work properly.
-            dockerOperations.executeCommandInNetworkNamespace(containerName, IpTables.allowIcmp());
-            aclSpecs.stream()
-                    .map(ContainerAclSpec::ipAddress)
-                    .filter(AclMaintainer::isIpv6)
-                    .forEach(ipAddress -> dockerOperations.executeCommandInNetworkNamespace(containerName,
-                            IpTables.allowFromAddress(ipAddress)));
-            dockerOperations.executeCommandInNetworkNamespace(containerName, IpTables.chainPolicy(Action.DROP));
+            dockerOperations.executeCommandInNetworkNamespace(containerName, flush.asArray(IPTABLES_COMMAND));
+            commandList.commands().forEach(command -> dockerOperations.executeCommandInNetworkNamespace(containerName,
+                    command.asArray(IPTABLES_COMMAND)));
         } catch (Exception e) {
             log.error("Exception occurred while configuring ACLs, attempting rollback", e);
             try {
-                dockerOperations.executeCommandInNetworkNamespace(containerName, IpTables.chainPolicy(Action.ACCEPT));
+                dockerOperations.executeCommandInNetworkNamespace(containerName, rollback.asArray(IPTABLES_COMMAND));
             } catch (Exception ne) {
                 log.error("Rollback failed, giving up", ne);
             }
@@ -102,6 +115,25 @@ public class AclMaintainer implements Runnable {
         }
     }
 
+    private static CommandList commandListFrom(List<ContainerAclSpec> aclSpecs) {
+        final List<Command> allowedIps = aclSpecs.stream()
+                .map(ContainerAclSpec::ipAddress)
+                .filter(AclMaintainer::isIpv6)
+                .map(ipAddress -> new FilterCommand(Chain.INPUT, Action.ACCEPT)
+                        .withOption("-s", String.format("%s/128", ipAddress)))
+                .collect(Collectors.toList());
+        return new CommandList()
+                .add(new PolicyCommand(Chain.INPUT, Action.DROP))
+                .add(new PolicyCommand(Chain.FORWARD, Action.ACCEPT))
+                .add(new PolicyCommand(Chain.OUTPUT, Action.ACCEPT))
+                .add(new FilterCommand(Chain.INPUT, Action.ACCEPT)
+                        .withOption("-m", "state")
+                        .withOption("--state", "RELATED,ESTABLISHED"))
+                .add(new FilterCommand(Chain.INPUT, Action.ACCEPT)
+                        .withOption("-p", "ipv6-icmp"))
+                .addAll(allowedIps);
+    }
+
     private static boolean isIpv6(String ipAddress) {
         return InetAddresses.forString(ipAddress) instanceof Inet6Address;
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Action.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Action.java
new file mode 100644
index 00000000000..79a4e700b0c
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Action.java
@@ -0,0 +1,15 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * @author mpolden
+ */
+public enum Action {
+    DROP("DROP"),
+    ACCEPT("ACCEPT");
+
+    public final String name;
+
+    Action(String name) {
+        this.name = name;
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Chain.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Chain.java
new file mode 100644
index 00000000000..fbe7304ff6b
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Chain.java
@@ -0,0 +1,16 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * @author mpolden
+ */
+public enum Chain {
+    INPUT("INPUT"),
+    FORWARD("FORWARD"),
+    OUTPUT("OUTPUT");
+
+    public final String name;
+
+    Chain(String name) {
+        this.name = name;
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Command.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Command.java
new file mode 100644
index 00000000000..ab9131c8b52
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/Command.java
@@ -0,0 +1,19 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * Represents a single iptables command
+ *
+ * @author mpolden
+ */
+public interface Command {
+
+    String asString();
+
+    default String asString(String commandName) {
+        return commandName + " "  + asString();
+    }
+
+    default String[] asArray(String commandName) {
+        return asString(commandName).split(" ");
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/CommandList.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/CommandList.java
new file mode 100644
index 00000000000..6316d07e66e
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/CommandList.java
@@ -0,0 +1,40 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a list of iptables commands
+ *
+ * @author mpolden
+ */
+public class CommandList {
+
+    private final List<Command> commands;
+
+    public CommandList() {
+        this.commands = new ArrayList<>();
+    }
+
+    public CommandList add(Command command) {
+        this.commands.add(command);
+        return this;
+    }
+
+    public CommandList addAll(List<Command> commands) {
+        this.commands.addAll(commands);
+        return this;
+    }
+
+    public List<Command> commands() {
+        return Collections.unmodifiableList(this.commands);
+    }
+
+    public String asString() {
+        return commands.stream()
+                .map(Command::asString)
+                .collect(Collectors.joining("\n"));
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FilterCommand.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FilterCommand.java
new file mode 100644
index 00000000000..48e71a82eb5
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FilterCommand.java
@@ -0,0 +1,52 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * @author mpolden
+ */
+public class FilterCommand implements Command {
+
+    private final Chain chain;
+    private final Action action;
+    private final List<Option> options;
+
+    public FilterCommand(Chain chain, Action action) {
+        this.chain = chain;
+        this.action = action;
+        this.options = new ArrayList<>();
+    }
+
+    public FilterCommand withOption(String name, String argument) {
+        options.add(new Option(name, argument));
+        return this;
+    }
+
+    @Override
+    public String asString() {
+        final StringBuilder builder = new StringBuilder();
+        builder.append("-A ").append(chain.name);
+        if (!options.isEmpty()) {
+            builder.append(" ")
+                    .append(options.stream().map(Option::asString).collect(Collectors.joining(" ")));
+        }
+        builder.append(" -j ").append(action.name);
+        return builder.toString();
+    }
+
+    private static class Option {
+        private final String name;
+        private final String argument;
+
+        public Option(String name, String argument) {
+            this.name = name;
+            this.argument = argument;
+        }
+
+        public String asString() {
+            return String.format("%s %s", name, argument);
+        }
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FlushCommand.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FlushCommand.java
new file mode 100644
index 00000000000..6fa331d85eb
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/FlushCommand.java
@@ -0,0 +1,18 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * @author mpolden
+ */
+public class FlushCommand implements Command {
+
+    private final Chain chain;
+
+    public FlushCommand(Chain chain) {
+        this.chain = chain;
+    }
+
+    @Override
+    public String asString() {
+        return String.format("-F %s", chain.name);
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/ListCommand.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/ListCommand.java
new file mode 100644
index 00000000000..d5b81119b16
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/ListCommand.java
@@ -0,0 +1,11 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * @author mpolden
+ */
+public class ListCommand implements Command {
+    @Override
+    public String asString() {
+        return "-S";
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/PolicyCommand.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/PolicyCommand.java
new file mode 100644
index 00000000000..2e3aafe6c4e
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/iptables/PolicyCommand.java
@@ -0,0 +1,20 @@
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl.iptables;
+
+/**
+ * @author mpolden
+ */
+public class PolicyCommand implements Command {
+
+    private final Chain chain;
+    private final Action policy;
+
+    public PolicyCommand(Chain chain, Action policy) {
+        this.chain = chain;
+        this.policy = policy;
+    }
+
+    @Override
+    public String asString() {
+        return String.format("-P %s %s", chain.name, policy.name);
+    }
+}
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 391368642ee..ad63f0e8265 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
@@ -11,7 +11,7 @@ import com.yahoo.vespa.hosted.dockerapi.metrics.GaugeWrapper;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
 import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
-import com.yahoo.vespa.hosted.node.admin.maintenance.AclMaintainer;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.StorageMaintainer;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent;
 import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger;
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
index ae4ea905882..6c385708cac 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
@@ -8,7 +8,7 @@ import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
-import com.yahoo.vespa.hosted.node.admin.maintenance.AclMaintainer;
+import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.StorageMaintainer;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.NodeAdmin;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.NodeAdminImpl;
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/IpTables.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/IpTables.java
deleted file mode 100644
index 68dbc6b1dbc..00000000000
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/IpTables.java
+++ /dev/null
@@ -1,52 +0,0 @@
-package com.yahoo.vespa.hosted.node.admin.util;
-
-/**
- * Utility class for creating iptables commands
- *
- * @author mpolden
- */
-public class IpTables {
-
-    private static final String COMMAND = "ip6tables";
-
-    public static String[] allowFromAddress(String ipAddress) {
-        return new String[]{COMMAND, "-A", Chain.INPUT.name, "-s", ipAddress, "-j", Action.ACCEPT.target};
-    }
-
-    public static String[] allowAssociatedConnections() {
-        return new String[]{COMMAND, "-A", Chain.INPUT.name, "-m", "state", "--state", "RELATED,ESTABLISHED",  "-j",
-                Action.ACCEPT.target};
-    }
-
-    public static String[] allowIcmp() {
-        return new String[]{COMMAND, "-A", Chain.INPUT.name, "-p", "icmpv6", "-j", Action.ACCEPT.target};
-    }
-    public static String[] chainPolicy(Action action) {
-        return new String[]{COMMAND, "-P", Chain.INPUT.name, action.target};
-    }
-
-    public static String[] flushChain() {
-        return new String[]{COMMAND, "-F", Chain.INPUT.name};
-    }
-
-    public enum Action {
-        DROP("DROP"),
-        ACCEPT("ACCEPT");
-
-        private final String target;
-
-        Action(String target) {
-            this.target = target;
-        }
-    }
-
-    public enum Chain {
-        INPUT("INPUT");
-
-        private final String name;
-
-        Chain(String name) {
-            this.name = name;
-        }
-    }
-}
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 cf660cb51c9..d87c0eb15b9 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
@@ -107,7 +107,10 @@ public class DockerOperationsImplTest {
         ContainerName container = makeContainer("container-42", 42);
         List<String> capturedArgs = new ArrayList<>();
         DockerOperationsImpl dockerOperations = new DockerOperationsImpl(docker, environment,
-                new MetricReceiverWrapper(MetricReceiver.nullImplementation), capturedArgs::addAll);
+                new MetricReceiverWrapper(MetricReceiver.nullImplementation), (args) -> {
+            capturedArgs.addAll(args);
+            return "";
+        });
 
         dockerOperations.executeCommandInNetworkNamespace(container, new String[]{"iptables", "-nvL"});
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/AclMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
index 419ee0b7268..450cf7036b0 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/AclMaintainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
@@ -1,4 +1,4 @@
-package com.yahoo.vespa.hosted.node.admin.maintenance;
+package com.yahoo.vespa.hosted.node.admin.maintenance.acl;
 
 import com.yahoo.vespa.hosted.dockerapi.Container;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
@@ -10,6 +10,7 @@ import com.yahoo.vespa.hosted.node.admin.integrationTests.NodeRepoMock;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -18,7 +19,6 @@ import java.util.stream.IntStream;
 import static org.mockito.AdditionalMatchers.aryEq;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -44,11 +44,25 @@ public class AclMaintainerTest {
         Container container = makeContainer("container-1");
         List<ContainerAclSpec> aclSpecs = makeAclSpecs(3, container.name);
         nodeRepository.addContainerAclSpecs(NODE_ADMIN_HOSTNAME, aclSpecs);
+        mockExistingAcls(container.name);
         aclMaintainer.run();
         assertAclsApplied(container.name, aclSpecs);
     }
 
     @Test
+    public void does_not_configure_acl_if_unchanged() {
+        Container container = makeContainer("container-1");
+        List<ContainerAclSpec> aclSpecs = makeAclSpecs(3, container.name);
+        nodeRepository.addContainerAclSpecs(NODE_ADMIN_HOSTNAME, aclSpecs);
+        mockExistingAcls(container.name, aclSpecs);
+        aclMaintainer.run();
+        verify(dockerOperations).executeCommandInNetworkNamespace(eq(container.name),
+                aryEq(new String[]{"ip6tables", "-S"}));
+        verify(dockerOperations, never()).executeCommandInNetworkNamespace(eq(container.name),
+                aryEq(new String[]{"ip6tables", "-F", "INPUT"}));
+    }
+
+    @Test
     public void does_not_configure_acl_for_stopped_container() {
         Container stoppedContainer = makeContainer("container-1", false);
         nodeRepository.addContainerAclSpecs(NODE_ADMIN_HOSTNAME, makeAclSpecs(1, stoppedContainer.name));
@@ -60,11 +74,11 @@ public class AclMaintainerTest {
     public void rollback_is_attempted_when_applying_acl_fail() {
         Container container = makeContainer("container-1");
         nodeRepository.addContainerAclSpecs(NODE_ADMIN_HOSTNAME, makeAclSpecs(1, container.name));
+        mockExistingAcls(container.name);
 
-        doThrow(new RuntimeException("iptables command failed"))
-                .doNothing()
-                .when(dockerOperations)
-                .executeCommandInNetworkNamespace(any(), any());
+        when(dockerOperations.executeCommandInNetworkNamespace(any(), any()))
+                .thenReturn("success")
+                .thenThrow(new RuntimeException("iptables command failed"));
 
         aclMaintainer.run();
 
@@ -86,11 +100,11 @@ public class AclMaintainerTest {
         );
         verify(dockerOperations).executeCommandInNetworkNamespace(
                 eq(containerName),
-                aryEq(new String[]{"ip6tables", "-A", "INPUT", "-p", "icmpv6", "-j", "ACCEPT"})
+                aryEq(new String[]{"ip6tables", "-A", "INPUT", "-p", "ipv6-icmp", "-j", "ACCEPT"})
         );
         containerAclSpecs.forEach(aclSpec -> verify(dockerOperations).executeCommandInNetworkNamespace(
                 eq(containerName),
-                aryEq(new String[]{"ip6tables", "-A", "INPUT", "-s", aclSpec.ipAddress(), "-j", "ACCEPT"})
+                aryEq(new String[]{"ip6tables", "-A", "INPUT", "-s", aclSpec.ipAddress() + "/128", "-j", "ACCEPT"})
         ));
         verify(dockerOperations).executeCommandInNetworkNamespace(
                 eq(containerName),
@@ -98,6 +112,30 @@ public class AclMaintainerTest {
         );
     }
 
+    private void mockExistingAcls(ContainerName containerName) {
+        mockExistingAcls(containerName, Collections.emptyList());
+    }
+
+    private void mockExistingAcls(ContainerName containerName, List<ContainerAclSpec> containerAclSpecs) {
+        final String result;
+        if (containerAclSpecs.isEmpty()) {
+            result = "";
+        } else {
+            result = "-P INPUT DROP\n" +
+                    "-P FORWARD ACCEPT\n" +
+                    "-P OUTPUT ACCEPT\n" +
+                    "-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT\n" +
+                    "-A INPUT -p ipv6-icmp -j ACCEPT\n" +
+                    containerAclSpecs.stream().map(aclSpec -> String.format("-A INPUT -s %s/128 -j ACCEPT",
+                            aclSpec.ipAddress())).collect(Collectors.joining("\n")) +
+                    "\n";
+        }
+        when(dockerOperations.executeCommandInNetworkNamespace(
+                eq(containerName),
+                aryEq(new String[]{"ip6tables", "-S"})
+        )).thenReturn(result);
+    }
+
     private Container makeContainer(String hostname) {
         return makeContainer(hostname, true);
     }