diff --git a/modules/servers/VirtFusionDirect/lib/Cache.php b/modules/servers/VirtFusionDirect/lib/Cache.php index 7cc70c1..3954ded 100644 --- a/modules/servers/VirtFusionDirect/lib/Cache.php +++ b/modules/servers/VirtFusionDirect/lib/Cache.php @@ -3,11 +3,47 @@ namespace WHMCS\Module\Server\VirtFusionDirect; /** - * Two-tier cache: uses Redis when the ext-redis extension is available, with an atomic - * filesystem fallback stored in the system temp directory. + * Two-tier cache: Redis when ext-redis is available, atomic filesystem fallback otherwise. + * + * WHY TWO TIERS + * ------------- + * The module is deployed to every kind of WHMCS install — shared hosting, dedicated + * VPS, bare-metal. Requiring Redis would exclude the long tail of smaller operators + * who never installed the extension. But operators who DO have Redis get a huge + * performance win for cross-request caching (PowerDNS zone lists, OS template + * listings, traffic stats), so we opportunistically use it when present. + * + * The fallback is filesystem-based, using the OS temp directory. Writes are atomic + * via the classic tmp-file + rename pattern so a process crash mid-write can never + * corrupt an existing cache entry for another concurrent reader. + * + * EXPIRY SEMANTICS + * ---------------- + * Redis: native SETEX — the key auto-expires on the server side. + * Filesystem: we store a JSON envelope {expires, data} and check expiry on read, + * deleting stale entries lazily. This means a cache with lots of expired entries + * will slowly accumulate files until accessed — acceptable for the module's scale + * (tens-to-hundreds of keys per install) but worth noting if someone ports this + * to a higher-volume context. + * + * NAMESPACE + * --------- + * Every key is prefixed with "vfd:" to avoid collisions with anything else that + * shares the Redis instance. Nested keys add their own sub-prefix (e.g. + * "pdns:zones:" for PowerDNS zone lists) for semantic clarity in the logs. + * + * FAILURE MODES + * ------------- + * Redis unreachable: we set $redisAvailable = false on first failure, which + * permanently disables Redis for the rest of this PHP process (subsequent calls + * skip straight to the file cache). Prevents paying reconnect overhead on every + * miss when Redis is down. + * File cache write fails: silently skipped. Cache is best-effort; a failed SET + * just means the next GET will re-fetch from the authoritative source. */ class Cache { + /** Module-global key prefix — keeps us out of Redis key collisions on shared installs. */ const PREFIX = 'vfd:'; /** @var \Redis|null */ @@ -150,12 +186,18 @@ class Cache } } - // File cache fallback with atomic write (race condition safe) + // File cache fallback with atomic write. + // Writing to a temp file + rename ensures that readers either see the + // complete previous entry or the complete new entry — never a truncated + // or partially-written file. getmypid() suffix lets concurrent PHP + // processes write to the same key without stomping each other's temp files. $path = self::filePath($key); $tmp = $path . '.' . getmypid() . '.tmp'; $entry = json_encode(['expires' => time() + $ttl, 'data' => $value]); if (@file_put_contents($tmp, $entry, LOCK_EX) !== false) { + // rename() is atomic on POSIX when source and target are on the same + // filesystem (which they always are here — both in sys_get_temp_dir). @rename($tmp, $path); } } diff --git a/modules/servers/VirtFusionDirect/lib/ConfigureService.php b/modules/servers/VirtFusionDirect/lib/ConfigureService.php index e0d81af..5f6a013 100644 --- a/modules/servers/VirtFusionDirect/lib/ConfigureService.php +++ b/modules/servers/VirtFusionDirect/lib/ConfigureService.php @@ -8,9 +8,37 @@ use WHMCS\User\User; /** * Handles order-time and provisioning-time operations for VirtFusion servers. * - * Extends Module to provide package discovery, OS template fetching, server build - * initialization, and SSH key retrieval/creation. Used during WHMCS checkout and - * account creation flows rather than ongoing service management. + * WHY A SIBLING OF ModuleFunctions RATHER THAN METHODS ON IT + * ---------------------------------------------------------- + * ModuleFunctions handles the WHMCS LIFECYCLE (create, suspend, terminate, etc.) + * — operations driven by WHMCS service-state transitions. + * + * ConfigureService handles ORDER-TIME logic — package lookups, template fetching, + * SSH key creation, initial build triggering. These run during checkout (via the + * ClientAreaFooterOutput hook that populates dropdowns on the order form) and + * immediately after account creation (initServerBuild is called from + * ModuleFunctions::createAccount once the VirtFusion server exists). + * + * Splitting the concerns keeps ModuleFunctions focused on lifecycle state machines + * and ConfigureService focused on catalogue/discovery calls. They share the base + * Module's API plumbing via inheritance. + * + * CACHING + * ------- + * Package/template lookups use the module's Cache class with 10-minute TTLs. + * These values change rarely (a package list is typically edited once per + * month at most) but the endpoints are on the checkout hot path, so aggressive + * caching matters for page-load performance. + * + * CP RESOLVED IN CONSTRUCTOR + * -------------------------- + * Unlike ModuleFunctions which resolves the control panel per-request via the + * service ID, ConfigureService resolves it ONCE in the constructor via + * getCP(false, true) — "any available VirtFusion server". Order-time operations + * happen BEFORE a WHMCS service exists, so we can't dereference a specific + * server through mod_virtfusion_direct. "Any enabled server" is the pragmatic + * default for catalogue operations that typically return the same data + * regardless of which panel you hit. */ class ConfigureService extends Module { diff --git a/modules/servers/VirtFusionDirect/lib/Curl.php b/modules/servers/VirtFusionDirect/lib/Curl.php index 53fd40e..c9cccc7 100644 --- a/modules/servers/VirtFusionDirect/lib/Curl.php +++ b/modules/servers/VirtFusionDirect/lib/Curl.php @@ -17,7 +17,20 @@ class Curl /** @var array User-supplied cURL options that override defaults */ private $customOptions = []; - /** @var array Default cURL options applied to every request */ + /** + * @var array Default cURL options applied to every request. + * + * Rationale: + * VERIFYPEER/VERIFYHOST: Full TLS chain + hostname validation. Disabling + * either is a common source of MITM bugs, so we never do it silently. + * RETURNTRANSFER: We always want the response body back as a string. + * HEADER off: Callers almost never need headers. Saves a parse cycle. + * NOBODY off: Default to GET-style body-returning requests. + * TIMEOUT 30s: Covers slow API endpoints without letting a hung connection + * block a whole WHMCS request indefinitely. + * CONNECTTIMEOUT 10s: Separate from the total timeout so a failed TCP + * handshake (firewall black-hole) fails fast rather than burning 30s. + */ private $defaultOptions = [ CURLOPT_SSL_VERIFYPEER => true, CURLOPT_SSL_VERIFYHOST => 2, diff --git a/modules/servers/VirtFusionDirect/lib/Database.php b/modules/servers/VirtFusionDirect/lib/Database.php index fc0008d..967591d 100644 --- a/modules/servers/VirtFusionDirect/lib/Database.php +++ b/modules/servers/VirtFusionDirect/lib/Database.php @@ -7,12 +7,45 @@ use WHMCS\Database\Capsule as DB; /** * Handles all database operations for the module's custom table (mod_virtfusion_direct) * and queries against core WHMCS tables (tblhosting, tblclients, tblservers, etc.). + * + * SCHEMA AUTO-MIGRATION + * --------------------- + * schema() runs on every Module construction — the first call per request creates + * or migrates the module table and ensures all required custom fields exist on + * every VirtFusionDirect product. Subsequent calls within the same request hit + * the $fieldsChecked idempotency flag and short-circuit, so the overhead is + * one SHOW-columns query per request. + * + * This design means operators never need to run a separate install script — + * dropping the module files into place and hitting any admin page triggers the + * migration. The trade-off is small per-request overhead; we take it because + * WHMCS modules historically had fragile install/uninstall hooks. + * + * SCHEMA VERSIONING + * ----------------- + * No explicit version table. Migrations are expressed as "create if missing" + * checks — hasTable(), hasColumn() — which makes forward migration additive + * and safe to re-run. Deletions would require a proper versioning scheme, but + * we have none so far; every column added has been non-breaking. + * + * WHMCS TABLE ACCESS + * ------------------ + * Reads from tblhosting / tblclients / tblconfiguration are done via Capsule's + * fluent query builder, not raw SQL, to inherit WHMCS's database abstraction + * (connection pooling, character set, prepared statement handling). */ class Database { + /** Module's own per-service state table. Created on first Module instantiation. */ const SYSTEM_TABLE = 'mod_virtfusion_direct'; - /** @var bool Tracks whether custom field existence has already been verified this request. */ + /** + * @var bool Tracks whether custom field existence has already been verified this request. + * + * Custom-field creation is idempotent (updateOrInsert) but touching every + * product on every request is wasteful. This flag ensures it runs exactly + * once per PHP request. + */ private static $fieldsChecked = false; /** diff --git a/modules/servers/VirtFusionDirect/lib/Log.php b/modules/servers/VirtFusionDirect/lib/Log.php index dcaf51e..88126fa 100644 --- a/modules/servers/VirtFusionDirect/lib/Log.php +++ b/modules/servers/VirtFusionDirect/lib/Log.php @@ -3,18 +3,46 @@ namespace WHMCS\Module\Server\VirtFusionDirect; /** - * Thin wrapper around the WHMCS logModuleCall() function for module-level logging. + * Thin wrapper around the WHMCS logModuleCall() function. + * + * WHY A WRAPPER + * ------------- + * Consolidating log writes lets us: + * - Pin the module name in one place (the LOG_MODULE constant). All entries + * go under "VirtFusionDirect" regardless of which caller inserted them, + * which keeps WHMCS Admin → Utilities → Logs → Module Log filterable. + * - Get a stable import path for every file that logs (Log::insert). + * - Add cross-cutting policy later (e.g. redaction, sampling) without + * touching every call site. + * + * OUTPUT SURFACE + * -------------- + * Entries appear in WHMCS Admin → Utilities → Logs → Module Log. The request + * and response parameters accept strings OR arrays — WHMCS serialises arrays + * to readable form automatically. Pass structured data (["zone" => $z, "ip" => $ip]) + * rather than string-concatenated messages; the UI renders arrays as key/value + * pairs which makes filtering and debugging much easier. + * + * REDACTION EXPECTATION + * --------------------- + * Callers are responsible for not passing secrets into logs. In particular: + * - Never log Authorization/X-API-Key headers + * - Never log full request_header info from the Curl class + * - Never log the decrypted VirtFusion bearer token or PowerDNS API key + * The Curl class deliberately defaults CURLOPT_HEADER to off so header capture + * doesn't accidentally populate a field that callers might log. */ class Log { + /** Keep this in sync with the WHMCS server module name, so filters work. */ const LOG_MODULE = 'VirtFusionDirect'; /** * Write an entry to the WHMCS module log. * - * @param string $action Name of the action being logged (e.g. 'CreateAccount') - * @param string|array $requestString Request data sent to the API - * @param string|array $responseData Response data received from the API + * @param string $action Short tag identifying the operation (used as the "Function" column in the log UI) + * @param string|array $requestString Outbound payload or context data. Arrays preferred — rendered as key/value pairs. + * @param string|array $responseData Inbound response or result. Same conventions as $requestString. */ public static function insert($action, $requestString, $responseData) { diff --git a/modules/servers/VirtFusionDirect/lib/ServerResource.php b/modules/servers/VirtFusionDirect/lib/ServerResource.php index c74feeb..02813f1 100644 --- a/modules/servers/VirtFusionDirect/lib/ServerResource.php +++ b/modules/servers/VirtFusionDirect/lib/ServerResource.php @@ -4,6 +4,47 @@ namespace WHMCS\Module\Server\VirtFusionDirect; /** * Transforms a VirtFusion API server response into a flat key-value array for Smarty templates and admin display. + * + * WHY A FLAT ARRAY + * ---------------- + * Smarty templates can traverse nested structures (`{$data.network.interfaces[0].ipv4[0].address}`) + * but that leaks the API shape into the template layer. A flat array ("hostname", + * "primaryNetwork.ipv4[]", "memoryRaw", etc.) decouples the template from the upstream + * schema: if VirtFusion renames `network.interfaces` tomorrow, only this file needs + * to change. + * + * PRIMARY-INTERFACE-ONLY DESIGN + * ----------------------------- + * process() only reads interfaces[0]. That's the primary network — the one the + * client-area "Overview" card displays. Servers with multiple interfaces (common + * for dedicated IPMI networks, storage networks, etc.) still work for display + * because the primary interface holds the customer-facing IP. + * + * The reverse-DNS subsystem (PowerDns\IpUtil::extractIps) walks ALL interfaces + * explicitly because PTRs matter for every IP no matter which NIC it's on. + * If you add a feature that needs secondary-interface data for display, do NOT + * generalise this class — add a new one or a helper that doesn't disturb the + * well-tested primary-interface behaviour. + * + * UNIT CONVERSIONS + * ---------------- + * VirtFusion stores: + * - traffic as bytes (usage) or GB (limits) + * - storage as GB (limits) or bytes (usage) + * - memory as MB + * WHMCS expects MB for storage/traffic in tblhosting. This class produces two + * pairs of values per resource: a human-readable string with unit suffix + * (e.g. "200 GB") AND a raw integer without the unit (for slider UIs and + * arithmetic). Keep both — removing one breaks a UI consumer somewhere. + * + * "-" SENTINELS + * ------------- + * Fields that are missing or empty are rendered as "-" rather than empty strings. + * That makes the client-area card always have content (a dash is a valid visual + * placeholder) and distinguishes "missing data" from "empty string returned by + * the API". Consumers who need boolean presence checks should test against "-", + * not "" / null — and upstream (e.g. updateWhmcsServiceParamsOnServerObject) + * already does. */ class ServerResource {