fix(jans-cedarling): disable token cache when max TTL is zero#14166
fix(jans-cedarling): disable token cache when max TTL is zero#14166pradhankukiran wants to merge 4 commits into
Conversation
✅ 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughTokenCache now treats ChangesToken Cache Behavior and Docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/jwt/token_cache.rs`:
- Around line 289-318: Update the three tests
(max_ttl_zero_uses_token_expiration,
max_ttl_zero_does_not_cache_token_without_exp,
positive_max_ttl_caps_token_expiration) to include descriptive messages on their
assertions: for the first two tests add a message to the assert! calls that
explains the expected presence or absence of the cached token when saving via
cache.save(&TokenKind::StatusList, "jwt", token, now) and retrieving with
cache.find(...), and for the third test add a message to assert_eq! that
explains that cache.cache_duration(&token, now) should return Some(5) when token
has an exp and cache created by token_cache(5); locate these tests and update
the assert!/assert_eq! calls to include the explanatory string arguments.
🪄 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: 3b06e995-979e-40bf-af12-58a772fba2f5
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/jwt/token_cache.rs
|
Hi @pradhankukiran thank you for the contributions but the issue #14154 has been updated, sorry I should have written the issue clearly first. If you're willing to update your pr, we'd appreciate it |
|
Updated the PR to match the revised issue behavior: |
771905a to
86cd1d5
Compare
| if self.max_ttl == 0 { | ||
| self.metrics.record_cache_miss(); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Nicely done but how about doing this check in the constructor and making TokenCache.cache optional so this check becomes
let Some(ref cache) = self.cache else {
self.metrics.record_cache_miss();
return None;
};Fix CEDARLING_TOKEN_CACHE_MAX_TTL=0 so SparkV receives an internal TTL ceiling that does not reject entries using the token exp claim. Fixes JanssenProject#14154 Signed-off-by: Kiran Kumar Pradhan <pradhankukiran@gmail.com>
Update CEDARLING_TOKEN_CACHE_MAX_TTL=0 handling to bypass token cache reads and writes entirely, avoiding SparkV ttl-too-long warnings and unbounded caching. Signed-off-by: Kiran Kumar Pradhan <pradhankukiran@gmail.com>
Document that CEDARLING_TOKEN_CACHE_MAX_TTL=0 disables the token cache entirely. Signed-off-by: Kiran Kumar Pradhan <pradhankukiran@gmail.com>
Represent CEDARLING_TOKEN_CACHE_MAX_TTL=0 as an absent backing cache so disabled cache behavior is handled at construction and cache operations naturally no-op. Signed-off-by: Kiran Kumar Pradhan <pradhankukiran@gmail.com>
86cd1d5 to
3f70a0d
Compare
|
Updated per your suggestion: disabled token cache is now represented as |
Prepare
Description
Target issue
closes #14154
Implementation Details
CEDARLING_TOKEN_CACHE_MAX_TTL=0now disables the token cache entirely. This avoids repeated SparkVttl too longwarnings and avoids unbounded cache growth when no TTL cap is configured.For non-zero values, existing token cache behavior is preserved:
max_ttl > 0: cache TTL is capped by configured valueexp: configuredmax_ttlis usedRegression tests cover disabled-cache behavior for tokens with and without
exp, plus the positive TTL cap path.Test and Document the changes
Tests run:
cargo test -p cedarling token_cache --libcargo clippy -p cedarling --libPlease 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.Summary by CodeRabbit
Bug Fixes
Documentation
Tests