fix(terraform): plan breaks when cloudflare_custom_domain is unset#882
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates Cloudflare custom-domain support in the production Terraform environment by tightening input validation, normalizing custom-domain values, enforcing plan-time gating, rewiring Cloudflare worker configuration, and expanding setup and troubleshooting documentation. ChangesCloudflare Custom Domain Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Summary
PR #882, fix(terraform): plan breaks when cloudflare_custom_domain is unset, by ColeMurray. Reviewed 6 files (+63/-19); the change replaces null-unsafe custom-domain handling with normalized locals, hard validation, canonical workers.dev behavior, and matching docs updates.
Critical Issues
None found.
Suggestions
None blocking. A future hardening improvement could add hostname length validation (per-label and total length), but the current bare-hostname check already prevents the important malformed inputs called out in the PR.
Positive Feedback
The normalized locals remove the Terraform coalesce edge case cleanly and avoid using trimmed values inconsistently across the gate, URL, and custom-domain resource.
Hard validation is a good improvement over the previous advisory check because it prevents silently deploying with OAuth/NEXTAUTH_URL pointed at an unintended origin.
Disabling workers.dev when a custom domain is attached makes the canonical-origin behavior explicit and the docs clearly call out the deployer-facing impact.
Questions
None.
Verdict
Approve. I reviewed the full diff and did not find correctness, security, or maintainability issues that should block merging. I could not run Terraform validation locally because this checkout is on main with unrelated dirty files rather than the PR contents, so this review is based on gh pr diff 882 and surrounding repository context.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Follow-ups to #747 (custom domain support for the Cloudflare web Worker).
Bug:
terraform planfails for every Cloudflare-platform deployment without a custom domainTerraform's
coalesce()errors when all arguments are null or empty strings —coalesce(null, "")is a hard error, not"":The
web_custom_domain_enabledlocal from #747 evaluatestrimspace(coalesce(var.cloudflare_custom_domain, ""))wheneverweb_platform == "cloudflare"(&&short-circuits, so Vercel deployments escape). Bothcloudflare_custom_domainandcloudflare_zone_iddefault tonull, so any existingweb_platform = "cloudflare"deployment that hasn't opted into a custom domain now fails at plan time. Settingcloudflare_custom_domain = ""explicitly errors the same way.This slipped past
terraform validatebecause validate treats variables as unknown values — the error only fires on plan/apply with concrete values.Fix: null-safe normalization locals (
var.x == null ? "" : trimspace(var.x)) consumed by the gate, the host, and thecloudflare_workers_custom_domainresource. This also fixes the inconsistency where the gate trimmed the domain but the resource/URL consumed the raw value.Hard validation instead of an advisory warning
The
checkblock from #747 had the samecoalesceproblem (it would report an evaluation error rather than its intended message), and as a warning it let a misconfigured domain silently fall back to workers.dev — leavingNEXTAUTH_URLand OAuth callbacks pointing somewhere the operator didn't intend. Replaced with two hard failures:validationblock oncloudflare_custom_domainfor hostname shape: must be a bare hostname — rejects scheme, port, path, trailing dot, whitespace, wildcardsterraform_data.cloudflare_custom_domain_gateprecondition inchecks.tffor the cross-input policy (same pattern as the existingaccess_control_gate), expressed against the normalized locals aslocal.web_custom_domain == "" || local.web_custom_domain_enabled: a set domain requiresweb_platform = "cloudflare"and a non-emptycloudflare_zone_id(also catches the previously-silent domain-set-on-Vercel case)null,"", and whitespace-only all still mean "not configured".Single canonical origin
Attaching a custom domain doesn't disable the workers.dev route, so the app was reachable on two origins with
NEXTAUTH_URLpointing at only one. The generatedwrangler.production.tomlnow setsworkers_dev = falsewhen the custom domain is enabled (trueotherwise — existing behavior, now explicit).Docs
GETTING_STARTED.md: custom-domain subsection under Step 8 (vars, token permission, OAuth-callback reminder), custom-domain entries in the callback-URL list and redirect-URI troubleshooting, commented vars in the tfvars sample.terraform.tfvars.example: notes that workers.dev is disabled and GitHub/Google OAuth callback URLs must be updated to the new hostname.Verification
terraform fmt -checkandterraform validatepass. Validation and locals exercised end-to-end withterraform planon a minimal harness under Terraform 1.14.6 (the repo's required floor):enabled = false— previously the coalesce errorweb_platform = "vercel"web_platform = "cloudflare"enabled = true, host = custom domainhttps://app.example.com""enabled = falseSummary by CodeRabbit
New Features
workers.devroute disabled when enabled).Bug Fixes
null/empty/whitespace as unset and validating against a bare hostname format.