Only apply ACLs if they change

5 files changed, 108 insertions, 35 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 52b5bede912..336e20253b6 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
@@ -27,7 +27,7 @@ public interface DockerOperations {
 
     void executeCommandInContainer(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 45c9f97d0d8..f405ade4081 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);
@@ -299,7 +299,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"));
@@ -315,7 +315,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));
@@ -364,16 +364,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/acl/AclMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
index 0359afe5883..73e50705fc4 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
@@ -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/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 2b68c869008..ed6808e1eac 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
@@ -135,7 +135,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/acl/AclMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
index 09372716349..450cf7036b0 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
@@ -7,10 +7,10 @@ import com.yahoo.vespa.hosted.node.admin.ContainerAclSpec;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.integrationTests.CallOrderVerifier;
 import com.yahoo.vespa.hosted.node.admin.integrationTests.NodeRepoMock;
-import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer;
 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;
@@ -19,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;
@@ -45,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));
@@ -61,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();
 
@@ -87,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),
@@ -99,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);
     }