Handle proxy start/reload issues

2 files changed, 255 insertions, 58 deletions

diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java
index 6d871b7283f..a9cd7466b73 100644
--- a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java
+++ b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java
@@ -7,11 +7,18 @@ import com.yahoo.jdisc.http.server.jetty.DataplaneProxyCredentials;
 
 import javax.inject.Inject;
 import java.io.IOException;
-import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
+import java.time.Duration;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Configures a data plane proxy. Currently using Nginx.
@@ -20,25 +27,46 @@ import java.nio.file.StandardCopyOption;
  */
 public class DataplaneProxyService extends AbstractComponent {
 
+    private static Logger logger = Logger.getLogger(DataplaneProxyService.class.getName());
     private static final String PREFIX = "/opt/vespa";
-    private static final Path CONFIG_TEMPLATE = Paths.get(PREFIX, "conf/nginx/nginx.conf.template");
 
-    private static final Path clientCertificateFile = Paths.get(PREFIX, "conf/nginx/client_cert.pem");
-    private static final Path clientKeyFile = Paths.get(PREFIX, "conf/nginx/client_key.pem");
-    private static final Path serverCertificateFile = Paths.get(PREFIX, "conf/nginx/server_cert.pem");
-    private static final Path serverKeyFile = Paths.get(PREFIX, "conf/nginx/server_key.pem");
+    private final Path configTemplate;
+    private final Path serverCertificateFile;
+    private final Path serverKeyFile;
+    private final Path nginxConf;
 
-    private static final Path nginxConf = Paths.get(PREFIX, "conf/nginx/nginx.conf");
+    private final ProxyCommands proxyCommands;
+    private final ScheduledThreadPoolExecutor executorService;
+    private final Path root;
+
+    enum NginxState {INITIALIZING, STARTING, RUNNING, RELOAD_REQUIRED, CONFIG_CHANGE_IN_PROGRESS, STOPPED};
+    private NginxState state;
 
-    private boolean started;
 
     @Inject
     public DataplaneProxyService() {
-        this.started = false;
+        this(Paths.get(PREFIX), new NginxProxyCommands(), 1);
+    }
+    
+    DataplaneProxyService(Path root, ProxyCommands proxyCommands, int reloadPeriodMinutes) {
+        this.root = root;
+        this.proxyCommands = proxyCommands;
+        changeState(NginxState.INITIALIZING);
+        configTemplate = root.resolve("conf/nginx/nginx.conf.template");
+        serverCertificateFile = root.resolve("conf/nginx/server_cert.pem");
+        serverKeyFile = root.resolve("conf/nginx/server_key.pem");
+        nginxConf = root.resolve("conf/nginx/nginx.conf");
+
+        executorService = new ScheduledThreadPoolExecutor(1);
+        executorService.scheduleAtFixedRate(this::startOrReloadNginx, reloadPeriodMinutes, reloadPeriodMinutes, TimeUnit.MINUTES);
+
     }
 
     public void reconfigure(DataplaneProxyConfig config, DataplaneProxyCredentials credentialsProvider) {
+        NginxState prevState = state;
+        changeState(NginxState.CONFIG_CHANGE_IN_PROGRESS);
         try {
+
             String serverCert = config.serverCertificate();
             String serverKey = config.serverKey();
 
@@ -47,73 +75,66 @@ public class DataplaneProxyService extends AbstractComponent {
             configChanged |= writeFile(serverKeyFile, serverKey);
             configChanged |= writeFile(nginxConf,
                       nginxConfig(
+                              configTemplate,
                               credentialsProvider.certificateFile(),
                               credentialsProvider.keyFile(),
                               serverCertificateFile,
                               serverKeyFile,
                               config.port(),
-                              PREFIX
+                              root
                       ));
-            if (!started) {
-                startNginx();
-                started = true;
-            } else if (configChanged){
-                reloadNginx();
+            if (prevState == NginxState.INITIALIZING) {
+                changeState(NginxState.STARTING);
+            } else if (configChanged && prevState == NginxState.RUNNING) {
+                changeState(NginxState.RELOAD_REQUIRED);
+            } else {
+                changeState(prevState);
             }
         } catch (IOException e) {
+            changeState(prevState);
             throw new RuntimeException("Error reconfiguring data plane proxy", e);
         }
     }
 
-    private void startNginx() {
-        try {
-            Process startCommand = new ProcessBuilder().command(
-                    "nginx",
-                    "-c", nginxConf.toString()
-            ).start();
-            int exitCode = startCommand.waitFor();
-            if (exitCode != 0) {
-                throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
-            }
-        } catch (IOException | InterruptedException e) {
-            throw new RuntimeException("Could not start nginx", e);
-        }
+    private synchronized void changeState(NginxState newState) {
+        state = newState;
     }
 
-    private void reloadNginx() {
-        try {
-            Process reloadCommand = new ProcessBuilder().command(
-                    "nginx",
-                    "-s", "reload"
-            ).start();
-            int exitCode = reloadCommand.waitFor();
-            if (exitCode != 0) {
-                throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
+    void startOrReloadNginx() {
+        if (state == NginxState.CONFIG_CHANGE_IN_PROGRESS) {
+            return;
+        } else if (state == NginxState.STARTING) {
+            try {
+                proxyCommands.start(nginxConf);
+                changeState(NginxState.RUNNING);
+            } catch (Exception e) {
+                logger.log(Level.INFO, "Failed to start nginx, will retry");
             }
-        } catch (IOException | InterruptedException e) {
-            throw new RuntimeException("Could not start nginx", e);
-        }
-    }
-
-    private void stopNginx() {
-        try {
-            Process stopCommand = new ProcessBuilder().command(
-                    "nginx",
-                    "-s", "stop"
-            ).start();
-            int exitCode = stopCommand.waitFor();
-            if (exitCode != 0) {
-                throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
+        } else if (state == NginxState.RELOAD_REQUIRED){
+            try {
+                proxyCommands.reload();
+                changeState(NginxState.RUNNING);
+            } catch (Exception e) {
+                logger.log(Level.INFO, "Failed to reconfigure nginx, will retry.");
             }
-        } catch (IOException | InterruptedException e) {
-            throw new RuntimeException("Could not start nginx", e);
         }
     }
 
     @Override
     public void deconstruct() {
         super.deconstruct();
-        stopNginx();
+        try {
+            executorService.shutdownNow();
+            executorService.awaitTermination(1, TimeUnit.MINUTES);
+        } catch (InterruptedException e) {
+            logger.log(Level.WARNING, "Error shutting down proxy reload thread");
+        }
+        try {
+            proxyCommands.stop();
+            changeState(NginxState.STOPPED);
+        } catch (Exception e) {
+            logger.log(Level.WARNING, "Failed shutting down nginx");
+        }
     }
 
     /*
@@ -121,7 +142,8 @@ public class DataplaneProxyService extends AbstractComponent {
      * return true if file was changed, false if no changes
      */
     private boolean writeFile(Path file, String contents) throws IOException {
-        Path tempPath = Paths.get(file.toFile().getAbsolutePath() + ".new");
+        Path tempPath = file.getParent().resolve(file.getFileName().toString() + ".new");
+//        Path tempPath = Paths.get(file.toFile().getAbsolutePath() + ".new");
         Files.createDirectories(tempPath.getParent());
         Files.writeString(tempPath, contents);
 
@@ -135,21 +157,22 @@ public class DataplaneProxyService extends AbstractComponent {
     }
 
     static String nginxConfig(
+            Path configTemplate,
             Path clientCert,
             Path clientKey,
             Path serverCert,
             Path serverKey,
             int vespaPort,
-            String prefix) {
+            Path root) {
 
         try {
-            String nginxTemplate = Files.readString(CONFIG_TEMPLATE);
+            String nginxTemplate = Files.readString(configTemplate);
             nginxTemplate = replace(nginxTemplate, "client_cert", clientCert.toString());
             nginxTemplate = replace(nginxTemplate, "client_key", clientKey.toString());
             nginxTemplate = replace(nginxTemplate, "server_cert", serverCert.toString());
             nginxTemplate = replace(nginxTemplate, "server_key", serverKey.toString());
             nginxTemplate = replace(nginxTemplate, "vespa_port", Integer.toString(vespaPort));
-            nginxTemplate = replace(nginxTemplate, "prefix", prefix);
+            nginxTemplate = replace(nginxTemplate, "prefix", root.toString());
 
             // TODO: verify that all template vars have been expanded
             return nginxTemplate;
@@ -161,4 +184,66 @@ public class DataplaneProxyService extends AbstractComponent {
     private static String replace(String template, String key, String value) {
         return template.replaceAll("\\$\\{%s\\}".formatted(key), value);
     }
+
+    NginxState state() {
+        return state;
+    }
+
+    public interface ProxyCommands {
+        void start(Path configFile);
+        void stop();
+        void reload();
+    }
+
+    public static class NginxProxyCommands implements ProxyCommands {
+
+        @Override
+        public void start(Path configFile) {
+            try {
+                Process startCommand = new ProcessBuilder().command(
+                        "nginx",
+                        "-c", configFile.toString()
+                ).start();
+                int exitCode = startCommand.waitFor();
+                if (exitCode != 0) {
+                    throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
+                }
+            } catch (IOException | InterruptedException e) {
+                throw new RuntimeException("Could not start nginx", e);
+            }
+        }
+
+        @Override
+        public void stop() {
+            try {
+                Process stopCommand = new ProcessBuilder().command(
+                        "nginx",
+                        "-s", "stop"
+                ).start();
+                int exitCode = stopCommand.waitFor();
+                if (exitCode != 0) {
+                    throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
+                }
+            } catch (IOException | InterruptedException e) {
+                throw new RuntimeException("Could not start nginx", e);
+            }
+
+        }
+
+        @Override
+        public void reload() {
+            try {
+                Process reloadCommand = new ProcessBuilder().command(
+                        "nginx",
+                        "-s", "reload"
+                ).start();
+                int exitCode = reloadCommand.waitFor();
+                if (exitCode != 0) {
+                    throw new RuntimeException("Non-zero exitcode from nginx: %d".formatted(exitCode));
+                }
+            } catch (IOException | InterruptedException e) {
+                throw new RuntimeException("Could not start nginx", e);
+            }
+        }
+    }
 }
diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java
new file mode 100644
index 00000000000..95c05e9a1c4
--- /dev/null
+++ b/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java
@@ -0,0 +1,112 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.container.jdisc;
+
+import com.google.common.jimfs.Jimfs;
+import com.yahoo.cloud.config.DataplaneProxyConfig;
+import com.yahoo.jdisc.http.server.jetty.DataplaneProxyCredentials;
+import com.yahoo.security.KeyUtils;
+import com.yahoo.security.X509CertificateUtils;
+import com.yahoo.security.X509CertificateWithKey;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.FileSystem;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Duration;
+
+import static com.yahoo.yolean.Exceptions.uncheck;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+
+public class DataplaneProxyServiceTest {
+    private FileSystem fileSystem = Jimfs.newFileSystem();
+    DataplaneProxyService.ProxyCommands proxyCommands = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+
+    @Test
+    public void starts_and_reloads_if_no_errors() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+
+        assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        assertEquals(DataplaneProxyService.NginxState.STARTING, service.state());
+
+        // Simulate executor next tick
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+        // Trigger reload by recreating the proxy config (generates new server cert)
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        assertEquals(DataplaneProxyService.NginxState.RELOAD_REQUIRED, service.state());
+
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+    }
+
+    @Test
+    public void retries_startup_errors() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommands).start(any());
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+
+        assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        assertEquals(DataplaneProxyService.NginxState.STARTING, service.state());
+
+        // Start nginx,
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.STARTING, service.state());
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+    }
+
+    @Test
+    public void retries_reload_errors() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommands).reload();
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+
+        // Make sure service in running state
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+        // Trigger reload, verifies 2nd attempt succeeds
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        assertEquals(DataplaneProxyService.NginxState.RELOAD_REQUIRED, service.state());
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RELOAD_REQUIRED, service.state());
+        service.startOrReloadNginx();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+    }
+
+    private DataplaneProxyService dataplaneProxyService(DataplaneProxyService.ProxyCommands proxyCommands) throws IOException {
+        Path root = fileSystem.getPath("/opt/vespa");
+
+        Path nginxConf = root.resolve("conf/nginx/nginx.conf.template");
+        Files.createDirectories(nginxConf.getParent());
+        Files.write(nginxConf, "".getBytes(StandardCharsets.UTF_8));
+
+        DataplaneProxyService service = new DataplaneProxyService(root, proxyCommands, 100);
+        return service;
+    }
+
+    private DataplaneProxyConfig proxyConfig() {
+        X509CertificateWithKey selfSigned = X509CertificateUtils.createSelfSigned("cn=test", Duration.ofMinutes(10));
+        return new DataplaneProxyConfig.Builder()
+                .port(1234)
+                .serverCertificate(X509CertificateUtils.toPem(selfSigned.certificate()))
+                .serverKey(KeyUtils.toPem(selfSigned.privateKey()))
+                .build();
+    }
+
+    private DataplaneProxyCredentials credentials(FileSystem fileSystem) {
+        Path path = fileSystem.getPath("/tmp");
+        uncheck(() -> Files.createDirectories(path));
+        return new DataplaneProxyCredentials(path.resolve("cert.pem"), path.resolve("key.pem"));
+    }
+}