fix(compat): leave the BuildKit tunnel ack unset under request_body.buildkit - #444
Conversation
…uildkit GRPC=1 and SESSION=1 auto-set insecure_accept_opaque_buildkit_tunnels, and ApplyCompat runs before validation, so an operator who had already migrated to a request_body.buildkit policy but still carried GRPC=1 from their old Tecnativa environment hit a startup refusal naming a key they never wrote. The acknowledgment and a mediation policy are mutually exclusive. Compat now skips the auto-ack whenever request_body.buildkit is configured, at the top level or on any client profile, which is exactly the condition validateBuildkitAckMutualExclusion refuses on. Both sides ask one shared predicate so they cannot drift. The alternative, exempting the compat-sourced value from the exclusion check, needs provenance tracking on Config and breaks the #185 invariant that the wholesale ack and the mediator never stack, which other code comments already rely on. With a top-level policy this tightens the posture rather than relaxing it. The acknowledgment only ever gated startup admission; what happens to a /session or /grpc request at runtime is decided by filter.BuildkitOptions.TunnelConfigured, which comes from request_body.buildkit. So the generated /grpc and /session rules are admitted by validateBuildkitTunnelRulesForPolicy's buildkitConfigured arm instead of its acknowledgment arm, and every gRPC message on those tunnels goes through the mediator's per-method policy instead of passing uninspected. With the policy on a client profile only, startup still refuses, and should: that check reads the top-level request_body.buildkit, the compat rules are top-level, and they apply to every client matching no profile, so letting a profile's policy admit them would open an uninspected tunnel for exactly the clients it does not cover. The skip stays scoped to any-scope rather than narrowing to top-level-only because that gives the clearer message. Narrowing would put the profile-only case back on the mutual-exclusion error, which names a key the operator never wrote and tells them to set it false, when only unsetting GRPC/SESSION or adding a top-level policy actually works; it would also refuse the safe GET,HEAD-only shape (GRPC=1 without POST=1), which cannot open a tunnel at all. Instead the tunnel refusal now names its origin: when the rules under it were synthesized by compat, it says so, and gives the two cures that work. Origin comes from what ApplyCompat actually did, not from env presence, so a hand-written rule with GRPC=1 idle in the environment keeps the original message and its acknowledgment cure. An explicitly configured acknowledgment alongside request_body.buildkit is still refused, compat never clears one, and the skip log reports the flag's real value instead of claiming it is unset.
|
@coderabbitai review |
|
Deployment failed for project sockguard-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ 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 (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCompat mode now detects BuildKit mediation at the top level and in client profiles. It records whether rules came from Tecnativa compatibility variables and changes acknowledgement handling accordingly. Validation reports compat-generated rule origins, keeps explicit acknowledgement and mediation mutually exclusive, and distinguishes profile-only mediation from top-level mediation. Tests and documentation cover these outcomes. Suggested labels: Merge Risk: ⚪ Minimal · up to BuildKit compatibility variables now avoid adding an incompatible opaque-tunnel acknowledgement when mediation is configured, while preserving refusal for profile-only mediation that cannot cover generated top-level rules. The documented and tested behavior is ready to merge. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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 |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
the
second-opinion
|
@greptileai Review exact head |
biggest-littlest
left a comment
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
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.
3 participants
GRPC=1orSESSION=1auto-setsinsecure_accept_opaque_buildkit_tunnels, and compat runs before validation. An operator who'd migrated to arequest_body.buildkitpolicy but still carriedGRPC=1from their old Tecnativa environment hit a startup refusal naming a key they never wrote, because the ack and a mediation policy are mutually exclusive.Compat now skips the auto-ack whenever
request_body.buildkitis configured at the top level or on any client profile, which is exactly the condition the mutual-exclusion check refuses on. Both sides ask one shared predicate (buildkitMediationConfigured) so they can't drift. An explicitly written ack alongside a policy is still refused.With a top-level policy this tightens the posture: the ack only ever gated startup admission, and what happens to a
/sessionor/grpcrequest at runtime is decided byrequest_body.buildkit. So the generated tunnel rules are admitted by the mediation arm instead of the ack arm, and every gRPC message on those tunnels is checked against the policy rather than passing uninspected.With the policy on a client profile only, startup still refuses, and should: the compat rules are top-level and apply to every client no profile matches. The refusal now says the rules came from
GRPC/SESSIONand names the two cures that work (unset them, or add a top-level policy), instead of pointing at a config key that isn't in the file. Origin comes from what compat actually generated, not from env presence, so a hand-written tunnel rule withGRPC=1idle in the environment keeps the original message.Tests for both branches.
migration.mdx,configuration.mdxandsecurity.mdxhad described the old behavior and are corrected. CHANGELOG under Unreleased### Fixed.Changelog
GRPC=1andSESSION=1compatibility handling to skip automatic acknowledgment when mediation is configured.HasCompatGeneratedRules().Concerns
buildkitMediationConfiguredand mutual-exclusion validation.insecure_accept_opaque_buildkit_tunnelsfor compatibility-generated rules.