fix: support remote database hosts in health check#21
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughUpdated README.md documentation link from Database management to Requirements in the Standalone Tasks section. Enhanced database health check logic in health.php with credential-based ping attempts, remote host detection, and conditional local process verification to determine database availability. Changes
Sequence DiagramsequenceDiagram
participant HC as Health Checker
participant CR as Credential Resolver
participant DB as Database
participant LP as Local Process
HC->>CR: resolveDatabaseCredentials()
alt Credentials Found
CR-->>HC: Return credentials + host info
HC->>DB: Ping with credentials (host, port, user, password)
alt Remote Host Detected
DB-->>HC: Connection failed / unreachable
HC->>HC: Log "Cannot reach remote host:port"
HC-->>HC: Mark unhealthy
else Local Host
alt Ping Successful
DB-->>HC: Success + hostInfo
HC-->>HC: Mark healthy with hostInfo
else Ping Failed
HC->>LP: Check local mysqld/mariadbd process
LP-->>HC: Process status
HC-->>HC: Mark health based on process status
end
end
else No Credentials
HC->>DB: Fallback ping via mysqladmin
alt Fallback Successful
DB-->>HC: Success
HC-->>HC: Mark healthy
else Fallback Failed
HC->>LP: Check local mysqld/mariadbd process
LP-->>HC: Process status
HC-->>HC: Mark health based on process status
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployer/requirements/task/health.php`:
- Around line 53-60: The current $pingCmd uses "--password=…" which exposes
credentials in the process table; instead create a temporary MySQL defaults file
on the remote host and supply it via --defaults-extra-file to the mysql client:
inside the Deployer run() sequence use run('mktemp') to get a safe remote path,
write a file containing a [client] section with user/host/port/password (use
escapeshellarg or printf with proper quoting when echoing to the remote file),
run the admin command using --defaults-extra-file=<tmp> (omit --password), then
securely remove the temp file with run('rm -f ' . escapeshellarg($tmpPath));
update references around $pingCmd and adminCmd to build the command this way and
ensure dbCredentials values are not embedded on the command line.
- Around line 69-83: The anonymous-socket fallback can produce a false positive
when credentials point to a remote host; update the logic so the fallback ping
is skipped for remote DBs by using the existing remote check before attempting
the socket ping: ensure the code that sets $isRemoteDb (based on $dbCredentials
host not being 'localhost', '127.0.0.1' or a socket) is present and then change
the fallback guard from "if (!$dbChecked)" to "if (!$dbChecked &&
!$isRemoteDb)"; this prevents run("$adminCmd ping --silent...") from marking the
DB healthy when the configured remote host failed earlier (refer to
$dbCredentials, $dbChecked, $isRemoteDb, run(), and addRequirementRow).
| $pingCmd = sprintf( | ||
| '%s ping --silent -h %s -P %d -u %s --password=%s 2>/dev/null', | ||
| $adminCmd, | ||
| escapeshellarg($dbCredentials['host']), | ||
| $dbCredentials['port'], | ||
| escapeshellarg($dbCredentials['user']), | ||
| escapeshellarg($dbCredentials['password']) | ||
| ); |
There was a problem hiding this comment.
Password exposed in command-line argument (--password=…)
Using --password=password on the command line is convenient but insecure; on some systems the password becomes visible to status programs such as ps that may be invoked by other users to display command lines. The 2>/dev/null redirect only silences stderr — it does not mask the argument in the process table.
The safest approach is a temp --defaults-extra-file. A simpler middle-ground is a [client]-style option file written to a temp path, deleted immediately after the run() call. If the deployment context is trusted and the remote execution environment is sufficiently isolated, this risk may be acceptable, but it is worth an explicit acknowledgement.
🔒️ Suggested alternative: temp defaults file
- $pingCmd = sprintf(
- '%s ping --silent -h %s -P %d -u %s --password=%s 2>/dev/null',
- $adminCmd,
- escapeshellarg($dbCredentials['host']),
- $dbCredentials['port'],
- escapeshellarg($dbCredentials['user']),
- escapeshellarg($dbCredentials['password'])
- );
-
- try {
- run($pingCmd);
+ $tmpCnf = tempnam(sys_get_temp_dir(), 'deployer_db_');
+ $cnfContent = sprintf(
+ "[client]\nhost=%s\nport=%d\nuser=%s\npassword=%s\n",
+ $dbCredentials['host'],
+ $dbCredentials['port'],
+ $dbCredentials['user'],
+ $dbCredentials['password']
+ );
+ $pingCmd = sprintf(
+ '%s --defaults-extra-file=%s ping --silent 2>/dev/null',
+ $adminCmd,
+ escapeshellarg($tmpCnf)
+ );
+
+ try {
+ run(sprintf('printf %%s %s > %s', escapeshellarg($cnfContent), escapeshellarg($tmpCnf)));
+ run($pingCmd);Note: The temp file must be created on the remote host (inside a Deployer
run()call), not locally viatempnam(). Adjust accordingly for Deployer's remote execution model — e.g. generate a remote temp path withrun('mktemp'), write the cnf, ping, thenrun('rm -f ' . escapeshellarg($tmpCnf)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployer/requirements/task/health.php` around lines 53 - 60, The current
$pingCmd uses "--password=…" which exposes credentials in the process table;
instead create a temporary MySQL defaults file on the remote host and supply it
via --defaults-extra-file to the mysql client: inside the Deployer run()
sequence use run('mktemp') to get a safe remote path, write a file containing a
[client] section with user/host/port/password (use escapeshellarg or printf with
proper quoting when echoing to the remote file), run the admin command using
--defaults-extra-file=<tmp> (omit --password), then securely remove the temp
file with run('rm -f ' . escapeshellarg($tmpPath)); update references around
$pingCmd and adminCmd to build the command this way and ensure dbCredentials
values are not embedded on the command line.
There was a problem hiding this comment.
@coderabbitai Valid point in general, but in this context the risk is acceptable: Deployer executes commands via short-lived SSH sessions, and other tasks in this project (e.g. check_database_grants.php, check_database.php) use the same CLI credential pattern. A temp defaults file would add disproportionate complexity (mktemp, write, cleanup, error handling) for a health-check ping. Acknowledged, but won't change.
There was a problem hiding this comment.
@konradmichalik Understood — thank you for the context. Given the short-lived SSH session model and consistency with existing tasks in the repository, I agree the trade-off is reasonable for this use case. I've noted this pattern for future reference.
🧠 Learnings used
Learnt from: konradmichalik
Repo: move-elevator/deployer-tools PR: 11
File: deployer/typo3/task/feature_scheduler.php:15-15
Timestamp: 2026-02-18T10:07:30.947Z
Learning: In the move-elevator/deployer-tools repository, shell variables in `test()` and `runExtended()` calls are intentionally left unquoted as an established codebase convention. Deploy paths are server-configured and do not contain spaces, so defensive quoting is unnecessary and would be inconsistent with the existing pattern.
1 participant
Summary
resolveDatabaseCredentials()in health check to connect to remote database servers viamysqladmin pingwith host, port, and credentialspgrepcheck whendatabase_hostpoints to a remote serverChanges
deployer/requirements/task/health.php- Resolve DB credentials and ping remote host; fall back to local process check only for local databasesREADME.md- Update standalone tasks linksSummary by CodeRabbit
Documentation
Bug Fixes