Feat: A/AAA record support #9627
Conversation
WalkthroughThis pull request restructures domain configuration and validation logic by replacing the single Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant Ctrl as Controller
participant DV as Domain Validator
participant IPV as IP Validator
participant DNSV as DNS Validator
participant AnyOf as Aggregator Validator
participant Resp as Response Document
Client->>Ctrl: Request domain validation
Ctrl->>DV: Validate _APP_DOMAIN_TARGET_CNAME
DV-->>Ctrl: Domain validation result
Ctrl->>IPV: Validate _APP_DOMAIN_TARGET_A and _APP_DOMAIN_TARGET_AAAA
IPV-->>Ctrl: IPv4/IPv6 validation results
Ctrl->>DNSV: Create DNS validators for CNAME, A, AAAA
DNSV-->>Ctrl: Individual DNS validation outcomes
Ctrl->>AnyOf: Aggregate validation results
alt At least one validator passes
AnyOf-->>Ctrl: Valid domain response
Ctrl->>Resp: Return successful response with domain targets
else No validators pass
AnyOf-->>Ctrl: Aggregated error logs
Ctrl->>Resp: Throw exception with error details
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)tests/e2e/Services/Proxy/ProxyCustomServerTest.php (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
app/config/variables.php (1)
100-126: Clarify in the docstring that at least one domain target is required.
Although the code checks for at least one of these environment variables to be set, each is marked as optional. Consider adding a line in each variable’s description indicating that at least one must be configured for custom domain setup to work.'description' => 'A domain that can be used as DNS CNAME record. +At least one of _APP_DOMAIN_TARGET_CNAME, _APP_DOMAIN_TARGET_A, or _APP_DOMAIN_TARGET_AAAA must be set for proper domain validation.',src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
115-132: Robust multi-record validation mechanismThe implementation correctly builds a flexible domain validation system that supports CNAME, A, and AAAA records. This enables apex domain validation without requiring nameserver setup, which aligns with the PR objectives.
Consider adding a brief comment explaining the purpose of the
!$targetCNAME->isKnown() || $targetCNAME->isTest()condition for better maintainability.$targetCNAME = new Domain(System::getEnv('_APP_DOMAIN_TARGET_CNAME', '')); +// Only validate CNAME if the target isn't a known domain or is a test domain if (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { $validators[] = new DNS($targetCNAME->get(), DNS::RECORD_CNAME); }src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
107-124: Enhanced domain validation with apex domain supportThe implementation correctly enables apex domain validation by supporting A and AAAA records in addition to CNAME. The validation logic is consistent with the pattern used in the site rules creation.
Consider extracting this validation logic into a shared utility function since it's duplicated in both the API and Site Create classes.
app/controllers/api/console.php (1)
45-59: Comprehensive domain validation with multiple record typesThe implementation validates all three DNS record types (CNAME, A, AAAA) and enables the domain feature if any of them are valid, which aligns with the PR objectives.
Consider extracting this validation logic into a helper method to avoid code duplication across different controllers and improve maintainability.
// Example helper function function validateDomainTargets() { $validator = new Domain(System::getEnv('_APP_DOMAIN')); $isDomainValid = !empty(System::getEnv('_APP_DOMAIN', '')) && $validator->isKnown() && !$validator->isTest(); $isCNAMEValid = false; $isAValid = false; $isAAAAValid = false; // CNAME validation $validator = new Domain(System::getEnv('_APP_DOMAIN_TARGET_CNAME')); $isCNAMEValid = !empty(System::getEnv('_APP_DOMAIN_TARGET_CNAME', '')) && $validator->isKnown() && !$validator->isTest(); // A record validation $validator = new IP(IP::V4); $isAValid = !empty(System::getEnv('_APP_DOMAIN_TARGET_A', '')) && ($validator->isValid(System::getEnv('_APP_DOMAIN_TARGET_A'))); // AAAA record validation $validator = new IP(IP::V6); $isAAAAValid = !empty(System::getEnv('_APP_DOMAIN_TARGET_AAAA', '')) && $validator->isValid(System::getEnv 8000 ('_APP_DOMAIN_TARGET_AAAA')); return [ 'isDomainValid' => $isDomainValid, 'isCNAMEValid' => $isCNAMEValid, 'isAValid' => $isAValid, 'isAAAAValid' => $isAAAAValid, 'isDomainEnabled' => $isDomainValid && ($isAAAAValid || $isAValid || $isCNAMEValid) ]; }app/controllers/api/proxy.php (1)
233-248: Prevent potential log overload
Aggregating logs from multiple DNS validators can produce very large output. Consider truncating or summarizing logs to avoid excessive log usage.src/Appwrite/Platform/Workers/Certificates.php (1)
310-328: Single aggregator logic
Collecting logs from each DNS validator provides helpful debug info, but be mindful of log sizes. Consider truncating or summarizing if logs grow too large.src/Appwrite/Network/Validator/DNS.php (1)
42-86: Robust DNS record matching
Usingmatchexpressions to dynamically check DNS record types is well-structured. However, DNS responses sometimes include trailing periods or variants in naming. Consider normalizing domain names before the check to handle such cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.env(1 hunks)app/config/variables.php(1 hunks)app/controllers/api/console.php(3 hunks)app/controllers/api/proxy.php(3 hunks)docker-compose.yml(4 hunks)src/Appwrite/Network/Validator/DNS.php(3 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(3 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(3 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php(3 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php(3 hunks)src/Appwrite/Platform/Tasks/Doctor.php(2 hunks)src/Appwrite/Platform/Workers/Certificates.php(3 hunks)src/Appwrite/Utopia/Response/Model/ConsoleVariables.php(1 hunks)tests/e2e/Services/Console/ConsoleConsoleClientTest.php(1 hunks)tests/unit/Network/Validators/CNAMETest.php(0 hunks)tests/unit/Network/Validators/DNSTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/Network/Validators/CNAMETest.php
🧰 Additional context used
🧬 Code Definitions (4)
src/Appwrite/Platform/Tasks/Doctor.php (1)
src/Appwrite/Network/Validator/DNS.php (1)
isValid(48-86)
tests/unit/Network/Validators/DNSTest.php (1)
src/Appwrite/Network/Validator/DNS.php (2)
DNS(7-111)isValid(48-86)
app/controllers/api/console.php (1)
src/Appwrite/Network/Validator/DNS.php (1)
isValid(48-86)
app/controllers/api/proxy.php (3)
src/Appwrite/Network/Validator/DNS.php (3)
DNS(7-111)isValid(48-86)getLogs(36-39)src/Appwrite/Extend/Exception.php (1)
Exception(7-377)src/Appwrite/Utopia/Response.php (2)
dynamic(596-629)Response(131-862)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Benchmark
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
🔇 Additional comments (36)
tests/e2e/Services/Console/ConsoleConsoleClientTest.php (1)
27-30: Test updated to accommodate new domain target variables.The test correctly accommodates the restructuring from a single
_APP_DOMAIN_TARGETto three distinct variables (_APP_DOMAIN_TARGET_CNAME,_APP_DOMAIN_TARGET_A, and_APP_DOMAIN_TARGET_AAAA) that support different DNS record types. The assertion count has been properly incremented from 11 to 13, reflecting the addition of these new variables..env (1)
26-28: LGTM: Domain target split into record-specific variables.The restructuring of the domain configuration from a single target variable to three record-specific variables enhances support for both IPv4 (A) and IPv6 (AAAA) addresses in addition to the existing CNAME record. Default values are appropriate - the previous CNAME value is preserved, and standard documentation/local addresses are used for IP targets.
docker-compose.yml (4)
118-120: Configuration updated for main appwrite service.The environment variables for the main appwrite service have been properly updated to include all three domain target variables.
529-531: Worker certificates service updated with new domain targets.The environment variables for the certificate worker service have been properly updated to support all three DNS record types, which is essential for domain verification and certificate management.
697-699: Worker migrations service updated with new domain targets.The environment variables for the migrations worker service have been properly updated to support all three DNS record types.
731-733: Maintenance task service updated with new domain targets.The environment variables for the maintenance task service have been properly updated to support all three DNS record types.
tests/unit/Network/Validators/DNSTest.php (4)
1-18: Well-structured test class setup for DNS validator.The test class is properly organized with a clear namespace, appropriate imports, and standard setup/teardown methods. While the setup and teardown methods are currently empty, they provide a good structure for potential future enhancements.
19-27: Comprehensive CNAME record validation tests.The test properly validates the CNAME record functionality against various input cases (empty, null, boolean) and both positive and negative DNS resolution cases.
29-38: Thorough A record validation tests with proper IPv4 documentation address.The test correctly uses a documentation-purpose IPv4 address (203.0.113.1 from the TEST-NET-3 range) which is ideal for testing. The validation cases are comprehensive, covering edge cases and both positive and negative DNS resolution scenarios.
40-49: Complete AAAA record validation tests with proper IPv6 documentation address.The test appropriately uses a documentation-purpose IPv6 address (2001:db8::1) and follows the same comprehensive testing pattern as the other record types, ensuring consistent validation across all DNS record types.
src/Appwrite/Platform/Tasks/Doctor.php (1)
18-18: No concerns on the added import statement.app/config/variables.php (1)
93-93: Deprecation notice update looks correct.
You are correctly indicating that_APP_DOMAIN_TARGETis deprecated since version 1.7.0. No further issues here.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (6)
8-8: Added import for DNS validator.
No concerns here.
22-22: Introduction of AnyOf validator.
Good approach to allow multiple record types simultaneously.
24-24: Added import for IP validator.
No issues.
115-126: Check the domain condition for_APP_DOMAIN_TARGET_CNAME.
You might have intended to validate only when the domain is known and not a test domain. The current conditionif (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { … }can invert your logic. Consider verifying correctness.Would you like to confirm this condition is correct? If not, fix by changing it to something like:
-if (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { +if ($targetCNAME->isKnown() && !$targetCNAME->isTest()) { $validators[] = new DNS($targetCNAME->get(), DNS::RECORD_CNAME); }
127-129: Good catch for requiring at least one target.
This check enforces the user to have at least one valid_APP_DOMAIN_TARGET_*variable configured. No issues here.
131-131: AnyOf validator usage.
Nice solution to gather multiple validators into a single pass/fail check.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (6)
8-8: Import DNS validator.
Straightforward addition; no issues.
21-21: Introduction of AnyOf validator.
Keeps logic consistent across domain checks. Approved.
23-23: Added IP validator import.
No concerns.
107-118: Verify the domain condition.
Similar potential issue as in the function rules: consider whetherif (!$targetCNAME->isKnown() || $targetCNAME->isTest())is correct or reversed.
119-121: At-least-one-target check.
Works well to ensure domain validation can proceed with a valid record.
123-123: AnyOf validator usage.
Same approach as the function rules. Looks good.src/Appwrite/Utopia/Response/Model/ConsoleVariables.php (1)
13-30: LGTM: Well-structured DNS record target variablesThe implementation adds support for three different DNS record types (CNAME, A, AAAA) by replacing the single
_APP_DOMAIN_TARGETwith three specific variables. This supports the PR objective of enabling apex domain verification through multiple DNS record types.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
8-24: LGTM: Appropriate imports for DNS validationThe new imports correctly support the enhanced domain validation mechanism with DNS and IP validation.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
8-24: LGTM: Appropriate imports for DNS validationThe new imports correctly support the enhanced domain validation mechanism with DNS and IP validation.
app/controllers/api/console.php (2)
11-13: LGTM: Required imports for enhanced validationThe Domain and IP validator imports are necessary for the improved domain validation mechanism.
70-72: LGTM: Updated response with all three DNS record targetsThe response correctly includes all three DNS record target variables, which aligns with the PR objectives.
app/controllers/api/proxy.php (3)
7-7: Imports look correct
The new validators (DNS, AnyOf, IP) appear properly referenced and will be used in the updated domain verification logic.Also applies to: 24-25
225-226: Minimum one domain target requirement
Good fallback to ensure that at least one domain target environment variable is configured before proceeding.
213-223:❓ Verification inconclusive
Double-check domain condition
This block adds a DNS validator only if the CNAME target is unknown or is a test ((!$targetCNAME->isKnown() || $targetCNAME->isTest())). Verify if this is intentional, as one might expect domain validation to occur only for legitimate (known) domains instead.
🏁 Script executed:
#!/bin/bash # Searching for references to domain->isKnown() or domain->isTest() in codebase rg "->isKnown|->isTest"Length of output: 430
Verify Domain Condition for DNS Validator
The current implementation adds a DNS validator when the CNAME target’s domain is either not known or marked as a test (i.e. using the condition
(!$targetCNAME->isKnown() || $targetCNAME->isTest())). This is counterintuitive since one might expect domain validation to occur only for legitimate, known domains. However, our initial attempt to locate additional usage references forisKnown()andisTest()using a regular expression produced a shell error due to quoting issues. Please re-run the search (for example, using a command like the one below) and manually verify that this domain check is intentional and aligns with the overall validation strategy in the codebase.
- File:
app/controllers/api/proxy.php(lines 213-223)- Re-run Verification Script Example:
#!/bin/bash # Corrected search for method usage with proper escaping rg "\-\>isKnown\s*\(" . rg "\-\>isTest\s*\(" .- Action: Confirm whether the use of
(!$targetCNAME->isKnown() || $targetCNAME->isTest())should indeed trigger the addition of a DNS validator or if the condition needs to be revised so that validation applies only to verified (known) domains.src/Appwrite/Platform/Workers/Certificates.php (2)
11-11: Imports look correct
The new imports (DNS, AnyOf, IP) support the multi-record validation strategy and appear to be referenced appropriately.Also applies to: 31-32
295-308:❓ Verification inconclusive
Check domain validation condition
Similar to the Proxy controller, a DNS validator is added only if the CNAME target is unknown or is test. Confirm whether this condition is reversed or correct for production domains.
🏁 Script executed:
#!/bin/bash # Searching for references to domain->isKnown() or domain->isTest() in codebase rg "->isKnown|->isTest"Length of output: 430
Confirm Domain Validation Logic for Production Domains
The current logic adds a DNS validator only when the CNAME target is either not recognized or flagged as a test domain:
if (!$targetCNAME->isKnown() || $targetCNAME->isTest()) { $validators[] = new DNS($targetCNAME->get(), DNS::RECORD_CNAME); }Please verify that this behavior is intentional and consistent with how validation is handled in the Proxy controller. In production, if the CNAME target is known and not a test environment, no DNS validator is added—confirm that this is as expected. If production domains require validation, the condition might need to be reversed. Manual verification is recommended due to the earlier regex issues encountered when searching the codebase.
src/Appwrite/Network/Validator/DNS.php (2)
7-11: New constants for multiple DNS record types
Defining RECORD_A, RECORD_AAAA, and RECORD_CNAME greatly expands flexibility beyond handling just CNAME records.
21-21: Enhanced constructor and generic description
Allowing the constructor to specify record type (with a default of CNAME) and providing a broader "Invalid DNS record" message is a clean, extensible design.Also applies to: 30-30
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
Adds ipv4 and ipv6 support for custom domain verificaton. This allows apex domains without self-hosting entire nameserver.
Test Plan
Related PRs and Issues
x
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests