Repository navigation
Distinguish Cloudflare DNS validation in domain status - #346
Conversation
![agent-zhang-beihai[bot]](https://avatars.githubusercontent.com/in/3908133?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by Wang Miao
This PR changes insta domain check so that Cloudflare's _cf-custom-hostname TXT record (purpose: edge_ownership) no longer takes the platform's ownership-TXT slot. When that record is unchecked, it no longer counts as a blocker telling the user to re-run the check; the certificate stage and configured decide the verdict instead. I traced it against the compute plane's contract and found nothing to change, so I approve.
No findings.
Evidence
read-the-code:
src/commands/compute.ts:139-233:domainStatusLines.servingstill requiresr.configuredand an empty blocker list.test/compute-domain-region.test.ts:121-147.InsForge/instacloud-computeinternal/api/domains.go:200-280.domainRecordsalways sends the edge TXT asunchecked, and the comment above it names the edge's own hostname state as the authority.InsForge/instacloud-computedocs/custom-domains.md:140-215. Compute setsactiveonly after Cloudflare reports the hostname active and the certificate active, so dropping the edge-TXT blocker can't produce a falseserving.- A payload with no
purposefield (an older platform) behaves exactly as before. - No other
'TXT'consumers exist insrc/.
There was a problem hiding this comment.
Reviewed by Yang Dong
This change teaches the CLI to treat Cloudflare’s edge-ownership TXT as informational and defer readiness to certificate status. The discriminator is lost in the control plane before reaching the CLI, so the intended behavior never occurs and I would request changes.
The control plane drops purpose, so the Cloudflare exception never runs
important · defect · correctness · src/commands/compute.ts:196
The compute daemon emits purpose: "edge_ownership", but the platform adapter reconstructs every DNS record without purpose, and the response envelope consequently cannot forward it. Every edge TXT therefore reaches this branch with purpose === undefined, remains an unchecked blocker, and active domains still end with serving not yet; forward the field through the platform’s CustomDomainStatus, DnsRecord, and response mapping.
Evidence
read-the-code — src/commands/compute.ts:59-64,139-201; InsForge/instacloud-platform: src/adapters/insta-compute.ts:2273-2287, src/adapters/types.ts:276-282, src/provisioning/deploy.ts:110-121, src/server.ts:3368-3375; InsForge/instacloud-compute: internal/api/domains.go:199-278, internal/api/domains_test.go:296-329
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/compute.ts">
<violation number="1" location="src/commands/compute.ts:198">
P3: The special-casing only covers the `unchecked` state: an edge-ownership TXT that the plane reports as `ok` (reachable via the `r.configured ? 'ok'` fallback when the plane omits the per-record status) falls into this branch and renders under the generic `txt` label, so the new distinction disappears right when the record validates. Render it as `edge TXT` there too, or the purpose distinction is only half applied.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (st === 'ok') stage(lbl, 'ok', where) | ||
| if (d.type === 'TXT' && d.purpose === 'edge_ownership' && st === 'unchecked') { | ||
| stage('edge TXT', 'unchecked', `${where} (Cloudflare validates this record; see certificate status below)`) | ||
| } else if (st === 'ok') stage(lbl, 'ok', where) |
There was a problem hiding this comment.
P3: The special-casing only covers the unchecked state: an edge-ownership TXT that the plane reports as ok (reachable via the r.configured ? 'ok' fallback when the plane omits the per-record status) falls into this branch and renders under the generic txt label, so the new distinction disappears right when the record validates. Render it as edge TXT there too, or the purpose distinction is only half applied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/compute.ts, line 198:
<comment>The special-casing only covers the `unchecked` state: an edge-ownership TXT that the plane reports as `ok` (reachable via the `r.configured ? 'ok'` fallback when the plane omits the per-record status) falls into this branch and renders under the generic `txt` label, so the new distinction disappears right when the record validates. Render it as `edge TXT` there too, or the purpose distinction is only half applied.</comment>
<file context>
@@ -194,14 +188,14 @@ export function domainStatusLines(r: DomainView, ctx: DomainCmdCtx = {}): string
- if (st === 'ok') stage(lbl, 'ok', where)
+ if (d.type === 'TXT' && d.purpose === 'edge_ownership' && st === 'unchecked') {
+ stage('edge TXT', 'unchecked', `${where} (Cloudflare validates this record; see certificate status below)`)
+ } else if (st === 'ok') stage(lbl, 'ok', where)
else if (st === 'mismatch') { stage(lbl, 'mismatch', `${d.type} ${d.name} must point at ${d.value}`); blockers.push(`fix the ${d.type} ${d.name}`) }
else if (st === 'missing') { stage(lbl, 'pending', `add ${where}`); blockers.push(`add the ${d.type} ${d.name}`) }
</file context>
| } else if (st === 'ok') stage(lbl, 'ok', where) | |
| } else if (st === 'ok') stage(d.purpose === 'edge_ownership' ? 'edge TXT' : lbl, 'ok', where) |
There was a problem hiding this comment.
Declining: this state is not emitted by the current compute contract. internal/api/domains.go:domainRecords always sets both Purpose: purposeEdgeOwnership and Status: "unchecked" on this TXT, including active domains. The platform companion #602 preserves both. For hypothetical future statuses the existing name/value and explicit verdict are still displayed; changing that label does not fix a current defect.
|
The platform dependency is implemented in InsForge/instacloud-platform#602, head The rollout remains platform first, then the required E2E assertion. No CLI change is needed to address the passthrough finding. This CLI remains compatible with older responses and is not a CLI release. |
2 participants
insta domain checktells users to re-run the check for Cloudflare validation TXT records even though compute deliberately reports those records as unchecked. Use the optional DNS record purpose to show who validates the record and defer its verdict to the existing certificate status. Keep the record name/value visible and exclude an edge TXT from the platform-ownership slot.Advances InsForge/instacloud-platform#506. Requires the platform to preserve its provider-supplied purpose field; older responses retain existing behavior. No commands, flags or CLI release are changed.
Validation: typecheck, build and all 2,008 tests passed. Independent review is clean; mutation checks cover the new distinction and displayed TXT values.
Summary by cubic
Fixes
insta domain checkmisreporting Cloudflare validation TXT records as needing a re-run when compute intentionally leaves them unchecked. Uses the optional DNS record purpose to label edge TXT records and defer their verdict to the existing certificate status.Requires the platform to preserve its provider-supplied
purposefield; older responses retain existing behavior. Advances InsForge/instacloud-platform#506.Written for commit 4b96c6b. Summary will update on new commits.