Do not return HTML and 500 when unable to check convergence for a services

VESPA-6351
* Return 404 if we cannot check convergnce for a service
* Catch exceptions and make sure we always return Json
* Rewrite Json generation and make separate classes for responses

4 files changed, 132 insertions, 147 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index 4512042e857..3150b2c2cfb 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -149,14 +149,14 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return logServerLogGrabber.grabLog(application);
     }
 
-    public HttpResponse nodeConvergenceCheck(Tenant tenant, ApplicationId applicationId, String hostname, URI uri) {
+    public HttpResponse serviceConvergenceCheck(Tenant tenant, ApplicationId applicationId, String hostname, URI uri) {
         Application application = getApplication(tenant, applicationId);
-        return convergeChecker.nodeConvergenceCheck(application, hostname, uri);
+        return convergeChecker.serviceConvergenceCheck(application, hostname, uri);
     }
 
-    public HttpResponse listConfigConvergence(Tenant tenant, ApplicationId applicationId, URI uri) {
+    public HttpResponse serviceListToCheckForConfigConvergence(Tenant tenant, ApplicationId applicationId, URI uri) {
         Application application = getApplication(tenant, applicationId);
-        return convergeChecker.listConfigConvergence(application, uri);
+        return convergeChecker.serviceListToCheckForConfigConvergence(application, uri);
     }
 
     public Long getApplicationGeneration(Tenant tenant, ApplicationId applicationId) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceChecker.java
index 5027c00007f..f883796804c 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceChecker.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceChecker.java
@@ -5,12 +5,9 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.google.inject.Inject;
 import com.yahoo.cloud.config.ModelConfig;
 import com.yahoo.component.AbstractComponent;
-import com.yahoo.container.jdisc.HttpResponse;
-import com.yahoo.vespa.config.server.http.HttpConfigResponse;
+import com.yahoo.slime.Cursor;
+import com.yahoo.vespa.config.server.http.JSONResponse;
 import org.glassfish.jersey.client.proxy.WebResourceFactory;
-import org.json.JSONArray;
-import org.json.JSONException;
-import org.json.JSONObject;
 
 import javax.ws.rs.GET;
 import javax.ws.rs.Path;
@@ -19,15 +16,14 @@ import javax.ws.rs.client.Client;
 import javax.ws.rs.client.ClientBuilder;
 import javax.ws.rs.client.WebTarget;
 import java.io.IOException;
-import java.io.OutputStream;
 import java.net.URI;
-import java.nio.charset.StandardCharsets;
 import java.util.*;
 
 /**
  * Checks for convergence of config generation for a given application.
  *
  * @author lulf
+ * @author hmusum
  */
 public class ApplicationConvergenceChecker extends AbstractComponent {
     private static final String statePath = "/state/v1/";
@@ -54,78 +50,36 @@ public class ApplicationConvergenceChecker extends AbstractComponent {
         this.stateApiFactory = stateApiFactory;
     }
 
-    public HttpResponse listConfigConvergence(Application application, URI uri) {
-        final JSONObject answer = new JSONObject();
-        final JSONArray nodes = new JSONArray();
-        final ModelConfig config;
+    public ServiceListResponse serviceListToCheckForConfigConvergence(Application application, URI uri) {
+        List<Service> services = new ArrayList<>();
+        Long wantedGeneration = application.getApplicationGeneration();
         try {
-            config = application.getConfig(ModelConfig.class, "");
+            // Note: Uses latest config model version to get config
+            ModelConfig config = application.getConfig(ModelConfig.class, "");
+            config.hosts()
+                  .forEach(host -> host.services().stream()
+                                       .filter(service -> serviceTypes.contains(service.type()))
+                                       .forEach(service -> getStatePort(service).ifPresent(
+                                               port -> services.add(new Service(host.name(), port, service.type())))));
+            return new ServiceListResponse(200, services, uri, wantedGeneration);
         } catch (IOException e) {
-            throw new RuntimeException("failed on get config model", e);
-        }
-        config.hosts()
-              .forEach(host -> {
-                  host.services().stream()
-                      .filter(service -> serviceTypes.contains(service.type()))
-                      .forEach(service -> {
-                          Optional<Integer> statePort = getStatePort(service);
-                          if (statePort.isPresent()) {
-                              JSONObject hostNode = new JSONObject();
-                              try {
-                                  hostNode.put("host", host.name());
-                                  hostNode.put("port", statePort.get());
-                                  hostNode.put("url", uri.toString() + "/" + host.name() + ":" + statePort.get());
-                                  hostNode.put("type", service.type());
-
-                              } catch (JSONException e) {
-                                  throw new RuntimeException(e);
-                              }
-                              nodes.put(hostNode);
-                          }
-                      });
-              });
-        try {
-            answer.put("services", nodes);
-            JSONObject debug = new JSONObject();
-            debug.put("wantedVersion", application.getApplicationGeneration());
-            answer.put("debug", debug);
-            answer.put("url", uri.toString());
-            return new JsonHttpResponse(200, answer);
-        } catch (JSONException e) {
-            try {
-                answer.put("error", e.getMessage());
-            } catch (JSONException e1) {
-                throw new RuntimeException("Failed while creating error message ", e1);
-            }
-            return new JsonHttpResponse(500, answer);
+            return new ServiceListResponse(500, services, uri, wantedGeneration);
         }
     }
 
-    public HttpResponse nodeConvergenceCheck(Application application, String hostFromRequest, URI uri) {
-        JSONObject answer = new JSONObject();
-        JSONObject debug = new JSONObject();
+    public ServiceResponse serviceConvergenceCheck(Application application, String hostAndPortToCheck, URI uri) {
+        Long wantedGeneration = application.getApplicationGeneration();
+        if ( ! hostInApplication(application, hostAndPortToCheck))
+            return ServiceResponse.createHostNotFoundInAppResponse(uri, hostAndPortToCheck, wantedGeneration);
+
         try {
-            answer.put("url", uri);
-            debug.put("wantedGeneration", application.getApplicationGeneration());
-            debug.put("host", hostFromRequest);
-
-            if (!hostInApplication(application, hostFromRequest)) {
-                debug.put("problem", "Host:port (service) no longer part of application, refetch list of services.");
-                answer.put("debug", debug);
-                return new JsonHttpResponse(410, answer);
-            }
-            final long generation = getServiceGeneration(URI.create("http://" + hostFromRequest));
-            debug.put("currentGeneration", generation);
-            answer.put("debug", debug);
-            answer.put("converged", generation >= application.getApplicationGeneration());
-            return new JsonHttpResponse(200, answer);
-        } catch (JSONException | ProcessingException e) {
-            try {
-                answer.put("error", e.getMessage());
-            } catch (JSONException e1) {
-                throw new RuntimeException("Fail while creating error message ", e1);
-            }
-            return new JsonHttpResponse(500, answer);
+            long currentGeneration = getServiceGeneration(URI.create("http://" + hostAndPortToCheck));
+            boolean converged = currentGeneration >= wantedGeneration;
+            return ServiceResponse.createOkResponse(uri, hostAndPortToCheck, wantedGeneration, currentGeneration, converged);
+        } catch (ProcessingException e) { // e.g. if we cannot connect to the service to find generation
+            return ServiceResponse.createNotFoundResponse(uri, hostAndPortToCheck, wantedGeneration, e.getMessage());
+        } catch (Exception e) {
+            return ServiceResponse.createErrorResponse(uri, hostAndPortToCheck, wantedGeneration, e.getMessage());
         }
     }
 
@@ -187,23 +141,71 @@ public class ApplicationConvergenceChecker extends AbstractComponent {
         return false;
     }
 
-    private static class JsonHttpResponse extends HttpResponse {
+    static class ServiceListResponse extends JSONResponse {
+        final Cursor debug;
+
+        private ServiceListResponse(int status, List<Service> services, URI uri, Long wantedGeneration) {
+            super(status);
+            Cursor serviceArray = object.setArray("services");
+            for (Service s : services) {
+                Cursor service = serviceArray.addObject();
+                service.setString("host", s.hostname);
+                service.setLong("port", s.port);
+                service.setString("type", s.type);
+                service.setString("url", uri.toString() + "/" + s.hostname + ":" + s.port);
+            }
+            debug = object.setObject("debug");
+            object.setString("url", uri.toString());
+            debug.setLong("wantedGeneration", wantedGeneration);
+        }
+    }
+
+    static class ServiceResponse extends JSONResponse {
+        final Cursor debug;
 
-        private final JSONObject answer;
+        private ServiceResponse(int status, URI uri, String hostname, Long wantedGeneration) {
+            super(status);
+            debug = object.setObject("debug");
+            object.setString("url", uri.toString());
+            debug.setString("host", hostname);
+            debug.setLong("wantedGeneration", wantedGeneration);
+        }
+
+        static ServiceResponse createOkResponse(URI uri, String hostname, Long wantedGeneration, Long currentGeneration, boolean converged) {
+            ServiceResponse serviceResponse = new ServiceResponse(200, uri, hostname, wantedGeneration);
+            serviceResponse.object.setBool("converged", converged);
+            serviceResponse.debug.setLong("currentGeneration", currentGeneration);
+            return serviceResponse;
+        }
 
-        JsonHttpResponse(int returnCode, JSONObject answer) {
-            super(returnCode);
-            this.answer = answer;
+        static ServiceResponse createHostNotFoundInAppResponse(URI uri, String hostname, Long wantedGeneration) {
+            ServiceResponse serviceResponse = new ServiceResponse(410, uri, hostname, wantedGeneration);
+            serviceResponse.debug.setString("problem", "Host:port (service) no longer part of application, refetch list of services.");
+            return serviceResponse;
         }
 
-        @Override
-        public void render(OutputStream outputStream) throws IOException {
-            outputStream.write(answer.toString().getBytes(StandardCharsets.UTF_8));
+        static ServiceResponse createErrorResponse(URI uri, String hostname, Long wantedGeneration, String error) {
+            ServiceResponse serviceResponse = new ServiceResponse(500, uri, hostname, wantedGeneration);
+            serviceResponse.object.setString("error", error);
+            return serviceResponse;
         }
 
-        @Override
-        public String getContentType() {
-            return HttpConfigResponse.JSON_CONTENT_TYPE;
+        static ServiceResponse createNotFoundResponse(URI uri, String hostname, Long wantedGeneration, String error) {
+            ServiceResponse serviceResponse = new ServiceResponse(404, uri, hostname, wantedGeneration);
+            serviceResponse.object.setString("error", error);
+            return serviceResponse;
+        }
+    }
+
+    private static class Service {
+        private final String hostname;
+        private final int port;
+        private final String type;
+
+        private Service(String hostname, int port, String type) {
+            this.hostname = hostname;
+            this.port = port;
+            this.type = type;
         }
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
index c60c4e1f64f..2c11a7303e2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
@@ -25,7 +25,6 @@ import com.yahoo.vespa.config.server.http.NotFoundException;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
-import java.time.Duration;
 import java.util.concurrent.Executor;
 
 /**
@@ -34,11 +33,8 @@ import java.util.concurrent.Executor;
  * @author hmusum
  * @since 5.4
  */
-// TODO: Move business logic out of the http layer
 public class ApplicationHandler extends HttpHandler {
 
-    private static final String REQUEST_PROPERTY_TIMEOUT = "timeout";
-
     private final Zone zone;
     private final ApplicationRepository applicationRepository;
 
@@ -67,7 +63,7 @@ public class ApplicationHandler extends HttpHandler {
         Tenant tenant = verifyTenantAndApplication(applicationId);
 
         if (isServiceConvergeRequest(request)) {
-            return applicationRepository.nodeConvergenceCheck(tenant, applicationId, getHostFromRequest(request), request.getUri());
+            return applicationRepository.serviceConvergenceCheck(tenant, applicationId, getHostFromRequest(request), request.getUri());
         }
         if (isContentRequest(request)) {
             long sessionId = applicationRepository.getSessionIdForApplication(tenant, applicationId);
@@ -87,7 +83,7 @@ public class ApplicationHandler extends HttpHandler {
         }
 
         if (isServiceConvergeListRequest(request)) {
-            return applicationRepository.listConfigConvergence(tenant, applicationId, request.getUri());
+            return applicationRepository.serviceListToCheckForConfigConvergence(tenant, applicationId, request.getUri());
         }
         return new GetApplicationResponse(Response.Status.OK, applicationRepository.getApplicationGeneration(tenant, applicationId));
     }
@@ -144,14 +140,6 @@ public class ApplicationHandler extends HttpHandler {
         }
     }
 
-    private Duration durationFromRequestTimeout(HttpRequest request) {
-        long timeoutInSeconds = 60;
-        if (request.hasProperty(REQUEST_PROPERTY_TIMEOUT)) {
-            timeoutInSeconds = Long.parseLong(request.getProperty(REQUEST_PROPERTY_TIMEOUT));
-        }
-        return Duration.ofSeconds(timeoutInSeconds);
-    }
-
     // Note: Update src/main/resources/configserver-app/services.xml if you do any changes to the bindings
     private static BindingMatch<?> getBindingMatch(HttpRequest request) {
         return HttpConfigRequests.getBindingMatch(request,
@@ -209,14 +197,14 @@ public class ApplicationHandler extends HttpHandler {
     }
 
     private static class DeleteApplicationResponse extends JSONResponse {
-        public DeleteApplicationResponse(int status, ApplicationId applicationId) {
+        DeleteApplicationResponse(int status, ApplicationId applicationId) {
             super(status);
             object.setString("message", "Application '" + applicationId + "' deleted");
         }
     }
 
     private static class GetApplicationResponse extends JSONResponse {
-        public GetApplicationResponse(int status, long generation) {
+        GetApplicationResponse(int status, long generation) {
             super(status);
             object.setLong("generation", generation);
         }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceCheckerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceCheckerTest.java
index 3a634a57985..4a9d17a44c7 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceCheckerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationConvergenceCheckerTest.java
@@ -19,6 +19,7 @@ import com.yahoo.vespa.config.ConfigKey;
 import com.yahoo.vespa.config.ConfigPayload;
 import com.yahoo.vespa.config.buildergen.ConfigDefinition;
 import com.yahoo.vespa.config.server.ServerCache;
+import com.yahoo.vespa.config.server.http.SessionHandlerTest;
 import com.yahoo.vespa.config.server.monitoring.MetricUpdater;
 import org.junit.Before;
 import org.junit.Rule;
@@ -26,17 +27,16 @@ import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.xml.sax.SAXException;
 
-import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.net.URI;
-import java.nio.charset.StandardCharsets;
 import java.util.Collection;
 import java.util.Optional;
 import java.util.Set;
 
 import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
-import static org.junit.Assert.fail;
+import static com.yahoo.vespa.config.server.application.ApplicationConvergenceChecker.ServiceResponse;
 
 /**
  * @author lulf
@@ -61,46 +61,41 @@ public class ApplicationConvergenceCheckerTest {
     public void converge() throws IOException, SAXException {
         ApplicationConvergenceChecker checker = new ApplicationConvergenceChecker(
                 (client, serviceUri) -> () -> string2json("{\"config\":{\"generation\":3}}"));
-        final HttpResponse httpResponse = checker.listConfigConvergence(application, URI.create("http://foo:234/serviceconverge"));
-        assertThat(httpResponse.getStatus(), is(200));
-        assertJsonResponseEquals(httpResponse, "{\"services\":[" +
-                "{\"port\":1337,\"host\":\"localhost\"," +
-                "\"url\":\"http://foo:234/serviceconverge/localhost:1337\"," +
-                "\"type\":\"container\"}]," +
-                "\"debug\":{\"wantedVersion\":3}," +
-                "\"url\":\"http://foo:234/serviceconverge\"}");
-        final HttpResponse nodeHttpResponse = checker.nodeConvergenceCheck(application,
-                                                                           "localhost:1337",
-                                                                           URI.create("http://foo:234/serviceconverge/localhost:1337"));
-        assertThat(nodeHttpResponse.getStatus(), is(200));
-        assertJsonResponseEquals(nodeHttpResponse, "{" +
-                "\"converged\":true," +
-                "\"debug\":{\"wantedGeneration\":3," +
-                "\"currentGeneration\":3," +
-                "\"host\":\"localhost:1337\"}," +
-                "\"url\":\"http://foo:234/serviceconverge/localhost:1337\"}");
-        final HttpResponse hostMissingHttpResponse = checker.nodeConvergenceCheck(application,
-                                                                                  "notPresent:1337",
-                                                                                  URI.create("http://foo:234/serviceconverge/notPresent:1337"));
-        assertThat(hostMissingHttpResponse.getStatus(), is(410));
-        assertJsonResponseEquals(hostMissingHttpResponse, "{\"debug\":{" +
-                "\"problem\":\"Host:port (service) no longer part of application, refetch list of services.\"," +
-                "\"wantedGeneration\":3," +
-                "\"host\":\"notPresent:1337\"}," +
-                "\"url\":\"http://foo:234/serviceconverge/notPresent:1337\"}");
-    }
-
-    private void assertJsonResponseEquals(HttpResponse httpResponse, String expected) throws IOException {
-        ByteArrayOutputStream out = new ByteArrayOutputStream();
-        httpResponse.render(out);
-        String response = out.toString(StandardCharsets.UTF_8.name());
-        ObjectMapper mapper = new ObjectMapper();
-        JsonNode jsonResponse = mapper.readTree(response);
-        JsonNode jsonExpected = mapper.readTree(expected);
-        if (jsonExpected.equals(jsonResponse)) {
-            return;
-        }
-        fail("Not equal, response is '" + response + "' expected '"+ expected + "'");
+        HttpResponse serviceListResponse = checker.serviceListToCheckForConfigConvergence(application,
+                                                                                          URI.create("http://foo:234/serviceconverge"));
+        assertThat(serviceListResponse.getStatus(), is(200));
+        assertEquals("{\"services\":[" +
+                             "{\"host\":\"localhost\"," +
+                             "\"port\":1337," +
+                             "\"type\":\"container\"," +
+                             "\"url\":\"http://foo:234/serviceconverge/localhost:1337\"}]," +
+                             "\"debug\":{\"wantedGeneration\":3}," +
+                             "\"url\":\"http://foo:234/serviceconverge\"}",
+                     SessionHandlerTest.getRenderedString(serviceListResponse));
+
+        ServiceResponse serviceResponse = checker.serviceConvergenceCheck(application,
+                                                                          "localhost:1337",
+                                                                          URI.create("http://foo:234/serviceconverge/localhost:1337"));
+        assertThat(serviceResponse.getStatus(), is(200));
+        assertEquals("{" +
+                             "\"debug\":{" +
+                             "\"host\":\"localhost:1337\"," +
+                             "\"wantedGeneration\":3," +
+                             "\"currentGeneration\":3}," +
+                             "\"url\":\"http://foo:234/serviceconverge/localhost:1337\"," +
+                             "\"converged\":true}",
+                     SessionHandlerTest.getRenderedString(serviceResponse));
+
+        ServiceResponse hostMissingResponse = checker.serviceConvergenceCheck(application,
+                                                                              "notPresent:1337",
+                                                                              URI.create("http://foo:234/serviceconverge/notPresent:1337"));
+        assertThat(hostMissingResponse.getStatus(), is(410));
+        assertEquals("{\"debug\":{" +
+                             "\"host\":\"notPresent:1337\"," +
+                             "\"wantedGeneration\":3," +
+                             "\"problem\":\"Host:port (service) no longer part of application, refetch list of services.\"}," +
+                             "\"url\":\"http://foo:234/serviceconverge/notPresent:1337\"}",
+                     SessionHandlerTest.getRenderedString(hostMissingResponse));
     }
 
     private JsonNode string2json(String data) {