Fixes after review round

Also:
 - Handle ConnectException (connection refused) as a synonym for DOWN
 - Add test of StateImpl

10 files changed, 133 insertions, 30 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
index 1cbb93c8482..fd34fede291 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
@@ -81,23 +81,17 @@ public class ConfigServerApiImpl implements ConfigServerApi {
     /**
      * Creates an api for talking to the config servers with a fixed socket factory.
      *
-     * <p>This should only be necessary for certificate signing requests (CSR) against
-     * the config server when client validation is enabled in the config server.
+     * <p>This may be used to avoid requiring background certificate signing requests (CSR)
+     * against the config server when client validation is enabled in the config server.
      */
     public static ConfigServerApiImpl createWithSocketFactory(
-            ConfigServerInfo configServerInfo,
+            List<URI> configServerHosts,
             SSLConnectionSocketFactory socketFactory) {
-        return new ConfigServerApiImpl(configServerInfo.getConfigServerUris(), socketFactory);
+        return new ConfigServerApiImpl(configServerHosts, socketFactory);
     }
 
-    public static ConfigServerApiImpl createForTestingRemote(URI configServerUri) {
-        return new ConfigServerApiImpl(
-                Collections.singletonList(configServerUri),
-                SSLConnectionSocketFactory.getSocketFactory());
-    }
-
-    public static ConfigServerApiImpl createForTestingWithClient(List<URI> configServerHosts,
-                                                                 SelfCloseableHttpClient client) {
+    static ConfigServerApiImpl createForTestingWithClient(List<URI> configServerHosts,
+                                                          SelfCloseableHttpClient client) {
         return new ConfigServerApiImpl(configServerHosts, client);
     }
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdater.java
index 06659bdf058..57fa5526d73 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdater.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdater.java
@@ -52,12 +52,12 @@ public class SslConnectionSocketFactoryUpdater implements AutoCloseable {
         this.configServerInfo = configServerInfo;
         this.socketFactoryCreator = socketFactoryCreator;
 
-        // The default socket factory is one without a keystore
-        socketFactory = socketFactoryCreator.createSocketFactory(configServerInfo, Optional.empty());
-
         // ConfigServerApi used to refresh the key store. Does not itself rely on a socket
         // factory with key store, of course.
-        configServerApi = ConfigServerApiImpl.createWithSocketFactory(configServerInfo, socketFactory);
+        SSLConnectionSocketFactory socketFactoryWithoutKeyStore =
+                socketFactoryCreator.createSocketFactory(configServerInfo, Optional.empty());
+        configServerApi = ConfigServerApiImpl.createWithSocketFactory(
+                configServerInfo.getConfigServerUris(), socketFactoryWithoutKeyStore);
 
         // If we have keystore options, we should make sure we use the keystore with the latest certificate,
         // start the keystore refresher.
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthCode.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthCode.java
new file mode 100644
index 00000000000..7ca7a1b30dd
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthCode.java
@@ -0,0 +1,32 @@
+// 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.configserver.state;
+
+/**
+ * The healthiness of a remote Vespa server based on REST API
+ *
+ * @author hakon
+ */
+public enum HealthCode {
+    DOWN("down"),
+    INITIALIZING("initializing"),
+    UP("up");
+
+    private final String code;
+
+    HealthCode(String code) {
+        this.code = code;
+    }
+
+    public static HealthCode fromString(String code) {
+        return HealthCode.valueOf(code.toUpperCase());
+    }
+
+    public String asString() {
+        return code;
+    }
+
+    @Override
+    public String toString() {
+        return asString();
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/State.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/State.java
index 5befe703fa4..ab9d0786f5a 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/State.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/State.java
@@ -8,5 +8,5 @@ package com.yahoo.vespa.hosted.node.admin.configserver.state;
  */
 public interface State {
     /** Issue GET on /state/v1/health */
-    HealthResponse getHealth();
+    HealthCode getHealth();
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
index 61ae33d723a..efeb3039379 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.configserver.state;
 
 import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApi;
+import com.yahoo.vespa.hosted.node.admin.configserver.state.bindings.HealthResponse;
 
 /**
  * @author hakon
@@ -14,7 +15,27 @@ public class StateImpl implements State {
     }
 
     @Override
-    public HealthResponse getHealth() {
-        return configServerApi.get("/state/v1/health", HealthResponse.class);
+    public HealthCode getHealth() {
+        HealthResponse response;
+        try {
+            response = configServerApi.get("/state/v1/health", HealthResponse.class);
+        } catch (RuntimeException e) {
+            if (causedByConnectionRefused(e)) {
+                return HealthCode.DOWN;
+            }
+
+            throw e;
+        }
+        return HealthCode.fromString(response.status.code);
+    }
+
+    private static boolean causedByConnectionRefused(Throwable throwable) {
+        for (Throwable cause = throwable; cause != null; cause = cause.getCause()) {
+            if (cause instanceof java.net.ConnectException) {
+                return true;
+            }
+        }
+
+        return false;
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponse.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/bindings/HealthResponse.java
index 73caef8567c..26ad5413fb8 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponse.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/bindings/HealthResponse.java
@@ -1,5 +1,5 @@
 // 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.configserver.state;
+package com.yahoo.vespa.hosted.node.admin.configserver.state.bindings;
 
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -17,7 +17,7 @@ public class HealthResponse {
     @JsonIgnoreProperties(ignoreUnknown = true)
     public static class Status {
         @JsonProperty("code")
-        public String code;
+        public String code = "down";
 
         @Override
         public String toString() {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdaterTest.java
index 8be6ea8c85a..490f45b094c 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdaterTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/SslConnectionSocketFactoryUpdaterTest.java
@@ -2,15 +2,20 @@
 package com.yahoo.vespa.hosted.node.admin.configserver;
 
 import com.yahoo.vespa.hosted.node.admin.component.ConfigServerInfo;
+import com.yahoo.vespa.hosted.node.admin.configserver.certificate.ConfigServerKeyStoreRefresher;
 import com.yahoo.vespa.hosted.node.admin.configserver.certificate.ConfigServerKeyStoreRefresherFactory;
 import com.yahoo.vespa.hosted.node.admin.util.KeyStoreOptions;
 import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.util.Optional;
 
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -21,23 +26,33 @@ public class SslConnectionSocketFactoryUpdaterTest {
     private final String hostname = "host.oath.com";
     private final ConfigServerKeyStoreRefresherFactory refresherFactory =
             mock(ConfigServerKeyStoreRefresherFactory.class);
+    private final ConfigServerKeyStoreRefresher refresher =
+            mock(ConfigServerKeyStoreRefresher.class);
     private final SslConnectionSocketFactoryCreator socketFactoryCreator =
             mock(SslConnectionSocketFactoryCreator.class);
+    private final SSLConnectionSocketFactory socketFactory = mock(SSLConnectionSocketFactory.class);
 
-    @Test
-    public void testBasics() {
-        SSLConnectionSocketFactory socketFactory = mock(SSLConnectionSocketFactory.class);
+    @Before
+    public void setUp() {
+        KeyStoreOptions keyStoreOptions = mock(KeyStoreOptions.class);
+        when(configServerInfo.getKeyStoreOptions()).thenReturn(Optional.of(keyStoreOptions));
+        when(refresherFactory.create(any(), any(), any(), any())).thenReturn(refresher);
         when(socketFactoryCreator.createSocketFactory(any(), any()))
                 .thenReturn(socketFactory);
+    }
 
-        Optional<KeyStoreOptions> keyStoreOptions = Optional.empty();
-        when(configServerInfo.getKeyStoreOptions()).thenReturn(keyStoreOptions);
-
+    @Test
+    public void testSettingOfSocketFactory() {
         SslConnectionSocketFactoryUpdater updater = new SslConnectionSocketFactoryUpdater(
                 configServerInfo,
                 hostname,
                 refresherFactory,
                 socketFactoryCreator);
-    }
 
+        assertTrue(socketFactory == updater.getCurrentSocketFactory());
+
+        ConfigServerApi api = mock(ConfigServerApi.class);
+        updater.registerConfigServerApi(api);
+        verify(api, times(1)).setSSLConnectionSocketFactory(socketFactory);
+    }
 }
\ No newline at end of file
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepositoryTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepositoryTest.java
index 6e113f06a09..269c79b125c 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepositoryTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepositoryTest.java
@@ -10,6 +10,7 @@ import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApiImpl;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAttributes;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.testutils.ContainerConfig;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -18,6 +19,7 @@ import java.io.IOException;
 import java.net.ServerSocket;
 import java.net.URI;
 import java.time.Instant;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 
@@ -61,8 +63,9 @@ public class RealNodeRepositoryTest {
             try {
                 final int port = findRandomOpenPort();
                 container = JDisc.fromServicesXml(ContainerConfig.servicesXmlV2(port), Networking.enable);
-                configServerApi = ConfigServerApiImpl.createForTestingRemote(
-                        URI.create("http://127.0.0.1:" + port));
+                configServerApi = ConfigServerApiImpl.createWithSocketFactory(
+                        Collections.singletonList(URI.create("http://127.0.0.1:" + port)),
+                        SSLConnectionSocketFactory.getSocketFactory());
                 return;
             } catch (RuntimeException e) {
                 lastException = e;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponseTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponseTest.java
index fc331fc7422..fcb6f6786a8 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponseTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/HealthResponseTest.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.configserver.state;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.yahoo.vespa.hosted.node.admin.configserver.state.bindings.HealthResponse;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java
new file mode 100644
index 00000000000..01aaa385d85
--- /dev/null
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java
@@ -0,0 +1,37 @@
+// 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.configserver.state;
+
+import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApi;
+import com.yahoo.vespa.hosted.node.admin.configserver.state.bindings.HealthResponse;
+import org.junit.Test;
+
+import java.net.ConnectException;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class StateImplTest {
+    private final ConfigServerApi api = mock(ConfigServerApi.class);
+    private final StateImpl state = new StateImpl(api);
+
+    @Test
+    public void testWhenUp() {
+        HealthResponse response = new HealthResponse();
+        response.status.code = "up";
+        when(api.get(any(), any())).thenReturn(response);
+
+        HealthCode code = state.getHealth();
+        assertEquals(HealthCode.UP, code);
+    }
+
+    @Test
+    public void connectException() {
+        RuntimeException exception = new RuntimeException(new ConnectException("connection refused"));
+        when(api.get(any(), any())).thenThrow(exception);
+
+        HealthCode code = state.getHealth();
+        assertEquals(HealthCode.DOWN, code);
+    }
+}
\ No newline at end of file