feat(metering): add external metering filter with balance checks and usage reporting - #581
feat(metering): add external metering filter with balance checks and usage reporting#581noyitz wants to merge 4 commits into
Conversation
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-structured filter with good test coverage and correct conventions. Three medium-severity items found.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 3 |
Findings:
- [Medium]
strip_client_credentialsusesif let Okfor an infallibleHeaderNameparse, creating a silent-failure pattern for security-critical credential stripping. UseHeaderName::from_static("x-api-key")instead. - [Medium]
validate_configonly checks for empty prefix but does not validate that the prefix contains valid HTTP header name characters. An invalid prefix (e.g. containing spaces or control characters) would silently match no headers, leaving tenant identity headers unstripped. - [Medium] No test covers multi-value tenant headers (
x-tenant-username: alice\r\nx-tenant-username: mallory). Add a unit test verifying all values are marked for removal when a client sends duplicate tenant headers.
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-structured filter with good test coverage and correct conventions. The prior review covered three medium items; this pass adds one more.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
Findings:
- [Medium]
strip_identity_headersperforms two unnecessary per-request allocations: loweringHeaderName::as_str()(already lowercase byhttpcrate contract) and lowering the immutable prefix on every call instead of once at construction.
praxis-bot
left a comment
There was a problem hiding this comment.
No new findings beyond prior review.
praxis-bot
left a comment
There was a problem hiding this comment.
No new findings beyond prior reviews.
Fourth pass confirms the four existing findings (infallible HeaderName parse, prefix validation gap, missing multi-value header test, redundant to_ascii_lowercase) are the substantive items. Code structure, conventions, test coverage, integration tests, documentation, and registry wiring are all correct.
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Fifth pass. One new medium finding (assertion messages in unit tests). Four items from prior reviews remain unaddressed in the code.
| Severity | Count (new) | Count (prior, open) |
|---|---|---|
| Critical | 0 | 0 |
| Large | 0 | 0 |
| Medium | 1 | 4 |
New finding:
- [Medium] Unit test assertions missing messages (
tests.rs). All 15assert!/assert_eq!calls lack a message string. The workspace enforcesmissing_assert_message = "deny"(Cargo.toml line 223), and the test module's#[allow]only coversunwrap_usedandexpect_used. The integration tests correctly include messages on every assertion.
Open prior findings (no code changes since last review):
HeaderName::from_static("x-api-key")instead of fallible.parse()withif let Ok(mod.rs:102)- Prefix validation for valid HTTP header characters (config.rs:30)
- Missing multi-value tenant header test (tests.rs)
- Redundant per-request
to_ascii_lowercaseon both key and prefix (mod.rs:88)
|
@noyitz please resolve the conflicts and address the bot's comments thanks |
1bea336 to
ff138bb
Compare
ff138bb to
7018d3c
Compare
jordigilh
left a comment
There was a problem hiding this comment.
A few things flagged inline, plus some broader ones here. Identity stripping, the CloudEvents payload, and URL-encoding of client-influenced path segments all held up -- no SSRF or credential-leakage concerns.
Circuit breaking is available but unused for this gateway. SubRequestConnector supports one (with_options, praxis#879) and the base proxy wires it up; praxis-ai's server never does (server.rs#L141, commands.rs#L40 both use plain ::new()). runtime.subrequest_circuit_breaker parses fine in an AI-gateway config today and is silently dropped. external_metering is the first mandatory-every-request consumer of that shared pool (the other 5 are opportunistic) -- worth a follow-up before a high-traffic rollout.
execute_url (+ client/error types) are now pub, not pub(crate) -- needed cross-crate, but worth flagging as a public API addition on praxis-ai-apis, semver-relevant going forward.
Generated docs skip the 3-tier identity design -- it only lives in a doc comment on the private read_identity_headers, which the doc generator never harvests (it reads a second paragraph of the module/struct doc -- credential_inject.md already does this). Worth moving it up.
#577 (this filter's linked design doc) still describes the single-filter/CalloutClient design this PR (split across #581 + #709) supersedes.
Minor: iterative_request_router's validate() doesn't block nesting external_metering inside a step (it does block nested IRR and compression) -- would double-report with no idempotency key if misconfigured. Not urgent, just flagging.
|
@jordigilh thanks for the thorough pass — all six inline findings are addressed in 79c3709 (replied on each thread),
|
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (6th pass)
All four code findings from prior reviews have been addressed:
HeaderName::from_static("x-api-key")(fixed, line 667)- Prefix validation for valid header characters (fixed, config.rs lines 99–106)
- Multi-value identity header test (added, tests.rs lines 411–426)
- Redundant per-request
to_ascii_lowercase(pre-lowered at construction, line 229)
Assertion messages (R5) are partially addressed: security and spoofing tests now include messages, config/parsing tests still lack them.
One new medium finding below.
| Severity | Count (new) |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
79c3709 to
f4ab2b1
Compare
jordigilh
left a comment
There was a problem hiding this comment.
Approving — all six findings resolved: balance-check contract documented, tier-gating fixed, configurable metadata namespace added, failure-reporting metric added, nested-JSON model extraction hardened (79c3709), and the cross-filter ordering-constraint gap now has a tracked follow-up (ai#812).
…usage reporting
Add an external_metering filter that integrates the gateway with an
external metering service:
- Pre-request balance check against the metering service's entitlement
endpoint, with configurable fail-open behavior: 429 when the tenant's
token budget is exhausted, 503 when metering is unreachable and
fail_open is disabled.
- Post-response usage reporting as CloudEvents 1.0 events
(inference.tokens.used / inference.request.error), fire-and-forget so
a slow metering service never delays a response the upstream already
answered. Token counts are read from the token_count filter's
filter_metadata keys, including the prompt cache breakdown.
- Three-tier identity resolution, most trusted source first: verified
unnamespaced {prefix}* metadata written by an authentication filter,
then the identity_header_guard's namespaced identity.{prefix}*
metadata, then raw {prefix}* request headers for deployments where a
trusted upstream auth layer injects them. Once a higher tier supplies
identity, lower tiers are ignored entirely so forged headers cannot
override verified claims. Identity headers and client credentials
(authorization, x-api-key) are always stripped before the request is
forwarded upstream.
HTTP callouts use praxis-core's SubRequestClient. register_ai_filters
threads the shared server-level client into the filter following the
existing openai_file_resolve pattern, and
praxis_ai_apis::subrequest::execute_url is made public so filters can
execute full-URL sub-requests.
Part of praxis-proxy#577.
Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
…ction, and observability Review follow-up, one finding per change: - Gate the lower identity tiers on any verified field, not just the username: an auth filter may map only some claims (e.g. group without username), and a partially verified identity must not be extended by forgeable sources. - Make the identity_header_guard metadata namespace configurable (identity_metadata_namespace, default "identity") instead of hardcoding the "identity." prefix. - Extract the model with a string- and depth-aware scanner that only matches the top-level "model" key, so a decoy inside message content can no longer misattribute the request. - Count failed usage-report deliveries in a praxis_ai_metering_report_failures_total metric and log them at warn, so dropped billing events are visible on a dashboard. - Document the balance-check contract: the metering service expresses denial only via 2xx + hasAccess=false, so any non-2xx is handled by the availability policy; log those at warn with the status. - Strip x-api-key via HeaderName::from_static instead of a fallible parse that silently skipped removal on error. - Validate that identity_header_prefix contains only valid HTTP header name characters, so a misconfigured prefix fails at startup instead of silently matching nothing. - Lowercase the prefix once at construction and compare against HeaderName::as_str() directly, dropping a per-header allocation. - Surface the three-tier identity design in the struct documentation so the generated filter docs carry it. Part of praxis-proxy#577. Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
f4ab2b1 to
190db56
Compare
leseb
left a comment
There was a problem hiding this comment.
mostly stamping since @jordigilh did some thorough reviews already, thanks!
praxis-bot
left a comment
There was a problem hiding this comment.
PR Re-review
Re-review: new commits since last review on 2026-08-20.
The force-pushed commits address four of the six prior findings (from_static for x-api-key, prefix validation, multi-value header test, pre-lowered prefix). The query-string encoding gap and partial assertion-message gap remain open from prior reviews and are not repeated here.
Summary: One new medium finding — the default_username and default_model fallback paths lack runtime test coverage.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
|
@noyitz see ci failures |
…ame in assert - examples/configs/external-metering.yaml: add insecure_options.allow_private_endpoints (loopback backend endpoints now require the opt-in, per schema validation added on main) - filters/src/metering/tests.rs: drop username value from assert message (CodeQL: cleartext logging of sensitive information) Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
Head branch was pushed to by a user without write access
…backs Extract the report-time model resolution into resolve_report_model so the fallback chain (captured model, body/header metadata, configured default) is directly testable. Add a runtime test for default_username metering an unauthenticated request, per praxis-bot review feedback. Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
Adds an
external_meteringHTTP filter: pre-request balance checks andpost-response token usage reporting against an external metering service.
Part of #577.
What the filter does
Request phase — resolves tenant identity, strips identity headers and
client credentials (
authorization,x-api-key), and asks the meteringservice whether the tenant may spend more tokens:
GET {metering_url}/api/v1/customers/{username}/entitlements/{feature_key}/value?model={model}fail_open: true(default), 503 otherwise, so a metering outage degrades to unmetered service
rather than an inference outage.
Response phase — emits a CloudEvents 1.0 event
(
inference.tokens.used, orinference.request.erroron upstream failure) toPOST {metering_url}/api/v1/events. Delivery is fire-and-forget: a slow orfailing metering service never delays a response the upstream already
answered. Token counts come from the
token_countfilter'sfilter_metadatakeys (
token.input/output/totaland the prompt cache breakdown from #582).Identity resolution — three tiers, most trusted first
{prefix}*keys) written by anauthentication filter from verified credentials. When present, every lower
tier is ignored entirely, so a client cannot spoof
subscriptionormodelvia forged headers alongside valid credentials.identity.{prefix}*keys) written by theidentity_header_guardfilter (feat(filter): add identity header guard filter #709), which owns identity header capture.{prefix}*headers, for deployments where a trusted upstream authlayer (e.g. an external authorizer) injects them directly.
Identity headers are always stripped before the request reaches the upstream,
regardless of which tier supplied the identity.
Relationship to #709 and #577
This is the metering half of the split: #709 owns identity header capture into
namespaced metadata; this filter consumes that metadata (tier 2) and owns
admission control, credential stripping, and usage reporting. #577 documents
the original single-filter design and needs an update to reflect the split.
HTTP callouts use
praxis-core'sSubRequestClient(theCalloutClientnamedin #577 was removed upstream in #849).
register_ai_filtersthreads the sharedserver-level client into the filter following the existing
openai_file_resolvepattern; without one the filter creates a privateconnector, same as
openai_file_resolve::from_config.praxis_ai_apis::subrequest::execute_urlis made public so filters can executefull-URL sub-requests.
Size
1,876 insertions, of which 861 are unit + integration tests, 103 generated
docs + README rows, and 64 the example config. Functional filter code is
~1,010 lines across
mod.rs,config.rs, and registration. Balance check,event construction, and identity resolution are cohesive enough that splitting
them again would leave non-functional intermediate states; happy to split if
reviewers prefer.
Testing
filters/src/metering/tests.rs), including spoofing-closuretests for the tier precedence rules
examples/configs/external-metering.yaml(balance allowed, fail-closed rejection, header stripping, no-identity skip)
make test— 4,905 passed, 0 failed;make lintgreen