Create client once in ConfigServerRestExecutorImpl

6 files changed, 66 insertions, 50 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
index f356979b191..5af3c765e22 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
@@ -2,9 +2,13 @@
 package com.yahoo.vespa.hosted.controller.proxy;
 
 import com.google.inject.Inject;
+import com.yahoo.component.AbstractComponent;
 import com.yahoo.jdisc.http.HttpRequest.Method;
 import com.yahoo.log.LogLevel;
+import com.yahoo.vespa.athenz.api.AthenzIdentity;
 import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider;
+import com.yahoo.vespa.athenz.tls.AthenzIdentityVerifier;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import org.apache.http.Header;
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.CloseableHttpResponse;
@@ -14,14 +18,17 @@ import org.apache.http.client.methods.HttpPatch;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.client.methods.HttpPut;
 import org.apache.http.client.methods.HttpRequestBase;
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
 import org.apache.http.entity.InputStreamEntity;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.util.EntityUtils;
 
 import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLSession;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.UncheckedIOException;
 import java.net.URI;
 import java.time.Duration;
 import java.util.ArrayList;
@@ -30,7 +37,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 
@@ -40,18 +49,24 @@ import static com.yahoo.yolean.Exceptions.uncheck;
  * @author bjorncs
  */
 @SuppressWarnings("unused") // Injected
-public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
+public class ConfigServerRestExecutorImpl extends AbstractComponent implements ConfigServerRestExecutor {
 
     private static final Logger log = Logger.getLogger(ConfigServerRestExecutorImpl.class.getName());
 
     private static final Duration PROXY_REQUEST_TIMEOUT = Duration.ofSeconds(10);
     private static final Set<String> HEADERS_TO_COPY = Set.of("X-HTTP-Method-Override", "Content-Type");
 
-    private final ServiceIdentityProvider sslContextProvider;
+    private final CloseableHttpClient client;
 
     @Inject
-    public ConfigServerRestExecutorImpl(ServiceIdentityProvider sslContextProvider) {
-        this.sslContextProvider = sslContextProvider;
+    public ConfigServerRestExecutorImpl(ZoneRegistry zoneRegistry, ServiceIdentityProvider sslContextProvider) {
+        RequestConfig config = RequestConfig.custom()
+                .setConnectTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis())
+                .setConnectionRequestTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis())
+                .setSocketTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis()).build();
+
+        this.client = createHttpClient(config, sslContextProvider,
+                new ControllerOrConfigserverHostnameVerifier(zoneRegistry));
     }
 
     @Override
@@ -60,7 +75,7 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
         List<URI> allServers = new ArrayList<>(proxyRequest.getTargets());
 
         StringBuilder errorBuilder = new StringBuilder();
-        if (queueFirstServerIfDown(allServers, proxyRequest.getTargetHostnameVerifier())) {
+        if (queueFirstServerIfDown(allServers)) {
             errorBuilder.append("Change ordering due to failed ping.");
         }
         for (URI uri : allServers) {
@@ -85,11 +100,7 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
                 .setConnectTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis())
                 .setConnectionRequestTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis())
                 .setSocketTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis()).build();
-        try (
-                CloseableHttpClient client = createHttpClient(
-                        config, sslContextProvider, proxyRequest.getTargetHostnameVerifier());
-                CloseableHttpResponse response = client.execute(requestBase)
-        ) {
+        try (CloseableHttpResponse response = client.execute(requestBase)) {
             String content = getContent(response);
             int status = response.getStatusLine().getStatusCode();
             if (status / 100 == 5) {
@@ -167,7 +178,7 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
      * if it is not responding, we try the other servers first. False positive/negatives are not critical,
      * but will increase latency to some extent.
      */
-    private boolean queueFirstServerIfDown(List<URI> allServers, HostnameVerifier hostnameVerifier) {
+    private boolean queueFirstServerIfDown(List<URI> allServers) {
         if (allServers.size() < 2) {
             return false;
         }
@@ -179,10 +190,8 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
                 .setConnectTimeout(timeout)
                 .setConnectionRequestTimeout(timeout)
                 .setSocketTimeout(timeout).build();
-        try (
-                CloseableHttpClient client = createHttpClient(config, sslContextProvider, hostnameVerifier);
-                CloseableHttpResponse response = client.execute(httpget)
-        ) {
+        httpget.setConfig(config);
+        try (CloseableHttpResponse response = client.execute(httpget)) {
             if (response.getStatusLine().getStatusCode() == 200) {
                 return false;
             }
@@ -195,6 +204,15 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
         return true;
     }
 
+    @Override
+    public void deconstruct() {
+        try {
+            client.close();
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+    }
+
     private static CloseableHttpClient createHttpClient(RequestConfig config,
                                                         ServiceIdentityProvider sslContextProvider,
                                                         HostnameVerifier hostnameVerifier) {
@@ -203,7 +221,31 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
                 .setSslcontext(sslContextProvider.getIdentitySslContext())
                 .setSSLHostnameVerifier(hostnameVerifier)
                 .setDefaultRequestConfig(config)
+                .setMaxConnPerRoute(10)
+                .setMaxConnTotal(500)
+                .setConnectionTimeToLive(1, TimeUnit.MINUTES)
                 .build();
     }
 
+    private static class ControllerOrConfigserverHostnameVerifier implements HostnameVerifier {
+
+        private final HostnameVerifier controllerVerifier = new DefaultHostnameVerifier();
+        private final HostnameVerifier configserverVerifier;
+
+        ControllerOrConfigserverHostnameVerifier(ZoneRegistry registry) {
+            this.configserverVerifier = createConfigserverVerifier(registry);
+        }
+
+        private static HostnameVerifier createConfigserverVerifier(ZoneRegistry registry) {
+            Set<AthenzIdentity> configserverIdentities = registry.zones().all().zones().stream()
+                    .map(zone -> registry.getConfigServerHttpsIdentity(zone.getId()))
+                    .collect(Collectors.toSet());
+            return new AthenzIdentityVerifier(configserverIdentities);
+        }
+
+        @Override
+        public boolean verify(String hostname, SSLSession session) {
+            return controllerVerifier.verify(hostname, session) || configserverVerifier.verify(hostname, session);
+        }
+    }
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
index 7f85eb212c4..f398683567b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.controller.proxy;
 
 import com.yahoo.container.jdisc.HttpRequest;
 
-import javax.net.ssl.HostnameVerifier;
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -27,7 +26,6 @@ public class ProxyRequest {
     private final InputStream requestData;
 
     private final List<URI> targets;
-    private final HostnameVerifier targetHostnameVerifier;
     private final String targetPath;
 
     /**
@@ -35,17 +33,16 @@ public class ProxyRequest {
      *
      * @param request the request from the jdisc framework.
      * @param targets list of targets this request should be proxied to (targets are tried once in order until a response is returned).
-     * @param targetHostnameVerifier hostname verifier to use when proxying the request.
      * @param targetPath the path to proxy to.
      * @throws ProxyException on errors
      */
-    public ProxyRequest(HttpRequest request, List<URI> targets, HostnameVerifier targetHostnameVerifier, String targetPath) throws ProxyException {
+    public ProxyRequest(HttpRequest request, List<URI> targets, String targetPath) throws ProxyException {
         this(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), request.getData(),
-             targets, targetHostnameVerifier, targetPath);
+             targets, targetPath);
     }
 
     ProxyRequest(Method method, URI requestUri, Map<String, List<String>> headers, InputStream body,
-                 List<URI> targets, HostnameVerifier targetHostnameVerifier, String targetPath) throws ProxyException {
+                 List<URI> targets, String targetPath) throws ProxyException {
         Objects.requireNonNull(requestUri, "Request must be non-null");
         if (!requestUri.getPath().endsWith(targetPath))
             throw new ProxyException(ErrorResponse.badRequest(String.format(
@@ -58,7 +55,6 @@ public class ProxyRequest {
 
         this.targets = List.copyOf(targets);
         this.targetPath = targetPath.startsWith("/") ? targetPath : "/" + targetPath;
-        this.targetHostnameVerifier = targetHostnameVerifier;
     }
 
 
@@ -78,10 +74,6 @@ public class ProxyRequest {
         return targets;
     }
 
-    public HostnameVerifier getTargetHostnameVerifier() {
-        return targetHostnameVerifier;
-    }
-
     public URI createConfigServerRequestUri(URI baseURI) {
         try {
             return new URI(baseURI.getScheme(), baseURI.getUserInfo(), baseURI.getHost(),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
index f23161664a2..99cc78a2614 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
@@ -10,7 +10,6 @@ import com.yahoo.restapi.Path;
 import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
-import com.yahoo.vespa.athenz.tls.AthenzIdentityVerifier;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLoggingRequestHandler;
@@ -18,12 +17,9 @@ import com.yahoo.vespa.hosted.controller.proxy.ConfigServerRestExecutor;
 import com.yahoo.vespa.hosted.controller.proxy.ProxyException;
 import com.yahoo.vespa.hosted.controller.proxy.ProxyRequest;
 import com.yahoo.yolean.Exceptions;
-import org.apache.http.conn.ssl.DefaultHostnameVerifier;
 
-import javax.net.ssl.HostnameVerifier;
 import java.net.URI;
 import java.util.List;
-import java.util.Set;
 import java.util.logging.Level;
 import java.util.stream.Stream;
 
@@ -98,8 +94,7 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler {
         }
 
         try {
-            return proxy.handle(new ProxyRequest(
-                    request, List.of(getEndpoint(zoneId)), createHostnameVerifier(zoneId), cfgPath));
+            return proxy.handle(new ProxyRequest(request, List.of(getEndpoint(zoneId)), cfgPath));
         } catch (ProxyException e) {
             throw new RuntimeException(e);
         }
@@ -129,9 +124,4 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler {
     private URI getEndpoint(ZoneId zoneId) {
         return CONTROLLER_ZONE.equals(zoneId) ? zoneRegistry.apiUrl() : zoneRegistry.getConfigServerVipUri(zoneId);
     }
-
-    private HostnameVerifier createHostnameVerifier(ZoneId zoneId) {
-        if (CONTROLLER_ZONE.equals(zoneId)) return new DefaultHostnameVerifier();
-        return new AthenzIdentityVerifier(Set.of(zoneRegistry.getConfigServerHttpsIdentity(zoneId)));
-    }
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
index 8053944734c..1a7002c5759 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
@@ -11,7 +11,6 @@ import com.yahoo.restapi.Path;
 import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
-import com.yahoo.vespa.athenz.tls.AthenzIdentityVerifier;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLoggingRequestHandler;
@@ -20,10 +19,8 @@ import com.yahoo.vespa.hosted.controller.proxy.ProxyException;
 import com.yahoo.vespa.hosted.controller.proxy.ProxyRequest;
 import com.yahoo.yolean.Exceptions;
 
-import javax.net.ssl.HostnameVerifier;
 import java.net.URI;
 import java.util.List;
-import java.util.Set;
 import java.util.logging.Level;
 
 /**
@@ -87,8 +84,7 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler {
             throw new IllegalArgumentException("No such zone: " + zoneId.value());
         }
         try {
-            return proxy.handle(new ProxyRequest(
-                    request, getConfigserverEndpoints(zoneId), createHostnameVerifier(zoneId), path.getRest()));
+            return proxy.handle(new ProxyRequest(request, getConfigserverEndpoints(zoneId), path.getRest()));
         } catch (ProxyException e) {
             throw new RuntimeException(e);
         }
@@ -125,8 +121,4 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler {
             return zoneRegistry.getConfigServerUris(zoneId);
         }
     }
-
-    private HostnameVerifier createHostnameVerifier(ZoneId zoneId) {
-        return new AthenzIdentityVerifier(Set.of(zoneRegistry.getConfigServerHttpsIdentity(zoneId)));
-    }
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
index 5b8c99799b1..d8373cb8928 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
@@ -23,7 +23,7 @@ public class ProxyRequestTest {
     @Test
     public void testEmpty() throws Exception {
         exception.expectMessage("Request must be non-null");
-        new ProxyRequest(HttpRequest.Method.GET, null, Map.of(), null, List.of(), null, "/zone/v2");
+        new ProxyRequest(HttpRequest.Method.GET, null, Map.of(), null, List.of(), "/zone/v2");
     }
 
     @Test
@@ -69,6 +69,6 @@ public class ProxyRequestTest {
 
     private static ProxyRequest testRequest(String url, String pathPrefix) throws ProxyException {
         return new ProxyRequest(
-                HttpRequest.Method.GET, URI.create(url), Map.of(), null, List.of(), null, pathPrefix);
+                HttpRequest.Method.GET, URI.create(url), Map.of(), null, List.of(), pathPrefix);
     }
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
index bc8b4169235..0aac59321b5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
@@ -20,7 +20,7 @@ public class ProxyResponseTest {
     @Test
     public void testRewriteUrl() throws Exception {
         ProxyRequest request = new ProxyRequest(HttpRequest.Method.GET, URI.create("http://domain.tld/zone/v2/dev/us-north-1/configserver"),
-                Map.of(), null, List.of(), null, "configserver");
+                Map.of(), null, List.of(), "configserver");
         ProxyResponse proxyResponse = new ProxyResponse(
                 request,
                 "response link is http://configserver:1234/bla/bla/",
@@ -38,7 +38,7 @@ public class ProxyResponseTest {
     @Test
     public void testRewriteSecureUrl() throws Exception {
         ProxyRequest request = new ProxyRequest(HttpRequest.Method.GET, URI.create("https://domain.tld/zone/v2/prod/eu-south-3/configserver"),
-                Map.of(), null, List.of(), null, "configserver");
+                Map.of(), null, List.of(), "configserver");
         ProxyResponse proxyResponse = new ProxyResponse(
                 request,
                 "response link is http://configserver:1234/bla/bla/",