Merge pull request #27471 from vespa-engine/mortent/dataplane-reconfig-thread

Handle proxy start/reload issues

3 files changed, 375 insertions, 77 deletions

diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/DataplaneProxyCredentials.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/DataplaneProxyCredentials.java
index a30252b1626..05c6f5be467 100644
--- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/DataplaneProxyCredentials.java
+++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/DataplaneProxyCredentials.java
@@ -37,8 +37,16 @@ public class DataplaneProxyCredentials extends AbstractComponent {
 
     @Inject
     public DataplaneProxyCredentials() {
-        certificateFile = Paths.get(Defaults.getDefaults().underVespaHome("tmp/proxy_cert.pem"));
-        keyFile = Paths.get(Defaults.getDefaults().underVespaHome("tmp/proxy_key.pem"));
+        this(
+                Paths.get(Defaults.getDefaults().underVespaHome("tmp/proxy_cert.pem")),
+                Paths.get(Defaults.getDefaults().underVespaHome("tmp/proxy_key.pem"))
+        );
+    }
+
+    public DataplaneProxyCredentials(Path certificateFile, Path keyFile){
+        this.certificateFile = certificateFile;
+        this.keyFile = keyFile;
+
         var existing = regenerateCredentials(certificateFile, keyFile).orElse(null);
         if (existing == null) {
             X509CertificateWithKey selfSigned = X509CertificateUtils.createSelfSigned("cn=vespa dataplane proxy", Duration.ofDays(30));
@@ -48,6 +56,7 @@ public class DataplaneProxyCredentials extends AbstractComponent {
         } else {
             this.certificate = existing;
         }
+
     }
 
     /**
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..e6af65c0bc8 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,14 @@ 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.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Configures a data plane proxy. Currently using Nginx.
@@ -20,100 +23,137 @@ 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, RUNNING, RELOAD_REQUIRED, STOPPED};
+    private NginxState state;
+    private NginxState wantedState;
+
+    private DataplaneProxyConfig cfg;
+    private Path proxyCredentialsCert;
+    private Path proxyCredentialsKey;
 
-    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);
+        wantedState = NginxState.RUNNING;
+        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::converge, reloadPeriodMinutes, reloadPeriodMinutes, TimeUnit.MINUTES);
     }
 
     public void reconfigure(DataplaneProxyConfig config, DataplaneProxyCredentials credentialsProvider) {
-        try {
-            String serverCert = config.serverCertificate();
-            String serverKey = config.serverKey();
-
-            boolean configChanged = false;
-            configChanged |= writeFile(serverCertificateFile, serverCert);
-            configChanged |= writeFile(serverKeyFile, serverKey);
-            configChanged |= writeFile(nginxConf,
-                      nginxConfig(
-                              credentialsProvider.certificateFile(),
-                              credentialsProvider.keyFile(),
-                              serverCertificateFile,
-                              serverKeyFile,
-                              config.port(),
-                              PREFIX
-                      ));
-            if (!started) {
-                startNginx();
-                started = true;
-            } else if (configChanged){
-                reloadNginx();
-            }
-        } catch (IOException e) {
-            throw new RuntimeException("Error reconfiguring data plane proxy", e);
+        synchronized (this) {
+            this.cfg = config;
+            this.proxyCredentialsCert = credentialsProvider.certificateFile();
+            this.proxyCredentialsKey = credentialsProvider.keyFile();
         }
     }
 
-    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 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));
-            }
-        } catch (IOException | InterruptedException e) {
-            throw new RuntimeException("Could not start nginx", e);
+    void converge() {
+        DataplaneProxyConfig config;
+        Path proxyCredentialsCert;
+        Path proxyCredentialsKey;
+        synchronized (this) {
+            config = cfg;
+            proxyCredentialsCert = this.proxyCredentialsCert;
+            proxyCredentialsKey = this.proxyCredentialsKey;
+            this.cfg = null;
+            this.proxyCredentialsCert = null;
+            this.proxyCredentialsKey = null;
         }
-    }
+        if (config != null) {
+            try {
 
-    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));
+                String serverCert = config.serverCertificate();
+                String serverKey = config.serverKey();
+
+                boolean configChanged = false;
+                configChanged |= writeFile(serverCertificateFile, serverCert);
+                configChanged |= writeFile(serverKeyFile, serverKey);
+                configChanged |= writeFile(nginxConf,
+                                           nginxConfig(
+                                                   configTemplate,
+                                                   proxyCredentialsCert,
+                                                   proxyCredentialsKey,
+                                                   serverCertificateFile,
+                                                   serverKeyFile,
+                                                   config.port(),
+                                                   root
+                                           ));
+                if (configChanged && state == NginxState.RUNNING) {
+                    changeState(NginxState.RELOAD_REQUIRED);
+                }
+            } catch (IOException e) {
+                throw new RuntimeException("Error reconfiguring data plane proxy", e);
+            }
+        }
+        if (wantedState == NginxState.RUNNING) {
+            boolean nginxRunning = proxyCommands.isRunning();
+            if (!nginxRunning) {
+                try {
+                    proxyCommands.start(nginxConf);
+                    changeState(wantedState);
+                } catch (Exception e) {
+                    logger.log(Level.INFO, "Failed to start nginx, will retry");
+                }
+            } else if (nginxRunning && state == NginxState.RELOAD_REQUIRED) {
+                try {
+                    proxyCommands.reload();
+                    changeState(wantedState);
+                } catch (Exception e) {
+                    logger.log(Level.INFO, "Failed to reconfigure nginx, will retry.");
+                }
+            }
+        } else if (wantedState == NginxState.STOPPED) {
+            if (proxyCommands.isRunning()) {
+                try {
+                    proxyCommands.stop();
+                    changeState(wantedState);
+                    executorService.shutdownNow();
+                } catch (Exception e) {
+                    logger.log(Level.INFO, "Failed to stop nginx, will retry");
+                }
             }
-        } catch (IOException | InterruptedException e) {
-            throw new RuntimeException("Could not start nginx", e);
+        } else {
+            logger.warning("Unknown state " + wantedState);
         }
     }
 
     @Override
     public void deconstruct() {
         super.deconstruct();
-        stopNginx();
+        wantedState = NginxState.STOPPED;
+        try {
+            executorService.awaitTermination(5, TimeUnit.MINUTES);
+        } catch (InterruptedException e) {
+            logger.log(Level.WARNING, "Error shutting down proxy reload thread");
+        }
     }
 
     /*
@@ -121,7 +161,7 @@ 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");
         Files.createDirectories(tempPath.getParent());
         Files.writeString(tempPath, contents);
 
@@ -135,21 +175,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 +202,78 @@ 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;
+    }
+
+    NginxState wantedState() {
+        return wantedState;
+    }
+
+    public interface ProxyCommands {
+        void start(Path configFile);
+        void stop();
+        void reload();
+        boolean isRunning();
+    }
+
+    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);
+            }
+        }
+
+        @Override
+        public boolean isRunning() {
+            return ProcessHandle.allProcesses()
+                    .map(ProcessHandle::info)
+                    .anyMatch(info -> info.command().orElse("").endsWith("nginx"));
+        }
+    }
 }
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..947c99adf51
--- /dev/null
+++ b/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java
@@ -0,0 +1,174 @@
+// 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.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class DataplaneProxyServiceTest {
+    private FileSystem fileSystem = Jimfs.newFileSystem();
+    DataplaneProxyService.ProxyCommands proxyCommandsMock = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+
+    @Test
+    public void starts_and_reloads_if_no_errors() throws IOException {
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
+
+        assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+
+        // Simulate executor next tick
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+        // Trigger reload by recreating the proxy config (generates new server cert)
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+    }
+
+    @Test
+    public void retries_startup_errors() throws IOException {
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommandsMock).start(any());
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
+
+        assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+
+        // Start nginx, starting will fail, so the service should be in INITIALIZING state
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+    }
+
+    @Test
+    public void retries_reload_errors() throws IOException {
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommandsMock).reload();
+        when(proxyCommandsMock.isRunning()).thenReturn(false);
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
+
+        // Make sure service in running state
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        when(proxyCommandsMock.isRunning()).thenReturn(true);
+
+        // Trigger reload, verifies 2nd attempt succeeds
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RELOAD_REQUIRED, service.state());
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        verify(proxyCommandsMock, times(2)).reload();
+    }
+
+    @Test
+    public void converges_to_wanted_state_when_nginx_not_running() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = new TestProxyCommands();
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+
+        assertFalse(proxyCommands.isRunning());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        assertTrue(proxyCommands.isRunning());
+
+        // Simulate nginx process dying
+        proxyCommands.stop();
+        assertFalse(proxyCommands.isRunning());
+        service.converge();
+        assertTrue(proxyCommands.isRunning());
+    }
+
+    @Test
+    public void shuts_down() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = new TestProxyCommands();
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+        service.converge();
+        assertTrue(proxyCommands.isRunning());
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+        new Thread(service::deconstruct).start(); // deconstruct will block until nginx is stopped
+        // Wait for above thread to set the wanted state to STOPPED
+        while (service.wantedState() != DataplaneProxyService.NginxState.STOPPED) {
+            try {
+                Thread.sleep(10);
+            } catch (InterruptedException e) {
+            }
+        }
+        service.converge();
+        assertEquals(service.state(), DataplaneProxyService.NginxState.STOPPED);
+        assertFalse(proxyCommands.isRunning());
+    }
+
+    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"));
+    }
+
+    private static class TestProxyCommands implements DataplaneProxyService.ProxyCommands {
+        private boolean running = false;
+
+        @Override
+        public void start(Path configFile) {
+            running = true;
+        }
+
+        @Override
+        public void stop() {
+            running = false;
+        }
+
+        @Override
+        public void reload() {
+
+        }
+
+        @Override
+        public boolean isRunning() {
+            return running;
+        }
+    }
+}