Conversation
|
cc @araujof |
0f68adc to
4fd37eb
Compare
|
Hi @hexfusion, Nice work! I haven't had a chance to do a full review, but I wanted to mention one thing I noticed. The new plugin is using the reqwest package. In the last version of PPE, we added the ability to use the "hosts" HttpTransport layer so that the host can manage things like connection pooling etc... In this case, Praxis pingora layer.. This is done by enabling a capability on the plugin and the http api is passed as an extension object.. See the delegation plugin as an example: . There should be some docs on it as well I can dig in them. Let me know if something doesn't make sense.I'll try to take a deeper review later. |
praxis-bot
left a comment
There was a problem hiding this comment.
One finding on the config type. The rest of the plugin is well-structured -- clean backend abstraction, thorough test coverage (including the stateful mock Limitador e2e), correct fail-closed/fail-open posture, and the TOCTOU trade-off is explicitly documented.
|
@hexfusion I am wondering why this is needed in PPE right now? This likely add 2 network calls to any quota enforcement. 1) to Limitador and then likely 2) Limitador to a backend store. One of the things we are evaluating in the near future is if Limitador should be offered as a plugin as part of PPE. Limitador offers a server, but it has core counter logic that it should be possible to be used via a PPE integration cc @alexsnaps . PPE already has a backend store option so rather than duplicating connections to stores, it would make sense for Limitador as a plugin to PPE to be given a connection to a backend store to use for counter storage. Is this for an experimental feature / demo? |
|
yeah I am trying to push the limits on what is possible with v3 praxis with ai-grid for TP. so having integration with the MaaS Limitador at the PPE layer is exactly what I want. how can we make progress here I will sync with @alexsnaps thanks for the input. |
You should be able to get the |
|
@maleck13 @hexfusion @alexsnaps @shaneutt what if we moved this plugin over to referece/plugins? We can "graduate" it once we settle on the design, without requiring us to ship this as part of PPE builtins just know. Reference plugins can be compiled in and registered as host-provided plugins (see example here). Longer term, it would be great if we could implement this in-process, using the limitador crate + backend store. @alexsnaps @maleck13 what if we replicated the dependency injection pattern we defined for HTTP transport with session/backend storage. Praxis would inject its own session/backend store client over to PPE. |
|
No @alexsnaps pay attention! Don't jump from one thing to the next. I was thinking of how to manage the Redis connection, and that's not gRPC at all, nor h2, sorry... |
|
@araujof @alexsnaps I think it makes sense for the host to manage connections like these and pass them into filters with the same patterns as used for the HTTP transport. Better to avoid lots of filters managing their own connections. |
|
@maleck13 @alexsnaps @shaneutt created the issue #127 for the injection of the host-managed backend stores. |
shaneutt
left a comment
There was a problem hiding this comment.
Thanks @hexfusion 🙇
cc @rikatz @nerdalert <= you'll both wanna take a peek at this PR
| /// `ext`, so the process keeps one connection pool and TLS stack. A backend | ||
| /// holds no HTTP client of its own. | ||
| #[async_trait] | ||
| pub trait QuotaBackend: std::fmt::Debug + Send + Sync { |
There was a problem hiding this comment.
Please keep the backend types crate-internal for now. No public API accepts a custom backend, so publishing QuotaBackend and its error types unnecessarily commits us to their current design. Make the backend module private, use pub(crate) items, and remove the root re-export. If custom backends are intended now, instead add a constructor that accepts Box<dyn QuotaBackend> and mark the enums #[non_exhaustive].
| fn classify(err: &HttpRequestError) -> BackendErrorKind { | ||
| match err { | ||
| HttpRequestError::Unavailable(_) => BackendErrorKind::Unavailable, | ||
| HttpRequestError::Transport(HttpTransportError::Rejected(_)) => { |
There was a problem hiding this comment.
We shouldn't treat every Rejected result as an egress denial. CircuitOpen is also mapped to Rejected, so once the host circuit opens, on_error: allow is bypassed and all requests are denied.
Either apply on_error to Rejected while retaining the quota.egress_denied code and log, or add a distinct circuit-open variant in ppe-core. At minimum, document this behavior in the README.
| STATUS_TOO_MANY_REQUESTS => Ok(CheckOutcome::OverLimit), | ||
| other => Err(BackendError { | ||
| message: format!("Limitador /check returned unexpected status {other}"), | ||
| kind: BackendErrorKind::Transport, |
There was a problem hiding this comment.
We'll want to classify /check responses in a way that preserves fail-closed behavior for configuration errors. Treat 4xx responses other than 429 as Unavailable, keep 5xx responses as Transport, and update the README failure table accordingly. With on_error: allow, misconfigured deployments must not silently disable enforcement.
Might need some more tests around this as well.
| /// Per-call HTTP timeout in seconds, so a slow Limitador fails fast into | ||
| /// the `on_error` path rather than stalling the request. Default 5. | ||
| #[serde(default = "default_timeout_seconds")] | ||
| pub timeout_seconds: u64, |
| /// | ||
| /// A message when `endpoint` or `namespace` is empty. | ||
| pub fn validate(&self) -> Result<(), String> { | ||
| if self.endpoint.trim().is_empty() { |
| allow_unauthenticated: false | ||
| ``` | ||
|
|
||
| The plugin registers both hooks itself; the `hooks` list in config has no |
There was a problem hiding this comment.
Is this a working configuration example?
I think we need to either set engine_settings.dispatch: hooks, or add run(token-quota) to both pre_invocation and post_invocation on the llm: route’s authorization block. 🤔
| the authenticated subject id. A client-supplied claim can be dropped or forged | ||
| to change the descriptor the budget is keyed on, so do not key a budget on one. | ||
|
|
||
| ## Deployment |
There was a problem hiding this comment.
missing requirements: authenticate requests before this plugin, isolate each issuer in its own Limitador namespace, and secure the Limitador connection with mTLS or an equivalent control. We need to explain how deployments enforce one quota instance per issuer, since the plugin sends no credentials and identifies principals only by subject ID.
| `on_error` governs only the last row. Every permanent fault fails closed on its | ||
| own, so a misconfiguration cannot silently stop enforcement. | ||
|
|
||
| The debit path (`cmf.llm_output`) never denies. The response is already out, so |
There was a problem hiding this comment.
We'll want to document that the quota debit must run first on cmf.llm_output. Under hook dispatch, it needs the lowest priority among sequential plugins; under policy dispatch, it must be the first run(...) in the result phase. Otherwise, an earlier deny can skip the debit and leave provider usage uncounted.
| feature "jwt" => praxis_policy_plugin_identity_jwt::JwtIdentityFactory, | ||
| feature "oauth" => praxis_policy_plugin_delegator_oauth::OAuthDelegatorFactory, | ||
| feature "elicitation-ciba" => praxis_policy_plugin_elicitation_ciba::CibaApproverFactory, | ||
| feature "quota" => praxis_policy_plugin_quota::QuotaFactory, |
There was a problem hiding this comment.
Let's add (cfg!(feature = "quota"), QUOTA_KIND) to the expected list in every_enabled_builtin_resolves_its_kind. This ensures the umbrella feature registers the quota factory and prevents kind: quota from failing at runtime. Also align the row’s => with the entries above because... because it makes my brain happier ![]()
| "jwt", | ||
| "oauth", | ||
| "elicitation-ciba", | ||
| "quota", |
There was a problem hiding this comment.
Please update all builtin-set documentation for quota: add its kind to docs/content/builtins.md, revise the builtins feature description, update the facade and register_builtin_plugins docs, and add it to the root README feature list. Also update AGENTS.md to reflect the new crate layout and 18-crate workspace.
|
@shaneutt I opened issue #137 to coalesce our builtin crates as one published crate. Please hold on merging this PR so we can fold it as a submodule in the bultings crate, as opposed to a new published crate. @hexfusion please address the reviews. It won't be hard to rebase this PR after 137 closes. Thanks! |
|
@hexfusion can you please merge/rebase from @shaneutt I just merged the refactoring PR for the builtin crates: #140 |
Meters a per-principal LLM token budget as a PPE policy against a durable Limitador counter. A pre-invoke check on cmf.llm_input probes the counter and refuses once it reaches the limit; a post-invoke debit on cmf.llm_output records the token cost. The backend is a QuotaBackend trait with the Limitador HTTP client sealed as a pub(crate) implementor, and the operator declares it as kind: quota. Fail-closed by default (on_error deny), and an over-budget request maps to HTTP 429. The debit prefers the gateway's typed usage and falls back to the response body. Covered by unit tests and a stateful e2e that proves a principal is denied once its debits reach the budget. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Adds a divan bench over the two functions the plugin runs per request off the Limitador round trip: identity resolution and the response-body usage fallback parse. The functions are exposed through a bench-only feature, so a normal build keeps them private. Measured on the pinned toolchain: the sub identity path is 3.5 ns and allocates nothing, and the body fallback parse is 350 ns, about 100x a typed-usage read. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
A typo in an optional key (identityClaim for identity_claim, onError for on_error) was silently accepted and the default used, which could key budgets on the wrong identity claim or fail open. Reject the parse instead. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
The plugin owned a reqwest+rustls client for the Limitador /check and /report calls. Route those through the host transport instead (the perform_http capability, via Extensions::http_request), so the process keeps one connection pool and TLS stack and the plugin ships no TLS of its own. Removing reqwest drops it from the whole workspace. Behavior is preserved: two-phase check/report, request bodies and content type, the per-call timeout, and a single attempt (no retry, so a repeated /report cannot double-charge). A withheld perform_http or an uninstalled transport is a permanent wiring fault, not an unreachable Limitador, so it always denies rather than falling through on_error; a reachable-but-failing Limitador still honors on_error. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…sport failures The client classified only the outer HttpRequestError::Transport as transient, so HttpTransportError::Rejected (the egress/SSRF denial the host transport returns for an in-cluster Limitador ClusterIP) rode on_error and, under on_error: allow, served every request unmetered. Inspect the inner error: Rejected denies with a distinct quota.egress_denied, a malformed request denies as backend-unavailable, and the transient set (timeout, connect, io, oversize) is enumerated so an unknown future variant of the non_exhaustive HttpTransportError fails closed rather than open. Correct the /check retry comment: /check is idempotent and skips retry for tail latency, not to avoid a double-charge. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
A missing or typo'd read_subject/read_claims capability, or a dropped identity claim, left the subject unresolved and the request was served UNMETERED, while a missing perform_http failed closed. For a metering gate that asymmetry is the hole. Deny an unresolved-identity request by default with quota.no_identity, matching the never-serve-unmetered posture. BEHAVIOR CHANGE: the previous default served a no-identity request; it now denies. Deployments that enforce authentication upstream and want an unauthenticated request served unmetered set allow_unauthenticated: true (default false), which serves it with a warning. identity_claim must name a verified, always-present claim; a client-suppliable claim is droppable and would bypass the budget. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Document what the plugin does (per-principal token quota via Limitador, two-phase check/report, the soft-cap/TOCTOU nature), the full config reference, the required capabilities and that missing any fails closed, a complete example, the verified-always-present identity_claim requirement, and the host-transport egress deployment caveat for an in-cluster Limitador. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Move the standalone praxis-policy-plugin-quota crate into praxis-policy-builtins as plugins::quota, matching the sibling plugins, and wire it through the builtins feature and registration table rather than a separate published crate (review on praxis-proxy#116, after praxis-proxy#140 consolidated the bundled extensions into one crate). The plugin adds no new dependency to the builtins crate: its deps (core, bytes, serde, tracing) are already present, so it pulls in no crypto. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
7a2ebe0 to
06a0b82
Compare
Move the standalone praxis-policy-plugin-quota crate into praxis-policy-builtins as plugins::quota, matching the sibling plugins, and wire it through the builtins feature and registration table rather than a separate published crate (review on praxis-proxy#116, after praxis-proxy#140 consolidated the bundled extensions into one crate). The plugin adds no new dependency to the builtins crate: its deps (core, bytes, serde, tracing) are already present, so it pulls in no crypto. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
06a0b82 to
39164c8
Compare
|
@araujof all set |
A policy plugin that meters a per-principal token budget against a durable Limitador counter, declared as
kind: quota. The counter lives outside the pod, so a budget survives a restart and stays correct across replicas.builtins/plugins/quota, registered behind an off-by-defaultquotafeature in the engine. Additive: nothing changes unless the feature is enabled.cmf.llm_inputand a post-invoke debit oncmf.llm_output, keyed on the resolved subject (identity_claim, defaultsub). Over-budget denies withquota.exhausted, mapped to HTTP 429.QuotaBackendtrait. The Limitador HTTP client is apub(crate)implementor in a private module, so no backend type reaches the plugin surface and a second backend slots in without touching the hooks.on_errordefaults to deny. The debit prefers the gateway's typed token usage and falls back to the response body.benchfeature.SECURITY.mdstates the deployment contract: the reported-usage trust boundary, the bounded self-correcting consistency model, subject-only scope behindrequire(authenticated), one Limitador namespace per issuer, and mesh mTLS for the backend hop.Part of #115