Feature: Plugin for podman support#1024
Open
WillButAgain wants to merge 2 commits intoProject-MONAI:developfrom
Open
Feature: Plugin for podman support#1024WillButAgain wants to merge 2 commits intoProject-MONAI:developfrom
WillButAgain wants to merge 2 commits intoProject-MONAI:developfrom
Conversation
WalkthroughAdds a Podman task plugin and tests: new Podman project, container creation via libpod socket, lifecycle monitoring, artifact handling/uploading, DI registrations, logging, exceptions, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Workflow Client
participant Plugin as PodmanPlugin
participant Factory as IPodmanClientFactory
participant Creator as IPodmanContainerCreator
participant Docker as Podman/Docker API
participant Storage as Storage Service
Client->>Plugin: ExecuteTask(TaskDispatchEvent)
Plugin->>Storage: Download input artifacts
Storage-->>Plugin: Artifacts available
Plugin->>Factory: CreateClient(podmanEndpoint)
Factory->>Docker: Initialize client
Docker-->>Factory: Client instance
Plugin->>Creator: CreateContainerAsync(spec)
Creator->>Docker: POST /libpod/containers/create
Docker-->>Creator: ContainerId
Plugin->>Docker: Start container
Docker-->>Plugin: Container started
Plugin-->>Client: Return Accepted (monitor may run)
sequenceDiagram
participant Monitor as ContainerStatusMonitor
participant Docker as Podman/Docker API
participant FileSystem as File System
participant Storage as Storage Service
participant Broker as Message Broker
Monitor->>Docker: InspectContainer(containerId)
loop poll until finished or timeout
Docker-->>Monitor: Container state
Monitor->>Monitor: IsContainerCompleted?
end
Monitor->>FileSystem: Enumerate output files
FileSystem-->>Monitor: File list
loop for each file
Monitor->>Storage: Upload file
Storage-->>Monitor: Upload confirmed
end
Monitor->>Broker: Publish TaskCallbackEvent
Broker-->>Monitor: Ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
WillButAgain
force-pushed
the
feat/add-podman-support
branch
from
7273860 to
f7c40bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
src/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cs (1)
68-68: Consider documenting or making the Podman API version configurable.The API version
v4.0.0is hardcoded. Consider adding a comment noting the minimum supported Podman version, or making this configurable for environments with different Podman versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cs` at line 68, The hardcoded Podman API version in the POST URL inside the container creation call should be made configurable or documented; update the code in the method that calls httpClient.PostAsync (in IPodmanContainerCreator implementation where "/v4.0.0/libpod/containers/create" is used) to pull the API prefix from a configurable setting (e.g., constructor-injected podmanApiVersion or options) and fall back to a sensible default, or add an inline comment stating the minimum supported Podman version if you choose not to make it configurable; ensure the identifier (podmanApiVersion / PodmanOptions) is used to build the request path consistently across methods that call the Podman HTTP API.src/TaskManager/Plug-ins/Podman/Keys.cs (1)
21-24: Update documentation comments to reference Podman instead of Docker.Several XML comments reference "Docker" but this is the Podman plugin:
- Line 22: "Docker server" → "Podman server"
- Line 37: "docker run" → "podman run"
- Line 82: "Docker workflow" → "Podman workflow"
📝 Proposed documentation fixes
/// <summary> - /// Key for the endpoint where the Docker server is running. + /// Key for the endpoint where the Podman server is running. /// </summary> public static readonly string BaseUrl = "server_url"; /// <summary> - /// Key for specifying the user to the container. Same as -u argument for docker run. + /// Key for specifying the user to the container. Same as -u argument for podman run. /// </summary> public static readonly string User = "user"; /// <summary> - /// Required arguments to run the Docker workflow. + /// Required arguments to run the Podman workflow. /// </summary>Also applies to: 36-39, 82-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TaskManager/Plug-ins/Podman/Keys.cs` around lines 21 - 24, The XML documentation in the Podman plugin contains Docker-specific wording; update the comments in the Keys class (e.g., the BaseUrl field and other XML comments around lines referencing "docker run" and "Docker workflow") to say "Podman" instead—replace "Docker server" with "Podman server", "docker run" with "podman run", and "Docker workflow" with "Podman workflow" so the comments accurately describe the Podman plugin and its keys.src/TaskManager/Plug-ins/Podman/Logging/Log.cs (1)
41-48: Consider renamingDocker*methods toPodman*for consistency.Methods like
DockerInputMapped,DockerOutputMapped,DockerEnvironmentVariableAdded, andDockerIntermediateVolumeMappeduse "Docker" prefix in a Podman plugin. While functionally correct, this may cause confusion during maintenance.Also applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TaskManager/Plug-ins/Podman/Logging/Log.cs` around lines 41 - 48, Rename the logging methods to use the Podman prefix instead of Docker to match the plugin context: change DockerInputMapped to PodmanInputMapped, DockerOutputMapped to PodmanOutputMapped, DockerEnvironmentVariableAdded to PodmanEnvironmentVariableAdded, and DockerIntermediateVolumeMapped to PodmanIntermediateVolumeMapped (keep the LoggerMessage attributes and EventId/Message text unless message wording must change). Update all call sites and any unit tests or references to these symbols to the new method names so compilation succeeds.src/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cs (1)
17-17: Unused import:Ardalis.GuardClausesis not used.The code uses
ArgumentNullException.ThrowIfNullandThrowIfNullOrWhiteSpaceinstead of Ardalis guard clauses.🧹 Proposed fix
-using Ardalis.GuardClauses; - namespace Monai.Deploy.WorkflowManager.TaskManager.Podman🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cs` at line 17, Remove the unused Ardalis.GuardClauses using directive from ContainerVolumeMount.cs: delete the line "using Ardalis.GuardClauses;" and ensure no other code in the ContainerVolumeMount class or related methods (e.g., constructors or validation helpers) reference Ardalis guard methods; rely on the existing ArgumentNullException.ThrowIfNull/ThrowIfNullOrWhiteSpace calls as currently implemented.tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs (2)
195-240: This test doesn't observe the background failure it describes.
Startthrows from a fire-and-forget task, so the exception is off the awaited code path and these assertions still pass even ifStartis never invoked. Add aVerifyorTaskCompletionSourceso the monitor invocation is observed deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs` around lines 195 - 240, The test ExerciseTask_WhenFailedToMonitorContainer_ExpectTaskToBeAccepted does not observe the background exception because ContainerStatusMonitor.Start is invoked fire-and-forget; update the test to deterministically observe the monitor call by either (a) replacing the current _containerStatusMonitor.Setup(...).ThrowsAsync(...) with a setup that signals a TaskCompletionSource when Start is invoked and then throws inside a Task returned from Start so the test can await the TCS and assert the invocation, or (b) add a Verify on _containerStatusMonitor.Verify(m => m.Start(...), Times.Once()) after ExecuteTask to ensure Start was called; reference the mocked member _containerStatusMonitor and the Start method and coordinate with a TaskCompletionSource in the test to ensure the background failure is observed before asserting result.
376-495: Add a non-zeroexited/deadregression here.The
GetStatussuite covers paused, OOM-killed, and successful exit, but notStatus=exited|deadwith a non-zero exit code. That missing case is exactly what lets the current implementation report failed containers asSucceeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs` around lines 376 - 495, Add a new unit test in the PodmanPlugin GetStatus suite that simulates a container with State.Status set to exited (or dead) and a non-zero ExitCode to cover the regression; create a test (e.g., GetStatus_WhenContainerIsExitedWithNonZeroExitCode_ExpectFailureStatus) that setups _podmanClient.Containers.InspectContainerAsync to return ContainerInspectResponse with State.Status = Strings.DockerStatusExited (or "dead") and ExitCode = 100, instantiate PodmanPlugin and call GetStatus("identity", ...), then assert result.Status == TaskExecutionStatus.Failed, result.FailureReason == FailureReason.ExternalServiceError (or the same code path used for non-zero exits in PodmanPlugin), result.Errors contains "Exit code=100", and verify InspectContainerAsync was called for "identity".tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs (1)
99-125: Add a transient-failure regression for the completion path.These tests only exercise the no-artifact and all-success cases. A case where
PutObjectAsyncorPublishfails after the container reachesexitedwould catch duplicate uploads/callbacks and callbacks sent with missing artifacts.Also applies to: 127-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs` around lines 99 - 125, Add a new unit test covering a transient-failure regression for the completion path: simulate ContainerStatusMonitor.Start receiving an exited container (use the same ContainerInspectResponse setup) but make IStorageService.PutObjectAsync and/or IMessageBrokerPublisherService.Publish throw on the first call and succeed on a retry, then assert that artifacts are uploaded exactly once and the final callback is published exactly once; reference the existing test Start_WhenCalledWithoutAnyArtifacts_ExpectToSendCallbackEvent for structure and the methods PutObjectAsync and Publish to locate the mocks and the Start method invocation on ContainerStatusMonitor for where to introduce the failure-and-retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs`:
- Around line 91-107: The polling loop currently calls UploadOutputArtifacts and
SendCallbackMessage inside the try so any exception re-enters the retry loop and
can cause duplicate side effects; change the logic in the method that uses
dockerClient.Containers.InpectContainerAsync and IsContainerCompleted so that
when IsContainerCompleted(response.State) is true you set a local flag (e.g.,
completed = true) and break out of the polling loop (do not call
UploadOutputArtifacts or SendCallbackMessage there), then after the loop
(outside the retry/inspect try/catch/finally) invoke
UploadOutputArtifacts(intermediateVolumeMount, outputVolumeMounts,
cancellationToken) and SendCallbackMessage(taskDispatchEvent, containerId) once,
each wrapped with their own appropriate try/catch/error logging so failures do
not cause the poll loop to re-run and produce duplicates.
- Around line 175-177: The catch block in ContainerStatusMonitor that calls
_logger.ErrorUploadingFile(file, ex) is swallowing upload failures so Start can
still publish callbacks with missing artifacts; after logging, rethrow or
propagate the exception (e.g., throw; or throw a new exception that wraps ex) so
the failure prevents Start from publishing the callback, or alternatively set
and check a failure flag that prevents PublishCallback from running — update the
catch that contains _logger.ErrorUploadingFile(file, ex) to either rethrow the
exception or mark the upload as failed and ensure Start checks that flag before
publishing.
- Around line 164-165: The object key assembly is unsafe: using
file.Replace(artifactsPath, ...) and _fileSystem.Path.Combine produces wrong
results (prefix collisions, rooted paths dropping the prefix, and OS-specific
backslashes). Change the logic that sets objectName (the code referencing file,
artifactsPath and destination.RelativeRootPath and calling
_fileSystem.Path.Combine) to first ensure you only strip a leading artifactsPath
prefix (e.g., check file.StartsWith(artifactsPath) and remove exactly that
prefix) then normalize the remaining segment by trimming any leading slashes,
and finally join destination.RelativeRootPath and the normalized segment using
explicit forward-slash concatenation (not Path.Combine) so the resulting object
key always uses '/' separators; apply the same fix to the Docker variant where
the same pattern appears.
In `@src/TaskManager/Plug-ins/Podman/Keys.cs`:
- Around line 62-64: Fix the typo in the XML comment above the
EnvironmentVariableKeyPrefix field in Keys.cs: change "envrionment" to
"environment" so the summary reads "Prefix for environment variables." and
ensure the XML comment remains well-formed for the public static readonly string
EnvironmentVariableKeyPrefix.
In `@src/TaskManager/Plug-ins/Podman/Logging/Log.cs`:
- Around line 41-48: Fix the typos in the LoggerMessage Message strings: update
DockerInputMapped and DockerOutputMapped messages to use "host={hostPath}"
instead of "host=={hostPath}", change DockerEnvironmentVariableAdded message to
"Environment variable added {key}={value}." instead of "variabled", and find any
other logger messages around the same area (the two messages referenced at the
end of the comment) that use "retreiving" and correct them to "retrieving";
ensure each LoggerMessage attribute's Message text is corrected for these
spelling and punctuation issues.
- Around line 116-117: The ErrorSettingDirectoryPermission logger method should
accept an Exception so stack traces are captured: change the partial method
signature from ErrorSettingDirectoryPermission(this ILogger logger, string path,
string user) to include an Exception parameter (e.g.,
ErrorSettingDirectoryPermission(this ILogger logger, Exception exception, string
path, string user)) and update any call sites to pass the caught exception; keep
the LoggerMessage attribute as-is (the exception is provided as a method
parameter and will be logged by the generated source) and mirror the parameter
ordering used by other methods like ErrorGeneratingContainerSpecification and
ErrorDeployingContainer for consistency.
- Around line 65-66: The LoggerMessage attribute on the
DockerIntermediateVolumeMapped partial method has a typo in EventId (10014) that
breaks the project's EventId sequence; update the attribute to use EventId =
1014 on the DockerIntermediateVolumeMapped method in Log.cs and ensure it
matches the surrounding 1000-1031 range (and confirm no other log entry already
uses 1014 to avoid duplicates).
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs`:
- Around line 366-372: The loop in PodmanPlugin (iterating
Event.TaskPluginArguments and using Keys.EnvironmentVariableKeyPrefix) currently
logs raw env var values via _logger.DockerEnvironmentVariableAdded(envVarKey,
Event.TaskPluginArguments[key]) which can leak secrets; change the logging to
only record the env var name or a redacted placeholder (e.g. "***REDACTED***")
when calling _logger.DockerEnvironmentVariableAdded, while still assigning the
real value into envvars[envVarKey]; if needed, update or add an overload on
_logger.DockerEnvironmentVariableAdded to accept a single parameter (key) to
avoid passing the secret value.
- Around line 245-270: The current logic in PodmanPlugin.cs uses
ContainerStatusMonitor.IsContainerCompleted(response.State) to mark a task as
Succeeded regardless of exit code; update the branch in the method that builds
ExecutionStatus so it only returns Status = TaskExecutionStatus.Succeeded when
the container is terminal AND response.State.ExitCode == 0; otherwise treat
terminal non-zero exit codes (or response.State.Dead/response.State.OOMKilled)
as failures—use FailureReason.ExternalServiceError for OOMKilled/Dead and
FailureReason.Unknown (or a more specific reason if available) for non-zero exit
codes, and populate Errors with the exit code and state (reference
response.State, ContainerStatusMonitor.IsContainerCompleted, and the
ExecutionStatus construction sites).
- Around line 452-454: The code incorrectly casts the result of
storageService.GetObjectAsync(input.Bucket, obj.FilePath, cancellationToken) to
MemoryStream and calls WriteTo, which can NRE for non-MemoryStream types;
instead assign the returned Stream to a variable (e.g., stream = await
storageService.GetObjectAsync(... ) ?? throw new
InvalidOperationException(...)), remove the MemoryStream cast, and replace
stream.WriteTo(fileStream) with await stream.CopyToAsync(fileStream,
cancellationToken) so any Stream implementation is supported; keep using the
using declarations for stream and fileStream.
- Around line 193-197: The monitor is being fire-and-forget via Task.Run calling
IContainerStatusMonitor.Start(containerId, ...) with the caller's
cancellationToken, so the monitor can be cancelled mid-upload and its exceptions
go unobserved; change the call in PodmanPlugin.cs to start the monitor with a
dedicated token (e.g., new CancellationTokenSource or CancellationToken.None)
instead of the caller's cancellationToken so artifact uploads/callbacks can
finish, capture the returned Task (from Task.Run or Start if it returns a Task)
and observe exceptions by either awaiting it where appropriate or attaching a
ContinueWith/try/catch to log faults (so exceptions from monitor.Start are not
lost), and ensure any intended cooperative cancellation from the caller is
translated into a graceful stop request rather than directly canceling the
upload (use a linked CTS if you need to forward cancellation requests while
allowing finalization).
---
Nitpick comments:
In `@src/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cs`:
- Line 17: Remove the unused Ardalis.GuardClauses using directive from
ContainerVolumeMount.cs: delete the line "using Ardalis.GuardClauses;" and
ensure no other code in the ContainerVolumeMount class or related methods (e.g.,
constructors or validation helpers) reference Ardalis guard methods; rely on the
existing ArgumentNullException.ThrowIfNull/ThrowIfNullOrWhiteSpace calls as
currently implemented.
In `@src/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cs`:
- Line 68: The hardcoded Podman API version in the POST URL inside the container
creation call should be made configurable or documented; update the code in the
method that calls httpClient.PostAsync (in IPodmanContainerCreator
implementation where "/v4.0.0/libpod/containers/create" is used) to pull the API
prefix from a configurable setting (e.g., constructor-injected podmanApiVersion
or options) and fall back to a sensible default, or add an inline comment
stating the minimum supported Podman version if you choose not to make it
configurable; ensure the identifier (podmanApiVersion / PodmanOptions) is used
to build the request path consistently across methods that call the Podman HTTP
API.
In `@src/TaskManager/Plug-ins/Podman/Keys.cs`:
- Around line 21-24: The XML documentation in the Podman plugin contains
Docker-specific wording; update the comments in the Keys class (e.g., the
BaseUrl field and other XML comments around lines referencing "docker run" and
"Docker workflow") to say "Podman" instead—replace "Docker server" with "Podman
server", "docker run" with "podman run", and "Docker workflow" with "Podman
workflow" so the comments accurately describe the Podman plugin and its keys.
In `@src/TaskManager/Plug-ins/Podman/Logging/Log.cs`:
- Around line 41-48: Rename the logging methods to use the Podman prefix instead
of Docker to match the plugin context: change DockerInputMapped to
PodmanInputMapped, DockerOutputMapped to PodmanOutputMapped,
DockerEnvironmentVariableAdded to PodmanEnvironmentVariableAdded, and
DockerIntermediateVolumeMapped to PodmanIntermediateVolumeMapped (keep the
LoggerMessage attributes and EventId/Message text unless message wording must
change). Update all call sites and any unit tests or references to these symbols
to the new method names so compilation succeeds.
In `@tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs`:
- Around line 99-125: Add a new unit test covering a transient-failure
regression for the completion path: simulate ContainerStatusMonitor.Start
receiving an exited container (use the same ContainerInspectResponse setup) but
make IStorageService.PutObjectAsync and/or
IMessageBrokerPublisherService.Publish throw on the first call and succeed on a
retry, then assert that artifacts are uploaded exactly once and the final
callback is published exactly once; reference the existing test
Start_WhenCalledWithoutAnyArtifacts_ExpectToSendCallbackEvent for structure and
the methods PutObjectAsync and Publish to locate the mocks and the Start method
invocation on ContainerStatusMonitor for where to introduce the
failure-and-retry behavior.
In `@tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs`:
- Around line 195-240: The test
ExerciseTask_WhenFailedToMonitorContainer_ExpectTaskToBeAccepted does not
observe the background exception because ContainerStatusMonitor.Start is invoked
fire-and-forget; update the test to deterministically observe the monitor call
by either (a) replacing the current
_containerStatusMonitor.Setup(...).ThrowsAsync(...) with a setup that signals a
TaskCompletionSource when Start is invoked and then throws inside a Task
returned from Start so the test can await the TCS and assert the invocation, or
(b) add a Verify on _containerStatusMonitor.Verify(m => m.Start(...),
Times.Once()) after ExecuteTask to ensure Start was called; reference the mocked
member _containerStatusMonitor and the Start method and coordinate with a
TaskCompletionSource in the test to ensure the background failure is observed
before asserting result.
- Around line 376-495: Add a new unit test in the PodmanPlugin GetStatus suite
that simulates a container with State.Status set to exited (or dead) and a
non-zero ExitCode to cover the regression; create a test (e.g.,
GetStatus_WhenContainerIsExitedWithNonZeroExitCode_ExpectFailureStatus) that
setups _podmanClient.Containers.InspectContainerAsync to return
ContainerInspectResponse with State.Status = Strings.DockerStatusExited (or
"dead") and ExitCode = 100, instantiate PodmanPlugin and call
GetStatus("identity", ...), then assert result.Status ==
TaskExecutionStatus.Failed, result.FailureReason ==
FailureReason.ExternalServiceError (or the same code path used for non-zero
exits in PodmanPlugin), result.Errors contains "Exit code=100", and verify
InspectContainerAsync was called for "identity".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d05a00ca-9536-4f3d-baa6-1c8453407771
📒 Files selected for processing (22)
src/Common/Miscellaneous/ValidationConstants.cssrc/Monai.Deploy.WorkflowManager.slnsrc/TaskManager/Plug-ins/Podman/AssemblyInfo.cssrc/TaskManager/Plug-ins/Podman/ContainerMonitorException.cssrc/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cssrc/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cssrc/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cssrc/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cssrc/TaskManager/Plug-ins/Podman/Keys.cssrc/TaskManager/Plug-ins/Podman/Logging/Log.cssrc/TaskManager/Plug-ins/Podman/Monai.Deploy.WorkflowManager.TaskManager.Podman.csprojsrc/TaskManager/Plug-ins/Podman/PodmanPlugin.cssrc/TaskManager/Plug-ins/Podman/SetPermissionException.cssrc/TaskManager/Plug-ins/Podman/Strings.cssrc/TaskManager/TaskManager/Extensions/TaskManagerExtensions.cssrc/TaskManager/TaskManager/Monai.Deploy.WorkflowManager.TaskManager.csprojsrc/TaskManager/TaskManager/PluginStrings.cssrc/TaskManager/TaskManager/packages.lock.jsontests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cstests/UnitTests/TaskManager.Podman.Tests/Monai.Deploy.WorkflowManager.TaskManager.Podman.Tests.csprojtests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cstests/UnitTests/TaskManager.Podman.Tests/Usings.cs
Comment on lines
+164
to
+165
| var objectName = file.Replace(artifactsPath, string.Empty).TrimStart('/'); | ||
| objectName = _fileSystem.Path.Combine(destination.RelativeRootPath, objectName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -type f -name "ContainerStatusMonitor.cs"Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 206
🏁 Script executed:
# Read the relevant section with more context
cat -n "src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs" | sed -n '150,180p'Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1768
🏁 Script executed:
# Search for how objectName is used after these lines
rg -A 10 -B 5 "objectName" "src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs" | head -60Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1306
🏁 Script executed:
# Check what artifactsPath is and how it's defined
rg "artifactsPath" "src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs" -B 2 -A 2Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1236
🏁 Script executed:
# Check what type of storage service is being used
rg -B 5 -A 5 "PutObjectAsync" "src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs"Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 725
🏁 Script executed:
# Check the IStorageService interface to understand object storage expectations
find . -type f -name "*.cs" | xargs rg -l "interface IStorageService"Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 71
🏁 Script executed:
# Look at the destination object to understand RelativeRootPath
rg "RelativeRootPath" --type cs -B 2 -A 2 | head -40Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 3643
🏁 Script executed:
# Check if there's any documentation or tests about objectName formatting
rg "objectName" --type cs -B 2 -A 2 | grep -E "(objectName|storage|S3|blob)" | head -30Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 6206
🏁 Script executed:
# Verify the prefix-safe concern by checking edge cases
python3 << 'EOF'
# Test the current problematic approach
artifactsPath1 = "/home/user/artifacts"
file1 = "/home/user/artifacts2/file.txt"
result1 = file1.replace(artifactsPath1, "").lstrip('/')
print(f"Case 1 - Path that starts with same prefix:")
print(f" artifactsPath: {artifactsPath1}")
print(f" file: {file1}")
print(f" Result: {result1}")
print(f" Expected: 2/file.txt (WRONG!)")
print()
# Correct approach with GetRelativePath-like logic
import os
file2 = "/home/user/artifacts/subdir/file.txt"
artifactsPath2 = "/home/user/artifacts"
result2 = os.path.relpath(file2, artifactsPath2)
print(f"Case 2 - Proper relative path:")
print(f" artifactsPath: {artifactsPath2}")
print(f" file: {file2}")
print(f" Result: {result2}")
EOFRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 401
🏁 Script executed:
# Let's verify Path.Combine behavior on Windows with leading slashes
python3 << 'EOF'
# Simulate Path.Combine behavior on Windows
# On Windows, Path.Combine drops the second path if it starts with /
import ntpath
import posixpath
# Simulate Windows Path.Combine behavior
relativeRootPath = "minio/workflow"
objectName = "/artifacts/file.txt" # Leading slash from TrimStart('/')
# Windows behavior - rooted paths reset the path
windows_result = ntpath.join(relativeRootPath, objectName)
print(f"Windows Path.Combine simulation:")
print(f" RelativeRootPath: {relativeRootPath}")
print(f" objectName: {objectName}")
print(f" Result: {windows_result}")
print(f" Expected: minio/workflow/artifacts/file.txt")
print()
# Also test with backslashes in objectName
objectName2 = "artifacts\\file.txt"
windows_result2 = ntpath.join(relativeRootPath, objectName2)
print(f"With backslashes:")
print(f" Result: {windows_result2}")
EOFRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 327
Use forward slash string concatenation for object storage keys instead of Path.Combine.
The current implementation has three critical issues:
-
Prefix-safety:
file.Replace(artifactsPath, ...)is unsafe—it will incorrectly match overlapping paths (e.g.,/artifactswon't properly strip from/artifacts2/file.txt). -
Leading slash behavior on Windows:
Path.Combinetreats rooted paths (those starting with/) as absolute, dropping theRelativeRootPathentirely. For example,Path.Combine("minio/workflow", "/artifacts/file.txt")returns/artifacts/file.txtinstead of the intendedminio/workflow/artifacts/file.txt. -
OS-specific separators in object keys: On Windows,
Path.Combineproduces backslashes, but object storage systems (MinIO/S3) expect forward slashes only.
This issue exists in both the Podman and Docker variants.
🛠️ Suggested fix
- var objectName = file.Replace(artifactsPath, string.Empty).TrimStart('/');
- objectName = _fileSystem.Path.Combine(destination.RelativeRootPath, objectName);
+ var relativePath = _fileSystem.Path.GetRelativePath(artifactsPath, file)
+ .Replace('\\', '/')
+ .TrimStart('/');
+ var objectName = string.IsNullOrWhiteSpace(destination.RelativeRootPath)
+ ? relativePath
+ : $"{destination.RelativeRootPath.TrimEnd('/')}/{relativePath}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs` around lines 164 -
165, The object key assembly is unsafe: using file.Replace(artifactsPath, ...)
and _fileSystem.Path.Combine produces wrong results (prefix collisions, rooted
paths dropping the prefix, and OS-specific backslashes). Change the logic that
sets objectName (the code referencing file, artifactsPath and
destination.RelativeRootPath and calling _fileSystem.Path.Combine) to first
ensure you only strip a leading artifactsPath prefix (e.g., check
file.StartsWith(artifactsPath) and remove exactly that prefix) then normalize
the remaining segment by trimming any leading slashes, and finally join
destination.RelativeRootPath and the normalized segment using explicit
forward-slash concatenation (not Path.Combine) so the resulting object key
always uses '/' separators; apply the same fix to the Docker variant where the
same pattern appears.
added 2 commits
March 13, 2026 08:08
Signed-off-by: will tepe <will.tepe@cchmc.org>
Signed-off-by: will tepe <will.tepe@cchmc.org>
WillButAgain
force-pushed
the
feat/add-podman-support
branch
from
855a0e7 to
59f46e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/TaskManager/Plug-ins/Podman/Strings.cs (1)
32-32: Avoid mutable static state forDockerEndStates.Expose a read-only collection/set to prevent accidental mutation of terminal-state logic.
🔧 Proposed fix
- public static readonly List<string> DockerEndStates = new() { DockerStatusExited, DockerStatusDead }; + public static readonly IReadOnlyList<string> DockerEndStates = + Array.AsReadOnly(new[] { DockerStatusExited, DockerStatusDead });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TaskManager/Plug-ins/Podman/Strings.cs` at line 32, DockerEndStates is exposed as a mutable List; change it to an immutable/read-only collection to prevent accidental mutation. Replace the List<string> DockerEndStates with a read-only type (for example IReadOnlyCollection<string> or ImmutableHashSet<string>) and initialize it with an immutable initializer (e.g., ImmutableHashSet.Create(DockerStatusExited, DockerStatusDead) or Array.AsReadOnly(...)). Keep the symbol name DockerEndStates and its contents the same but ensure the field is not modifiable at runtime.tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs (1)
194-209: Isolate environment-variable side effects in tests.
Environment.SetEnvironmentVariable(...)here mutates global process state and can leak across parallel tests. Please restore the previous value in teardown/dispose.Suggested pattern
-public class ContainerStatusMonitorTest +public class ContainerStatusMonitorTest : IDisposable { + private readonly string? _originalHostTempStorage; + public ContainerStatusMonitorTest() { + _originalHostTempStorage = Environment.GetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName); ... } + + public void Dispose() + { + Environment.SetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName, _originalHostTempStorage); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs` around lines 194 - 209, The test helper GenerateTaskDispatchEventWithValidArguments sets an environment variable via Environment.SetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName, "storage") which mutates global process state; modify this to preserve and restore the previous value (fetch Environment.GetEnvironmentVariable before setting) and restore it after the test—either by returning the previous value to the caller so the test teardown/dispose can reset it, or by wrapping the mutation in a try/finally inside the test that calls GenerateTaskDispatchEventWithValidArguments and resets Environment.SetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName, previousValue) in the finally block; ensure references to GenerateTaskDispatchEventWithValidArguments and Strings.HostTemporaryStorageEnvironmentVariableName are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cs`:
- Around line 28-30: The plugin currently creates an IDockerClient via
CreateClient (new DockerClientConfiguration(...).CreateClient()) but never
disposes it; update the plugin's Dispose method to explicitly call
_dockerClient?.Dispose() (reference the _dockerClient field and the Dispose()
method on the plugin) either before or after calling _scope.Dispose() so the
HttpClient handlers/sockets are released and no leaks occur.
In `@src/TaskManager/Plug-ins/Podman/Keys.cs`:
- Around line 84-91: RequiredParameters currently uses a mutable List<string> as
its backing collection which allows runtime mutation; change its initializer to
an immutable backing (e.g., ImmutableArray.Create(...) or Array.AsReadOnly(new[]
{ ... })) instead of new List<string> so the IReadOnlyList<string> cannot be
modified after construction, and add the required using
(System.Collections.Immutable) if you choose ImmutableArray; update the
RequiredParameters declaration to use that immutable initializer and keep the
same element symbols (BaseUrl, EntryPoint, Command, ContainerImage,
TemporaryStorageContainerPath).
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs`:
- Around line 65-73: The code currently accepts zero or negative values for
Event.TaskPluginArguments[Keys.TaskTimeoutMinutes]; update the parsing block in
PodmanPlugin (where _containerTimeout is set) to validate that int.TryParse
succeeds AND timeoutMinutes > 0; if the value is missing or not > 0, fail fast
(e.g., log an error and throw an ArgumentException or set the plugin to a failed
state) rather than silently using or accepting 0/negative — ensure the check
references Event.TaskPluginArguments, Keys.TaskTimeoutMinutes and assigns
_containerTimeout = TimeSpan.FromMinutes(timeoutMinutes) only when
timeoutMinutes > 0.
- Around line 467-480: The code uses obj.FilePath to build filePath (via
Path.Combine with inputContainerDirRoot and input.RelativeRootPath) without
canonicalizing or validating, allowing path traversal; fix by normalizing
obj.FilePath (remove leading separators), combine it, then get the full path
(Path.GetFullPath) and verify the resulting path starts with the canonical full
path of inputContainerDirRoot (or inputContainerDirRoot +
Path.DirectorySeparatorChar) before creating directories or writing; if the
check fails, throw/skip and log an error via
_logger.DownloadingArtifactFromStorageService, ensuring all uses around
filePath, fileDirectory, Directory.CreateDirectory, new
FileStream(FileMode.CreateNew), and storageService.GetObjectAsync are updated to
operate only after validation.
- Around line 544-545: The current Process.Start call that uses an interpolated
single argument string is fragile; update the PodmanPlugin code where
Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}") is
used to instead create a ProcessStartInfo for "chown", set UseShellExecute =
false, and add arguments via ArgumentList (e.g., add "-R", the user
(Event.TaskPluginArguments[Keys.User]) and the path separately) so
spaces/special chars are preserved; ensure you still WaitForExit() and properly
dispose the Process/ProcessStartInfo and validate the user/path are non-null
before starting.
In `@src/TaskManager/Plug-ins/Podman/Strings.cs`:
- Line 35: The constant MimeTypeUnknown currently uses "application/unknown";
update the value of the public const string MimeTypeUnknown in Strings.cs to use
the interoperable default "application/octet-stream" so unknown binary artifacts
fall back to the standard MIME type.
---
Nitpick comments:
In `@src/TaskManager/Plug-ins/Podman/Strings.cs`:
- Line 32: DockerEndStates is exposed as a mutable List; change it to an
immutable/read-only collection to prevent accidental mutation. Replace the
List<string> DockerEndStates with a read-only type (for example
IReadOnlyCollection<string> or ImmutableHashSet<string>) and initialize it with
an immutable initializer (e.g., ImmutableHashSet.Create(DockerStatusExited,
DockerStatusDead) or Array.AsReadOnly(...)). Keep the symbol name
DockerEndStates and its contents the same but ensure the field is not modifiable
at runtime.
In `@tests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cs`:
- Around line 194-209: The test helper
GenerateTaskDispatchEventWithValidArguments sets an environment variable via
Environment.SetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName,
"storage") which mutates global process state; modify this to preserve and
restore the previous value (fetch Environment.GetEnvironmentVariable before
setting) and restore it after the test—either by returning the previous value to
the caller so the test teardown/dispose can reset it, or by wrapping the
mutation in a try/finally inside the test that calls
GenerateTaskDispatchEventWithValidArguments and resets
Environment.SetEnvironmentVariable(Strings.HostTemporaryStorageEnvironmentVariableName,
previousValue) in the finally block; ensure references to
GenerateTaskDispatchEventWithValidArguments and
Strings.HostTemporaryStorageEnvironmentVariableName are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8328d239-4497-472e-abeb-4e060ed4e521
📒 Files selected for processing (22)
src/Common/Miscellaneous/ValidationConstants.cssrc/Monai.Deploy.WorkflowManager.slnsrc/TaskManager/Plug-ins/Podman/AssemblyInfo.cssrc/TaskManager/Plug-ins/Podman/ContainerMonitorException.cssrc/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cssrc/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cssrc/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cssrc/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cssrc/TaskManager/Plug-ins/Podman/Keys.cssrc/TaskManager/Plug-ins/Podman/Logging/Log.cssrc/TaskManager/Plug-ins/Podman/Monai.Deploy.WorkflowManager.TaskManager.Podman.csprojsrc/TaskManager/Plug-ins/Podman/PodmanPlugin.cssrc/TaskManager/Plug-ins/Podman/SetPermissionException.cssrc/TaskManager/Plug-ins/Podman/Strings.cssrc/TaskManager/TaskManager/Extensions/TaskManagerExtensions.cssrc/TaskManager/TaskManager/Monai.Deploy.WorkflowManager.TaskManager.csprojsrc/TaskManager/TaskManager/PluginStrings.cssrc/TaskManager/TaskManager/packages.lock.jsontests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cstests/UnitTests/TaskManager.Podman.Tests/Monai.Deploy.WorkflowManager.TaskManager.Podman.Tests.csprojtests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cstests/UnitTests/TaskManager.Podman.Tests/Usings.cs
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/UnitTests/TaskManager.Podman.Tests/Usings.cs
- src/TaskManager/TaskManager/Monai.Deploy.WorkflowManager.TaskManager.csproj
- src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs
- src/TaskManager/Plug-ins/Podman/ContainerMonitorException.cs
- src/TaskManager/Plug-ins/Podman/AssemblyInfo.cs
- src/TaskManager/TaskManager/Extensions/TaskManagerExtensions.cs
- tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs
Comment on lines
+28
to
+30
| public IDockerClient CreateClient(Uri podmanEndpoint) | ||
| { | ||
| return new DockerClientConfiguration(podmanEndpoint).CreateClient(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect PodmanPlugin lifecycle handling for scoped services and IDockerClient disposal.
fd PodmanPlugin.cs
rg -n -C3 'CreateScope\(|CreateClient\(|_dockerClient|_scope|Dispose\(|IAsyncDisposable|IDisposable' src/TaskManager/Plug-ins/Podman/PodmanPlugin.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 5072
🏁 Script executed:
fd IPodmanClientFactory.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 142
🏁 Script executed:
fd -e cs | xargs rg -l 'class.*PodmanClientFactory|IPodmanClientFactory.*new\|services\.|AddScoped.*PodmanClientFactory|AddTransient.*PodmanClientFactory'Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 206
🏁 Script executed:
cat -n src/TaskManager/Plug-ins/Podman/IPodmanClientFactory.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1376
🏁 Script executed:
rg -n -A 10 -B 2 'IPodmanClientFactory\|PodmanClientFactory' src/TaskManager/TaskManager/Extensions/TaskManagerExtensions.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 69
🏁 Script executed:
cat -n src/TaskManager/TaskManager/Extensions/TaskManagerExtensions.cs | head -100Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 5230
🏁 Script executed:
rg -n 'IPodmanClientFactory\|AddScoped\|AddTransient' src/TaskManager/TaskManager/Extensions/TaskManagerExtensions.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 69
🌐 Web query:
Docker.DotNet IDockerClient IDisposable disposal
💡 Result:
- In Docker.DotNet,
IDockerClientinheritsIDisposable, so anyIDockerClientyou create should be disposed when you’re done with it. [1] - The concrete
DockerClientimplementation’sDispose()disposes itsConfigurationand its internalHttpClient(so it releases sockets/handlers, etc.). [2] DockerClientConfigurationis alsoIDisposable; disposing it disposes the configuredCredentials. [3]
Recommended usage
Dispose the client (and let it cascade):
using var client =
new DockerClientConfiguration(new Uri("npipe://./pipe/docker_engine"))
.CreateClient();
// use client...Lifetime gotcha
DockerClientConfiguration.CreateClient() passes the same configuration instance into the client, and DockerClient.Dispose() calls Configuration.Dispose(). [2][3]
So if you reuse a single DockerClientConfiguration to create multiple clients, disposing any one of those clients will dispose the shared configuration/credentials for all of them—avoid sharing the same configuration across independently-lived clients. [2][3]
Sources:
[1] IDockerClient : IDisposable definition (Docker.DotNet source) [1]
[2] DockerClient.Dispose() implementation (Docker.DotNet source) [2]
[3] DockerClientConfiguration : IDisposable + Dispose() (Docker.DotNet source) [3]
Dispose IDockerClient explicitly in the plugin's Dispose method.
IDockerClient implements IDisposable and its Dispose() method closes internal HttpClient handlers and sockets. The client created at line 59 is never disposed—only _scope is disposed in the Dispose() method (line 561), but the scope does not own the client since it was instantiated outside the container. Add _dockerClient?.Dispose(); before or after _scope.Dispose() to prevent socket and handler leaks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cs` around lines 28 -
30, The plugin currently creates an IDockerClient via CreateClient (new
DockerClientConfiguration(...).CreateClient()) but never disposes it; update the
plugin's Dispose method to explicitly call _dockerClient?.Dispose() (reference
the _dockerClient field and the Dispose() method on the plugin) either before or
after calling _scope.Dispose() so the HttpClient handlers/sockets are released
and no leaks occur.
Comment on lines
+84
to
+91
| public static readonly IReadOnlyList<string> RequiredParameters = | ||
| new List<string> { | ||
| BaseUrl, | ||
| EntryPoint, | ||
| Command, | ||
| ContainerImage, | ||
| TemporaryStorageContainerPath | ||
| }; |
There was a problem hiding this comment.
Use a non-mutable backing collection for RequiredParameters.
Current backing type is List<string>, so it can still be mutated after initialization and change validation behavior at runtime.
🔧 Proposed fix
- public static readonly IReadOnlyList<string> RequiredParameters =
- new List<string> {
+ public static readonly IReadOnlyList<string> RequiredParameters =
+ Array.AsReadOnly(new[] {
BaseUrl,
EntryPoint,
Command,
ContainerImage,
TemporaryStorageContainerPath
- };
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/Keys.cs` around lines 84 - 91,
RequiredParameters currently uses a mutable List<string> as its backing
collection which allows runtime mutation; change its initializer to an immutable
backing (e.g., ImmutableArray.Create(...) or Array.AsReadOnly(new[] { ... }))
instead of new List<string> so the IReadOnlyList<string> cannot be modified
after construction, and add the required using (System.Collections.Immutable) if
you choose ImmutableArray; update the RequiredParameters declaration to use that
immutable initializer and keep the same element symbols (BaseUrl, EntryPoint,
Command, ContainerImage, TemporaryStorageContainerPath).
Comment on lines
+65
to
+73
| if (Event.TaskPluginArguments.ContainsKey(Keys.TaskTimeoutMinutes) && | ||
| int.TryParse(Event.TaskPluginArguments[Keys.TaskTimeoutMinutes], out var timeoutMinutes)) | ||
| { | ||
| _containerTimeout = TimeSpan.FromMinutes(timeoutMinutes); | ||
| } | ||
| else | ||
| { | ||
| _containerTimeout = TimeSpan.FromMinutes(5); | ||
| } |
There was a problem hiding this comment.
Reject non-positive task timeouts.
TaskTimeoutMinutes currently accepts 0/negative values, which can force immediate monitor timeout behavior. Validate > 0 and fail fast on invalid input.
Proposed fix
- if (Event.TaskPluginArguments.ContainsKey(Keys.TaskTimeoutMinutes) &&
- int.TryParse(Event.TaskPluginArguments[Keys.TaskTimeoutMinutes], out var timeoutMinutes))
+ if (Event.TaskPluginArguments.ContainsKey(Keys.TaskTimeoutMinutes) &&
+ int.TryParse(Event.TaskPluginArguments[Keys.TaskTimeoutMinutes], out var timeoutMinutes))
{
- _containerTimeout = TimeSpan.FromMinutes(timeoutMinutes);
+ if (timeoutMinutes <= 0)
+ {
+ throw new InvalidTaskException($"'{Keys.TaskTimeoutMinutes}' must be greater than 0.");
+ }
+ _containerTimeout = TimeSpan.FromMinutes(timeoutMinutes);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 65 - 73, The
code currently accepts zero or negative values for
Event.TaskPluginArguments[Keys.TaskTimeoutMinutes]; update the parsing block in
PodmanPlugin (where _containerTimeout is set) to validate that int.TryParse
succeeds AND timeoutMinutes > 0; if the value is missing or not > 0, fail fast
(e.g., log an error and throw an ArgumentException or set the plugin to a failed
state) rather than silently using or accepting 0/negative — ensure the check
references Event.TaskPluginArguments, Keys.TaskTimeoutMinutes and assigns
_containerTimeout = TimeSpan.FromMinutes(timeoutMinutes) only when
timeoutMinutes > 0.
Comment on lines
+467
to
+480
| foreach (var obj in objects) | ||
| { | ||
| // Task Manager Container Path of the Input File. | ||
| var filePath = Path.Combine(inputContainerDirRoot, obj.FilePath.Replace(input.RelativeRootPath, "").TrimStart('/')); | ||
|
|
||
| // Task Manager Container Path of the Directory for this Input File. | ||
| var fileDirectory = Path.GetDirectoryName(filePath); | ||
| Directory.CreateDirectory(fileDirectory!); | ||
|
|
||
| _logger.DownloadingArtifactFromStorageService(obj.Filename, filePath); | ||
| using var stream = await storageService.GetObjectAsync(input.Bucket, obj.FilePath, cancellationToken).ConfigureAwait(false) | ||
| ?? throw new InvalidOperationException($"Unable to download '{obj.FilePath}' from bucket '{input.Bucket}'."); | ||
| using var fileStream = new FileStream(filePath, FileMode.CreateNew, FileAccess.Write); | ||
| await stream.CopyToAsync(fileStream, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Harden artifact download path against traversal.
obj.FilePath is used to build a local file path without canonical root validation. Crafted object keys can escape the intended input directory and overwrite arbitrary files.
Proposed fix
- var filePath = Path.Combine(inputContainerDirRoot, obj.FilePath.Replace(input.RelativeRootPath, "").TrimStart('/'));
+ var relativePath = obj.FilePath.Replace(input.RelativeRootPath, string.Empty).TrimStart('/', '\\');
+ var rootPath = Path.GetFullPath(inputContainerDirRoot);
+ var filePath = Path.GetFullPath(Path.Combine(rootPath, relativePath));
+ if (!filePath.StartsWith(rootPath + Path.DirectorySeparatorChar, StringComparison.Ordinal) &&
+ !string.Equals(filePath, rootPath, StringComparison.Ordinal))
+ {
+ throw new InvalidTaskException($"Invalid input object path '{obj.FilePath}'.");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 467 - 480, The
code uses obj.FilePath to build filePath (via Path.Combine with
inputContainerDirRoot and input.RelativeRootPath) without canonicalizing or
validating, allowing path traversal; fix by normalizing obj.FilePath (remove
leading separators), combine it, then get the full path (Path.GetFullPath) and
verify the resulting path starts with the canonical full path of
inputContainerDirRoot (or inputContainerDirRoot + Path.DirectorySeparatorChar)
before creating directories or writing; if the check fails, throw/skip and log
an error via _logger.DownloadingArtifactFromStorageService, ensuring all uses
around filePath, fileDirectory, Directory.CreateDirectory, new
FileStream(FileMode.CreateNew), and storageService.GetObjectAsync are updated to
operate only after validation.
Comment on lines
+544
to
+545
| var process = Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}"); | ||
| process.WaitForExit(); |
There was a problem hiding this comment.
Use structured process arguments for chown.
Passing interpolated args as one string is brittle (spaces/special chars in user or path) and can execute with unintended argument splitting. Use ProcessStartInfo.ArgumentList.
Proposed fix
- var process = Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}");
+ var startInfo = new ProcessStartInfo("chown");
+ startInfo.ArgumentList.Add("-R");
+ startInfo.ArgumentList.Add(Event.TaskPluginArguments[Keys.User]);
+ startInfo.ArgumentList.Add(path);
+ using var process = Process.Start(startInfo) ?? throw new SetPermissionException("Failed to start chown process.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var process = Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}"); | |
| process.WaitForExit(); | |
| var startInfo = new ProcessStartInfo("chown"); | |
| startInfo.ArgumentList.Add("-R"); | |
| startInfo.ArgumentList.Add(Event.TaskPluginArguments[Keys.User]); | |
| startInfo.ArgumentList.Add(path); | |
| using var process = Process.Start(startInfo) ?? throw new SetPermissionException("Failed to start chown process."); | |
| process.WaitForExit(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 544 - 545, The
current Process.Start call that uses an interpolated single argument string is
fragile; update the PodmanPlugin code where Process.Start("chown", $"-R
{Event.TaskPluginArguments[Keys.User]} {path}") is used to instead create a
ProcessStartInfo for "chown", set UseShellExecute = false, and add arguments via
ArgumentList (e.g., add "-R", the user (Event.TaskPluginArguments[Keys.User])
and the path separately) so spaces/special chars are preserved; ensure you still
WaitForExit() and properly dispose the Process/ProcessStartInfo and validate the
user/path are non-null before starting.
| public static readonly List<string> DockerEndStates = new() { DockerStatusExited, DockerStatusDead }; | ||
|
|
||
| public const string MimeTypeDicom = "application/dicom"; | ||
| public const string MimeTypeUnknown = "application/unknown"; |
There was a problem hiding this comment.
Use a standard fallback MIME type for unknown artifacts.
Line 35 uses application/unknown; application/octet-stream is the interoperable default for unknown binary content types.
🔧 Proposed fix
- public const string MimeTypeUnknown = "application/unknown";
+ public const string MimeTypeUnknown = "application/octet-stream";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public const string MimeTypeUnknown = "application/unknown"; | |
| public const string MimeTypeUnknown = "application/octet-stream"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TaskManager/Plug-ins/Podman/Strings.cs` at line 35, The constant
MimeTypeUnknown currently uses "application/unknown"; update the value of the
public const string MimeTypeUnknown in Strings.cs to use the interoperable
default "application/octet-stream" so unknown binary artifacts fall back to the
standard MIME type.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary:
Summary by CodeRabbit