Merge remote-tracking branch 'origin/dev/v2.1' into perf/ownership-label-injection-streaming - #484
Conversation
Every request the ownership middleware injects an owner label into ran filter.RejectDuplicateCaseVariantJSONKeys over the raw body and then decoded the same bytes again for the mutation, so each inspected create built two identical map[string]any trees and threw one away. The ambiguity guard now runs against the tree the mutation already decoded, through a new filter.RejectDuplicateCaseVariantJSONValue that wraps the same unexported walk the byte-taking form already delegated to. Same walk, same value, same verdict. Measured over a realistic 3.9 KB POST /containers/create body: 101.45us -> 64.26us (-37%), 79.1 KiB -> 48.4 KiB (-39%), 1415 -> 808 allocations (-43%). - perf(ownership): run the ambiguity check on the decoded tree - feat(filter): add RejectDuplicateCaseVariantJSONValue - test(ownership): guard the single decode and pin the body edge cases - test(filter): lock the two ambiguity entry points to one verdict
…bel-injection-streaming
|
Deployment failed for project sockguard-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
biggest-littlest
left a comment
There was a problem hiding this comment.
Reviewed against the CHANGELOG entry and the diff; CI green outside the qlty/Vercel quota noise.
ALARGECOMPANY
left a comment
There was a problem hiding this comment.
Reviewed against the CHANGELOG entry and the diff; CI green outside the qlty/Vercel quota noise.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe filter package adds Suggested labels: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
The ownership middleware parsed every create body it stamps an owner label into twice.
filter.RejectDuplicateCaseVariantJSONKeysdecoded the raw bytes into amap[string]any, walked it for case-variant sibling keys and threw it away, and thenmutateJSONBodydecoded the identical bytes again to do the injection. This runs the ambiguity check against the tree the mutation already has, through a newfilter.RejectDuplicateCaseVariantJSONValuethat wraps the same unexported walk the byte-taking form already delegated to, so the verdict is unchanged and there's one decode instead of two. On a realistic 3.9 KBPOST /containers/createbody that's 101.45us to 64.26us per request (-37%), 79.1 KiB to 48.4 KiB allocated (-39%), and 1415 to 808 allocations (-43%), measured with benchstat over 12 runs on each side. Nothing changes about what gets injected, when, or what bytes go upstream. The one visible difference is that a body whose JSON doesn't parse now comes back as a decode failure instead of the ambiguity check's wrapping of the same parse error.PERF-20 asked for a different shape: decode only the top-level object into
json.RawMessagevalues, touch theLabelsmember, and re-emit the other members verbatim. I built that first and measured it against the same body. It lands at 1276 allocations, because the duplicate-key check still decodes the whole body regardless, and becauseHostConfigandNetworkingConfig, the two members ownership has to read to collect cross-owner references, are more than half of a real create payload. Splitting the top level isn't free either, at 8.5 KiB and 92 allocations on its own even with zero-copy spans instead ofRawMessagecopies. Net it came out at roughly no change in allocated bytes and about -10% allocations, so I dropped it in favour of the version above and the roadmap item's Done-when needs rewriting.TestMutateContainerCreateOwnershipBodyDecodesTheBodyOnceis the regression guard, an allocation ceiling of 1000 that sits between the old 1415 and the new 808 and fails the moment the body gets parsed twice again.TestMutateJSONBodyRejectsAmbiguousBodyBeforeMutatingkeeps the fail-closed rejection and its wording now that the check moved off the raw bytes.TestMutateJSONBodyReportsMalformedBodyAsADecodeFailurepins the one error path that changed, andTestMutateJSONBodyRejectsNonObjectBodiescovers null, array, string, number and bool payloads.TestRejectDuplicateCaseVariantJSONValueMatchesByteFormlocks the two filter entry points to one verdict across a clean body, top-level and nested case-variant duplicates, the case-sensitive data-map exemption, a struct nested under a data map, large integers, and arrays of objects.BenchmarkMutateContainerCreateOwnershipBodyis where the numbers come from.go build ./...,go vet ./...,go test ./...,golangci-lint run ./...andgo test -race ./internal/ownership/ ./internal/filter/all pass, andFuzzOwnershipMutateBodyandFuzzOwnershipMutateServiceBodyeach ran 60s clean.CHANGELOG entry is under
### Changedin## [Unreleased].Changelog
✨ Added
RejectDuplicateCaseVariantJSONValuefor validation of decoded JSON values.🔧 Changed
libpodNamespaceRefsvalidation comment.🐛 Fixed