Conversation
Exercise three Basic Auth subjects through one shared endpoint and verify independent subject-keyed quotas across two consumer gateways. Preserve the existing routing, concurrency, expiry, restart, outage, NetworkPolicy, evidence, and cleanup coverage while removing the temporary identity-gateway design. Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
620ea90 to
a185c14
Compare
|
This feature needs to go in as an experimental flag. |
hexfusion
left a comment
There was a problem hiding this comment.
the implementation is solid, three apps on one listener and one token_rate_limit rule, each keeping its own per-subject budget, is really neat, and testing concurrency plus a gateway restart against the shared counter is nice coverage.
what i'd want to settle before is what the per-user budget is meant to be. the value here from use-case feels like the openrouter shape. one endpoint fronting every site, all usage metered per user through it. that makes the budget global, not per-site. so the shared counter's behavior under partition and upgrade is really the core of the distributed system problem.
-
is this a hard gate or a meter. the readme calls it hard enforcement, but settled usage can exceed the reservation and isn't treated as a second cap, so today it's a hard cap on concurrent reservations and a soft meter on tokens. worth being explicit, because hard enforcement is a much higher bar for the store.
-
reservation_timeout is 30s, under both the 60s window and a plausible long generation. a request over 30s frees its own hold mid-flight and over-admits on one node with no partition, and a late settlement writes against an expired hold. seems like it should be at least the max request time.
-
is the failover path exercised? one non-ha valkey means only total-outage fail-closed is tested. an async replica promoted under partition can lose an acked reservation and fail open, spending a budget twice. fail-closed-on-outage is a narrower guarantee than failover safety.
-
a couple i couldn't confirm from the diff: is settle idempotent under retry, and is reserve a single atomic op rather than get-then-set that could race even on a healthy valkey.
mostly it'd help to hear the intended use case and rollout. how many gateway replicas, how valkey (could we use postgres?) is deployed and failed over, max_tokens in real traffic, and the upgrade strategy. that pins down which of these actually bite.
| gateway, | ||
| app.credential, | ||
| )?; | ||
| denied_on_both = denied_on_both && record(results, result) == Outcome::QuotaDenied; |
There was a problem hiding this comment.
nit: && short-circuits, so record() gets skipped once this is false and those probes miss the log. same at 1701.
| .lines() | ||
| .map(|line| { | ||
| let lower = line.to_ascii_lowercase(); | ||
| if ["authorization:", "password:", "token:", "secret:", "privatekey:"] |
There was a problem hiding this comment.
nit: the redaction misses the json form, inline secrets, and the redis://:pass@host url. can we add a test covering those cases?
| hash ^= u32::from(byte); | ||
| hash = hash.wrapping_mul(0x0100_0193); | ||
| } | ||
| let third_octet = 20 + (hash % 200); |
There was a problem hiding this comment.
question: does this only give 200 subnets? two runs could hash to the same one and collide.
| /// Scenario: valid Alice auth is admitted on both gateways with attribution. | ||
| fn scenario_valid_auth(results: &mut Vec<HttpResult>) -> Result<ScenarioResult, Box<dyn std::error::Error>> { | ||
| /// Return whether attribution covers every and only configured provider site. | ||
| fn covers_all_provider_sites(distribution: &BTreeMap<String, u32>) -> bool { |
There was a problem hiding this comment.
nit: this might be flaky. only 6 probes cover it, and a transport retry can double-advance the round robin and drop a provider site.
| return Ok(result); | ||
| } | ||
| results.push(result); | ||
| std::thread::park_timeout(RESTART_PROBE_INTERVAL); |
There was a problem hiding this comment.
park_timeout can wake early on a spurious wakeup, and with no deadline recheck it can burn the restart retries before a slow gateway is ready. use sleep?
| std::thread::park_timeout(RESTART_PROBE_INTERVAL); | |
| std::thread::sleep(RESTART_PROBE_INTERVAL); |
| // into the topology (one application-scoped listener per app). They are used | ||
| // as-is; nothing is regenerated from a fixture, so deployed configuration is | ||
| // exactly what is reviewed in the repository. | ||
| let _ = state_dir; |
There was a problem hiding this comment.
question: is this used? state_dir is referenced two lines down.
| /// Alice principal password used only by this qualification topology. | ||
| const ALICE_PASS: &str = "alice-secret"; | ||
| /// Application-A principal used by the legacy quota scenarios. | ||
| const ALICE_USER: &str = "application-a"; |
There was a problem hiding this comment.
nit: the const is named ALICE_USER but the value is application-a, not a huge deal but could we make this a little more clear?
| - main | ||
|
|
||
| - name: proxy | ||
| address: 0.0.0.0:8443 |
There was a problem hiding this comment.
minor: plaintext on 8443 is deceiving, since the same port runs mtls on the provider side later. use a plaintext-looking port or add a note.
|
|
||
| /// Validate an IPv4 CIDR without accepting host-only or IPv6 forms. | ||
| fn validate_ipv4_cidr(cidr: &str) -> Result<(), ForgeError> { | ||
| pub(crate) fn validate_ipv4_cidr(cidr: &str) -> Result<(), ForgeError> { |
There was a problem hiding this comment.
minor: this only rejects a prefix over 32, so host bits (10.240.5.7/24) and /31 or /32 slip through. could tighten it for user-supplied subnets.
|
Holding on converting to experimental. |
Summary
Extends the distributed token-quota topology and first-class qualification to prove three independent application quotas through one shared consumer endpoint.
application-a,application-b, andapplication-cauthenticate with Basic Auth on the same listener. Praxis publishes the verified subject privately; Praxis AI uses that subject as the quota key; Grid routes admitted requests across the shared west, central, and east provider overlay.This keeps three responsibilities separate:
What the qualification proves
Topology changes
key: authenticated_subjectwith one common rule and Valkey namespace; the verified subject partitions the ledger securely.Quota contract
Admission is reservation-based because actual response usage is unknown before routing. The enforced invariant is:
Settlement records actual usage after the response. Actual settled usage may exceed the reservation estimate without indicating over-admission. Provider distribution is sampled independently from tight quota boundaries.
Cross-repository dependencies
This PR is the Grid qualification and reference topology for a contract implemented across repositories:
AuthenticatedIdentity.key: authenticated_subject, consumes that trusted identity, and hashes it into an opaque quota key.The identity type is authentication-method-neutral. Basic Auth enables this demo now, while JWT/OIDC/OAuth-backed authentication can publish the same identity for reuse by the AI quota filter.
Related to #101 and praxis-proxy/ai#121.
Depends on praxis-proxy/praxis#1108 and praxis-proxy/ai#980.
Validation
Two fresh Kind qualifications passed all 9 scenarios with automatic teardown. Static validation included xtask tests, workspace Clippy, formatting,
make test,make doc,make lint, Forge validation, andgit diff --check.Evidence and generated
.forge.resolved.*.yamlfiles are excluded from the commit.Landing order
Keep this PR open until Praxis #1108 is released and AI #980 consumes that release. Then rebuild the feature-enabled AI image from committed dependencies and rerun the qualification before merge.
Companion demo: praxis-proxy/demos#20.