Fix: Don't autostart containers on host network with TS enabled#2043
Conversation
WalkthroughThe Docker initialization script has been rewritten from Bash to PHP. It now reads a list of auto-start containers from Changes
Sequence Diagram(s)sequenceDiagram
participant S as Script (PHP)
participant FS as File System
participant XT as XML Checker
S->>FS: Read autostart file (/var/lib/docker/unraid-autostart)
alt File exists and is not empty
loop For each container entry
S->>FS: Check for XML template (/boot/config/plugins/dockerMan/templates-user/my-*.xml)
alt XML exists
S->>XT: Load XML and evaluate "Network" & "TailscaleEnabled"
alt Network is host and Tailscale is true
XT->>S: Log autostart disabled message
else
XT->>S: Container qualifies for autostart
end
else
S->>S: Add container to autostart list
end
end
else
S->>S: Exit script
end
S->>FS: Write updated autostart list back to file
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/scripts/docker_init(1 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix.docker.manager/scripts/docker_init (2)
1-2: Shebang transition to PHP looks fine.
Switching the script to be executed by PHP is appropriate for XML processing, but ensure that/usr/bin/phpis the correct path on the target system.
4-5: Silenced file read may mask I/O errors.
Using@file(...)ignores any warning if the file doesn’t exist or can’t be read. Make sure this is intentional, or consider explicit error handling/logging to simplify troubleshooting.
| $doc = new DOMDocument(); | ||
| $doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml"); | ||
| if ( ! $doc ) { | ||
| $newAuto[] = $container; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Load return value check is missing.
$doc is always a valid DOMDocument instance after instantiation, even if $doc->load(...) fails. The condition if (!$doc) won’t catch parse errors.
Here's a suggested fix to assess the load result properly:
$doc = new DOMDocument();
-$doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml");
-if (!$doc) {
+if (!$doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml")) {
$newAuto[] = $container;
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.
| $doc = new DOMDocument(); | |
| $doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml"); | |
| if ( ! $doc ) { | |
| $newAuto[] = $container; | |
| continue; | |
| } | |
| $doc = new DOMDocument(); | |
| - $doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml"); | |
| - if ( ! $doc ) { | |
| + if (! $doc->load("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml")) { | |
| $newAuto[] = $container; | |
| continue; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.docker.manager/scripts/docker_init (1)
22-28:⚠️ Potential issueFix null pointer safety and boolean comparison.
The code has potential null pointer issues when accessing DOM elements and uses loose comparison for boolean values.
- if ( ($doc->getElementsByTagName("Network")->item(0)->nodeValue ?? false) == "host" ) { - if ( ($doc->getElementsByTagName("TailscaleEnabled")->item(0)->nodeValue ?? false) == true ) { + $networkNode = $doc->getElementsByTagName("Network")->item(0); + $tailscaleNode = $doc->getElementsByTagName("TailscaleEnabled")->item(0); + + $network = $networkNode ? $networkNode->nodeValue : null; + $tailscaleEnabled = $tailscaleNode ? filter_var($tailscaleNode->nodeValue, FILTER_VALIDATE_BOOLEAN) : false; + + if ($network === "host" && $tailscaleEnabled === true) {
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.docker.manager/scripts/docker_init (1)
4-5: Improve error handling for file operations.The error suppression operator (@) masks potential file access issues. Consider implementing proper error handling and logging.
-$autostart = @file("/var/lib/docker/unraid-autostart",FILE_IGNORE_NEW_LINES); -if ( ! $autostart ) exit(); +$autostartFile = "/var/lib/docker/unraid-autostart"; +if (!file_exists($autostartFile)) { + exec("logger 'Docker autostart configuration file not found'"); + exit(1); +} +$autostart = file($autostartFile, FILE_IGNORE_NEW_LINES); +if ($autostart === false) { + exec("logger 'Failed to read Docker autostart configuration'"); + exit(1); +} +if (empty($autostart)) { + exit(0); +}
| } | ||
| $newAuto[] = $container; | ||
| } | ||
| file_put_contents("/var/lib/docker/unraid-autostart",implode("\n",$newAuto)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement atomic file writing with error handling.
The current file writing implementation could lead to data corruption if the script is interrupted.
-file_put_contents("/var/lib/docker/unraid-autostart",implode("\n",$newAuto));
+$tempFile = "/var/lib/docker/unraid-autostart.tmp";
+if (file_put_contents($tempFile, implode("\n", $newAuto)) === false) {
+ exec("logger 'Failed to write Docker autostart configuration'");
+ exit(1);
+}
+if (!rename($tempFile, "/var/lib/docker/unraid-autostart")) {
+ unlink($tempFile);
+ exec("logger 'Failed to update Docker autostart configuration'");
+ exit(1);
+}📝 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.
| file_put_contents("/var/lib/docker/unraid-autostart",implode("\n",$newAuto)); | |
| $tempFile = "/var/lib/docker/unraid-autostart.tmp"; | |
| if (file_put_contents($tempFile, implode("\n", $newAuto)) === false) { | |
| exec("logger 'Failed to write Docker autostart configuration'"); | |
| exit(1); | |
| } | |
| if (!rename($tempFile, "/var/lib/docker/unraid-autostart")) { | |
| unlink($tempFile); | |
| exec("logger 'Failed to update Docker autostart configuration'"); | |
| exit(1); | |
| } |
| $cont = explode(" ",$container); | ||
| if ( ! is_file("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml")) { | ||
| $newAuto[] = $container; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for container names.
The code assumes the container string format is valid and directly uses it in a file path without validation.
- $cont = explode(" ",$container);
- if ( ! is_file("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml")) {
+ $cont = explode(" ", trim($container));
+ if (empty($cont[0]) || !preg_match('/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/', $cont[0])) {
+ exec("logger 'Invalid container name format: {$container}'");
+ continue;
+ }
+ $templateFile = "/boot/config/plugins/dockerMan/templates-user/my-" . $cont[0] . ".xml";
+ if (!is_file($templateFile)) {📝 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.
| $cont = explode(" ",$container); | |
| if ( ! is_file("/boot/config/plugins/dockerMan/templates-user/my-{$cont[0]}.xml")) { | |
| $newAuto[] = $container; | |
| continue; | |
| } | |
| $cont = explode(" ", trim($container)); | |
| if (empty($cont[0]) || !preg_match('/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/', $cont[0])) { | |
| exec("logger 'Invalid container name format: {$container}'"); | |
| continue; | |
| } | |
| $templateFile = "/boot/config/plugins/dockerMan/templates-user/my-" . $cont[0] . ".xml"; | |
| if (!is_file($templateFile)) { | |
| $newAuto[] = $container; | |
| continue; | |
| } |
2 participants
Summary by CodeRabbit
New Features
Refactor