Skip to content

fix(reconcile): reject unknown top-level fields in desired-state documents - #1888

Draft
rohilsurana wants to merge 2 commits into
mainfrom
fix/reconcile-framework-known-fields
Draft

fix(reconcile): reject unknown top-level fields in desired-state documents#1888
rohilsurana wants to merge 2 commits into
mainfrom
fix/reconcile-framework-known-fields

Conversation

@rohilsurana

@rohilsurana rohilsurana commented Aug 17, 2026

Copy link
Copy Markdown
Member

What

The reconcile document decoder ignored unknown top-level keys. A stray key (a typo of spec, a misplaced metadata, a misspelled apiVersion that silently falls back to v1) was dropped instead of failing the file.

This adds KnownFields(true) to the document decoder, so an unknown top-level key fails the whole file up front, the same way entry decoding already rejects unknown fields. The spec's own content is decoded separately, so only the outer apiVersion/kind/spec envelope is guarded.

Why

RFC 0001 Rule 3 ("validate before apply") says the whole file is checked first, including that unknown fields are rejected. The document level did not enforce this, so a typo could pass validation and hide a mistake.

Testing

  • New unit test: a document with an unknown top-level field is rejected before any reconciler runs.
  • Full internal/reconcile package passes.

Stack

Base of a four-PR stack fixing reconcile-kind findings from the RFC 0001 audit:

  1. fix/reconcile-framework-known-fields (this PR)
  2. fix/reconcile-permission-spicedb-grammar
  3. fix/reconcile-billingproduct-r2
  4. docs/reconcile-platformuser-disabled-out-of-scope

Note on casing

Top-level keys are now case-sensitive. apiversion: or KIND: fail instead of silently defaulting. That is the intended typo-catch, and export always writes the correct casing, so exported files are unaffected.

@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview Aug 17, 2026 7:41am

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8eff51cb-2949-45a3-b401-398d32b4789d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Aug 17, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 32006910327

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.05%) to 48.747%

Details

  • Coverage increased (+0.05%) from the base build.
  • Patch coverage: 5 of 5 lines across 1 file are fully covered (100%).
  • 60 coverage regressions across 4 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

60 previously-covered lines in 4 files lost coverage.

File Lines Losing Coverage Coverage
internal/api/v1beta1connect/permission.go 29 59.8%
cmd/permission.go 17 45.58%
internal/bootstrap/schema/schema.go 12 26.83%
internal/reconcile/permission_reconciler.go 2 80.2%

Coverage Stats

Coverage Status
Relevant Lines: 40093
Covered Lines: 19544
Line Coverage: 48.75%
Coverage Strength: 15.67 hits per line

💛 - Coveralls

@rohilsurana

Copy link
Copy Markdown
Member Author

Review: reject unknown top-level fields (RFC 0001 Rule 3)

Verified on this branch: go build ./..., go test ./internal/reconcile/..., and golangci-lint run ./internal/reconcile/... all pass.

This does what it claims. dec.KnownFields(true) is set on the shared decoder in parseDocuments (reconcile.go:67) and the decoder is reused across the document loop, so every document's envelope is checked, before the validate and reconcile loops and any server call. Spec-level unknown fields were already caught by decodeSpec, so Rule 3 now holds at both levels. Export emits only apiVersion/kind/spec, so an exported file never trips the new check (Rules 1 and 5 stay intact).

Verdict: approve with nits.

Nits:

  1. The rejection error leaks the internal Go type name to operators. kind: PlatformUser\nspec: []\nspce: oops yields ...field spce not found in type reconcile.document. It names the field and line, which is good, but reconcile.document is an implementation detail. Consider wrapping to drop the type name.
  2. reconcile_test.go only covers a single-document file. The whole point is "check the whole file," so add a multi-document case where the stray key is in the second document, to lock in that the check fires on every document, not just the first.
  3. Not a bug, worth a line in the PR body: top-level keys are now case-sensitive, so apiversion: or KIND: now fail instead of silently defaulting. That is the intended typo-catch, and export always writes correct casing.

@rohilsurana

Copy link
Copy Markdown
Member Author

Follow-up on the review nits.

  • Multi-document test: done. Added a subtest where the stray key sits in the second document, so the check is locked to fire on every document, not just the first.
  • Case-sensitivity: added a note to the PR description. It is the intended typo-catch, and export always writes correct casing.
  • Type name in the error: keeping it. The useful signal is the field name and the line, which are both there. Stripping the trailing in type reconcile.document means string-surgery on the yaml library's error text, which is brittle. The type name is stable and harmless, so it is not worth that.

@rohilsurana

Copy link
Copy Markdown
Member Author

Re-review (head 3e21c89)

Build, tests, and lint are all green on the branch. The multi-document test an unknown field in a later document is rejected correctly locks in the every-document property: it puts the stray key in the second document and asserts a parse-time failure with no reconciler dispatched (rec.called == 0). Rule 3 and the export round-trip both still hold.

One nit still open, cosmetic: the error still surfaces the internal Go type name, ...field spce not found in type reconcile.document. Not a blocker.

Verdict: approve with nits.

// instead of ignoring them, the same way entry decoding does. The document's
// own spec content is decoded separately, so this only guards the outer
// apiVersion/kind/spec envelope.
dec.KnownFields(true)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (cosmetic, not a blocker): the strict decode this enables produces an error that leaks the internal Go type name, for example ...field spce not found in type reconcile.document. It names the offending field and line, which is good, but reconcile.document is an implementation detail an operator should not see. Consider wrapping the error at the return just below to drop the type name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this as is. The field name and the line number are both in the message, which is the part an operator acts on. Dropping the trailing in type reconcile.document means string-editing the yaml library's error text, which is brittle and would break quietly if the library changes its wording. The type name is stable and harmless, so it is not worth that. Noting it as cosmetic and out of scope here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants