config: fix two yaml tags that never matched the wire - #18
Conversation
PassthroughPatterns was tagged passthrupatterns; the published config
says passthroughpatterns. The field never populated, so Lookup's
passthrough branch has been dead since it was written. Harmless so far
only because all three providers publish an empty list.
Provider.VerifyHostname was tagged verifyhostname, but that field is the
one place flashlight's ProviderConfig carries an explicit yaml tag, and
it is camelCase. It is also omitempty and unset everywhere, so the key
never reaches the wire today — the tag would have failed the first time
a provider pinned a hostname. Masquerade.VerifyHostname is untagged
upstream and does arrive lowercased; the asymmetry is the generator's.
Both bugs are invisible to the existing tests, which build Provider as a
Go literal and never parse the wire. Adds tests that do:
- TestWireKeysAreClaimed diffs the keys in the committed
fronted.yaml.gz against the structs' yaml tags. Value assertions
cannot catch this class of bug — a key that fails to bind is
indistinguishable from one the config left empty, which is exactly
how passthrupatterns survived.
- TestVerifyHostnameCasing pins the casing the key-set test cannot
see, since the key is absent from the wire entirely.
- TestPublishedConfigParses asserts the parsed result is non-degenerate.
validator is on the wire with no field to bind to; it is listed in
unmodeledKeys as deliberately ignored rather than modeled, since
domainfront does not enforce rejectstatus.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughChangesConfiguration wire format
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config_wire_test.go`:
- Around line 22-24: Scope the validator exception to Provider by replacing the
global unmodeledKeys allowlist with a reflect.Type-keyed mapping and updating
assertClaimed to consult the allowlist for the current mapping type. Ensure only
Provider permits validator, while root, Masquerade, SNIConfig, and CA mappings
continue rejecting unexpected keys.
- Around line 55-68: Remove the break statements from the masquerades and
frontingsnis loops in the relevant wire-configuration test so every nested
mapping is passed to assertClaimed, preserving the existing type checks and
failure paths for each entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f203b2cf-174e-4fb9-b8e4-69e05a5a8289
📒 Files selected for processing (2)
config.goconfig_wire_test.go
There was a problem hiding this comment.
Pull request overview
This PR fixes two YAML struct tags in config.go that previously didn’t match the published fronted.yaml.gz wire keys, which could silently leave fields zero-valued during yaml.Unmarshal. It also adds tests that validate the committed artifact’s key set against the parser’s struct tags to prevent future silent drift.
Changes:
- Fix
Provider.PassthroughPatternsYAML tag to match the published key (passthroughpatterns). - Fix
Provider.VerifyHostnameYAML tag casing to match the generator’s camelCase key (verifyHostname). - Add tests that (a) ensure all published keys are claimed by struct tags, (b) ensure the published config parses into non-degenerate values, and (c) pin the special-case casing for
verifyHostname.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| config.go | Corrects YAML tags for PassthroughPatterns and VerifyHostname, and documents why wire-key exactness matters. |
| config_wire_test.go | Adds artifact-based key-coverage tests and a casing regression test for verifyHostname. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses PR review. unmodeledKeys was global, so "validator" was tolerated at any level even though it is a per-provider key — an unexpected validator under a masquerade, SNI config or CA would have slipped through. It is now keyed by reflect.Type, and only Provider excepts it. The per-level sampling is gone too. All 2090 masquerades in the current artifact publish an identical key set, and they always will while the generator marshals a tagless struct, but that is the generator's business and this test exists to stop trusting its shape. assertClaimed now takes every node at a level, resolving the tag set once and reporting unclaimed keys as a set, so full coverage costs one pass and still yields a single readable failure. Co-Authored-By: Claude <noreply@anthropic.com>
Two yaml tags in
config.gohave never matched the config we actually publish. Neither produces an error —yaml.Unmarshalignores keys no field claims, and matching is case-sensitive — so both fail by leaving the field zero.1.
passthrupatterns→passthroughpatterns(live bug)The published
fronted.yaml.gzwritespassthroughpatterns(lines 4150 / 11756 / 15790). The struct asked forpassthrupatterns.PassthroughPatternstherefore never populated, which makes the passthrough branch ofLookup(config.go:76-82) dead code, along with the lowercasing inExpandedProvider(config.go:155-156).No behavior change today. All three providers publish
passthroughpatterns: [], so the field is empty either way — verified by parsing the committed artifact:That is also precisely why it went unnoticed, and why it would have become an outage the day someone populated the list.
2.
verifyhostname→verifyHostnameonProvider(latent)The generator marshals flashlight's
config.ProviderConfig, whose fields carry no yaml tags — so keys arrive as lowercase-concatenated Go field names.VerifyHostnameis the single exception; it has an explicit tag upstream:So the wire key is camelCase. Because it is also
omitemptyand unset on every provider, the key is absent from the published config entirely and the mismatch cannot bite yet — it would have, the first time a provider pinned a hostname.Masquerade.VerifyHostnameis untagged upstream and genuinely does arrive lowercased. The resulting asymmetry looks like a typo, so there is now a comment and a test saying it isn't one.Why the existing tests missed both
Every test that touches
Providerbuilds it as a Go literal (config_test.go:16,bench_test.go:83) and sets the field directly, which passes regardless of the tag. The only code that reads the real artifact isintegration_test.go, behind//go:build integrationand dependent on live CloudFront.The struct already carried a comment warning about exactly this failure mode on
FrontingSNIs— someone hit it, documented it, and didn't audit the neighbors.Tests added
TestWireKeysAreClaimeddiffs the key set of the committedfronted.yaml.gzagainst the yaml tags on the structs, via reflection, at every level (root, provider, masquerade, SNI config, CA).Asserting on parsed values cannot catch this class of bug: a key that fails to bind is indistinguishable from one the config legitimately left empty. Comparing key sets fails on the tag itself. Verified as a real guard — reverting the tag to
passthrupatternsproduces:TestVerifyHostnameCasingpins the casing the key-set test structurally cannot see, since that key never appears on the wire. Also verified by reverting: fails with "provider verifyHostname must bind from the camelCase key the generator emits".TestPublishedConfigParsesasserts the parsed result is non-degenerate (CAs build a usable pool, every provider has masquerades / test URL / host aliases).A note on
validatorThe wire carries a per-provider
validator: {rejectstatus: [403]}block that no field binds to. It is listed inunmodeledKeysas deliberately ignored rather than modeled — domainfront doesn't enforcerejectstatus. Flagging it explicitly so the strict key check doesn't fail, and so the omission is a recorded decision rather than another silent gap.Worth knowing
TestWireKeysAreClaimedruns against the committed artifact, which the pipeline updates regularly ("updated fronted.yaml.gz" commits). If the generator starts emitting a new key, CI now fails on that update commit instead of silently dropping the field. That is the intent, but it does mean an artifact update can turn CI red — the fix is to model the key or add it tounmodeledKeyswith a reason.Verification
CI's exact command (
go test -race -count=1 -timeout 60s ./...) passes, as dogo build ./...andgo vet ./....gofmtclean on both changed files —bench_test.go,cache.goandconnected_test.goare unformatted onmainalready and are left alone.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests