feat(jans-cedarling): updating policies from URL by ttl#14174
feat(jans-cedarling): updating policies from URL by ttl#14174haileyesus2433 wants to merge 50 commits into
Conversation
… validation - Added a new `policy_store_refresh` module to handle background refresh of remote policy stores. - Introduced `CacheValidators` to parse and manage HTTP cache headers for conditional GET requests. - Updated `MetricsCollector` to track policy store refresh attempts and outcomes. - Updated `PolicyStoreConfig` to include a refresh interval setting for URL-based sources. - Implemented logic to handle `304 Not Modified` responses efficiently, ensuring the system remains responsive to policy updates. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
- Introduced a new `policy_store_refresh` module to manage background refresh of remote policy stores. - Updated `Cedarling` struct to utilize `ArcSwap` for atomic updates of the `Authz` instance. - Added support for a configurable refresh interval for policy stores in `BootstrapConfig`. - Implemented logic to spawn a refresh worker based on the configured refresh interval. - Enhanced deserialization for policy store refresh interval to ensure proper clamping of values. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
- Updated multiple benchmark files to include default values for `policy_store_config` in `BootstrapConfig`. - Ensured consistency across `authz_authorize_multi_issuer_benchmark.rs`, `authz_authorize_unsigned_benchmark.rs`, `context_data_store_benchmark.rs`, and `startup_benchmark.rs`. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
…ons in examples - Added default values for `policy_store_config` in multiple example files including `authorize_unsigned.rs`, `bulk_authorization_benchmark.rs`, `lock_integration.rs`, `log_init.rs`, `profiling_multi_issuer.rs`, and `profiling_unsigned.rs`. - Ensured consistency across examples to streamline configuration setup. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
…ons in test files - Updated `policy_store_config` in `policy_store_loader.rs`, `ssa_validation_integration.rs`, and `cedarling_util.rs` to include default values. - Ensured consistency across test files to streamline configuration setup. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
- Updated authorization methods in `blocking.rs` to utilize the `load()` method for improved instance management. - Refactored the refresh worker spawning logic in `lib.rs` into a new `maybe_spawn_refresh_worker` function for better clarity and maintainability. - Improved cache header handling in `cache_headers.rs` by simplifying the `from_headers` method and ensuring proper handling of cache control directives. - Enhanced `RefreshState` struct in `policy_store_refresh.rs` to streamline initialization and improve clarity in managing refresh states. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
📝 WalkthroughWalkthroughAdds opt-in background refresh for URL policy stores: new refresh-interval config, cache-header parsing and conditional GET/HEAD, loader metadata seeding and body-hash short-circuit, a refresh-worker strategy ladder that parses/rebuilds and atomically swaps Authz via ArcSwap, refresh telemetry, and docs/examples updates. ChangesPolicy Store Refresh Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
comp-jans-cedarling
|
Error: Hi @haileyesus2433, You did not reference an open issue in your PR. I attempted to create an issue for you. |
store Introduce a multi-tiered refresh strategy (Conditional → HeadThenGet → PlainGet) that automatically degrades when the upstream server fails to support efficient cache validation. Includes a periodic probe mechanism to attempt upgrades back to the efficient path and uses AHasher for faster body hashing. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Track detailed refresh outcomes and strategy transitions in the metrics collector. Counters are emitted sparsely—omitting zero values—to allow dashboards to distinguish between missing metrics and genuine zero readings. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
validators Added `head_validators` to `HttpClient` to probe upstream servers for caching headers before a full GET request. This supports a fallback strategy where clients can detect if a server supports HEAD-based validation or requires a full fetch. Exposes `HeadOutcome` to handle cases where HEAD is explicitly rejected (405/501). Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/bootstrap_config/policy_store_config.rs`:
- Around line 28-31: The constant PolicyStoreConfig::MIN_REFRESH_INTERVAL_SECS
is declared pub but not re-exported from the module root, so change its
visibility to the more restrictive pub(crate) unless you intend it to be part of
the public API; if it should be public instead, add it to the module exports
(mod.rs / lib root) alongside PolicyStoreConfig, PolicyStoreConfigRaw, and
PolicyStoreSource so it is truly exported. Locate MIN_REFRESH_INTERVAL_SECS in
the PolicyStoreConfig impl and either make it pub(crate) or add an explicit
re-export for it in the module root to match the crate's export policy.
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs`:
- Around line 447-454: Add unit tests that exercise BootstrapConfigRaw's
deserialize_policy_store_refresh_interval path by decoding env vars for
CEDARLING_POLICY_STORE_REFRESH_INTERVAL and asserting
BootstrapConfigRaw.policy_store_refresh_interval_secs behaves like the other
refresh fields: when env var is unset the value is 0; when set to "0" it remains
0; when set to "1".."4" it is clamped to 5; when set to "5" or larger it is
unchanged. Use the same test harness used for other refresh-interval tests
(invoke the serde/env decoding path), reference BootstrapConfigRaw and
deserialize_policy_store_refresh_interval in the test names, and ensure each
case sets/clears the CEDARLING_POLICY_STORE_REFRESH_INTERVAL env var so tests
are isolated.
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/json_util.rs`:
- Around line 92-107: Add a doc comment for the pub(super) deserializer function
deserialize_policy_store_refresh_interval explaining what it does: that it
accepts a u64 (or numeric string) via deserialize_or_parse_string_as_json,
treats 0 as "disabled" and returns 0, and otherwise clamps non-zero values up to
PolicyStoreConfig::MIN_REFRESH_INTERVAL_SECS; include expected input form,
behavior, and mention that it returns a normalized refresh interval or
deserialization error.
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 261-267: The error construction in head_validators currently uses
HttpRequestReasonError::MaxRetriesExceeded and .with_retry_count(1), which is
misleading for a single-attempt HEAD probe; update the error to use a more
appropriate reason (e.g., HttpRequestReasonError::NetworkError or
HttpRequestReasonError::RequestFailed) and either remove the
.with_retry_count(1) call or set it to 0, and update the last_error message to
clarify this was a single probe failure; adjust the HttpRequestError::new
invocation accordingly so head_validators returns a clear, accurate error when
the HEAD send().await fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f4010f6-5509-40ec-8089-cbcc08dc1675
📒 Files selected for processing (27)
jans-cedarling/cedarling/benches/authz_authorize_multi_issuer_benchmark.rsjans-cedarling/cedarling/benches/authz_authorize_unsigned_benchmark.rsjans-cedarling/cedarling/benches/context_data_store_benchmark.rsjans-cedarling/cedarling/benches/startup_benchmark.rsjans-cedarling/cedarling/examples/authorize_unsigned.rsjans-cedarling/cedarling/examples/bulk_authorization_benchmark.rsjans-cedarling/cedarling/examples/lock_integration.rsjans-cedarling/cedarling/examples/log_init.rsjans-cedarling/cedarling/examples/profiling_multi_issuer.rsjans-cedarling/cedarling/examples/profiling_unsigned.rsjans-cedarling/cedarling/src/authz/metrics.rsjans-cedarling/cedarling/src/blocking.rsjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/mod.rsjans-cedarling/cedarling/src/bootstrap_config/policy_store_config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/json_util.rsjans-cedarling/cedarling/src/http/cache_headers.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/init/policy_store_refresh.rsjans-cedarling/cedarling/src/init/service_factory.rsjans-cedarling/cedarling/src/lib.rsjans-cedarling/cedarling/src/tests/policy_store_loader.rsjans-cedarling/cedarling/src/tests/ssa_validation_integration.rsjans-cedarling/cedarling/src/tests/utils/cedarling_util.rs
Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Document the new configuration property for enabling periodic policy store refreshing via background workers for URL-based sources across all language bindings and documentation. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
mo-auto
added
area-documentation
| /// Inserts `value` into `map` under `key` only when `value` is non-zero. | ||
| /// Used to keep "no observation yet" indistinguishable from missing in the | ||
| /// emitted metric map. | ||
| fn insert_if_nonzero(map: &mut HashMap<String, i64>, key: &str, value: i64) { |
There was a problem hiding this comment.
Instead of i64, we can take atomic for usability, but outcome will be the same
- Update `no-store` to zero out the in-memory freshness window - Adopt refreshed cache headers from `304 Not Modified` responses - Handle non-success HTTP status codes during HEAD probes explicitly - Introduce `is_decode_error` and `is_http_status_error` to `HttpRequestError` Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
test assertions - Update cache headers on 304 Not Modified responses. - Introduce `RefreshWorkerSeed` to manage initial worker state. - Add descriptive error messages to strategy and refresh tests. - Fix typo in `RefreshStrategy` enum variant. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
haileyesus2433
dismissed stale reviews from dagregi, coderabbitai[bot], and olehbozhok
via
1315130
most of the comments could be addresses in this PR so i have fixed them in c6c21ed i only left the cjar detection related comments which are being worked on in #14201 by @dagregi |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
jans-cedarling/cedarling/src/http/cache_headers.rs (1)
59-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse all
Cache-Controlfield-values, not just the first one.
HeaderMap::get()only sees a singleCache-Controlentry, so a valid response likeCache-Control: max-age=600plus a secondCache-Control: no-storewill silently drop the later directive here. That breaks the refresh worker's cache-semantics contract by missing revalidation or freshness instructions. Please foldparse_cache_control()acrossheaders.get_all(CACHE_CONTROL)and add a regression test with split header fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/http/cache_headers.rs` around lines 59 - 62, The code currently uses headers.get(reqwest::header::CACHE_CONTROL) which only returns one field-value and can drop subsequent Cache-Control headers; update the logic that computes (cc_max_age, no_cache) to iterate headers.get_all(reqwest::header::CACHE_CONTROL), convert each value with to_str().ok(), and fold/accumulate the results by calling parse_cache_control for each field-value and combining outcomes (preserve the most restrictive/no-cache flag and the effective max-age per parse_cache_control semantics) so directives from multiple header lines are honored; update the code that produces cc_max_age and no_cache (the binding using parse_cache_control) to use this folding approach and add a regression test that sends split Cache-Control header fields (e.g., one line with max-age and another with no-store/no-cache) to assert the combined parsed result is correct.jans-cedarling/cedarling/src/init/policy_store_refresh.rs (3)
616-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't commit HEAD validators before the matching body is applied.
Lines 617-625 update
state.validatorsbefore the GET/rebuild path succeeds. If that GET fails, the next HEAD returning the same validators will hit thevalidators_match()fast path and reportNotModified, even thoughauthz_swapis still serving the older policy set. Keepnew_validatorslocal and only write them intostateafter a successful GET / short-circuit on identical bytes. Based on PR objectives, load failures must keep the previously loaded policy store active, so refresh metadata needs to keep describing that active snapshot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs` around lines 616 - 625, Do not write new_validators into state before the GET/rebuild succeeds: keep new_validators local, call tick_plain_get(ctx, state) (or a variant that accepts new_validators) to fetch/rebuild using the local new_validators, and only on successful GET/rebuild assign state.validators = new_validators and reset state.consecutive_failures/record_helped; if the GET fails, leave state.validators unchanged so future HEADs that match the currently active snapshot still go through the rebuild path rather than returning RefreshOutcome::NotModified. Ensure validators_match still short-circuits only when comparing against state.validators, and move state.strategy.record_helped calls to occur after success where appropriate.
151-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStart the reprobe cooldown when you downgrade the strategy.
choose_for_tick()treatslast_probe_at == Noneas “probe now”. Because neitherrecord_degraded()norforce_degrade()stampslast_probe_at, the first tick after every downgrade immediately retriesConditionalinstead of honoringSTRATEGY_REPROBE_INTERVAL_SECS. That burns an extra request right after each downgrade and contradicts the “minimum elapsed time between attempts to upgrade back” behavior documented in this module.Also applies to: 181-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs` around lines 151 - 176, When downgrading the strategy, ensure you start the reprobe cooldown by recording the current time into StrategyState.last_probe_at so choose_for_tick() will respect STRATEGY_REPROBE_INTERVAL_SECS; update both record_degraded() and force_degrade() to set last_probe_at = Some(Utc::now()) (or the equivalent time source used elsewhere) when they change current to a degraded strategy, leaving choose_for_tick() unchanged so it will treat the downgrade as having just probed.
1059-1277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd messages to the remaining bare assertions in this test block.
Several new assertions here still omit the required explanation (
assert!(validators_match(&a, &b));,assert_eq!(s.current, ...),assert_ne!(body_hash(...), ...), etc.). Please add descriptive messages to the remaining bare assertions so failures explain which validator or strategy case regressed. As per coding guidelines, “All assertions must include a descriptive message explaining what is being tested”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs` around lines 1059 - 1277, Update the tests that use bare assertions to include descriptive failure messages: for all validators_match tests (calls to validators_match(&a,&b)) add messages explaining which validator case is expected (e.g., "ETag matches", "Neither validator present", "ETag differs", "Last-Modified fallback", etc.), for StrategyState tests (assert_eq/assert! on s.current, counters, degraded_count, last_probe_at) include messages naming the strategy and expected behavior (e.g., "strategy remains PlainGet after many degrades", "degrade counter unchanged on choose_for_tick", "last_probe_at remains None when Conditional"), and for body_hash tests (assert_eq/assert_ne) include messages describing the hash property being tested (e.g., "empty bytes produce consistent hash", "empty vs single-byte differ", "order-sensitive hash for 'ab' vs 'ba'"). Locate these assertions by the unique symbols validators_match, StrategyState, RefreshStrategy, choose_for_tick, record_degraded, body_hash and replace the bare assert/eq/ne calls with the same macros but passing a final string literal message describing the specific expectation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 255-261: The NotModified branch currently builds a brand-new
CacheHeadersState via CacheHeadersState::from_headers and returns it in
ConditionalFetch::NotModified, which discards any previously-known validators
(ETag/Last-Modified) when the 304 response omits fields; instead, merge the
incoming headers into the existing state.validators field: call a merging helper
(or update fields manually) to copy only non-empty/Some values from the new
CacheHeadersState into the prior state.validators so tick_conditional retains
existing ETag/Last-Modified and freshness metadata rather than replacing them
wholesale.
- Around line 584-587: The tests use bare assertions that violate the repo
style; update each assert!(matches!(result, super::ConditionalFetch::NotModified
{ .. })) and the related assert_eq! calls to include descriptive messages: for
the matches assertions (e.g., where result is matched against
super::ConditionalFetch::NotModified) add a message like "expected
ConditionalFetch::NotModified for <describe scenario> but got {:#?}", and for
assert_eq! include a message that prints both expected and actual (e.g.,
"refresh token invariants: expected X, got {:?}"). Apply these changes to the
three blocks shown (the NotModified matches around the result variable and the
assert_eq! pairs in the 621-627 and 658-665 sections) so each assertion includes
a clear, contextual message referencing the symbol names (result,
super::ConditionalFetch::NotModified, and the compared values).
In `@jans-cedarling/http_utils/src/lib.rs`:
- Around line 430-464: Replace the optional `if let Err(e) = bytes_result`
branch with a deterministic assertion that the read fails (use
`bytes_result.expect_err("expected mockito to truncate response and produce
DecodeResponseBytes")`), capture that error into `e`, then construct the
`HttpRequestError` via
`HttpRequestError::new(HttpRequestReasonError::DecodeResponseBytes(e),
Some(reqwest::StatusCode::OK))` and run the existing
`assert!(err.is_decode_error(), ...)` and related negative checks; this ensures
`bytes_result` actually exercises the `DecodeResponseBytes` path rather than
silently succeeding.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/http/cache_headers.rs`:
- Around line 59-62: The code currently uses
headers.get(reqwest::header::CACHE_CONTROL) which only returns one field-value
and can drop subsequent Cache-Control headers; update the logic that computes
(cc_max_age, no_cache) to iterate
headers.get_all(reqwest::header::CACHE_CONTROL), convert each value with
to_str().ok(), and fold/accumulate the results by calling parse_cache_control
for each field-value and combining outcomes (preserve the most
restrictive/no-cache flag and the effective max-age per parse_cache_control
semantics) so directives from multiple header lines are honored; update the code
that produces cc_max_age and no_cache (the binding using parse_cache_control) to
use this folding approach and add a regression test that sends split
Cache-Control header fields (e.g., one line with max-age and another with
no-store/no-cache) to assert the combined parsed result is correct.
In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs`:
- Around line 616-625: Do not write new_validators into state before the
GET/rebuild succeeds: keep new_validators local, call tick_plain_get(ctx, state)
(or a variant that accepts new_validators) to fetch/rebuild using the local
new_validators, and only on successful GET/rebuild assign state.validators =
new_validators and reset state.consecutive_failures/record_helped; if the GET
fails, leave state.validators unchanged so future HEADs that match the currently
active snapshot still go through the rebuild path rather than returning
RefreshOutcome::NotModified. Ensure validators_match still short-circuits only
when comparing against state.validators, and move state.strategy.record_helped
calls to occur after success where appropriate.
- Around line 151-176: When downgrading the strategy, ensure you start the
reprobe cooldown by recording the current time into StrategyState.last_probe_at
so choose_for_tick() will respect STRATEGY_REPROBE_INTERVAL_SECS; update both
record_degraded() and force_degrade() to set last_probe_at = Some(Utc::now())
(or the equivalent time source used elsewhere) when they change current to a
degraded strategy, leaving choose_for_tick() unchanged so it will treat the
downgrade as having just probed.
- Around line 1059-1277: Update the tests that use bare assertions to include
descriptive failure messages: for all validators_match tests (calls to
validators_match(&a,&b)) add messages explaining which validator case is
expected (e.g., "ETag matches", "Neither validator present", "ETag differs",
"Last-Modified fallback", etc.), for StrategyState tests (assert_eq/assert! on
s.current, counters, degraded_count, last_probe_at) include messages naming the
strategy and expected behavior (e.g., "strategy remains PlainGet after many
degrades", "degrade counter unchanged on choose_for_tick", "last_probe_at
remains None when Conditional"), and for body_hash tests (assert_eq/assert_ne)
include messages describing the hash property being tested (e.g., "empty bytes
produce consistent hash", "empty vs single-byte differ", "order-sensitive hash
for 'ab' vs 'ba'"). Locate these assertions by the unique symbols
validators_match, StrategyState, RefreshStrategy, choose_for_tick,
record_degraded, body_hash and replace the bare assert/eq/ne calls with the same
macros but passing a final string literal message describing the specific
expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b61511b-b568-496d-a8c3-eccb7c5e05c9
📒 Files selected for processing (4)
jans-cedarling/cedarling/src/http/cache_headers.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store_refresh.rsjans-cedarling/http_utils/src/lib.rs
logic - Properly fold all `Cache-Control` header lines per RFC 9110. - Introduce `merge_from` to ensure `304 Not Modified` responses only update cache headers rather than overwriting existing validators (ETag/ Last-Modified). - Fix `StrategyState` and `tick` logic to correctly handle transition cooldowns and prevent incorrect short-circuiting during HEAD requests. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
jans-cedarling/cedarling/src/http/mod.rs (3)
791-792: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd descriptive message to this assertion.
As per coding guidelines, "All assertions must include a descriptive message explaining what is being tested".
📝 Suggested fix
let outcome = client.head_validators(&url).await.expect("request"); - assert!(matches!(outcome, super::HeadOutcome::NotSupported)); + assert!( + matches!(outcome, super::HeadOutcome::NotSupported), + "HEAD returning 405 must map to HeadOutcome::NotSupported", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/http/mod.rs` around lines 791 - 792, The test assertion using assert!(matches!(outcome, super::HeadOutcome::NotSupported)) lacks a descriptive message; update the assert! call for HeadOutcome::NotSupported to include a clear message explaining what is being verified (e.g., that the outcome equals HeadOutcome::NotSupported and why), referencing the HeadOutcome::NotSupported enum variant and the local variable outcome so reviewers can quickly understand the intent of the assertion.Source: Coding guidelines
808-809: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd descriptive message to this assertion.
As per coding guidelines, "All assertions must include a descriptive message explaining what is being tested".
📝 Suggested fix
let outcome = client.head_validators(&url).await.expect("request"); - assert!(matches!(outcome, super::HeadOutcome::NotSupported)); + assert!( + matches!(outcome, super::HeadOutcome::NotSupported), + "HEAD returning 501 must map to HeadOutcome::NotSupported", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/http/mod.rs` around lines 808 - 809, The assertion on the head_validators result lacks a descriptive message; update the assert! that checks matches!(outcome, super::HeadOutcome::NotSupported) to include a clear failure message that states the expectation and prints the actual outcome (e.g., "expected HeadOutcome::NotSupported but got {:?}", outcome) so failures are informative; modify the assertion around the variable outcome returned by client.head_validators(...) accordingly.Source: Coding guidelines
738-748: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd descriptive messages to these HEAD validator test assertions.
These assertions lack the descriptive messages required by coding guidelines.
As per coding guidelines, "All assertions must include a descriptive message explaining what is being tested".
📝 Suggested fix
match outcome { super::HeadOutcome::Headers(v) => { - assert_eq!(v.etag.as_deref(), Some("\"v3\"")); + assert_eq!( + v.etag.as_deref(), + Some("\"v3\""), + "HEAD response ETag must be captured", + ); assert_eq!( v.last_modified.as_deref(), - Some("Sun, 22 May 2026 12:00:00 GMT") + Some("Sun, 22 May 2026 12:00:00 GMT"), + "HEAD response Last-Modified must be captured", ); - assert!(v.has_validator()); + assert!( + v.has_validator(), + "has_validator() must be true when ETag and Last-Modified are present", + ); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/http/mod.rs` around lines 738 - 748, The test's assertions inside the HeadOutcome::Headers match arm lack descriptive messages; update each assert_eq! and assert! (the checks on v.etag.as_deref(), v.last_modified.as_deref(), and v.has_validator()) to include a clear message string describing what is being validated (for example: "expected ETag to be \"v3\"", "expected Last-Modified to be ...", "expected validator to be present"), and similarly update the panic path for HeadOutcome::NotSupported to include context like "expected Headers, got NotSupported"; ensure the surrounding test flow (including mock.assert_async().await) remains unchanged.Source: Coding guidelines
jans-cedarling/http_utils/src/lib.rs (1)
452-461: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd descriptive messages to cross-check assertions.
These assertions verify the negative/other-variant behavior but lack descriptive messages.
As per coding guidelines, "All assertions must include a descriptive message explaining what is being tested".
📝 Suggested fix
// Cross-check the negative directions on stable error variants. let max_retries = HttpRequestError::new(HttpRequestReasonError::MaxRetriesExceeded, None); - assert!(!max_retries.is_decode_error()); - assert!(max_retries.is_max_retries_exceeded()); + assert!( + !max_retries.is_decode_error(), + "MaxRetriesExceeded must NOT satisfy is_decode_error()", + ); + assert!( + max_retries.is_max_retries_exceeded(), + "MaxRetriesExceeded must satisfy is_max_retries_exceeded()", + ); let status = HttpRequestError::new(HttpRequestReasonError::HttpStatusError, None); - assert!(!status.is_decode_error()); - assert!(!status.is_max_retries_exceeded()); - assert!(status.is_http_status_error()); + assert!( + !status.is_decode_error(), + "HttpStatusError must NOT satisfy is_decode_error()", + ); + assert!( + !status.is_max_retries_exceeded(), + "HttpStatusError must NOT satisfy is_max_retries_exceeded()", + ); + assert!( + status.is_http_status_error(), + "HttpStatusError must satisfy is_http_status_error()", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/http_utils/src/lib.rs` around lines 452 - 461, The assertions around HttpRequestError::new using HttpRequestReasonError::MaxRetriesExceeded and HttpRequestReasonError::HttpStatusError lack descriptive messages; update each assert! (the ones calling is_decode_error(), is_max_retries_exceeded(), and is_http_status_error()) to include a clear string describing the expectation (e.g., "MaxRetriesExceeded should not be a decode error", "MaxRetriesExceeded should be max retries exceeded", "HttpStatusError should not be decode error", "HttpStatusError should not be max retries exceeded", "HttpStatusError should be http status error") so each check on the HttpRequestError::new results contains an explanatory message.Source: Coding guidelines
jans-cedarling/cedarling/src/init/policy_store_refresh.rs (1)
1081-1081: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd descriptive message to this assertion.
As per coding guidelines, "All assertions must include a descriptive message explaining what is being tested".
📝 Suggested fix
- assert!(validators_match(&a, &b), "matching ETag wins"); + assert!( + validators_match(&a, &b), + "when both sides have ETag, ETag match takes precedence over differing Last-Modified", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs` at line 1081, The assertion assert!(validators_match(&a, &b), "matching ETag wins"); lacks a descriptive message; update it to include what is being validated and expected outcome (e.g., explain that two validator sets should be equal when ETags match), by replacing the lightweight message with a clear description referencing validators_match, the inputs (&a, &b) and that matching ETag implies identical validators so failures show those details for debugging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs`:
- Around line 1232-1233: The two assertions asserting
s.conditional_to_head_transitions == 0 and s.head_to_plain_transitions == 0 lack
descriptive messages; update the assert_eq! calls in the test (where the
variable s is inspected in policy_store_refresh.rs) to include descriptive
messages that state what is being tested and show expected vs actual (e.g.,
"conditional_to_head_transitions should be 0" and "head_to_plain_transitions
should be 0") so failures are self-explanatory; keep the assertions on the same
symbols s.conditional_to_head_transitions and s.head_to_plain_transitions and
only add the message argument to each assert_eq! macro.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/http/mod.rs`:
- Around line 791-792: The test assertion using assert!(matches!(outcome,
super::HeadOutcome::NotSupported)) lacks a descriptive message; update the
assert! call for HeadOutcome::NotSupported to include a clear message explaining
what is being verified (e.g., that the outcome equals HeadOutcome::NotSupported
and why), referencing the HeadOutcome::NotSupported enum variant and the local
variable outcome so reviewers can quickly understand the intent of the
assertion.
- Around line 808-809: The assertion on the head_validators result lacks a
descriptive message; update the assert! that checks matches!(outcome,
super::HeadOutcome::NotSupported) to include a clear failure message that states
the expectation and prints the actual outcome (e.g., "expected
HeadOutcome::NotSupported but got {:?}", outcome) so failures are informative;
modify the assertion around the variable outcome returned by
client.head_validators(...) accordingly.
- Around line 738-748: The test's assertions inside the HeadOutcome::Headers
match arm lack descriptive messages; update each assert_eq! and assert! (the
checks on v.etag.as_deref(), v.last_modified.as_deref(), and v.has_validator())
to include a clear message string describing what is being validated (for
example: "expected ETag to be \"v3\"", "expected Last-Modified to be ...",
"expected validator to be present"), and similarly update the panic path for
HeadOutcome::NotSupported to include context like "expected Headers, got
NotSupported"; ensure the surrounding test flow (including
mock.assert_async().await) remains unchanged.
In `@jans-cedarling/cedarling/src/init/policy_store_refresh.rs`:
- Line 1081: The assertion assert!(validators_match(&a, &b), "matching ETag
wins"); lacks a descriptive message; update it to include what is being
validated and expected outcome (e.g., explain that two validator sets should be
equal when ETags match), by replacing the lightweight message with a clear
description referencing validators_match, the inputs (&a, &b) and that matching
ETag implies identical validators so failures show those details for debugging.
In `@jans-cedarling/http_utils/src/lib.rs`:
- Around line 452-461: The assertions around HttpRequestError::new using
HttpRequestReasonError::MaxRetriesExceeded and
HttpRequestReasonError::HttpStatusError lack descriptive messages; update each
assert! (the ones calling is_decode_error(), is_max_retries_exceeded(), and
is_http_status_error()) to include a clear string describing the expectation
(e.g., "MaxRetriesExceeded should not be a decode error", "MaxRetriesExceeded
should be max retries exceeded", "HttpStatusError should not be decode error",
"HttpStatusError should not be max retries exceeded", "HttpStatusError should be
http status error") so each check on the HttpRequestError::new results contains
an explanatory message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6f0abba-a905-496f-81aa-8dd87013ccba
📒 Files selected for processing (4)
jans-cedarling/cedarling/src/http/cache_headers.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store_refresh.rsjans-cedarling/http_utils/src/lib.rs
|
@coderabbitai if no more comments approve |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the open review comments on PR `#14174`
gh pr view 14174 --repo JanssenProject/jans --json reviewDecision,reviews,state
echo "---"
# List review threads/comments
gh api repos/JanssenProject/jans/pulls/14174/reviews --jq '.[] | {id: .id, user: .user.login, state: .state, submitted_at: .submitted_at}'
echo "---"
# Check for unresolved review comments from coderabbitai
gh api repos/JanssenProject/jans/pulls/14174/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[:100], in_reply_to_id: .in_reply_to_id}'Length of output: 133902
From the latest reviews (2026-06-09), not yet resolved: 🟠 Major — outside diff (need fixes before approval):
🔵 Trivial (nits — missing descriptive assertion messages):
Once those major items are addressed, I'm happy to approve. The trivial nits can be done in the same pass or filed as follow-up work if preferred. |
@coderabbitai all of those are addresed from commit c6c21ed - 71ab100 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the relevant sections in cache_headers.rs for no-store handling
rg -n "no_store|no-store|no_cache|fresh_for" jans-cedarling/cedarling/src/http/cache_headers.rs | head -60
echo "=== StrategyState / reprobe / last_probe_at ==="
rg -n "last_probe_at|record_degraded|force_degrade|STRATEGY_REPROBE" jans-cedarling/cedarling/src/init/policy_store_refresh.rs | head -60
echo "=== HEAD validators commit logic ==="
rg -n "new_validators|state\.validators" jans-cedarling/cedarling/src/init/policy_store_refresh.rs | head -60Length of output: 7238
Everything looks good — approving! 🐇 ✨ [approve] |
Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Introduce `read_response_capped` to the `HttpClient` to ensure that all policy store fetches and refresh ticks honor the configured `max_response_size_bytes` limit. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Extract error classification logic into `classify_fetch_error` to improve testability and fix a bug where retried HTTP errors were misidentified as network errors. Added `status_code` accessor to `HttpRequestError` to ensure errors containing HTTP responses are correctly categorized even after retry exhaustion. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
store swap Ensure the metrics gauge reflects the current policy count after a successful refresh. Previously, it remained pinned to the bootstrap value, causing the reported metric to diverge from the active policy set. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
headers Ensure `fresh_for` is overwritten by incoming values (including `None`) to prevent old `max-age` values from pinning the worker refresh cadence indefinitely when subsequent 304 responses provide no new freshness claims. Maintain sticky behavior for resource validators. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
validators Replace validator overwriting with a merge operation during body hash short circuits to prevent the loss of cache validation headers when a server or proxy omits them in subsequent responses. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Consolidate the degradation logic into a single source of truth, `degrade_one_step`, to ensure consistent transition handling, counter updates, and cooldown stamping across both automatic and forced degradations. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
Compare ETags using weak comparison semantics by stripping the optional `W/` prefix. This prevents unnecessary full GET requests when a server returns a weak ETag in a HEAD probe but a strong ETag in a subsequent GET response. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/http/cache_headers.rs (1)
189-194:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake repeated
max-agedirectives min-win even inside one header value.
parse_cache_control("max-age=60, max-age=600")currently returns600because the later directive overwrites the earlier one. That disagrees with the “most restrictive wins” merge you already apply across separateCache-Controllines, and intermediaries are allowed to coalesce repeated header lines into one comma-separated value. The same response can therefore produce a different refresh cadence depending on wire formatting.Suggested fix
} else if let Some(rest) = lower.strip_prefix("max-age=") { // Strip optional quotes; ignore unparseable / negative values. let trimmed = rest.trim().trim_matches('"'); if let Ok(secs) = trimmed.parse::<u64>() { - max_age = Some(secs); + max_age = Some(match max_age { + Some(prev) => prev.min(secs), + None => secs, + }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/http/cache_headers.rs` around lines 189 - 194, The max-age parsing branch currently overwrites any previous max_age when encountering another "max-age=" token in the same header value; modify the block in cache_headers.rs (the branch using lower.strip_prefix("max-age=") that sets max_age) to take the minimum of the existing max_age and the newly parsed secs instead of replacing it: if max_age is None set Some(secs), otherwise set Some(existing.min(secs)); keep the same parsing/trim logic and error handling.jans-cedarling/http_utils/src/lib.rs (1)
73-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose oversized-body failures as body-read errors too.
read_response_capped()reports cap breaches asResponseTooLarge, butis_decode_error()only matchesDecodeResponseBytes(_). Downstream, that makes an oversized conditional GET surface asHttpErrorviaclassify_fetch_error(), whiletick_plain_get()records the same over-cap body asDecodeError. The outcome bucket now depends on strategy, not failure mode.Please broaden this helper or add a separate
is_body_read_error()that covers bothDecodeResponseBytes(_)andResponseTooLarge, then use that in the refresh worker’s classifier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/http_utils/src/lib.rs` around lines 73 - 95, The helper is currently only matching HttpRequestReasonError::DecodeResponseBytes in is_decode_error(), so oversized-body failures (ResponseTooLarge) are not classified as body-read errors; add a new helper is_body_read_error() (or broaden is_decode_error()) that returns true for both HttpRequestReasonError::DecodeResponseBytes(_) and HttpRequestReasonError::ResponseTooLarge, and update callers (e.g., the refresh worker classifier) to use the new is_body_read_error() so capped-response errors and decode errors are treated the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@jans-cedarling/cedarling/src/http/cache_headers.rs`:
- Around line 189-194: The max-age parsing branch currently overwrites any
previous max_age when encountering another "max-age=" token in the same header
value; modify the block in cache_headers.rs (the branch using
lower.strip_prefix("max-age=") that sets max_age) to take the minimum of the
existing max_age and the newly parsed secs instead of replacing it: if max_age
is None set Some(secs), otherwise set Some(existing.min(secs)); keep the same
parsing/trim logic and error handling.
In `@jans-cedarling/http_utils/src/lib.rs`:
- Around line 73-95: The helper is currently only matching
HttpRequestReasonError::DecodeResponseBytes in is_decode_error(), so
oversized-body failures (ResponseTooLarge) are not classified as body-read
errors; add a new helper is_body_read_error() (or broaden is_decode_error())
that returns true for both HttpRequestReasonError::DecodeResponseBytes(_) and
HttpRequestReasonError::ResponseTooLarge, and update callers (e.g., the refresh
worker classifier) to use the new is_body_read_error() so capped-response errors
and decode errors are treated the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f2c4807-c5c6-4d88-82de-4d9d4627cd2c
📒 Files selected for processing (6)
jans-cedarling/cedarling/src/authz/metrics.rsjans-cedarling/cedarling/src/http/cache_headers.rsjans-cedarling/cedarling/src/http/mod.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/init/policy_store_refresh.rsjans-cedarling/http_utils/src/lib.rs
Review: 4 findingsChecked each against current 1.
|
i have created a separate issue for all of them in under improvements #14245 #14244 #14243 #14242 |
5 participants
Prepare
Description
Target issue
closes #issue-number-here
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Closes #14127,
Summary by CodeRabbit
New Features
Documentation