This repository has been archived by the owner on Jul 23, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 27
Shell runner tasks #113
Merged
Merged
Shell runner tasks #113
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
96 changes: 96 additions & 0 deletions
96
parodos-model-api/src/main/java/com/redhat/parodos/workflow/task/shell/ShellRunnerTask.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
package com.redhat.parodos.workflow.task.shell; | ||
|
||
import com.redhat.parodos.workflow.task.BaseWorkFlowTask; | ||
import com.redhat.parodos.workflows.work.DefaultWorkReport; | ||
import com.redhat.parodos.workflows.work.WorkContext; | ||
import com.redhat.parodos.workflows.work.WorkReport; | ||
import com.redhat.parodos.workflows.work.WorkStatus; | ||
import lombok.extern.slf4j.Slf4j; | ||
|
||
import java.io.*; | ||
import java.nio.file.Files; | ||
import java.nio.file.attribute.PosixFilePermissions; | ||
import java.util.*; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Consumer; | ||
|
||
import static java.nio.file.attribute.PosixFilePermission.*; | ||
|
||
/** | ||
* ShellRunnerTask takes a script and an optional env and runs them to completion and | ||
* returns the exit code. For exit code 0 the return status should be successful and for | ||
* anything else it should be a failure. | ||
*/ | ||
@Slf4j | ||
public class ShellRunnerTask extends BaseWorkFlowTask { | ||
|
||
public static final int DEFAULT_EXECUTION_TIMEOUT_IN_SECONDS = 600; | ||
|
||
Consumer<String> outputConsumer = log::info; | ||
|
||
private static class ShellRunnerTaskParams { | ||
|
||
/** | ||
* cmdline to execute including arguments, must contain at least one string | ||
*/ | ||
ArrayList<String> cmdline; | ||
|
||
int timeoutInSeconds = DEFAULT_EXECUTION_TIMEOUT_IN_SECONDS; | ||
|
||
ShellRunnerTaskParams(WorkContext workContext) { | ||
String command = (String) workContext.get("command"); | ||
if (command == null || command.isBlank()) { | ||
throw new IllegalArgumentException("argument 'command' is empty or blank"); | ||
} | ||
this.cmdline = new ArrayList<>() { | ||
}; | ||
this.cmdline.add(command); | ||
if (workContext.get("args") != null) { | ||
this.cmdline.addAll(List.of(workContext.get("args").toString().split(" "))); | ||
} | ||
if (workContext.get("timeoutInSeconds") != null) { | ||
this.timeoutInSeconds = Integer.parseInt(workContext.get("timeoutInSeconds").toString()); | ||
} | ||
} | ||
|
||
} | ||
|
||
@Override | ||
public WorkReport execute(WorkContext workContext) { | ||
var params = new ShellRunnerTaskParams(workContext); | ||
var pb = new ProcessBuilder(params.cmdline); | ||
File tmpDir = null; | ||
try { | ||
tmpDir = Files | ||
.createTempDirectory("parodos-shelltask-runner", | ||
PosixFilePermissions.asFileAttribute(EnumSet.of(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE))) | ||
.toFile(); | ||
pb.directory(tmpDir); | ||
pb.redirectErrorStream(true); | ||
|
||
log.info("begin shell task invocation"); | ||
Process p = pb.start(); | ||
var elapsed = !p.waitFor(params.timeoutInSeconds, TimeUnit.SECONDS); | ||
if (elapsed) { | ||
// timeouts, assume incomplete because we can't read the exit code | ||
return new DefaultWorkReport(WorkStatus.FAILED, workContext); | ||
} | ||
// read output only if we finished waiting otherwise the thread | ||
// waits till the end of execution. | ||
try (var r = new BufferedReader(new InputStreamReader(p.getInputStream()))) { | ||
r.lines().forEach(outputConsumer); | ||
} | ||
log.info("ended shell task invocation"); | ||
return new DefaultWorkReport(p.exitValue() == 0 ? WorkStatus.COMPLETED : WorkStatus.FAILED, workContext); | ||
} | ||
catch (IOException | InterruptedException e) { | ||
throw new RuntimeException(e); | ||
} | ||
finally { | ||
if (tmpDir != null) { | ||
tmpDir.delete(); | ||
} | ||
} | ||
} | ||
|
||
} |
63 changes: 63 additions & 0 deletions
63
...s-model-api/src/test/java/com/redhat/parodos/workflow/task/shell/ShellRunnerTaskTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package com.redhat.parodos.workflow.task.shell; | ||
|
||
import com.redhat.parodos.workflows.work.WorkContext; | ||
import com.redhat.parodos.workflows.work.WorkStatus; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
|
||
import java.util.UUID; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
|
||
public class ShellRunnerTaskTest { | ||
|
||
ShellRunnerTask underTest; | ||
|
||
WorkContext ctx; | ||
|
||
@Before | ||
public void setUp() { | ||
underTest = new ShellRunnerTask(); | ||
ctx = new WorkContext(); | ||
} | ||
|
||
@Test | ||
public void executeWithOutput() { | ||
UUID randomUUID = UUID.randomUUID(); | ||
ctx.put("command", "echo"); | ||
ctx.put("args", randomUUID); | ||
var output = new StringBuilder(); | ||
underTest.outputConsumer = output::append; | ||
|
||
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.COMPLETED); | ||
assertThat(output.toString()).isEqualTo(randomUUID.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we assert that the temp folder is deleted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. following the comment above, we can count on the jvm to execute that. but for extra cation we can parametrize the call to pass in a temp dir ... wonder if its worth it. |
||
} | ||
|
||
@Test | ||
public void failsWhenTimeouts() { | ||
ctx.put("command", "sleep"); | ||
ctx.put("args", "5s"); | ||
ctx.put("timeoutInSeconds", "1"); | ||
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.FAILED); | ||
} | ||
|
||
@Test | ||
public void completesIfExitZero() { | ||
ctx.put("command", "true"); | ||
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.COMPLETED); | ||
} | ||
|
||
@Test | ||
public void failsIfExitNonZero() { | ||
ctx.put("command", "false"); | ||
assertThat(underTest.execute(ctx).getStatus()).isEqualTo(WorkStatus.FAILED); | ||
} | ||
|
||
@Test() | ||
public void failsOnMissingCommand() { | ||
ctx.put("command", ""); | ||
assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> underTest.execute(ctx)); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not a security issue? A user can submit a command that scales privilege, no? like sh - sudo rm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very very good point. The thing is that I really don't know what the deployment model is yet. For example, will the user the jvm runs under is in
sudoers
at all? could we actually ensure podman is part of the deployment and so run every execution part of a ubi9/micro image? (likepodman run ubi9/micro $cmd $args
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lshannon any input here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, and also, I would run at least in a separate shell, like sh 'command', if not can get the ENV variables and maybe connect to the database itself.
TBH, I would move this to "container shell command", and be able to specify the image and the command to execute, for me, it will be 10000 times safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to work on deployment story and we can make sure that ability to exploit this issue is limited or not possible. Let's discuss this issue while working on FLPATH-198.