Fail service dump request gracefully if JSON is invalid

3 files changed, 52 insertions, 10 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/ServiceDumpReport.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/ServiceDumpReport.java
index 452f786301b..7ce287a83f1 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/ServiceDumpReport.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/ServiceDumpReport.java
@@ -89,10 +89,15 @@ class ServiceDumpReport extends BaseReport {
     }
 
     public static ServiceDumpReport createErrorReport(
-            ServiceDumpReport request, Instant startedAt, Instant failedAt, String message) {
+            ServiceDumpReport reqOrNull, Instant startedAt, Instant failedAt, String message) {
+        Long createdAt = reqOrNull != null ? reqOrNull.getCreatedMillisOrNull() : Long.valueOf(startedAt.toEpochMilli());
+        String configId = reqOrNull != null ? reqOrNull.configId() : "unknown";
+        Long expireAt = reqOrNull != null ? reqOrNull.expireAt() : null;
+        List<String> artifacts = reqOrNull != null ? reqOrNull.artifacts() : List.of();
+        DumpOptions dumpOptions = reqOrNull != null ? reqOrNull.dumpOptions() : null;
         return new ServiceDumpReport(
-                request.getCreatedMillisOrNull(), startedAt.toEpochMilli(), null, failedAt.toEpochMilli(), null,
-                request.configId(), request.expireAt(), message, request.artifacts(), request.dumpOptions());
+                createdAt, startedAt.toEpochMilli(), null, failedAt.toEpochMilli(), null,
+                configId, expireAt, message, artifacts, dumpOptions);
     }
 
     @JsonGetter(STARTED_AT_FIELD) public Long startedAt() { return startedAt; }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java
index d942e5c9e80..3a0cd412a2e 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java
@@ -17,6 +17,7 @@ import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 import com.yahoo.yolean.concurrent.Sleeper;
 
+import java.io.UncheckedIOException;
 import java.net.URI;
 import java.time.Clock;
 import java.time.Instant;
@@ -66,8 +67,14 @@ public class VespaServiceDumperImpl implements VespaServiceDumper {
 
         Instant startedAt = clock.instant();
         NodeSpec nodeSpec = context.node();
-        ServiceDumpReport request = nodeSpec.reports().getReport(ServiceDumpReport.REPORT_ID, ServiceDumpReport.class)
-                .orElse(null);
+        ServiceDumpReport request;
+        try {
+            request = nodeSpec.reports().getReport(ServiceDumpReport.REPORT_ID, ServiceDumpReport.class)
+                    .orElse(null);
+        } catch (IllegalArgumentException | UncheckedIOException e) {
+            handleFailure(context, null, startedAt, e, "Invalid JSON in service dump request");
+            return;
+        }
         if (request == null || request.isCompletedOrFailed()) {
             context.log(log, Level.FINE, "No service dump requested or dump already completed/failed");
             return;
@@ -114,7 +121,7 @@ public class VespaServiceDumperImpl implements VespaServiceDumper {
             uploadArtifacts(context, destination, producedArtifacts);
             storeReport(context, ServiceDumpReport.createSuccessReport(request, startedAt, clock.instant(), destination));
         } catch (Exception e) {
-            handleFailure(context, request, startedAt, e);
+            handleFailure(context, request, startedAt, e, e.getMessage());
         } finally {
             if (unixPathDirectory.exists()) {
                 context.log(log, Level.INFO, "Deleting directory '" + unixPathDirectory +"'.");
@@ -147,15 +154,16 @@ public class VespaServiceDumperImpl implements VespaServiceDumper {
                 : Instant.ofEpochMilli(request.expireAt());
     }
 
-    private void handleFailure(NodeAgentContext context, ServiceDumpReport request, Instant startedAt, Exception failure) {
+    private void handleFailure(NodeAgentContext context, ServiceDumpReport requestOrNull, Instant startedAt,
+                               Exception failure, String message) {
         context.log(log, Level.WARNING, failure.toString(), failure);
-        ServiceDumpReport report = ServiceDumpReport.createErrorReport(request, startedAt, clock.instant(), failure.toString());
+        ServiceDumpReport report = ServiceDumpReport.createErrorReport(requestOrNull, startedAt, clock.instant(), message);
         storeReport(context, report);
     }
 
-    private void handleFailure(NodeAgentContext context, ServiceDumpReport request, Instant startedAt, String message) {
+    private void handleFailure(NodeAgentContext context, ServiceDumpReport requestOrNull, Instant startedAt, String message) {
         context.log(log, Level.WARNING, message);
-        ServiceDumpReport report = ServiceDumpReport.createErrorReport(request, startedAt, clock.instant(), message);
+        ServiceDumpReport report = ServiceDumpReport.createErrorReport(requestOrNull, startedAt, clock.instant(), message);
         storeReport(context, report);
     }
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImplTest.java
index fcdef83e06f..081f0038e06 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImplTest.java
@@ -1,6 +1,9 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin.maintenance.servicedump;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.yahoo.test.ManualClock;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeState;
@@ -170,6 +173,32 @@ class VespaServiceDumperImplTest {
         assertSyncedFiles(context, syncClient, expectedUris);
     }
 
+    @Test
+    void fails_gracefully_on_invalid_request_json() {
+        // Setup mocks
+        ContainerOperations operations = mock(ContainerOperations.class);
+        SyncClient syncClient = createSyncClientMock();
+        NodeRepoMock nodeRepository = new NodeRepoMock();
+        ManualClock clock = new ManualClock(Instant.ofEpochMilli(1600001000000L));
+        JsonNodeFactory fac = new ObjectMapper().getNodeFactory();
+        ObjectNode invalidRequest = new ObjectNode(fac)
+                .set("dumpOptions", new ObjectNode(fac).put("duration", "invalidDurationDataType"));
+        NodeSpec spec = NodeSpec.Builder
+                .testSpec(HOSTNAME, NodeState.active)
+                .report(ServiceDumpReport.REPORT_ID, invalidRequest)
+                .build();
+        nodeRepository.updateNodeSpec(spec);
+        VespaServiceDumper reporter = new VespaServiceDumperImpl(
+                ArtifactProducers.createDefault(Sleeper.NOOP), operations, syncClient, nodeRepository, clock);
+        NodeAgentContextImpl context = NodeAgentContextImpl.builder(spec)
+                .fileSystem(fileSystem)
+                .build();
+        reporter.processServiceDumpRequest(context);
+        String expectedJson = "{\"createdMillis\":1600001000000,\"startedAt\":1600001000000,\"failedAt\":1600001000000," +
+                "\"configId\":\"unknown\",\"error\":\"Invalid JSON in service dump request\",\"artifacts\":[]}";
+        assertReportEquals(nodeRepository, expectedJson);
+    }
+
     private static NodeSpec createNodeSpecWithDumpRequest(NodeRepoMock repository, List<String> artifacts,
                                                           ServiceDumpReport.DumpOptions options) {
         ServiceDumpReport request = ServiceDumpReport.createRequestReport(