feat(llmo): resolve canonical host (redirect/DNS follow) for Cloudflare worker + route - #3065
feat(llmo): resolve canonical host (redirect/DNS follow) for Cloudflare worker + route#3065ssilare-adobe wants to merge 3 commits into
Conversation
…ng back to apex when www has no record
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
GitHub Actions
Inactive
There was a problem hiding this comment.
Hey @ssilare-adobe,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one blocking concern about unbounded DNS latency in the deploy request path.
Complexity: HIGH - medium diff; API surface risk flag.
Changes: Adds DNS verification to Cloudflare worker targetHost derivation so apex-only sites fall back to the apex instead of pointing the worker at a nonexistent www host (4 files).
Must fix before merge
- [Important] DNS resolution has no timeout, can stall the deploy endpoint for 30-60s under adverse conditions -
src/support/edge-routing-utils.js:83(details inline)
Non-blocking (4): minor issues and suggestions
- nit: Return value casing is inconsistent - early-return path preserves original case from calculateForwardedHost, fallback path returns lowercased originalHost -
src/support/edge-routing-utils.js:116 - nit:
log.info(...)in resolveCanonicalHost fallback path uses direct access while hostResolves useslog?.info(...)- inconsistent optional handling despite log being documented as required -src/support/edge-routing-utils.js:121 - suggestion: Default mockResolveCanonicalHost stub is synchronous but the real function is async - use
async (baseURL, l) => calculateForwardedHost(baseURL, l)for contract fidelity -test/controllers/llmo/llmo-cloudflare.test.js:96 - suggestion: Demote the per-host DNS result log in hostResolves from info to debug - in steady state (www resolves fine) this adds noise to every deploy with no actionable signal -
src/support/edge-routing-utils.js:87
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 15s | Cost: $3.60 | Commit: 29414be84d2fc9d225985e2baa196725c1cf104f
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| * @returns {Promise<boolean>} True if any record type resolves, false otherwise. | ||
| */ | ||
| export async function hostResolves(host, log) { | ||
| const [cnames, ipv4, ipv6] = await Promise.all([ |
There was a problem hiding this comment.
issue (blocking): hostResolves fires three parallel DNS queries with no explicit timeout. Node dns.promises inherits the OS resolver config (often 5s per attempt with multiple retries per nameserver), so a fully-unresponsive authoritative NS can stall this call for 30+ seconds. Since this runs in the request path of POST /sites/:siteId/llmo/cdn-onboard/cloudflare/deploy, a slow lookup makes the endpoint appear hung to the caller.
Every other external I/O call in this module is timeout-guarded. A Promise.race with a ceiling (3-5s total) that treats timeout as "does not resolve" would keep behavior consistent and surface the fallback path faster:
const RESOLVE_TIMEOUT_MS = 5000;
const results = await Promise.race([
Promise.all([
dns.resolveCname(host).catch(() => []),
dns.resolve4(host).catch(() => []),
dns.resolve6(host).catch(() => []),
]),
new Promise((resolve) => setTimeout(() => resolve([[], [], []]), RESOLVE_TIMEOUT_MS)),
]);
const [cnames, ipv4, ipv6] = results;This keeps the tolerant "treat timeout as no records" semantics the function already uses for rejections.
MysticatBot
added
ai-reviewed
…ect-follow to canonical host resolution
|
This PR will trigger a minor release when merged. |
ssilare-adobe
changed the title
GitHub Actions
Inactive
…path) instead of deriving the whole pattern
ssilare-adobe
changed the title
GitHub Actions
Inactive
MysticatBot
left a comment
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @ssilare-adobe,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - the DNS timeout gap flagged in the prior review remains unaddressed.
Complexity: HIGH - medium diff; API surface risk flag.
Changes: Adds redirect-follow and DNS verification to Cloudflare worker targetHost derivation + route-pattern host normalization so apex-only sites route correctly (7 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently failing (branch-deploy, deploy-stage, semantic-release) - resolve before merge.
Must fix before merge
- [Important] DNS resolution has no timeout, can stall deploy/route endpoints for 30+ seconds under adverse conditions -
src/support/edge-routing-utils.js:82(details inline)
Non-blocking (5): minor issues and suggestions
- nit: Return value casing inconsistent across resolveCanonicalHost paths - early return preserves original case from calculateForwardedHost, fallback returns lowercased originalHost. Normalize with toLowerCase on the early return. -
src/support/edge-routing-utils.js:156 - nit:
log.info(...)without optional chaining in resolveCanonicalHost fallback path, while hostResolves and probeServingHost both uselog?.info(...)-src/support/edge-routing-utils.js:165 - suggestion: Demote per-host DNS result log in hostResolves from info to debug - in steady state this fires on every deploy/route call with no actionable signal -
src/support/edge-routing-utils.js:88 - suggestion: Use
new URL(location, baseURL)for Location header parsing to handle protocol-relative URLs per RFC 3986 -src/support/edge-routing-utils.js:125 - nit: Default mockResolveCanonicalHost should be async to match production contract fidelity -
test/controllers/llmo/llmo-cloudflare.test.js:98
| * @returns {Promise<boolean>} True if any record type resolves, false otherwise. | ||
| */ | ||
| export async function hostResolves(host, log) { | ||
| const [cnames, ipv4, ipv6] = await Promise.all([ |
There was a problem hiding this comment.
issue (blocking): hostResolves fires three parallel DNS queries (resolveCname, resolve4, resolve6) with no explicit timeout. Node dns.promises inherits the OS resolver config (often 5s per attempt with multiple retries), so a fully-unresponsive authoritative NS can stall this call for 30+ seconds. Since this runs in the request path of both POST .../deploy and POST .../routes, a slow lookup makes the endpoint appear hung to the caller.
Every other external I/O call in this module is timeout-guarded (probeServingHost uses AbortSignal.timeout(PROBE_TIMEOUT_MS)). A Promise.race with a ceiling that treats timeout as 'does not resolve' would keep behavior consistent and bound the worst-case latency of the endpoint.
MysticatBot
added
the
needs-human-review
|
|
||
| let redirectHost; | ||
| try { | ||
| redirectHost = new URL(location.startsWith('http') ? location : `https://${location}`) |
There was a problem hiding this comment.
[P1] Resolve relative redirects against the probed URL
Using the actual tracingFetch User-Agent, https://gentingsingapore.com returns 301 with Location: /en/home. This construction parses that value as https:///en/home, reads en as the hostname, and treats it as an off-domain redirect. The resolver then falls through to DNS and selects www.gentingsingapore.com even though www redirects back to the apex. Please use new URL(location, baseURL) so absolute, protocol-relative, and relative redirects resolve correctly.
|
|
||
| // calculateForwardedHost only rewrites a bare apex into www.<apex>. When it returns the original | ||
| // host unchanged (already www, or a subdomain) the candidate is authoritative — skip all I/O. | ||
| if (candidate.toLowerCase() === originalHost) { |
There was a problem hiding this comment.
[P1] Resolve registrable apexes using the Public Suffix List
With the actual stored base URL https://racq.com.au/, calculateForwardedHost returns racq.com.au because its dot-count heuristic mistakes multi-part public suffix apexes such as .com.au for subdomains. This early return skips the live 301 redirect to www.racq.com.au, so both targetHost and normalized root routes use the wrong host. Please identify apex/www root variants using PSL-based registrable-domain logic rather than candidate equality.
| } | ||
|
|
||
| // candidate is a www host synthesized from the bare apex `originalHost`. | ||
| const served = await probeServingHost(baseURL, originalHost, log); |
There was a problem hiding this comment.
[P1] Probe and follow the preferred www candidate
The intended flow is to prefer www, request it, and use the apex when www redirects there. This implementation probes only the base apex and, when that result is inconclusive, checks only whether www has DNS; it never requests or follows www. For Genting, www.gentingsingapore.com redirects to the apex. Please probe the preferred candidate and follow same-domain redirects instead of treating DNS existence as canonical-host evidence.
|
|
||
| // Probe undetermined (network error, opaque status, off-domain redirect) — trust the synthesized | ||
| // www host only if it resolves in DNS, otherwise fall back to the apex. | ||
| if (await hostResolves(candidate, log)) { |
There was a problem hiding this comment.
[P2] Do not turn a WAF-blocked probe into a host decision
This probe currently inherits the Spacecat/1.0 User-Agent from tracingFetch, while related Optimize at Edge probes use AdobeEdgeOptimize-Test AdobeEdgeOptimize/1.0. WAFs may handle or block these product User-Agents differently. A 403, 429, 5xx, or network failure from either probe is inconclusive, but this fallback can select www solely because it has DNS. Explicitly choose and document the intended probe User-Agent, test both Spacecat/1.0 and AdobeEdgeOptimize/1.0 where relevant, and probe both root candidates or retain a validated/configured host when HTTP resolution is inconclusive.
| res = await fetch(baseURL, { | ||
| method: 'GET', | ||
| redirect: 'manual', | ||
| signal: AbortSignal.timeout(PROBE_TIMEOUT_MS), |
There was a problem hiding this comment.
[P2] Pass the timeout through tracingFetch
tracingFetch creates its own signal from options.timeout or its 10-second default and overwrites the supplied AbortSignal before calling Adobe fetch. Therefore AbortSignal.timeout(PROBE_TIMEOUT_MS) does not enforce the intended five-second ceiling here. Please pass timeout: PROBE_TIMEOUT_MS through the supported tracingFetch option.
| const clientHost = routePatternHostGlob(clientPattern); | ||
| if (clientHost === siteApex || clientHost === `www.${siteApex}`) { | ||
| const canonicalHost = await resolveCanonicalHost(site.getBaseURL(), log); | ||
| pattern = `${canonicalHost}${routePatternPath(clientPattern)}`; |
There was a problem hiding this comment.
[P1] Preserve a validated customer route-host correction path
Both apex and www client patterns are unconditionally replaced with the inferred host. If resolution is wrong, RACQ cannot submit www.racq.com.au/* and Genting cannot submit gentingsingapore.com/* to correct it because the API rewrites the pattern again. If customer correction is required, preserve an explicit validated root-host choice or expose a separate override; the worker upstream targetHost can remain server-controlled.
| export const routePatternPath = (pattern) => { | ||
| const withoutScheme = pattern.replace(/^https?:\/\//i, ''); | ||
| const slashIndex = withoutScheme.indexOf('/'); | ||
| return slashIndex === -1 ? '/*' : withoutScheme.slice(slashIndex); |
There was a problem hiding this comment.
[P2] Preserve the complete route pattern while replacing its host
Rebuilding the pattern from canonicalHost plus this extracted path removes an explicit https:// scheme and changes a pattern with no path into /*. Cloudflare treats scheme-specific, scheme-less, root-only, and wildcard-path routes differently. Please replace only the hostname in the original pattern so its scheme and exact path scope remain unchanged.
|
@jindaliiita could you please have a look at the canonical-host and route-resolution behavior in this PR and see if directions is correct. I added inline review comments. The two concrete customer cases use these stored base URLs:
I also flagged WAF/User-Agent ambiguity, the tracingFetch timeout option, customer route-host correction, and preservation of the complete Cloudflare route pattern. |
3 participants
In simple terms
When we onboard a customer onto Cloudflare, we set up two things: a worker (which forwards their traffic) and a route (which decides when the worker runs). Both need to point at the customer's real website host.
The tricky part is
www. Some sites live atwww.example.com, some at the bareexample.com. Our old logic just assumed "bare domain → addwww". That's wrong for sites that only work on the bare domain — e.g.gentingsingapore.comserves from the apex, so pointing atwww.gentingsingapore.com(which doesn't work) broke onboarding.This PR makes us figure out the real host instead of guessing:
wwwhost even exist?www, use the bare domain.We apply that resolved host to the worker (
targetHost/x-forwarded-host) and to the route. For the route we only fix the host — if the incoming pattern points at the site's root domain (its apex orwww), we swap in the correct host and keep the client's path (/blog/*,/*, …). Specific-subdomain and wildcard patterns are left exactly as sent. The client still sends the pattern; nothing about the path changes.Follow-up to #3027. Fixes the
gentingsingapore.comonboarding breakage.Technical detail
1.
resolveCanonicalHost(edge-routing-utils.js) — DNS + redirect followLayers on top of
calculateForwardedHost(apex→www):redirect: 'manual') — a 2xx means the apex serves (use apex); a 301/302/307/308 to a same-domain host follows to that host (apex→www resolves to www).2. Deploy
targetHost(llmo-cloudflare.js)targetHost(the worker'sx-forwarded-host) now derives viaresolveCanonicalHostinstead of the blindcalculateForwardedHost, sogentingsingaporeforwards to its apex.3. Route — host normalized, pattern/path preserved (llmo-cloudflare.js
addRoute)The client still supplies
pattern(unchanged contract). When the pattern's host is the site's root host (apex orwww.<apex>), only the host is swapped forresolveCanonicalHost(baseURL)and the client path is preserved viaroutePatternPath(e.g.example.com/blog/*+ apex-only site →gentingsingapore.com/blog/*). Specific-subdomain (shop.example.com/*) and wildcard (*.example.com/*) patterns are left untouched, and the existing in-domain validation + wildcard-aware conflict/idempotency detection are unchanged.Why not the cloudflare client
spacecat-shared-cloudflare-clientis a thin CF REST wrapper with no site/baseURL knowledge — host derivation belongs in api-service next to the existing DNS/probe helpers. Public DNS + an HTTP probe answer "which host serves" without the customer's CF zone.Test plan
resolveCanonicalHost: already-www/subdomain (no I/O), apex-serves-2xx→apex, same-domain 301→www, scheme-less redirect, off-domain redirect→DNS, no-Location→DNS, unparseable Location→DNS, opaque status→DNS, probe-error+www-dead→apex fallback, throw propagationhostResolves: CNAME/A/AAAA true, tolerant false on rejections, optional loggerroutePatternPath: path glob extraction,/*default, scheme strippingaddRoute: client pattern passthrough, apex→www and www→apex root-host normalization preserving the path, subdomain/wildcard left unchanged (resolver not called), 500 on derivation failure, plus the original conflict/idempotency/validation suitenpm run lintclean; 167 passing; coverage 100% lines/stmts/funcs on edge-routing-utils.js, llmo-cloudflare.js, llmo-cloudflare-utils.js