diff options
author | Martin Polden <mpolden@mpolden.no> | 2020-06-05 13:13:17 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-06-05 13:13:17 +0200 |
commit | e260e9d3b5eaa3d0709a065be0aa57b4d87ddb21 (patch) | |
tree | 6de6edc16f0527be8ee308baeda7518f4d5aea6d /controller-server | |
parent | ed3e41bf6cd040fec72773911a0e41a833017639 (diff) | |
parent | e3efa0be7870d14ff59fc9edca92de63991ba4c4 (diff) |
Merge pull request #13493 from vespa-engine/mpolden/vip-connection-reuse
Disable connection reuse for VIPs
Diffstat (limited to 'controller-server')
11 files changed, 279 insertions, 196 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutor.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutor.java index 529acc48cbe..110d994c179 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutor.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutor.java @@ -10,5 +10,7 @@ import com.yahoo.container.jdisc.HttpResponse; * @author Haakon Dybdahl */ public interface ConfigServerRestExecutor { - HttpResponse handle(ProxyRequest proxyRequest) throws ProxyException; + + HttpResponse handle(ProxyRequest request); + } 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 d10c4dd226b..f5dcae9c961 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 @@ -1,15 +1,16 @@ // 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 ai.vespa.util.http.retry.Sleeper; import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; import com.yahoo.jdisc.http.HttpRequest.Method; -import java.util.logging.Level; 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.HttpResponse; import org.apache.http.client.config.RequestConfig; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpDelete; @@ -19,11 +20,15 @@ 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.entity.InputStreamEntity; +import org.apache.http.impl.DefaultConnectionReuseStrategy; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpCoreContext; import org.apache.http.util.EntityUtils; import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLContext; import javax.net.ssl.SSLSession; import java.io.IOException; import java.io.InputStream; @@ -37,6 +42,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -50,75 +56,87 @@ import static com.yahoo.yolean.Exceptions.uncheck; @SuppressWarnings("unused") // Injected public class ConfigServerRestExecutorImpl extends AbstractComponent implements ConfigServerRestExecutor { - private static final Logger log = Logger.getLogger(ConfigServerRestExecutorImpl.class.getName()); - + private static final Logger LOG = Logger.getLogger(ConfigServerRestExecutorImpl.class.getName()); private static final Duration PROXY_REQUEST_TIMEOUT = Duration.ofSeconds(10); + private static final Duration PING_REQUEST_TIMEOUT = Duration.ofMillis(500); + private static final Duration SINGLE_TARGET_WAIT = Duration.ofSeconds(2); + private static final int SINGLE_TARGET_RETRIES = 3; private static final Set<String> HEADERS_TO_COPY = Set.of("X-HTTP-Method-Override", "Content-Type"); private final CloseableHttpClient client; + private final Sleeper sleeper; @Inject 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(zoneRegistry, sslContextProvider.getIdentitySslContext(), new Sleeper.Default(), + new ConnectionReuseStrategy(zoneRegistry)); + } - this.client = createHttpClient(config, sslContextProvider, - new ControllerOrConfigserverHostnameVerifier(zoneRegistry)); + ConfigServerRestExecutorImpl(ZoneRegistry zoneRegistry, SSLContext sslContext, + Sleeper sleeper, ConnectionReuseStrategy connectionReuseStrategy) { + this.client = createHttpClient(sslContext, + new ControllerOrConfigserverHostnameVerifier(zoneRegistry), + connectionReuseStrategy); + this.sleeper = sleeper; } @Override - public ProxyResponse handle(ProxyRequest proxyRequest) throws ProxyException { - // Make a local copy of the list as we want to manipulate it in case of ping problems. - List<URI> allServers = new ArrayList<>(proxyRequest.getTargets()); + public ProxyResponse handle(ProxyRequest request) { + List<URI> targets = new ArrayList<>(request.getTargets()); StringBuilder errorBuilder = new StringBuilder(); - if (queueFirstServerIfDown(allServers)) { + boolean singleTarget = targets.size() == 1; + if (singleTarget) { + for (int i = 0; i < SINGLE_TARGET_RETRIES - 1; i++) { + targets.add(targets.get(0)); + } + } else if (queueFirstServerIfDown(targets)) { errorBuilder.append("Change ordering due to failed ping."); } - for (URI uri : allServers) { - Optional<ProxyResponse> proxyResponse = proxyCall(uri, proxyRequest, errorBuilder); + + for (URI url : targets) { + Optional<ProxyResponse> proxyResponse = proxy(request, url, errorBuilder); if (proxyResponse.isPresent()) { return proxyResponse.get(); } + if (singleTarget) { + sleeper.sleep(SINGLE_TARGET_WAIT); + } } - // TODO Add logging, for now, experimental and we want to not add more noise. - throw new ProxyException(ErrorResponse.internalServerError("Failed talking to config servers: " - + errorBuilder.toString())); + + throw new RuntimeException("Failed talking to config servers: " + errorBuilder.toString()); } - private Optional<ProxyResponse> proxyCall(URI uri, ProxyRequest proxyRequest, StringBuilder errorBuilder) - throws ProxyException { - final HttpRequestBase requestBase = createHttpBaseRequest( - proxyRequest.getMethod(), proxyRequest.createConfigServerRequestUri(uri), proxyRequest.getData()); + private Optional<ProxyResponse> proxy(ProxyRequest request, URI url, StringBuilder errorBuilder) { + HttpRequestBase requestBase = createHttpBaseRequest( + request.getMethod(), request.createConfigServerRequestUri(url), request.getData()); // Empty list of headers to copy for now, add headers when needed, or rewrite logic. - copyHeaders(proxyRequest.getHeaders(), requestBase); + copyHeaders(request.getHeaders(), requestBase); try (CloseableHttpResponse response = client.execute(requestBase)) { String content = getContent(response); int status = response.getStatusLine().getStatusCode(); if (status / 100 == 5) { - errorBuilder.append("Talking to server ").append(uri.getHost()); + errorBuilder.append("Talking to server ").append(url.getHost()); errorBuilder.append(", got ").append(status).append(" ") .append(content).append("\n"); - log.log(Level.FINE, () -> String.format("Got response from %s with status code %d and content:\n %s", - uri.getHost(), status, content)); + LOG.log(Level.FINE, () -> String.format("Got response from %s with status code %d and content:\n %s", + url.getHost(), status, content)); return Optional.empty(); } - final Header contentHeader = response.getLastHeader("Content-Type"); - final String contentType; + Header contentHeader = response.getLastHeader("Content-Type"); + String contentType; if (contentHeader != null && contentHeader.getValue() != null && ! contentHeader.getValue().isEmpty()) { contentType = contentHeader.getValue().replace("; charset=UTF-8",""); } else { contentType = "application/json"; } // Send response back - return Optional.of(new ProxyResponse(proxyRequest, content, status, uri, contentType)); + return Optional.of(new ProxyResponse(request, content, status, url, contentType)); } catch (Exception e) { - errorBuilder.append("Talking to server ").append(uri.getHost()); + errorBuilder.append("Talking to server ").append(url.getHost()); errorBuilder.append(" got exception ").append(e.getMessage()); - log.log(Level.FINE, e, () -> "Got exception while sending request to " + uri.getHost()); + LOG.log(Level.FINE, e, () -> "Got exception while sending request to " + url.getHost()); return Optional.empty(); } } @@ -129,33 +147,32 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C .orElse(""); } - private static HttpRequestBase createHttpBaseRequest(Method method, URI uri, InputStream data) throws ProxyException { + private static HttpRequestBase createHttpBaseRequest(Method method, URI url, InputStream data) { switch (method) { case GET: - return new HttpGet(uri); + return new HttpGet(url); case POST: - HttpPost post = new HttpPost(uri); + HttpPost post = new HttpPost(url); if (data != null) { post.setEntity(new InputStreamEntity(data)); } return post; case PUT: - HttpPut put = new HttpPut(uri); + HttpPut put = new HttpPut(url); if (data != null) { put.setEntity(new InputStreamEntity(data)); } return put; case DELETE: - return new HttpDelete(uri); + return new HttpDelete(url); case PATCH: - HttpPatch patch = new HttpPatch(uri); + HttpPatch patch = new HttpPatch(url); if (data != null) { patch.setEntity(new InputStreamEntity(data)); } return patch; - default: - throw new ProxyException(ErrorResponse.methodNotAllowed("Will not proxy such calls.")); } + throw new IllegalArgumentException("Refusing to proxy " + method + " " + url + ": Unsupported method"); } private static void copyHeaders(Map<String, List<String>> headers, HttpRequestBase toRequest) { @@ -169,7 +186,7 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C } /** - * During upgrade, one server can be down, this is normal. Therefor we do a quick ping on the first server, + * During upgrade, one server can be down, this is normal. Therefore we do a quick ping on the first server, * if it is not responding, we try the other servers first. False positive/negatives are not critical, * but will increase latency to some extent. */ @@ -178,15 +195,15 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C return false; } URI uri = allServers.get(0); - HttpGet httpget = new HttpGet(uri); + HttpGet httpGet = new HttpGet(uri); - int timeout = 500; RequestConfig config = RequestConfig.custom() - .setConnectTimeout(timeout) - .setConnectionRequestTimeout(timeout) - .setSocketTimeout(timeout).build(); - httpget.setConfig(config); - try (CloseableHttpResponse response = client.execute(httpget)) { + .setConnectTimeout((int) PING_REQUEST_TIMEOUT.toMillis()) + .setConnectionRequestTimeout((int) PING_REQUEST_TIMEOUT.toMillis()) + .setSocketTimeout((int) PING_REQUEST_TIMEOUT.toMillis()).build(); + httpGet.setConfig(config); + + try (CloseableHttpResponse response = client.execute(httpGet)) { if (response.getStatusLine().getStatusCode() == 200) { return false; } @@ -208,18 +225,25 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C } } - private static CloseableHttpClient createHttpClient(RequestConfig config, - ServiceIdentityProvider sslContextProvider, - HostnameVerifier hostnameVerifier) { + private static CloseableHttpClient createHttpClient(SSLContext sslContext, + HostnameVerifier hostnameVerifier, + org.apache.http.ConnectionReuseStrategy connectionReuseStrategy) { + + RequestConfig config = RequestConfig.custom() + .setConnectTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis()) + .setConnectionRequestTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis()) + .setSocketTimeout((int) PROXY_REQUEST_TIMEOUT.toMillis()).build(); return HttpClientBuilder.create() - .setUserAgent("config-server-proxy-client") - .setSSLContext(sslContextProvider.getIdentitySslContext()) - .setSSLHostnameVerifier(hostnameVerifier) - .setDefaultRequestConfig(config) - .setMaxConnPerRoute(10) - .setMaxConnTotal(500) - .setConnectionTimeToLive(1, TimeUnit.MINUTES) - .build(); + .setUserAgent("config-server-proxy-client") + .setSSLContext(sslContext) + .setSSLHostnameVerifier(hostnameVerifier) + .setDefaultRequestConfig(config) + .setMaxConnPerRoute(10) + .setMaxConnTotal(500) + .setConnectionReuseStrategy(connectionReuseStrategy) + .setConnectionTimeToLive(1, TimeUnit.MINUTES) + .build(); + } private static class ControllerOrConfigserverHostnameVerifier implements HostnameVerifier { @@ -242,4 +266,36 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C return "localhost".equals(hostname) || configserverVerifier.verify(hostname, session); } } + + /** + * A connection reuse strategy which avoids reusing connections to VIPs. Since VIPs are TCP-level load balancers, + * a reconnect is needed to (potentially) switch real server. + */ + public static class ConnectionReuseStrategy extends DefaultConnectionReuseStrategy { + + private final Set<String> vips; + + public ConnectionReuseStrategy(ZoneRegistry zoneRegistry) { + this(zoneRegistry.zones().all().ids().stream() + .map(zoneRegistry::getConfigServerVipUri) + .map(URI::getHost) + .collect(Collectors.toUnmodifiableSet())); + } + + public ConnectionReuseStrategy(Set<String> vips) { + this.vips = Set.copyOf(vips); + } + + @Override + public boolean keepAlive(HttpResponse response, HttpContext context) { + HttpCoreContext coreContext = HttpCoreContext.adapt(context); + String host = coreContext.getTargetHost().getHostName(); + if (vips.contains(host)) { + return false; + } + return super.keepAlive(response, context); + } + + } + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ErrorResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ErrorResponse.java deleted file mode 100644 index 3673c0227a3..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ErrorResponse.java +++ /dev/null @@ -1,66 +0,0 @@ -// 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.container.jdisc.HttpResponse; -import com.yahoo.slime.Cursor; -import com.yahoo.slime.JsonFormat; -import com.yahoo.slime.Slime; - -import java.io.IOException; -import java.io.OutputStream; - -import static com.yahoo.jdisc.Response.Status.BAD_REQUEST; -import static com.yahoo.jdisc.Response.Status.INTERNAL_SERVER_ERROR; -import static com.yahoo.jdisc.Response.Status.METHOD_NOT_ALLOWED; -import static com.yahoo.jdisc.Response.Status.NOT_FOUND; - -/** - * Class for generating error responses. - * - * @author Haakon Dybdahl - */ -public class ErrorResponse extends HttpResponse { - - private final Slime slime = new Slime(); - public final String message; - - public ErrorResponse(int code, String errorType, String message) { - super(code); - this.message = message; - Cursor root = slime.setObject(); - root.setString("error-code", errorType); - root.setString("message", message); - } - - public enum errorCodes { - NOT_FOUND, - BAD_REQUEST, - METHOD_NOT_ALLOWED, - INTERNAL_SERVER_ERROR, - - } - - public static ErrorResponse notFoundError(String message) { - return new ErrorResponse(NOT_FOUND, errorCodes.NOT_FOUND.name(), message); - } - - public static ErrorResponse internalServerError(String message) { - return new ErrorResponse(INTERNAL_SERVER_ERROR, errorCodes.INTERNAL_SERVER_ERROR.name(), message); - } - - public static ErrorResponse badRequest(String message) { - return new ErrorResponse(BAD_REQUEST, errorCodes.BAD_REQUEST.name(), message); - } - - public static ErrorResponse methodNotAllowed(String message) { - return new ErrorResponse(METHOD_NOT_ALLOWED, errorCodes.METHOD_NOT_ALLOWED.name(), message); - } - - @Override - public void render(OutputStream stream) throws IOException { - new JsonFormat(true).encode(stream, slime); - } - - @Override - public String getContentType() { return "application/json"; } -} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyException.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyException.java deleted file mode 100644 index aa828bc0c83..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyException.java +++ /dev/null @@ -1,16 +0,0 @@ -// 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; - -/** - * Exceptions related to proxying calls to config servers. - * - * @author Haakon Dybdahl - */ -public class ProxyException extends Exception { - public final ErrorResponse errorResponse; - - public ProxyException(ErrorResponse errorResponse) { - super(errorResponse.message); - this.errorResponse = errorResponse; - } -} 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 f398683567b..a6314df9581 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 @@ -28,33 +28,21 @@ public class ProxyRequest { private final List<URI> targets; private final String targetPath; - /** - * The constructor calls exception if the request is invalid. - * - * @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 targetPath the path to proxy to. - * @throws ProxyException on errors - */ - public ProxyRequest(HttpRequest request, List<URI> targets, String targetPath) throws ProxyException { - this(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), request.getData(), - targets, targetPath); - } - - ProxyRequest(Method method, URI requestUri, Map<String, List<String>> headers, InputStream body, - 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( - "Request path '%s' does not end with proxy path '%s'", requestUri.getPath(), targetPath))); - + ProxyRequest(Method method, URI url, Map<String, List<String>> headers, InputStream body, List<URI> targets, + String path) { + Objects.requireNonNull(url); + if (!url.getPath().endsWith(path)) { + throw new IllegalArgumentException(String.format("Request path '%s' does not end with proxy path '%s'", url.getPath(), path)); + } + if (targets.isEmpty()) { + throw new IllegalArgumentException("targets must be non-empty"); + } this.method = Objects.requireNonNull(method); - this.requestUri = Objects.requireNonNull(requestUri); + this.requestUri = Objects.requireNonNull(url); this.headers = Objects.requireNonNull(headers); this.requestData = body; - this.targets = List.copyOf(targets); - this.targetPath = targetPath.startsWith("/") ? targetPath : "/" + targetPath; + this.targetPath = path.startsWith("/") ? path : "/" + path; } @@ -100,4 +88,16 @@ public class ProxyRequest { return "[targets: " + targets + " request: " + targetPath + "]"; } + /** Create a proxy request that tries all given targets in order */ + public static ProxyRequest tryAll(List<URI> targets, String path, HttpRequest request) { + return new ProxyRequest(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), + request.getData(), targets, path); + } + + /** Create a proxy request that repeatedly tries a single target */ + public static ProxyRequest tryOne(URI target, String path, HttpRequest request) { + return new ProxyRequest(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), + request.getData(), List.of(target), path); + } + } 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 6ffdea93a1c..f65f7534476 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 @@ -15,7 +15,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.ServiceRegistry; 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.yolean.Exceptions; @@ -95,11 +94,7 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler { "' through /configserver/v1, following APIs are permitted: " + String.join(", ", WHITELISTED_APIS)); } - try { - return proxy.handle(new ProxyRequest(request, List.of(getEndpoint(zoneId)), cfgPath)); - } catch (ProxyException e) { - throw new RuntimeException(e); - } + return proxy.handle(ProxyRequest.tryOne(getEndpoint(zoneId), cfgPath, request)); } private HttpResponse root(HttpRequest request) { @@ -126,4 +121,5 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler { private URI getEndpoint(ZoneId zoneId) { return CONTROLLER_ZONE.equals(zoneId) ? CONTROLLER_URI : zoneRegistry.getConfigServerVipUri(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 a127a44efb2..7c44ae6d1a5 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 @@ -16,12 +16,9 @@ import com.yahoo.vespa.hosted.controller.api.integration.ServiceRegistry; 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.yolean.Exceptions; -import java.net.URI; -import java.util.List; import java.util.logging.Level; /** @@ -84,11 +81,7 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler { if ( ! zoneRegistry.hasZone(zoneId)) { throw new IllegalArgumentException("No such zone: " + zoneId.value()); } - try { - return proxy.handle(new ProxyRequest(request, getConfigserverEndpoints(zoneId), path.getRest())); - } catch (ProxyException e) { - throw new RuntimeException(e); - } + return proxy.handle(proxyRequest(zoneId, path.getRest(), request)); } private HttpResponse root(HttpRequest request) { @@ -114,13 +107,12 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler { return ErrorResponse.notFoundError("Nothing at " + path); } - private List<URI> getConfigserverEndpoints(ZoneId zoneId) { + private ProxyRequest proxyRequest(ZoneId zoneId, String path, HttpRequest request) { // TODO: Use config server VIP for all zones that have one if (zoneId.region().value().startsWith("aws-") || zoneId.region().value().contains("-aws-")) { - return List.of(zoneRegistry.getConfigServerVipUri(zoneId)); - } else { - return zoneRegistry.getConfigServerUris(zoneId); + return ProxyRequest.tryOne(zoneRegistry.getConfigServerVipUri(zoneId), path, request); } + return ProxyRequest.tryAll(zoneRegistry.getConfigServerUris(zoneId), path, request); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerProxyMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerProxyMock.java index d6e1af07938..d481aaa2c77 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerProxyMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerProxyMock.java @@ -20,10 +20,10 @@ public class ConfigServerProxyMock extends AbstractComponent implements ConfigSe private volatile String requestBody = null; @Override - public HttpResponse handle(ProxyRequest proxyRequest) { - lastReceived = proxyRequest; + public HttpResponse handle(ProxyRequest request) { + lastReceived = request; // Copy request body as the input stream is drained once the request completes - requestBody = asString(proxyRequest.getData()); + requestBody = asString(request.getData()); return new StringResponse("ok"); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImplTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImplTest.java new file mode 100644 index 00000000000..1fce7ba5695 --- /dev/null +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImplTest.java @@ -0,0 +1,123 @@ +// Copyright Verizon Media. 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.github.tomakehurst.wiremock.junit.WireMockRule; +import com.github.tomakehurst.wiremock.stubbing.Scenario; +import com.yahoo.config.provision.SystemName; +import com.yahoo.container.jdisc.HttpRequest; +import com.yahoo.container.jdisc.HttpResponse; +import com.yahoo.vespa.hosted.controller.integration.ZoneRegistryMock; +import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpCoreContext; +import org.junit.Rule; +import org.junit.Test; + +import javax.net.ssl.SSLContext; +import java.io.ByteArrayOutputStream; +import java.net.URI; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.junit.Assert.assertEquals; + +/** + * @author mpolden + */ +public class ConfigServerRestExecutorImplTest { + + @Rule + public final WireMockRule wireMock = new WireMockRule(options().dynamicPort(), true); + + @Test + public void proxy_with_retries() throws Exception { + var connectionReuseStrategy = new CountingConnectionReuseStrategy(Set.of("127.0.0.1")); + var proxy = new ConfigServerRestExecutorImpl(new ZoneRegistryMock(SystemName.cd), SSLContext.getDefault(), + (duration) -> {}, connectionReuseStrategy); + + URI url = url(); + String path = url.getPath(); + stubRequests(path); + + HttpRequest request = HttpRequest.createTestRequest(url.toString(), com.yahoo.jdisc.http.HttpRequest.Method.GET); + ProxyRequest proxyRequest = ProxyRequest.tryOne(url, path, request); + + // Request is retried + HttpResponse response = proxy.handle(proxyRequest); + wireMock.verify(3, getRequestedFor(urlEqualTo(path))); + assertEquals(200, response.getStatus()); + ByteArrayOutputStream out = new ByteArrayOutputStream(); + response.render(out); + assertEquals("OK", out.toString()); + + // No connections are reused as host is a VIP + assertEquals(0, connectionReuseStrategy.reusedConnections.get(url.getHost()).intValue()); + } + + @Test + public void proxy_without_connection_reuse() throws Exception { + var connectionReuseStrategy = new CountingConnectionReuseStrategy(Set.of()); + var proxy = new ConfigServerRestExecutorImpl(new ZoneRegistryMock(SystemName.cd), SSLContext.getDefault(), + (duration) -> {}, connectionReuseStrategy); + + URI url = url(); + String path = url.getPath(); + stubRequests(path); + + HttpRequest request = HttpRequest.createTestRequest(url.toString(), com.yahoo.jdisc.http.HttpRequest.Method.GET); + ProxyRequest proxyRequest = ProxyRequest.tryOne(url, path, request); + + // Connections are reused + assertEquals(200, proxy.handle(proxyRequest).getStatus()); + assertEquals(3, connectionReuseStrategy.reusedConnections.get(url.getHost()).intValue()); + } + + private URI url() { + return URI.create("http://127.0.0.1:" + wireMock.port() + "/"); + } + + private void stubRequests(String path) { + String retryScenario = "Retry scenario"; + String retryRequest = "Retry request 1"; + String retryRequestAgain = "Retry request 2"; + + wireMock.stubFor(get(urlEqualTo(path)).inScenario(retryScenario) + .whenScenarioStateIs(Scenario.STARTED) + .willSetStateTo(retryRequest) + .willReturn(aResponse().withStatus(500))); + + wireMock.stubFor(get(urlEqualTo(path)).inScenario(retryScenario) + .whenScenarioStateIs(retryRequest) + .willSetStateTo(retryRequestAgain) + .willReturn(aResponse().withStatus(500))); + + wireMock.stubFor(get(urlEqualTo(path)).inScenario(retryScenario) + .whenScenarioStateIs(retryRequestAgain) + .willReturn(aResponse().withBody("OK"))); + } + + private static class CountingConnectionReuseStrategy extends ConfigServerRestExecutorImpl.ConnectionReuseStrategy { + + private final Map<String, Integer> reusedConnections = new HashMap<>(); + + public CountingConnectionReuseStrategy(Set<String> vips) { + super(vips); + } + + @Override + public boolean keepAlive(org.apache.http.HttpResponse response, HttpContext context) { + boolean keepAlive = super.keepAlive(response, context); + String host = HttpCoreContext.adapt(context).getTargetHost().getHostName(); + reusedConnections.putIfAbsent(host, 0); + if (keepAlive) reusedConnections.compute(host, (ignored, count) -> ++count); + return keepAlive; + } + + } + +} 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 d8373cb8928..15ec138354d 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 @@ -21,12 +21,6 @@ public class ProxyRequestTest { public final ExpectedException exception = ExpectedException.none(); @Test - public void testEmpty() throws Exception { - exception.expectMessage("Request must be non-null"); - new ProxyRequest(HttpRequest.Method.GET, null, Map.of(), null, List.of(), "/zone/v2"); - } - - @Test public void testBadUri() throws Exception { exception.expectMessage("Request path '/path' does not end with proxy path '/zone/v2/'"); testRequest("http://domain.tld/path", "/zone/v2/"); @@ -67,8 +61,9 @@ 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(), pathPrefix); + private static ProxyRequest testRequest(String url, String pathPrefix) { + return new ProxyRequest(HttpRequest.Method.GET, URI.create(url), Map.of(), null, + List.of(URI.create("http://example.com")), 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 0aac59321b5..539d4a6dd75 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(), "configserver"); + Map.of(), null, List.of(URI.create("http://example.com")), "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(), "configserver"); + Map.of(), null, List.of(URI.create("http://example.com")), "configserver"); ProxyResponse proxyResponse = new ProxyResponse( request, "response link is http://configserver:1234/bla/bla/", @@ -52,4 +52,5 @@ public class ProxyResponseTest { String document = new String(outputStream.toByteArray(), StandardCharsets.UTF_8); assertEquals("response link is https://domain.tld/zone/v2/prod/eu-south-3/bla/bla/", document); } + } |