fix: resolve database hostname to IP before .env template rendering#17
fix: resolve database hostname to IP before .env template rendering#17konradmichalik merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds hostname-to-IP resolution to MittwaldApi: after database readiness is confirmed, the hostname is resolved to an IP (with fallback) and that value is passed into database configuration initialization via an updated method signature. Changes
Sequence Diagram(s)sequenceDiagram
participant MittwaldApi
participant DNS as DNS Resolver
participant DB as MySQL Database
participant Config as Env Config
MittwaldApi->>DB: waitUntilReady()
DB-->>MittwaldApi: ready
MittwaldApi->>DNS: resolveHostnameToIp(hostname)
DNS-->>MittwaldApi: ipOrFallback
MittwaldApi->>Config: initDatabaseConfiguration(db, user, ipOrFallback)
Config-->>MittwaldApi: configured
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
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: 1
🧹 Nitpick comments (3)
src/Database/Manager/MittwaldApi.php (3)
265-269: Redundantdebugmessage on the exception path.When
run()throws, twodebug()calls are emitted: one insidecatchwith the error message, and then unconditionally the fallback message below thetry/catch. The fallback message adds no extra information on the exception path.♻️ Proposed fix to deduplicate the log
} catch (\Throwable $e) { - debug("Could not resolve {$hostname} to IP: " . $e->getMessage()); + debug("Could not resolve {$hostname} to IP, using hostname as fallback: " . $e->getMessage()); + return $hostname; } - debug("Could not resolve {$hostname} to IP, using hostname as fallback."); - return $hostname; + debug("Could not resolve {$hostname} to IP, using hostname as fallback."); + return $hostname;Or, more concisely, move the fallback
returninto the catch and keep a single message per path:} catch (\Throwable $e) { debug("Could not resolve {$hostname} to IP: " . $e->getMessage()); + debug("Using hostname as fallback."); + return $hostname; } - debug("Could not resolve {$hostname} to IP, using hostname as fallback."); return $hostname;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Manager/MittwaldApi.php` around lines 265 - 269, The code emits two debug messages when resolving {$hostname} fails: one inside the catch for \Throwable $e and another unconditional fallback debug afterwards; remove the unconditional debug call and instead move the fallback debug (and the fallback return/usage of $hostname) into the catch block so that the exception path logs only once; update the catch in the run() (or the method that calls debug() for hostname resolution) to log the error and then perform the fallback logic using $hostname.
265-269: Redundantdebug()message on the exception path.When
run()throws, twodebug()calls are emitted: one insidecatch(with the error message) and then the unconditional fallback message below. The second one adds no new information in that path.♻️ Proposed cleanup
} catch (\Throwable $e) { - debug("Could not resolve {$hostname} to IP: " . $e->getMessage()); + debug("Could not resolve {$hostname} to IP, using hostname as fallback: " . $e->getMessage()); + return $hostname; } - debug("Could not resolve {$hostname} to IP, using hostname as fallback."); return $hostname;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Manager/MittwaldApi.php` around lines 265 - 269, Redundant debug logging: remove the unconditional fallback debug call so we don't log the same message twice when run() throws; specifically, keep the catch block that logs debug("Could not resolve {$hostname} to IP: " . $e->getMessage()) and delete the subsequent debug("Could not resolve {$hostname} to IP, using hostname as fallback.") after the try/catch in MittwaldApi::run (or the surrounding method) so only the detailed error is emitted on exception and the fallback message is not duplicated.
255-271: Document the IPv4-only limitation ofgethostbyname()in the docblock.PHP's
gethostbyname()is documented as IPv4-only and will not resolve IPv6 records. If Mittwald ever introduces IPv6-only database hosts, this method will silently fall back to the hostname unchanged since theFILTER_VALIDATE_IPcheck will fail an IPv6 address. While the current behavior gracefully handles this via the fallback path, documenting this limitation prevents future readers from assuming IPv6 coverage.📝 Suggested docblock addition
/** * Resolves a database hostname to its IP address on the remote server. * + * Note: uses gethostbyname() which resolves to IPv4 only. IPv6-only hosts + * will be returned unchanged (hostname fallback). + * * This is called immediately after the TCP connectivity check passes, when DNS is🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Manager/MittwaldApi.php` around lines 255 - 271, Add a docblock to the resolveHostnameToIp method describing that it uses PHP's gethostbyname() (IPv4-only) so IPv6 records will not be resolved; state the current behavior (IPv6 will fail FILTER_VALIDATE_IP and the function will fall back to returning the original hostname) and note this limitation so future readers know it does not support IPv6-only hosts and may need a different resolver. Reference resolveHostnameToIp() and gethostbyname() in the docblock text.
🤖 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/Database/Manager/MittwaldApi.php`:
- Around line 255-271: Add a docblock to the resolveHostnameToIp method
explaining that it uses PHP's gethostbyname() (invoked via the inline PHP
command in $resolveCmd) which is IPv4-only and will return the original hostname
on failure (so IPv6-only hosts won't be resolved by this method); mention the
current guard ($ip !== $hostname && filter_var($ip, FILTER_VALIDATE_IP)) handles
this by falling back to the hostname and note the limitation and potential need
for a different resolver (e.g., dns_get_record or getaddrinfo) if Mittwald
introduces IPv6-only DB hosts so future maintainers don’t assume IPv6 coverage.
---
Nitpick comments:
In `@src/Database/Manager/MittwaldApi.php`:
- Around line 265-269: The code emits two debug messages when resolving
{$hostname} fails: one inside the catch for \Throwable $e and another
unconditional fallback debug afterwards; remove the unconditional debug call and
instead move the fallback debug (and the fallback return/usage of $hostname)
into the catch block so that the exception path logs only once; update the catch
in the run() (or the method that calls debug() for hostname resolution) to log
the error and then perform the fallback logic using $hostname.
- Around line 265-269: Redundant debug logging: remove the unconditional
fallback debug call so we don't log the same message twice when run() throws;
specifically, keep the catch block that logs debug("Could not resolve
{$hostname} to IP: " . $e->getMessage()) and delete the subsequent debug("Could
not resolve {$hostname} to IP, using hostname as fallback.") after the try/catch
in MittwaldApi::run (or the surrounding method) so only the detailed error is
emitted on exception and the fallback message is not duplicated.
- Around line 255-271: Add a docblock to the resolveHostnameToIp method
describing that it uses PHP's gethostbyname() (IPv4-only) so IPv6 records will
not be resolved; state the current behavior (IPv6 will fail FILTER_VALIDATE_IP
and the function will fall back to returning the original hostname) and note
this limitation so future readers know it does not support IPv6-only hosts and
may need a different resolver. Reference resolveHostnameToIp() and
gethostbyname() in the docblock text.
| private function resolveHostnameToIp(string $hostname): string | ||
| { | ||
| try { | ||
| $resolveCmd = sprintf('echo gethostbyname("%s");', addslashes($hostname)); | ||
| $ip = trim(run("php -r " . escapeshellarg($resolveCmd))); | ||
|
|
||
| if ($ip !== $hostname && filter_var($ip, FILTER_VALIDATE_IP)) { | ||
| info("Resolved database host {$hostname} to {$ip}"); | ||
| return $ip; | ||
| } | ||
| } catch (\Throwable $e) { | ||
| debug("Could not resolve {$hostname} to IP: " . $e->getMessage()); | ||
| } | ||
|
|
||
| debug("Could not resolve {$hostname} to IP, using hostname as fallback."); | ||
| return $hostname; | ||
| } |
There was a problem hiding this comment.
gethostbyname() is IPv4-only — document the limitation.
The PHP docs confirm gethostbyname() "returns the IPv4 address or a string containing the unmodified hostname on failure" and only accepts IPv4 addresses, not IPv6. If Mittwald ever introduces an IPv6-only database host, the resolution will silently return the original hostname (the $ip !== $hostname + FILTER_VALIDATE_IP gate correctly handles this), but the DNS-flapping workaround won't apply. Worth noting in the docblock so future maintainers don't assume IPv6 coverage.
📝 Suggested docblock addition
/**
* Resolves a database hostname to its IP address on the remote server.
*
+ * Note: uses PHP's gethostbyname() which resolves to IPv4 only.
+ * IPv6-only hosts will fall back to the original hostname silently.
+ *
* This is called immediately after the TCP connectivity check passes, when DNS is🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Manager/MittwaldApi.php` around lines 255 - 271, Add a docblock
to the resolveHostnameToIp method explaining that it uses PHP's gethostbyname()
(invoked via the inline PHP command in $resolveCmd) which is IPv4-only and will
return the original hostname on failure (so IPv6-only hosts won't be resolved by
this method); mention the current guard ($ip !== $hostname && filter_var($ip,
FILTER_VALIDATE_IP)) handles this by falling back to the hostname and note the
limitation and potential need for a different resolver (e.g., dns_get_record or
getaddrinfo) if Mittwald introduces IPv6-only DB hosts so future maintainers
don’t assume IPv6 coverage.
There was a problem hiding this comment.
@coderabbitai Acknowledged. The existing docblock already states "Falls back to the original hostname if resolution fails", which covers this behavior. Mittwald MySQL hosts are IPv4 — adding IPv6 future-proofing notes would be speculative at this point.
There was a problem hiding this comment.
@konradmichalik Fair point — the fallback documentation is already there, and if the infrastructure is IPv4 today, explicitly calling out a hypothetical IPv6 scenario adds noise rather than value. Thanks for the context!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Database/Manager/MittwaldApi.php (1)
257-264: Consider validating the resolved value is a well-formed IP address.After removing
filter_var, the only guard is$ip !== $hostname. If the remote PHP runtime emits any output before theechoresult (e.g., a deprecation notice or custom error handler output),trim()won't sanitise it and the mangled string passes the!== $hostnamecheck, silently corruptingdatabase_host.
inet_pton()has been available in core PHP since 5.1.0 and works with both IPv4 and IPv6 addresses without requiringext-filter, making it a suitable drop-in validator here.♻️ Suggested fix
- if ($ip !== $hostname) { + if ($ip !== $hostname && inet_pton($ip) !== false) { info("Resolved database host {$hostname} to {$ip}"); return $ip; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Manager/MittwaldApi.php` around lines 257 - 264, The resolved IP from the remote PHP run is not validated and may contain extraneous output; update the resolution logic in MittwaldApi.php (the block that builds $resolveCmd, calls run(), and assigns $ip) to verify that $ip is a valid IP address using inet_pton($ip) !== false before accepting it; if validation fails, log a warning (using info() or a warning logger) and fall back to returning the original $hostname instead of assigning the mangled value, so only syntactically valid IPv4/IPv6 addresses replace $hostname.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Manager/MittwaldApi.php`:
- Around line 246-254: Update the docblock for the hostname-resolution routine
in MittwaldApi.php to state that the shell command ($resolveCmd) uses
gethostbyname(), which performs IPv4-only resolution and will return the
original hostname unchanged on failure; explicitly mention the IPv4-only
limitation and the fallback behavior so future maintainers know IPv6 addresses
are not handled and failures leave the hostname unmodified.
---
Nitpick comments:
In `@src/Database/Manager/MittwaldApi.php`:
- Around line 257-264: The resolved IP from the remote PHP run is not validated
and may contain extraneous output; update the resolution logic in
MittwaldApi.php (the block that builds $resolveCmd, calls run(), and assigns
$ip) to verify that $ip is a valid IP address using inet_pton($ip) !== false
before accepting it; if validation fails, log a warning (using info() or a
warning logger) and fall back to returning the original $hostname instead of
assigning the mangled value, so only syntactically valid IPv4/IPv6 addresses
replace $hostname.
1 participant
Summary
.envtemplate, eliminating DNS dependency for all subsequent commandsERROR 2005 (HY000): Unknown MySQL server hostfailures duringfeature:syncBackground
After Mittwald creates a new database, the DNS entry for the host (e.g.
mysql-xyz.pg-s-xxx.db.project.host) flaps intermittently for several minutes. Previously, the.envwas uploaded with the hostname, and a separatefeature:wait_for_databasetask was supposed to patch it later. This task did not reliably execute, causingdb_sync_toolto fail on DNS resolution.The fix moves hostname-to-IP resolution into
MittwaldApi::create(), right after the connectivity check confirms DNS is working. The.envtemplate is then rendered with the stable IP address from the start.Changes
src/Database/Manager/MittwaldApi.php- AddresolveHostnameToIp()method, call it increate()beforeinitDatabaseConfiguration()Summary by CodeRabbit