Refactor ConfigServerRestExecutorImpl

4 files changed, 74 insertions, 113 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 6212569d788..e3c048e865a 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
@@ -17,7 +17,6 @@ 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.X509HostnameVerifier;
 import org.apache.http.entity.InputStreamEntity;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
@@ -26,7 +25,6 @@ import org.apache.http.util.EntityUtils;
 import javax.net.ssl.HostnameVerifier;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.UncheckedIOException;
 import java.net.URI;
 import java.time.Duration;
 import java.util.ArrayList;
@@ -37,7 +35,8 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.logging.Logger;
 
-import static java.util.Collections.singleton;
+import static com.yahoo.yolean.Exceptions.uncheck;
+
 
 /**
  * @author Haakon Dybdahl
@@ -55,17 +54,15 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
     private final ServiceIdentityProvider sslContextProvider;
 
     @Inject
-    public ConfigServerRestExecutorImpl(ZoneRegistry zoneRegistry,
-                                        ServiceIdentityProvider sslContextProvider) {
+    public ConfigServerRestExecutorImpl(ZoneRegistry zoneRegistry, ServiceIdentityProvider sslContextProvider) {
         this.zoneRegistry = zoneRegistry;
         this.sslContextProvider = sslContextProvider;
     }
 
     @Override
     public ProxyResponse handle(ProxyRequest proxyRequest) throws ProxyException {
-        ZoneId zoneId = ZoneId.from(proxyRequest.getEnvironment(), proxyRequest.getRegion());
-        HostnameVerifier hostnameVerifier = createHostnameVerifier(zoneId);
-        List<URI> allServers = getConfigserverEndpoints(zoneId);
+        HostnameVerifier hostnameVerifier = createHostnameVerifier(proxyRequest.getZoneId());
+        List<URI> allServers = getConfigserverEndpoints(proxyRequest.getZoneId());
 
         StringBuilder errorBuilder = new StringBuilder();
         if (queueFirstServerIfDown(allServers, hostnameVerifier)) {
@@ -86,25 +83,17 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
         // TODO: Use config server VIP for all zones that have one
         // Make a local copy of the list as we want to manipulate it in case of ping problems.
         if (zoneId.region().value().startsWith("aws-") || zoneId.region().value().contains("-aws-")) {
-            return Collections.singletonList(zoneRegistry.getConfigServerVipUri(zoneId));
+            return List.of(zoneRegistry.getConfigServerVipUri(zoneId));
         } else {
             return new ArrayList<>(zoneRegistry.getConfigServerUris(zoneId));
         }
     }
 
-    private static String removeFirstSlashIfAny(String url) {
-        if (url.startsWith("/")) {
-            return url.substring(1);
-        }
-        return url;
-    }
-
     private Optional<ProxyResponse> proxyCall(
             URI uri, ProxyRequest proxyRequest, HostnameVerifier hostnameVerifier, StringBuilder errorBuilder)
             throws ProxyException {
-        String fullUri = uri.toString() + removeFirstSlashIfAny(proxyRequest.getConfigServerRequest());
         final HttpRequestBase requestBase = createHttpBaseRequest(
-                proxyRequest.getMethod(), fullUri, proxyRequest.getData());
+                proxyRequest.getMethod(), proxyRequest.createConfigServerRequestUri(uri), proxyRequest.getData());
         // Empty list of headers to copy for now, add headers when needed, or rewrite logic.
         copyHeaders(proxyRequest.getHeaders(), requestBase);
 
@@ -145,20 +134,12 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
 
     private static String getContent(CloseableHttpResponse response) {
         return Optional.ofNullable(response.getEntity())
-                .map(entity ->
-                     {
-                         try {
-                             return EntityUtils.toString(entity);
-                         } catch (IOException e) {
-                             throw new UncheckedIOException(e);
-                         }
-                     }
-                ).orElse("");
+                .map(entity -> uncheck(() -> EntityUtils.toString(entity)))
+                .orElse("");
     }
 
-    private static HttpRequestBase createHttpBaseRequest(String method, String uri, InputStream data) throws ProxyException {
-        Method enumMethod =  Method.valueOf(method);
-        switch (enumMethod) {
+    private static HttpRequestBase createHttpBaseRequest(Method method, URI uri, InputStream data) throws ProxyException {
+        switch (method) {
             case GET:
                 return new HttpGet(uri);
             case POST:
@@ -216,7 +197,6 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
         try (
                 CloseableHttpClient client = createHttpClient(config, sslContextProvider, hostnameVerifier);
                 CloseableHttpResponse response = client.execute(httpget)
-
         ) {
             if (response.getStatusLine().getStatusCode() == 200) {
                 return false;
@@ -226,8 +206,7 @@ public class ConfigServerRestExecutorImpl implements ConfigServerRestExecutor {
             // We ignore this, if server is restarting this might happen.
         }
         // Some error happened, move this server to the back. The other servers should be running.
-        allServers.remove(0);
-        allServers.add(uri);
+        Collections.rotate(allServers, -1);
         return true;
     }
 
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 aa6205f63a3..100292a0bdc 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
@@ -1,15 +1,17 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.proxy;
 
+import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.container.jdisc.HttpRequest;
-import com.yahoo.net.HostName;
 
-import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
-import java.net.URLDecoder;
+import java.net.URISyntaxException;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
+
+import static com.yahoo.jdisc.http.HttpRequest.Method;
 
 /**
  * Keeping information about the calls that are being proxied.
@@ -19,95 +21,84 @@ import java.util.Map;
  */
 public class ProxyRequest {
 
-    private final String environment;
-    private final String region;
-    private final String configServerRequest;
-    private final InputStream requestData;
+    private final Method method;
+    private final URI requestUri;
     private final Map<String, List<String>> headers;
-    private final String method;
-    private final String controllerPrefix;
-    private final String scheme;
+    private final InputStream requestData;
+
+    private final ZoneId zoneId;
+    private final String proxyPath;
 
     /**
      * The constructor calls exception if the request is invalid.
      *
      * @param request the request from the jdisc framework.
-     * @param pathPrefix the path prefix of the proxy.
+     * @param zoneId the zone to proxy to.
+     * @param proxyPath the path to proxy to.
      * @throws ProxyException on errors
      */
-    public ProxyRequest(HttpRequest request, String pathPrefix) throws ProxyException, IOException {
-        this(request.getUri(), request.getJDiscRequest().headers(), request.getData(), request.getMethod().name(),
-             pathPrefix);
+    public ProxyRequest(HttpRequest request, ZoneId zoneId, String proxyPath) throws ProxyException {
+        this(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), request.getData(),
+             zoneId, proxyPath);
     }
 
-    ProxyRequest(URI requestUri, Map<String, List<String>> headers, InputStream body, String method,
-                 String pathPrefix) throws ProxyException, IOException {
-        if (requestUri == null) {
-            throw new ProxyException(ErrorResponse.badRequest("Request not set."));
-        }
-        final String path = URLDecoder.decode(requestUri.getPath(),"UTF-8");
-        if (! path.startsWith(pathPrefix)) {
-            // This has to be caused by wrong mapping of path in services.xml.
-            throw new ProxyException(ErrorResponse.notFoundError("Request not starting with " + pathPrefix));
-        }
-        final String uriNoPrefix = path.replaceFirst(pathPrefix, "")
-                + (requestUri.getRawQuery() == null ? "" : "?" + requestUri.getRawQuery());
-
-        final String[] parts = uriNoPrefix.split("/");
+    ProxyRequest(Method method, URI requestUri, Map<String, List<String>> headers, InputStream body,
+                 ZoneId zoneId, String proxyPath) throws ProxyException {
+        Objects.requireNonNull(requestUri, "Request must be non-null");
+        if (!requestUri.getPath().endsWith(proxyPath))
+            throw new ProxyException(ErrorResponse.badRequest(String.format(
+                    "Request path '%s' does not end with proxy path '%s'", requestUri.getPath(), proxyPath)));
 
-        this.environment = parts.length > 0 ? parts[0] : "";
-        this.region = parts.length > 1 ? parts[1] : "";
-        this.configServerRequest = parts.length > 2 ? uriNoPrefix.replace(environment + "/" + region, "") : "";
+        this.method = Objects.requireNonNull(method);
+        this.requestUri = Objects.requireNonNull(requestUri);
+        this.headers = Objects.requireNonNull(headers);
         this.requestData = body;
-        this.headers = headers;
-        this.method = method;
-
-        String hostPort = headers.containsKey("host")
-                ? headers.get("host").get(0)
-                : HostName.getLocalhost() + ":" + requestUri.getPort();
-        StringBuilder prefix = new StringBuilder(hostPort + pathPrefix);
-        if (! environment.isEmpty()) {
-            prefix.append(environment).append("/").append(region);
-        }
 
-        this.controllerPrefix = prefix.toString();
-        this.scheme = requestUri.getScheme();
+        this.zoneId = Objects.requireNonNull(zoneId);
+        this.proxyPath = proxyPath.startsWith("/") ? proxyPath : "/" + proxyPath;
     }
 
 
-    public String getRegion() {
-        return region;
-    }
-
-    public String getEnvironment() {
-        return environment;
+    public Method getMethod() {
+        return method;
     }
 
-    public String getConfigServerRequest() {
-        return configServerRequest;
+    public Map<String, List<String>> getHeaders() {
+        return headers;
     }
 
     public InputStream getData() {
         return requestData;
     }
 
-    @Override
-    public String toString() {
-        return "[ region: " + region + " env: " + environment + " request: " + configServerRequest + "]";
+    public ZoneId getZoneId() {
+        return zoneId;
     }
 
-    public Map<String, List<String>> getHeaders() {
-        return headers;
+    public URI createConfigServerRequestUri(URI baseURI) {
+        try {
+            return new URI(baseURI.getScheme(), baseURI.getUserInfo(), baseURI.getHost(),
+                    baseURI.getPort(), proxyPath, requestUri.getQuery(), requestUri.getFragment());
+        } catch (URISyntaxException e) {
+            throw new RuntimeException(e);
+        }
     }
 
-    public String getMethod() {
-        return method;
+    public URI getControllerPrefixUri() {
+        String prefixPath = proxyPath.equals("/") && !requestUri.getPath().endsWith("/") ?
+                requestUri.getPath() + proxyPath :
+                requestUri.getPath().substring(0, requestUri.getPath().length() - proxyPath.length() + 1);
+        try {
+            return new URI(requestUri.getScheme(), requestUri.getUserInfo(), requestUri.getHost(),
+                    requestUri.getPort(), prefixPath, null, null);
+        } catch (URISyntaxException e) {
+            throw new RuntimeException(e);
+        }
     }
 
-    public String getControllerPrefix() {
-        return controllerPrefix;
+    @Override
+    public String toString() {
+        return "[zone: " + zoneId + " request: " + proxyPath + "]";
     }
 
-    public String getScheme() { return scheme; }
-
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponse.java
index 64a82c2f5b5..b0b4f1a556a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponse.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponse.java
@@ -15,7 +15,7 @@ import java.nio.charset.StandardCharsets;
  *
  * @author Haakon Dybdahl
  */
-public class ProxyResponse  extends HttpResponse {
+public class ProxyResponse extends HttpResponse {
 
     private final String bodyResponseRewritten;
     private final String contentType;
@@ -29,11 +29,6 @@ public class ProxyResponse  extends HttpResponse {
         super(statusResponse);
         this.contentType = contentType;
 
-        if (controllerRequest.getControllerPrefix().isEmpty()) {
-            bodyResponseRewritten = bodyResponse;
-            return;
-        }
-
         final String configServerPrefix;
         final String controllerRequestPrefix;
         try {
@@ -41,12 +36,9 @@ public class ProxyResponse  extends HttpResponse {
                     .setScheme(configServer.getScheme())
                     .setHost(configServer.getHost())
                     .setPort(configServer.getPort())
+                    .setPath("/")
                     .build().toString();
-            controllerRequestPrefix = new URIBuilder()
-                    .setScheme(controllerRequest.getScheme())
-                    // controller prefix is more than host, so it is a bit hackish, but verified by tests.
-                    .setHost(controllerRequest.getControllerPrefix())
-                    .build().toString();
+            controllerRequestPrefix = controllerRequest.getControllerPrefixUri().toString();
         } catch (URISyntaxException e) {
             throw new RuntimeException(e);
         }
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 5ce679276f7..be601511763 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
@@ -1,25 +1,24 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.restapi.zone.v2;
 
+import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.config.provision.zone.ZoneList;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.LoggingRequestHandler;
+import com.yahoo.restapi.ErrorResponse;
 import com.yahoo.restapi.Path;
+import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
 import com.yahoo.vespa.hosted.controller.Controller;
-import com.yahoo.config.provision.zone.ZoneId;
-import com.yahoo.config.provision.zone.ZoneList;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLoggingRequestHandler;
 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.restapi.ErrorResponse;
-import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.yolean.Exceptions;
 
-import java.io.IOException;
 import java.util.logging.Level;
 
 /**
@@ -83,8 +82,8 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler {
             throw new IllegalArgumentException("No such zone: " + zoneId.value());
         }
         try {
-            return proxy.handle(new ProxyRequest(request, "/zone/v2/"));
-        } catch (ProxyException | IOException e) {
+            return proxy.handle(new ProxyRequest(request, zoneId, path.getRest()));
+        } catch (ProxyException e) {
             throw new RuntimeException(e);
         }
     }