feat(policy): add definition and admin api#107
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a complete guardrail policy management feature with JSON-schema-validated policy objects, CEL-based conditional expressions, a PoliciesStore with a name index, resource registry integration, and full admin CRUD HTTP endpoints and tests. ChangesPolicy Management Feature with CEL Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/admin/policies.rs`:
- Around line 200-219: The current check-then-write around policy.name (using
state.resources.policies.get_by_name, state.config_provider.get_all::<Policy>,
and state.config_provider.put) is racy; make the uniqueness check atomic by
serializing writes for a given derived name key before calling put: acquire a
per-policy-name mutex (e.g. a HashMap<String, Arc<Mutex<()>>> stored on state or
a dedicated name_locks helper) keyed by policy.name (or the derived name key),
hold the lock, re-check get_by_name and config_provider.get_all::<Policy> while
holding the lock, then call state.config_provider.put(&key, &policy) and release
the lock; alternatively, if the storage supports conditional
writes/transactions, use an atomic conditional_put/transaction API on
state.config_provider that fails if a different key already claims the same
name, and handle that error by returning APIError::BadRequest("Policy name
already exists").
🪄 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: CHILL
Plan: Pro
Run ID: 06e86e2f-9ff4-4813-945c-e9e7186f7a8e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlsrc/admin/mod.rssrc/admin/policies.rssrc/config/entities/mod.rssrc/config/entities/policies-schema.jsonsrc/config/entities/policies.rs
Comment on lines
+200
to
+219
| if let Some(found) = state.resources.policies.get_by_name(&policy.name) | ||
| && found.id != id | ||
| { | ||
| return APIError::BadRequest("Policy name already exists".to_string()).into_response(); | ||
| } | ||
|
|
||
| match state.config_provider.get_all::<Policy>("/policies").await { | ||
| Ok(data) => { | ||
| if data | ||
| .iter() | ||
| .any(|item| item.value.name == policy.name && item.key != key) | ||
| { | ||
| return APIError::BadRequest("Policy name already exists".to_string()) | ||
| .into_response(); | ||
| } | ||
| } | ||
| Err(err) => return APIError::InternalError(err).into_response(), | ||
| } | ||
|
|
||
| match state.config_provider.put(&key, &policy).await { |
There was a problem hiding this comment.
Enforce policy-name uniqueness atomically.
This is still a check-then-write race: two concurrent create/update requests can both pass the cache/provider scans and then both put the same policy.name. That breaks the uniqueness guarantee, and because the registry’s secondary name index is one-to-one, one policy will shadow the other on get_by_name.
Move the uniqueness constraint into an atomic storage operation, or serialize writes on the derived name key before calling put.
🤖 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 `@src/admin/policies.rs` around lines 200 - 219, The current check-then-write
around policy.name (using state.resources.policies.get_by_name,
state.config_provider.get_all::<Policy>, and state.config_provider.put) is racy;
make the uniqueness check atomic by serializing writes for a given derived name
key before calling put: acquire a per-policy-name mutex (e.g. a HashMap<String,
Arc<Mutex<()>>> stored on state or a dedicated name_locks helper) keyed by
policy.name (or the derived name key), hold the lock, re-check get_by_name and
config_provider.get_all::<Policy> while holding the lock, then call
state.config_provider.put(&key, &policy) and release the lock; alternatively, if
the storage supports conditional writes/transactions, use an atomic
conditional_put/transaction API on state.config_provider that fails if a
different key already claims the same name, and handle that error by returning
APIError::BadRequest("Policy name already exists").
Collaborator
Author
There was a problem hiding this comment.
This is to expect; we do not provide write guarantees for concurrent API calls.
There was a problem hiding this comment.
@bzp2010, understood — acknowledged and noted for future reviews.
(ﾟ･ﾟ*)(｡_｡)
✏️ Learnings added
Learnt from: bzp2010
Repo: api7/aisix PR: 107
File: src/admin/policies.rs:200-219
Timestamp: 2026-05-16T01:52:14.029Z
Learning: In the api7/aisix repository, the admin API (src/admin/) does **not** provide concurrent write guarantees. Check-then-write patterns for uniqueness constraints (e.g., policy name uniqueness in src/admin/policies.rs) are intentional and acceptable. Do not flag these as race conditions during code review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bzp2010
Repo: api7/aisix PR: 34
File: src/proxy/hooks2/authorization/mod.rs:1-1
Timestamp: 2026-04-10T09:35:05.745Z
Learning: In Rust code using the `anyhow` crate, treat `use anyhow::Result;` as potentially used even if the code uses non-default error types like `Result<T, SomeOtherError>` (i.e., `Result<T, E>` where `E` is not `anyhow::Error`). Because `anyhow::Result` is an alias for `std::result::Result<T, E=anyhow::Error>`, the import is referenced by the `Result<..., ...>` syntax. Do not flag `use anyhow::Result;` as unused solely due to the presence of `Result<T, SomeOtherError>` usages.
Learnt from: AlinsRan
Repo: api7/aisix PR: 46
File: src/lib.rs:122-126
Timestamp: 2026-04-17T09:32:54.079Z
Learning: Avoid adding `#[fastrace::trace]` to top-level process-lifetime entrypoints in `src/lib.rs`—specifically `run()`, `run_with_config()`, and `run_with_provider()`. These functions cover the entire server lifetime (startup → shutdown), while Fastrace spans should be used for request-scoped boundaries (e.g., HTTP/RPC handler functions) rather than global process entrypoints.
Learnt from: bzp2010
Repo: api7/aisix PR: 80
File: src/main.rs:9-9
Timestamp: 2026-05-03T12:45:26.883Z
Learning: In this repository (api7/aisix), `aisix::utils::instance::init()` is intentionally **not** guaranteed to be called by the library entrypoints (`src/lib.rs` via `run()`, `run_with_config()`, `run_with_provider()`). Treat it as the embedder/binary’s responsibility to call `instance::init()` before handling any requests. Also note that `instance_id()` and `run_id()` in `src/utils/instance/mod.rs` intentionally panic (via `.expect()`) when the `OnceLock`s are uninitialized—this is by design. During code review, do **not** flag missing `instance::init()` wiring inside the library startup path as a bug and do **not** suggest moving `instance::init()` into the library automatically.
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 `@tests/admin/policies.test.ts`:
- Around line 194-202: The test sends a policy body with name
'invalid_cel_policy' via buildPolicyBody but asserts the CEL error references
'invalid-cel-policy'; update the test to use the same name in both places—either
change the buildPolicyBody call to use 'invalid-cel-policy' or change the expect
assertion to look for 'invalid_cel_policy'—so the request payload name (in the
adminPut call using POLICIES_URL and buildPolicyBody) and the expected error
string in putResp.data.error_msg match exactly.
🪄 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: CHILL
Plan: Pro
Run ID: b930857d-1b8f-4c17-ad6a-c67ce1a6d637
📒 Files selected for processing (3)
src/config/entities/policies.rstests/admin/policies.test.tstests/utils/admin.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/admin.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/entities/policies.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests