Prevent start of containers with Host and TS Enabled#2047
Conversation
|
Warning Rate limit exceeded@Squidly271 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update in Changes
Sequence Diagram(s)sequenceDiagram
participant CM as ContainerManager
participant DC as DockerClient
participant C as Container
CM->>DC: Instantiate DockerClient
CM->>DC: getDockerContainers()
DC-->>CM: Return container info
loop For each container in list
C->>CM: Start action requested
note right of CM: Check if container name exists in info
alt Container with NetworkMode "host" and Cmd "/opt/unraid/tailscale"
CM->>CM: Skip starting container
else
CM->>C: Execute Docker start command
end
end
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Squidly271
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/include/ContainerManager.php (2)
22-23: Consider optimizing the Docker container information retrieval.The code creates a new DockerClient instance and retrieves all container information regardless of which action is being performed, but this information is only used when the action is "start". Consider only retrieving this information when needed.
$containers = DockerUtil::docker("ps -a --filter status='$status' --format='{{.Names}}'",true); -$dockerClient = new DockerClient(); -$info = $dockerClient->getDockerContainers(); if (file_exists($user_prefs)) { // existing code } +$info = []; +if ($action == "start") { + $dockerClient = new DockerClient(); + $info = $dockerClient->getDockerContainers(); +}
31-35: Consider adding a comment explaining the purpose of this check.The code now skips starting containers that use host networking mode and run Tailscale, but the reason isn't documented. Adding a comment would help future maintainers understand the rationale.
if ( $action == "start") { + // Skip starting containers using host networking mode and running Tailscale + // to prevent potential network conflicts or security issues $key = array_search($ct,array_column($info,"Name")); if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/ContainerManager.php(2 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix.docker.manager/include/ContainerManager.php (1)
2-3: Updated copyright years correctly.The copyright years have been updated from 2023 to 2025 for both Lime Technology and Bergware International, keeping the file's copyright information current.
| if ( $action == "start") { | ||
| $key = array_search($ct,array_column($info,"Name")); | ||
| if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for container lookup.
The current implementation assumes that array_search() will always find the container name in the $info array. If a container isn't found, $key would be false, potentially causing an undefined index error when accessing $info[$key].
if ( $action == "start") {
$key = array_search($ct,array_column($info,"Name"));
- if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale")
+ if ($key !== false && isset($info[$key]) &&
+ $info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale")
continue;
}📝 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.
| if ( $action == "start") { | |
| $key = array_search($ct,array_column($info,"Name")); | |
| if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") | |
| continue; | |
| } | |
| if ( $action == "start") { | |
| $key = array_search($ct,array_column($info,"Name")); | |
| if ($key !== false && isset($info[$key]) && | |
| $info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") | |
| continue; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/include/Events.php (2)
35-39: Consider using a more specific error response type.Currently, a successful security block returns a 'success' response with an explanation message. This could be confusing for client code that checks for success/error status.
Consider using an 'error' response type with an appropriate error code:
if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") { - $arrResponse = ['success'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')]; + $arrResponse = ['error'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')]; break; }This makes it clearer that the operation was not completed due to a security policy violation rather than suggesting it was successful.
34-36: Consider optimizing container information retrieval.The current implementation fetches all Docker containers even when we only need information about one specific container.
If the DockerClient API allows it, consider using a more targeted method to get information about just the specific container:
- $info = $DockerClient->getDockerContainers(); - $key = array_search($container,array_column($info,"Id")); - if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") { + $containerInfo = $DockerClient->getContainerDetails($container); + if ($containerInfo && $containerInfo['NetworkMode'] == "host" && $containerInfo['Cmd'] == "/opt/unraid/tailscale") {This would be more efficient, especially in environments with many containers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/Events.php(2 hunks)
🔇 Additional comments (1)
emhttp/plugins/dynamix.docker.manager/include/Events.php (1)
2-3: Copyright year updated correctly.The copyright years have been updated from 2023 to 2025 for both Lime Technology and Bergware International, keeping the copyright information current.
| if ($container) { | ||
| $info = $DockerClient->getDockerContainers(); | ||
| $key = array_search($container,array_column($info,"Id")); | ||
| if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") { | ||
| $arrResponse = ['success'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')]; | ||
| break; | ||
| } | ||
| $arrResponse = ['success' => $DockerClient->startContainer($container)]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in container checking logic.
The security check to prevent containers with Host network mode and Tailscale from starting is a good security improvement. However, there's a potential issue if the container ID isn't found in the array.
Consider adding additional validation:
if ($container) {
$info = $DockerClient->getDockerContainers();
$key = array_search($container,array_column($info,"Id"));
+ if ($key !== false) {
if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") {
$arrResponse = ['success'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')];
break;
}
+ }
$arrResponse = ['success' => $DockerClient->startContainer($container)];
}This ensures that the array index check only happens when the container is found, preventing potential undefined index errors.
📝 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.
| if ($container) { | |
| $info = $DockerClient->getDockerContainers(); | |
| $key = array_search($container,array_column($info,"Id")); | |
| if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") { | |
| $arrResponse = ['success'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')]; | |
| break; | |
| } | |
| $arrResponse = ['success' => $DockerClient->startContainer($container)]; | |
| } | |
| if ($container) { | |
| $info = $DockerClient->getDockerContainers(); | |
| $key = array_search($container, array_column($info, "Id")); | |
| if ($key !== false) { | |
| if ($info[$key]['NetworkMode'] == "host" && $info[$key]['Cmd'] == "/opt/unraid/tailscale") { | |
| $arrResponse = ['success'=> _('For security reasons, containers with Network Type "Host" should not have Tailscale enabled. Please disable Tailscale in this container or change the Network Type of the container.')]; | |
| break; | |
| } | |
| } | |
| $arrResponse = ['success' => $DockerClient->startContainer($container)]; | |
| } |
2 participants
Summary by CodeRabbit