Merge pull request #20021 from vespa-engine/freva/exec-asv7.501.17

Require UnixUser to ContainerEngine::execute

8 files changed, 35 insertions, 100 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
index a3bce4c687e..cfa0452ebf9 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
@@ -6,6 +6,7 @@ import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.container.image.Image;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
+import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixUser;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
 import java.time.Duration;
@@ -40,8 +41,8 @@ public interface ContainerEngine {
     /** Returns the network interface used by container in given context */
     String networkInterface(NodeAgentContext context);
 
-    /** Execute command inside container as root. Ignores non-zero exit code */
-    CommandResult executeAsRoot(NodeAgentContext context, Duration timeout, String... command);
+    /** Execute command inside container as given user. Ignores non-zero exit code */
+    CommandResult execute(NodeAgentContext context, UnixUser user, Duration timeout, String... command);
 
     /** Execute command inside the container's network namespace. Throws on non-zero exit code */
     CommandResult executeInNetworkNamespace(NodeAgentContext context, String... command);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
index 3017773700a..8a66373c28b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
@@ -7,6 +7,7 @@ import com.yahoo.vespa.hosted.node.admin.container.image.ContainerImageDownloade
 import com.yahoo.vespa.hosted.node.admin.container.image.ContainerImagePruner;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
+import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixUser;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandLine;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
@@ -66,13 +67,13 @@ public class ContainerOperations {
     }
 
     /** Executes a command inside container identified by given context. Does NOT throw on non-zero exit code */
-    public CommandResult executeCommandInContainerAsRoot(NodeAgentContext context, String... command) {
-        return executeCommandInContainerAsRoot(context, CommandLine.DEFAULT_TIMEOUT.toSeconds(), command);
+    public CommandResult executeCommandInContainer(NodeAgentContext context, UnixUser user, String... command) {
+        return executeCommandInContainer(context, user, CommandLine.DEFAULT_TIMEOUT, command);
     }
 
     /** Execute command inside container identified by given context. Does NOT throw on non-zero exit code */
-    public CommandResult executeCommandInContainerAsRoot(NodeAgentContext context, Long timeoutSeconds, String... command) {
-        return containerEngine.executeAsRoot(context, Duration.ofSeconds(timeoutSeconds), command);
+    public CommandResult executeCommandInContainer(NodeAgentContext context, UnixUser user, Duration timeout, String... command) {
+        return containerEngine.execute(context, user, timeout, command);
     }
 
     /** Execute command in inside containers network namespace, identified by given context. Throws on non-zero exit code */
@@ -142,7 +143,7 @@ public class ContainerOperations {
 
     private String executeNodeCtlInContainer(NodeAgentContext context, String program) {
         String[] command = new String[] {context.paths().underVespaHome("bin/vespa-nodectl").pathInContainer(), program};
-        return executeCommandInContainerAsRoot(context, command).getOutput();
+        return executeCommandInContainer(context, context.users().vespa(), command).getOutput();
     }
 
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java
index 60330984f57..60435082745 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java
@@ -34,10 +34,10 @@ public class CoreCollector {
     static final Map<String, Object> JAVA_HEAP_DUMP_METADATA =
             Map.of("bin_path", "java", "backtrace", List.of("Heap dump, no backtrace available"));
 
-    private final ContainerOperations docker;
+    private final ContainerOperations container;
 
-    public CoreCollector(ContainerOperations docker) {
-        this.docker = docker;
+    public CoreCollector(ContainerOperations container) {
+        this.container = container;
     }
 
     String getGdbPath(NodeAgentContext context) {
@@ -47,7 +47,7 @@ public class CoreCollector {
     String readBinPathFallback(NodeAgentContext context, ContainerPath coredumpPath) {
         String command = getGdbPath(context) + " -n -batch -core " + coredumpPath.pathInContainer() + " | grep \'^Core was generated by\'";
         String[] wrappedCommand = {"/bin/sh", "-c", command};
-        CommandResult result = docker.executeCommandInContainerAsRoot(context, wrappedCommand);
+        CommandResult result = container.executeCommandInContainer(context, context.users().root(), wrappedCommand);
 
         Matcher matcher = CORE_GENERATOR_PATH_PATTERN.matcher(result.getOutput());
         if (! matcher.find()) {
@@ -60,7 +60,7 @@ public class CoreCollector {
     String readBinPath(NodeAgentContext context, ContainerPath coredumpPath) {
         String[] command = {"file", coredumpPath.pathInContainer()};
         try {
-            CommandResult result = docker.executeCommandInContainerAsRoot(context, command);
+            CommandResult result = container.executeCommandInContainer(context, context.users().root(), command);
             if (result.getExitCode() != 0) {
                 throw new ConvergenceException("file command failed with " + asString(result));
             }
@@ -86,7 +86,7 @@ public class CoreCollector {
         String threads = allThreads ? "thread apply all bt" : "bt";
         String[] command = {getGdbPath(context), "-n", "-ex", threads, "-batch", binPath, coredumpPath.pathInContainer()};
 
-        CommandResult result = docker.executeCommandInContainerAsRoot(context, command);
+        CommandResult result = container.executeCommandInContainer(context, context.users().root(), command);
         if (result.getExitCode() != 0)
             throw new ConvergenceException("Failed to read backtrace " + asString(result) + ", Command: " + Arrays.toString(command));
 
@@ -96,7 +96,7 @@ public class CoreCollector {
     List<String> readJstack(NodeAgentContext context, ContainerPath coredumpPath, String binPath) {
         String[] command = {"jhsdb", "jstack", "--exe", binPath, "--core", coredumpPath.pathInContainer()};
 
-        CommandResult result = docker.executeCommandInContainerAsRoot(context, command);
+        CommandResult result = container.executeCommandInContainer(context, context.users().root(), command);
         if (result.getExitCode() != 0)
             throw new ConvergenceException("Failed to read jstack " + asString(result) + ", Command: " + Arrays.toString(command));
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/AbstractProducer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/AbstractProducer.java
deleted file mode 100644
index a1416d3274c..00000000000
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/AbstractProducer.java
+++ /dev/null
@@ -1,68 +0,0 @@
-// 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.yahoo.vespa.hosted.node.admin.container.ContainerOperations;
-import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
-
-import java.io.IOException;
-import java.util.List;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-import java.util.stream.Collectors;
-
-/**
- * @author bjorncs
- */
-abstract class AbstractProducer implements ArtifactProducer {
-
-    private final Logger log = Logger.getLogger(getClass().getName());
-
-    private final ContainerOperations container;
-
-    protected AbstractProducer(ContainerOperations container) { this.container = container; }
-
-    protected ContainerOperations container() { return container; }
-
-    protected CommandResult executeCommand(NodeAgentContext ctx, List<String> command, boolean logOutput) throws IOException {
-        CommandResult result = container.executeCommandInContainerAsRoot(ctx, command.toArray(new String[0]));
-        String cmdString = command.stream().map(s -> "'" + s + "'").collect(Collectors.joining(" ", "\"", "\""));
-        int exitCode = result.getExitCode();
-        String output = result.getOutput().trim();
-        String prefixedOutput = output.contains("\n")
-                ? "\n" + output
-                : (output.isEmpty() ? "<no output>" : output);
-        if (exitCode > 0) {
-            String errorMsg = logOutput
-                    ? String.format("Failed to execute %s (exited with code %d): %s", cmdString, exitCode, prefixedOutput)
-                    : String.format("Failed to execute %s (exited with code %d)", cmdString, exitCode);
-            throw new IOException(errorMsg);
-        } else {
-            String logMsg = logOutput
-                    ? String.format("Executed command %s. Exited with code %d and output: %s", cmdString, exitCode, prefixedOutput)
-                    : String.format("Executed command %s. Exited with code %d.", cmdString, exitCode);
-            ctx.log(log, logMsg);
-        }
-        return result;
-    }
-
-    protected int findVespaServicePid(NodeAgentContext ctx, String configId) throws IOException {
-        ContainerPath findPidBinary = ctx.paths().underVespaHome("libexec/vespa/find-pid");
-        CommandResult findPidResult = executeCommand(ctx, List.of(findPidBinary.pathInContainer(), configId), true);
-        return Integer.parseInt(findPidResult.getOutput());
-    }
-
-    protected double duration(NodeAgentContext ctx, ServiceDumpReport.DumpOptions options, double defaultValue) {
-        double duration = options != null && options.duration() != null && options.duration() > 0
-                ? options.duration() : defaultValue;
-        double maxDuration = 300;
-        if (duration > maxDuration) {
-            ctx.log(log, Level.WARNING,
-                    String.format("Specified duration %.3fs longer than max allowed (%.3fs)", duration, maxDuration));
-            return maxDuration;
-        }
-        return duration;
-    }
-
-}
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 86dc1ed983d..b30b8e22fc5 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
@@ -201,7 +201,7 @@ public class VespaServiceDumperImpl implements VespaServiceDumper {
 
         @Override
         public CommandResult executeCommandInNode(List<String> command, boolean logOutput) {
-            CommandResult result = container.executeCommandInContainerAsRoot(nodeAgentCtx, command.toArray(new String[0]));
+            CommandResult result = container.executeCommandInContainer(nodeAgentCtx, nodeAgentCtx.users().vespa(), command.toArray(new String[0]));
             String cmdString = command.stream().map(s -> "'" + s + "'").collect(Collectors.joining(" ", "\"", "\""));
             int exitCode = result.getExitCode();
             String output = result.getOutput().trim();
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
index 3eab24a7a66..25cdff4b726 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
@@ -6,6 +6,7 @@ import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.container.image.Image;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
+import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixUser;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
 import java.time.Duration;
@@ -109,7 +110,7 @@ public class ContainerEngineMock implements ContainerEngine {
     }
 
     @Override
-    public CommandResult executeAsRoot(NodeAgentContext context, Duration timeout, String... command) {
+    public CommandResult execute(NodeAgentContext context, UnixUser user, Duration timeout, String... command) {
         return new CommandResult(null, 0, "");
     }
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java
index 413fabb7880..8ab6bce2b8c 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java
@@ -177,7 +177,7 @@ public class CoreCollectorTest {
     }
 
     private void mockExec(NodeAgentContext context, String[] cmd, String output, String error) {
-        when(docker.executeCommandInContainerAsRoot(context, cmd))
+        when(docker.executeCommandInContainer(context, context.users().root(), cmd))
                 .thenReturn(new CommandResult(null, error.isEmpty() ? 0 : 1, error.isEmpty() ? output : error));
     }
 }
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 68127231554..452efecefe1 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,7 +1,6 @@
 // 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.yahoo.yolean.concurrent.Sleeper;
 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;
@@ -12,6 +11,7 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.sync.SyncFileInfo;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 import com.yahoo.vespa.test.file.TestFileSystem;
+import com.yahoo.yolean.concurrent.Sleeper;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
@@ -62,7 +62,7 @@ class VespaServiceDumperImplTest {
     void invokes_perf_commands_when_generating_perf_report() {
         // Setup mocks
         ContainerOperations operations = mock(ContainerOperations.class);
-        when(operations.executeCommandInContainerAsRoot(any(), any()))
+        when(operations.executeCommandInContainer(any(), any(), any()))
                 .thenReturn(new CommandResult(null, 0, "12345"))
                 .thenReturn(new CommandResult(null, 0, ""))
                 .thenReturn(new CommandResult(null, 0, ""));
@@ -78,13 +78,13 @@ class VespaServiceDumperImplTest {
                 .build();
         reporter.processServiceDumpRequest(context);
 
-        verify(operations).executeCommandInContainerAsRoot(
-                context, "/opt/vespa/libexec/vespa/find-pid", "default/container.1");
-        verify(operations).executeCommandInContainerAsRoot(
-                context, "perf", "record", "-g", "--output=/opt/vespa/tmp/vespa-service-dump/perf-record.bin",
+        verify(operations).executeCommandInContainer(
+                context, context.users().vespa(), "/opt/vespa/libexec/vespa/find-pid", "default/container.1");
+        verify(operations).executeCommandInContainer(
+                context, context.users().vespa(), "perf", "record", "-g", "--output=/opt/vespa/tmp/vespa-service-dump/perf-record.bin",
                 "--pid=12345", "sleep", "45");
-        verify(operations).executeCommandInContainerAsRoot(
-                context, "bash", "-c", "perf report --input=/opt/vespa/tmp/vespa-service-dump/perf-record.bin" +
+        verify(operations).executeCommandInContainer(
+                context, context.users().vespa(), "bash", "-c", "perf report --input=/opt/vespa/tmp/vespa-service-dump/perf-record.bin" +
                         " > /opt/vespa/tmp/vespa-service-dump/perf-report.txt");
 
         String expectedJson = "{\"createdMillis\":1600000000000,\"startedAt\":1600001000000,\"completedAt\":1600001000000," +
@@ -103,7 +103,7 @@ class VespaServiceDumperImplTest {
     void invokes_jcmd_commands_when_creating_jfr_recording() {
         // Setup mocks
         ContainerOperations operations = mock(ContainerOperations.class);
-        when(operations.executeCommandInContainerAsRoot(any(), any()))
+        when(operations.executeCommandInContainer(any(), any(), any()))
                 .thenReturn(new CommandResult(null, 0, "12345"))
                 .thenReturn(new CommandResult(null, 0, "ok"))
                 .thenReturn(new CommandResult(null, 0, "name=host-admin success"));
@@ -120,12 +120,12 @@ class VespaServiceDumperImplTest {
                 .build();
         reporter.processServiceDumpRequest(context);
 
-        verify(operations).executeCommandInContainerAsRoot(
-                context, "/opt/vespa/libexec/vespa/find-pid", "default/container.1");
-        verify(operations).executeCommandInContainerAsRoot(
-                context, "jcmd", "12345", "JFR.start", "name=host-admin", "path-to-gc-roots=true", "settings=profile",
+        verify(operations).executeCommandInContainer(
+                context, context.users().vespa(), "/opt/vespa/libexec/vespa/find-pid", "default/container.1");
+        verify(operations).executeCommandInContainer(
+                context, context.users().vespa(), "jcmd", "12345", "JFR.start", "name=host-admin", "path-to-gc-roots=true", "settings=profile",
                 "filename=/opt/vespa/tmp/vespa-service-dump/recording.jfr", "duration=30s");
-        verify(operations).executeCommandInContainerAsRoot(context, "jcmd", "12345", "JFR.check", "name=host-admin");
+        verify(operations).executeCommandInContainer(context, context.users().vespa(), "jcmd", "12345", "JFR.check", "name=host-admin");
 
         String expectedJson = "{\"createdMillis\":1600000000000,\"startedAt\":1600001000000," +
                 "\"completedAt\":1600001000000," +
@@ -142,7 +142,7 @@ class VespaServiceDumperImplTest {
     void handles_multiple_artifact_types() {
         // Setup mocks
         ContainerOperations operations = mock(ContainerOperations.class);
-        when(operations.executeCommandInContainerAsRoot(any(), any()))
+        when(operations.executeCommandInContainer(any(), any(), any()))
                 // For perf report:
                 .thenReturn(new CommandResult(null, 0, "12345"))
                 .thenReturn(new CommandResult(null, 0, ""))