Stop using report-cores-via-cfg

1 files changed, 35 insertions, 66 deletions

diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
index b096db2eaf4..1fd688558a0 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
@@ -1,12 +1,14 @@
 // 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.coredump;
 
+import com.yahoo.config.provision.DockerImage;
 import com.yahoo.security.KeyId;
 import com.yahoo.security.SealedSharedKey;
 import com.yahoo.security.SecretSharedKey;
 import com.yahoo.test.ManualClock;
 import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
+import com.yahoo.vespa.hosted.node.admin.configserver.cores.CoreDumpMetadata;
 import com.yahoo.vespa.hosted.node.admin.configserver.cores.Cores;
 import com.yahoo.vespa.hosted.node.admin.container.metrics.DimensionMetrics;
 import com.yahoo.vespa.hosted.node.admin.container.metrics.Metrics;
@@ -26,7 +28,6 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.FileTime;
 import java.time.Duration;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -36,9 +37,12 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
@@ -57,7 +61,6 @@ public class CoredumpHandlerTest {
 
     private final CoreCollector coreCollector = mock(CoreCollector.class);
     private final Cores cores = mock(Cores.class);
-    private final CoredumpReporter coredumpReporter = mock(CoredumpReporter.class);
     private final Metrics metrics = new Metrics();
     private final ManualClock clock = new ManualClock();
     @SuppressWarnings("unchecked")
@@ -65,7 +68,7 @@ public class CoredumpHandlerTest {
     private final SecretSharedKeySupplier secretSharedKeySupplier = mock(SecretSharedKeySupplier.class);
     private final InMemoryFlagSource flagSource = new InMemoryFlagSource();
     private final CoredumpHandler coredumpHandler =
-            new CoredumpHandler(coreCollector, cores, coredumpReporter, containerCrashPath.pathInContainer(),
+            new CoredumpHandler(coreCollector, cores, containerCrashPath.pathInContainer(),
                                 doneCoredumpsPath, metrics, clock, coredumpIdSupplier, secretSharedKeySupplier,
                                 flagSource);
 
@@ -148,31 +151,14 @@ public class CoredumpHandlerTest {
         verify(coredumpIdSupplier, times(1)).get();
     }
 
-    private static String buildExpectedMetadataString(Optional<String> decryptionToken) {
-        return "{\"fields\":{" +
-               "\"hostname\":\"host123.yahoo.com\"," +
-               "\"kernel_version\":\"3.10.0-862.9.1.el7.x86_64\"," +
-               "\"backtrace\":[\"call 1\",\"function 2\",\"something something\"]," +
-               "\"vespa_version\":\"6.48.4\"," +
-               "\"bin_path\":\"/bin/bash\"," +
-               "\"coredump_path\":\"/home/docker/dumps/container-123/id-123/dump_core.456\"," +
-               "\"docker_image\":\"vespa/ci:6.48.4\"" +
-               decryptionToken.map(",\"decryption_token\":\"%s\""::formatted).orElse("") +
-               "}}";
-    }
-
-    void do_get_metadata_test(Optional<String> oldDecryptionToken, Optional<String> newDecryptionToken) throws IOException {
-        Map<String, Object> metadata = new HashMap<>();
-        metadata.put("bin_path", "/bin/bash");
-        metadata.put("backtrace", List.of("call 1", "function 2", "something something"));
-
-        Map<String, Object> attributes = Map.of(
-                "hostname", "host123.yahoo.com",
-                "vespa_version", "6.48.4",
-                "kernel_version", "3.10.0-862.9.1.el7.x86_64",
-                "docker_image", "vespa/ci:6.48.4");
-
-        String expectedMetadataStr = buildExpectedMetadataString(oldDecryptionToken);
+    @Test
+    void gather_metadata_test() throws IOException {
+        var metadata = new CoreDumpMetadata().setKernelVersion("3.10.0-862.9.1.el7.x86_64")
+                                             .setBacktrace(List.of("call 1", "function 2", "something something"))
+                                             .setVespaVersion("6.48.4")
+                                             .setBinPath("/bin/bash")
+                                             .setCoreDumpPath(context.paths().of("/home/docker/dumps/container-123/id-123/dump_core.456"))
+                                             .setDockerImage(DockerImage.fromString("example.com/vespa/ci:6.48.4"));
 
         ContainerPath coredumpDirectory = context.paths().of("/var/crash/id-123");
         Files.createDirectories(coredumpDirectory.pathOnHost());
@@ -180,40 +166,20 @@ public class CoredumpHandlerTest {
         when(coreCollector.collect(eq(context), eq(coredumpDirectory.resolve("dump_core.456"))))
                 .thenReturn(metadata);
 
-        assertEquals(expectedMetadataStr, coredumpHandler.getMetadata(context, coredumpDirectory, () -> attributes, oldDecryptionToken));
+        assertEquals(metadata, coredumpHandler.gatherMetadata(context, coredumpDirectory));
         verify(coreCollector, times(1)).collect(any(), any());
 
-        // Calling it again will read the previously generated metadata from disk and selectively
-        // patch in an updated decryption token value, if one is provided.
-        // This avoids having to re-run a potentially expensive collector step.
-        expectedMetadataStr = buildExpectedMetadataString(newDecryptionToken);
-        assertEquals(expectedMetadataStr, coredumpHandler.getMetadata(context, coredumpDirectory, () -> attributes, newDecryptionToken));
+        // On second invocation the test already exist, so times(1) is not incremented
+        assertEquals(metadata, coredumpHandler.gatherMetadata(context, coredumpDirectory));
+        doThrow(new IllegalStateException("Should not be invoked"))
+                .when(coreCollector).collect(any(), any());
         verify(coreCollector, times(1)).collect(any(), any());
     }
 
     @Test
-    void get_metadata_test_without_encryption() throws IOException {
-        do_get_metadata_test(Optional.empty(), Optional.empty()); // No token in metadata
-    }
-
-    @Test
-    void get_metadata_test_with_encryption() throws IOException {
-        when(secretSharedKeySupplier.create(any())).thenReturn(Optional.of(makeFixedSecretSharedKey()));
-        do_get_metadata_test(Optional.of("AVeryCoolToken"), Optional.of("AnEvenCoolerToken"));
-    }
-
-    @Test
-    void get_metadata_test_without_encryption_then_with_encryption() throws IOException {
-        // Edge where encryption was enabled between attempted processing runs.
-        // We don't bother with testing the opposite edge case (encryption -> no encryption), since
-        // in that case the core dump itself won't be encrypted so the token will be a no-op.
-        do_get_metadata_test(Optional.empty(), Optional.of("TheSwaggestToken"));
-    }
-
-    @Test
     void cant_get_metadata_if_no_core_file() {
         assertThrows(IllegalStateException.class, () -> {
-            coredumpHandler.getMetadata(context, context.paths().of("/fake/path"), Map::of, Optional.empty());
+            coredumpHandler.gatherMetadata(context, context.paths().of("/fake/path"));
         });
     }
 
@@ -228,31 +194,34 @@ public class CoredumpHandlerTest {
         });
     }
 
-    void do_process_single_coredump_test(String expectedCoreFileName) throws IOException {
+    void do_process_single_coredump_test(String expectedCoreFileName, boolean encrypt) throws IOException {
         ContainerPath coredumpDirectory = context.paths().of("/path/to/coredump/proccessing/id-123");
         Files.createDirectories(coredumpDirectory);
-        Files.write(coredumpDirectory.resolve("metadata.json"), "{\"test-metadata\":{}}".getBytes());
+        Files.write(coredumpDirectory.resolve("metadata2.json"), "{\"test-metadata\":{}}".getBytes());
         Files.createFile(coredumpDirectory.resolve("dump_bash.core.431"));
-        assertFolderContents(coredumpDirectory, "metadata.json", "dump_bash.core.431");
+        assertFolderContents(coredumpDirectory, "metadata2.json", "dump_bash.core.431");
+        CoreDumpMetadata expectedMetadata = new CoreDumpMetadata();
+        if (encrypt)
+            expectedMetadata.setDecryptionToken("XWTN195KaZecNCloqRICq9YwxwWJEKGqyMYcTgHtwaZfnXtzHzd7bPrPPR60VdxxzA");
 
-        coredumpHandler.processAndReportSingleCoredump(context, coredumpDirectory, Map::of);
+        coredumpHandler.processAndReportSingleCoreDump(context, coredumpDirectory, Optional.empty());
         verify(coreCollector, never()).collect(any(), any());
-        verify(coredumpReporter, times(1)).reportCoredump(eq("id-123"), eq("{\"test-metadata\":{}}"));
+        verify(cores, times(1)).report(eq(context.hostname()), eq("id-123"), eq(expectedMetadata));
         assertFalse(Files.exists(coredumpDirectory));
         assertFolderContents(doneCoredumpsPath.resolve("container-123"), "id-123");
-        assertFolderContents(doneCoredumpsPath.resolve("container-123").resolve("id-123"), "metadata.json", expectedCoreFileName);
+        assertFolderContents(doneCoredumpsPath.resolve("container-123").resolve("id-123"), "metadata2.json", expectedCoreFileName);
     }
 
     @Test
     void process_single_coredump_test_without_encryption() throws IOException {
-        do_process_single_coredump_test("dump_bash.core.431.zst");
+        do_process_single_coredump_test("dump_bash.core.431.zst", false);
     }
 
     @Test
     void process_single_coredump_test_with_encryption() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), "bar-key");
         when(secretSharedKeySupplier.create(KeyId.ofString("bar-key"))).thenReturn(Optional.of(makeFixedSecretSharedKey()));
-        do_process_single_coredump_test("dump_bash.core.431.zst.enc");
+        do_process_single_coredump_test("dump_bash.core.431.zst.enc", true);
     }
 
     // TODO fail closed instead of open
@@ -260,7 +229,7 @@ public class CoredumpHandlerTest {
     void encryption_disabled_when_no_public_key_set_in_feature_flag() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), ""); // empty -> not set
         verify(secretSharedKeySupplier, never()).create(any());
-        do_process_single_coredump_test("dump_bash.core.431.zst"); // No .enc suffix; not encrypted
+        do_process_single_coredump_test("dump_bash.core.431.zst", false); // No .enc suffix; not encrypted
     }
 
     // TODO fail closed instead of open
@@ -268,7 +237,7 @@ public class CoredumpHandlerTest {
     void encryption_disabled_when_no_key_returned_for_key_id_specified_by_feature_flag() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), "baz-key");
         when(secretSharedKeySupplier.create(KeyId.ofString("baz-key"))).thenReturn(Optional.empty());
-        do_process_single_coredump_test("dump_bash.core.431.zst"); // No .enc suffix; not encrypted
+        do_process_single_coredump_test("dump_bash.core.431.zst", false); // No .enc suffix; not encrypted
     }
 
     @Test
@@ -278,7 +247,7 @@ public class CoredumpHandlerTest {
         Files.createFile(containerCrashPath.resolve("dump-2"));
         Files.createFile(containerCrashPath.resolve("hs_err_pid2.log"));
         Files.createDirectory(processingPath);
-        Files.createFile(processingPath.resolve("metadata.json"));
+        Files.createFile(processingPath.resolve("metadata2.json"));
         Files.createFile(processingPath.resolve("dump-3"));
 
         new UnixPath(doneCoredumpsPath.resolve("container-123").resolve("dump-3-folder").resolve("dump-3"))