fix(reconcile): reject unknown top-level fields in desired-state documents - #1888
fix(reconcile): reject unknown top-level fields in desired-state documents#1888rohilsurana wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Coverage Report for CI Build 32006910327Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.05%) to 48.747%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions60 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
Review: reject unknown top-level fields (RFC 0001 Rule 3) Verified on this branch: This does what it claims. Verdict: approve with nits. Nits:
|
|
Follow-up on the review nits.
|
|
Re-review (head 3e21c89) Build, tests, and lint are all green on the branch. The multi-document test One nit still open, cosmetic: the error still surfaces the internal Go type name, 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What
The reconcile document decoder ignored unknown top-level keys. A stray key (a typo of
spec, a misplacedmetadata, a misspelledapiVersionthat 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 outerapiVersion/kind/specenvelope 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
internal/reconcilepackage passes.Stack
Base of a four-PR stack fixing reconcile-kind findings from the RFC 0001 audit:
fix/reconcile-framework-known-fields(this PR)fix/reconcile-permission-spicedb-grammarfix/reconcile-billingproduct-r2docs/reconcile-platformuser-disabled-out-of-scopeNote on casing
Top-level keys are now case-sensitive.
apiversion:orKIND:fail instead of silently defaulting. That is the intended typo-catch, and export always writes the correct casing, so exported files are unaffected.