Merge pull request #17524 from vespa-engine/jonmv/more-generic-config-server-client-error

More generic error response exception

3 files changed, 150 insertions, 131 deletions

diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
index e11f0db7194..d467402f79f 100644
--- a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
+++ b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java
@@ -1,8 +1,6 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.hosted.client;
 
-import com.yahoo.slime.Inspector;
-import com.yahoo.slime.SlimeUtils;
 import org.apache.hc.client5.http.classic.methods.ClassicHttpRequests;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
@@ -10,11 +8,9 @@ import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.HttpEntity;
-import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.Method;
 import org.apache.hc.core5.http.io.entity.HttpEntities;
 import org.apache.hc.core5.net.URIBuilder;
-import org.apache.hc.core5.util.Timeout;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -25,12 +21,12 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
+import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
-import static ai.vespa.hosted.client.ConfigServerClient.ConfigServerException.ErrorCode.INCOMPLETE_RESPONSE;
 import static java.util.Objects.requireNonNull;
 import static java.util.logging.Level.FINE;
 import static java.util.logging.Level.WARNING;
@@ -40,19 +36,15 @@ import static java.util.logging.Level.WARNING;
  */
 public abstract class AbstractConfigServerClient implements ConfigServerClient {
 
-    static final RequestConfig defaultRequestConfig = RequestConfig.custom()
-                                                                   .setConnectionRequestTimeout(Timeout.ofSeconds(5))
-                                                                   .setConnectTimeout(Timeout.ofSeconds(5))
-                                                                   .setRedirectsEnabled(false)
-                                                                   .build();
-
     private static final Logger log = Logger.getLogger(AbstractConfigServerClient.class.getName());
 
     /** Executes the request with the given context. The caller must close the response. */
-    protected abstract ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientContext context) throws IOException;
+    abstract ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientContext context) throws IOException;
 
     /** Executes the given request with response/error handling and retries. */
-    private <T> T execute(RequestBuilder builder, Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) {
+    private <T> T execute(RequestBuilder builder,
+                          BiFunction<ClassicHttpResponse, ClassicHttpRequest, T> handler,
+                          Consumer<IOException> catcher) {
         HttpClientContext context = HttpClientContext.create();
         context.setRequestConfig(builder.config);
 
@@ -62,10 +54,11 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
             request.setEntity(builder.entity);
             try {
                 try {
-                    return handler.apply(execute(request, context));
+                    return handler.apply(execute(request, context), request);
                 }
                 catch (IOException e) {
-                    return catcher.apply(e);
+                    catcher.accept(e);
+                    throw new UncheckedIOException(e); // Throw unchecked if catcher doesn't throw.
                 }
             }
             catch (RetryException e) {
@@ -90,7 +83,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
                 throw new IllegalStateException("Illegal retry cause: " + thrown.getClass(), thrown);
         }
 
-        throw new IllegalArgumentException("No hosts to perform the request against");
+        throw new IllegalStateException("No hosts to perform the request against");
     }
 
     /** Append path to the given host, which may already contain a root path. */
@@ -102,8 +95,8 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         pathSegments.addAll(pathAndQuery.getPathSegments());
         try {
             return builder.setPathSegments(pathSegments)
-                    .setParameters(pathAndQuery.getQueryParams())
-                    .build();
+                          .setParameters(pathAndQuery.getQueryParams())
+                          .build();
         }
         catch (URISyntaxException e) {
             throw new IllegalArgumentException("URISyntaxException should not be possible here", e);
@@ -122,7 +115,9 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         private final HostStrategy hosts;
         private final URIBuilder uriBuilder = new URIBuilder();
         private HttpEntity entity;
-        private RequestConfig config = defaultRequestConfig;
+        private RequestConfig config = ConfigServerClient.defaultRequestConfig;
+        private BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler = ConfigServerClient::throwOnError;
+        private Consumer<IOException> catcher = ConfigServerClient::retryAll;
 
         private RequestBuilder(HostStrategy hosts, Method method) {
             if ( ! hosts.iterator().hasNext())
@@ -170,17 +165,23 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         @Override
         public RequestBuilder config(RequestConfig config) {
             this.config = requireNonNull(config);
+            return this;
+        }
 
+        @Override
+        public RequestBuilder catching(Consumer<IOException> catcher) {
+            this.catcher = requireNonNull(catcher);
             return this;
         }
 
         @Override
-        public <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException {
-            return execute(this, requireNonNull(handler), requireNonNull(catcher));
+        public RequestBuilder handling(BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler) {
+            this.handler = requireNonNull(handler);
+            return this;
         }
 
         @Override
-        public <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ConfigServerException {
+        public <T> T read(Function<byte[], T> mapper) {
             return mapIfSuccess(input -> {
                 try (input) {
                     return mapper.apply(input.readAllBytes());
@@ -192,7 +193,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public void discard() throws UncheckedIOException, ConfigServerException {
+        public void discard() throws UncheckedIOException, ResponseException {
             mapIfSuccess(input -> {
                 try (input) {
                     return null;
@@ -204,44 +205,44 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         }
 
         @Override
-        public InputStream stream() throws UncheckedIOException, ConfigServerException {
+        public InputStream stream() throws UncheckedIOException, ResponseException {
             return mapIfSuccess(input -> input);
         }
 
         /** Returns the mapped body, if successful, retrying any IOException. The caller must close the body stream. */
         private <T> T mapIfSuccess(Function<InputStream, T> mapper) {
-            return handle(response -> {
-                            try {
-                                InputStream body = response.getEntity() != null ? response.getEntity().getContent()
-                                                                                : InputStream.nullInputStream();
-                                if (response.getCode() >= HttpStatus.SC_REDIRECTION)
-                                    throw readException(body.readAllBytes());
-
-                                return mapper.apply(new ForwardingInputStream(body) {
-                                    @Override
-                                    public void close() throws IOException {
-                                        super.close();
-                                        response.close();
-                                    }
-                                });
-                            }
-                            catch (IOException | RuntimeException | Error e) {
-                                try {
-                                    response.close();
-                                }
-                                catch (IOException f) {
-                                    e.addSuppressed(f);
-                                }
-                                if (e instanceof IOException)
-                                    throw new RetryException((IOException) e);
-                                else
-                                    sneakyThrow(e);
-                                throw new AssertionError("Should not happen");
-                            }
-                          },
-                          ioException -> {
-                              throw new RetryException(ioException);
-                          });
+            return execute(this,
+                           (response, request) -> {
+                               try {
+                                   handler.accept(response, request); // This throws on unacceptable responses.
+
+                                   InputStream body = response.getEntity() != null ? response.getEntity().getContent()
+                                                                                   : InputStream.nullInputStream();
+                                   return mapper.apply(new ForwardingInputStream(body) {
+                                       @Override
+                                       public void close() throws IOException {
+                                           super.close();
+                                           response.close();
+                                       }
+                                   });
+                               }
+                               catch (IOException | RuntimeException | Error e) {
+                                   try {
+                                       response.close();
+                                   }
+                                   catch (IOException f) {
+                                       e.addSuppressed(f);
+                                   }
+                                   if (e instanceof IOException) {
+                                       catcher.accept((IOException) e);
+                                       throw new UncheckedIOException((IOException) e);
+                                   }
+                                   else
+                                       sneakyThrow(e); // e is a runtime exception or an error, so this is fine.
+                                   throw new AssertionError("Should not happen");
+                               }
+                           },
+                           catcher);
         }
 
     }
@@ -251,14 +252,4 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient {
         throw (T) t;
     }
 
-    private static ConfigServerException readException(byte[] serialised) {
-        Inspector root = SlimeUtils.jsonToSlime(serialised).get();
-        String codeName = root.field("error-code").asString();
-        ConfigServerException.ErrorCode code = Stream.of(ConfigServerException.ErrorCode.values())
-                                                     .filter(value -> value.name().equals(codeName))
-                                                     .findAny().orElse(INCOMPLETE_RESPONSE);
-        String message = root.field("message").valid() ? root.field("message").asString() : "(no message)";
-        return new ConfigServerException(code, message, "");
-    }
-
 }
\ No newline at end of file
diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
index 61b7edea0cc..c0f72378c1b 100644
--- a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
+++ b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java
@@ -2,9 +2,14 @@
 package ai.vespa.hosted.client;
 
 import org.apache.hc.client5.http.config.RequestConfig;
+import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.HttpEntity;
+import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.ParseException;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.hc.core5.util.Timeout;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -15,7 +20,8 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.function.BiFunction;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.stream.IntStream;
 
@@ -27,6 +33,38 @@ import static java.util.stream.Collectors.toUnmodifiableList;
  */
 public interface ConfigServerClient extends AutoCloseable {
 
+    RequestConfig defaultRequestConfig = RequestConfig.custom()
+                                                      .setConnectionRequestTimeout(Timeout.ofSeconds(5))
+                                                      .setConnectTimeout(Timeout.ofSeconds(5))
+                                                      .setRedirectsEnabled(false)
+                                                      .build();
+
+    /** Wraps with a {@link RetryException} and rethrows. */
+    static void retryAll(IOException e) {
+        throw new RetryException(e);
+    }
+
+    /** Throws a a {@link RetryException} if {@code statusCode == 503}, or a {@link ResponseException} unless {@code 200 <= statusCode < 300}. */
+    static void throwOnError(ClassicHttpResponse response, ClassicHttpRequest request) {
+        if (response.getCode() < HttpStatus.SC_OK || response.getCode() >= HttpStatus.SC_REDIRECTION) {
+            ResponseException e = ResponseException.of(response, request);
+            if (response.getCode() == HttpStatus.SC_SERVICE_UNAVAILABLE)
+                throw new RetryException(e);
+
+            throw e;
+        }
+    }
+
+    /** Reads the response body, throwing an {@link UncheckedIOException} if this fails, or {@code null} if there is none. */
+    static byte[] getBytes(ClassicHttpResponse response) {
+        try {
+            return response.getEntity() == null ? null : EntityUtils.toByteArray(response.getEntity());
+        }
+        catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+    }
+
     /** Creates a builder for sending the given method, using the specified host strategy. */
     RequestBuilder send(HostStrategy hosts, Method method);
 
@@ -60,37 +98,27 @@ public interface ConfigServerClient extends AutoCloseable {
         RequestBuilder config(RequestConfig config);
 
         /**
-         * Sets custom retry/failure logic for this.
-         * <p>
-         * Exactly one of the callbacks will be invoked, with a non-null argument.
-         * Return a value to have that returned to the caller;
-         * throw a {@link RetryException} to have the request retried; or
-         * throw any other unchecked exception to have this propagate out to the caller.
-         * The caller must close the provided response, if any.
+         * Sets the catch clause for {@link IOException}s during execution of this.
+         * The default is to wrap the IOException in a {@link RetryException} and rethrow this;
+         * this makes the client retry the request, as long as there are remaining entries in the {@link HostStrategy}.
+         * If the catcher returns normally, the {@link IOException} is unchecked and thrown instead.
          */
-        <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException;
+         RequestBuilder catching(Consumer<IOException> catcher);
 
-        /** Sets the response body mapper for this, for successful requests. */
-        <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ConfigServerException;
+        /**
+         * Sets the (error) response handler for this request. The default is {@link #throwOnError}.
+         * When the handler returns normally, the response is treated as a success, and passed on to a response mapper.
+         */
+         RequestBuilder handling(BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler);
 
-        /** Discards the response, but throws if the response is unsuccessful. */
-        void discard() throws UncheckedIOException, ConfigServerException;
+        /** Reads and maps the response, or throws if unsuccessful. */
+        <T> T read(Function<byte[], T> mapper);
 
-        /** Returns the raw input stream of the response, if successful. The caller must close the returned stream. */
-        InputStream stream() throws UncheckedIOException, ConfigServerException;
+        /** Discards the response, but throws if unsuccessful. */
+        void discard();
 
-    }
-
-    /** Exception wrapper that signals retries should be attempted. */
-    final class RetryException extends RuntimeException {
-
-        public RetryException(IOException cause) {
-            super(requireNonNull(cause));
-        }
-
-        public RetryException(RuntimeException cause) {
-            super(requireNonNull(cause));
-        }
+        /** Returns the raw response input stream, or throws if unsuccessful. The caller must close the returned stream. */
+        InputStream stream();
 
     }
 
@@ -119,37 +147,38 @@ public interface ConfigServerClient extends AutoCloseable {
 
     }
 
-    /** An exception due to server error, a bad request, or similar. */
-    class ConfigServerException extends RuntimeException {
+    /** Exception wrapper that signals retries should be attempted. */
+    final class RetryException extends RuntimeException {
+
+        public RetryException(IOException cause) {
+            super(requireNonNull(cause));
+        }
+
+        public RetryException(RuntimeException cause) {
+            super(requireNonNull(cause));
+        }
+
+    }
 
-        private final ErrorCode errorId;
-        private final String message;
+    /** An exception due to server error, a bad request, or similar, which resulted in a non-OK HTTP response. */
+    class ResponseException extends RuntimeException {
 
-        public ConfigServerException(ErrorCode errorId, String message, String context) {
-            super(context + ": " + message);
-            this.errorId = errorId;
-            this.message = message;
+        public ResponseException(String message, Throwable cause) {
+            super(message, cause);
         }
 
-        public ErrorCode errorId() { return errorId; }
-
-        public String message() { return message; }
-
-        public enum ErrorCode {
-            APPLICATION_LOCK_FAILURE,
-            BAD_REQUEST,
-            ACTIVATION_CONFLICT,
-            INTERNAL_SERVER_ERROR,
-            INVALID_APPLICATION_PACKAGE,
-            METHOD_NOT_ALLOWED,
-            NOT_FOUND,
-            OUT_OF_CAPACITY,
-            REQUEST_TIMEOUT,
-            UNKNOWN_VESPA_VERSION,
-            PARENT_HOST_NOT_READY,
-            CERTIFICATE_NOT_READY,
-            LOAD_BALANCER_NOT_READY,
-            INCOMPLETE_RESPONSE
+        public static ResponseException of(ClassicHttpResponse response, ClassicHttpRequest request) {
+            String detail;
+            Throwable thrown = null;
+            try {
+                detail = request.getEntity() == null ? " and no body"
+                                                     : " and body '" + EntityUtils.toString(request.getEntity()) + "'";
+            }
+            catch (IOException | ParseException e) {
+                detail = ". Reading body failed";
+                thrown = e;
+            }
+            return new ResponseException(request + " failed with status " + response.getCode() + detail, thrown);
         }
 
     }
diff --git a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
index cbf38f46f6f..3b46501a4bf 100644
--- a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
+++ b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java
@@ -1,7 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.hosted.client;
 
-import ai.vespa.hosted.client.ConfigServerClient.ConfigServerException;
+import ai.vespa.hosted.client.ConfigServerClient.ResponseException;
 import ai.vespa.hosted.client.ConfigServerClient.HostStrategy;
 import com.github.tomakehurst.wiremock.http.Fault;
 import com.yahoo.vespa.athenz.api.AthenzService;
@@ -35,7 +35,7 @@ class HttpConfigServerClientTest {
     @RegisterExtension
     final WireMockExtension server = new WireMockExtension();
 
-    final HttpConfigServerClient client = new HttpConfigServerClient(List.of(new AthenzService("mydomain", "yourservice")), "user");
+    final ConfigServerClient client = new HttpConfigServerClient(List.of(new AthenzService("mydomain", "yourservice")), "user");
 
     @Test
     void testRetries() {
@@ -81,15 +81,14 @@ class HttpConfigServerClientTest {
         server.verify(1, anyRequestedFor(anyUrl()));
         server.resetRequests();
 
-        // ConfigServerException is not retried.
+        // ResponseException is not retried.
         server.stubFor(get("/"))
-              .setResponse(aResponse().withStatus(409).withBody("{\"error-code\":\"ACTIVATION_CONFLICT\",\"message\":\"hi\"}").build());
-        ConfigServerException thrown = assertThrows(ConfigServerException.class,
-                                                    () -> client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10),
-                                                                      Method.GET)
-                                                                .read(String::new));
-        assertEquals(ConfigServerException.ErrorCode.ACTIVATION_CONFLICT, thrown.errorId());
-        assertEquals("hi", thrown.message());
+              .setResponse(aResponse().withStatus(409).withBody("hi").build());
+        ResponseException thrown = assertThrows(ResponseException.class,
+                                                () -> client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10),
+                                                                  Method.GET)
+                                                            .read(String::new));
+        assertEquals("GET / failed with status 409 and no body", thrown.getMessage());
         server.verify(1, getRequestedFor(urlEqualTo("/")));
         server.verify(1, anyRequestedFor(anyUrl()));