feat(guardrail): add config storage#103
Merged
Merged
Conversation
📝 WalkthroughWalkthroughRefactored BedrockGuardrailRuntime to remove stored credentials and accept explicit config parameters, introduced guardrails configuration with schema-backed validation and resource storage, integrated guardrails into the resource registry, linked guardrails to models via ID references, and exported the public API. ChangesGuardrails system introduction
Sequence Diagram(s)sequenceDiagram
participant Client
participant BedrockGuardrailRuntime
participant BedrockGuardrailConfig
Client->>BedrockGuardrailRuntime: check(request, config)
BedrockGuardrailRuntime->>BedrockGuardrailConfig: build_apply_url(config)
BedrockGuardrailConfig-->>BedrockGuardrailRuntime: url with region/endpoint
BedrockGuardrailRuntime->>BedrockGuardrailConfig: sign_request(config)
BedrockGuardrailConfig-->>BedrockGuardrailRuntime: signed request
BedrockGuardrailRuntime->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-guardrail/src/guardrails/bedrock.rs (1)
1-20: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winOrganize imports according to Rust conventions.
Imports should be sorted alphabetically with stdlib first, then external crates, then local modules. As per coding guidelines, Rust imports should follow: standard library first, then external crates (alphabetical), then local modules.
♻️ Proposed fix
-use std::{fmt, time::SystemTime}; - -use async_trait::async_trait; -use aws_credential_types::Credentials; -use aws_sigv4::{ - http_request::{SignableBody, SignableRequest, SigningSettings, sign}, - sign::v4, -}; -use aws_smithy_runtime_api::client::identity::Identity; -use http::{HeaderMap, HeaderValue, Method, Request, StatusCode, header::CONTENT_TYPE}; -use percent_encoding::{AsciiSet, CONTROLS, utf8_percent_encode}; -use reqwest::{Client, Url}; -use serde::{Deserialize, Serialize}; -use thiserror::Error; -use utoipa::ToSchema; - -use crate::traits::{ - GuardrailCheckPayload, GuardrailMessage, GuardrailMessageContent, GuardrailMeta, - GuardrailOutcome, GuardrailRuntime, GuardrailStage, -}; +use std::{fmt, time::SystemTime}; + +use async_trait::async_trait; +use aws_credential_types::Credentials; +use aws_sigv4::{ + http_request::{sign, SignableBody, SignableRequest, SigningSettings}, + sign::v4, +}; +use aws_smithy_runtime_api::client::identity::Identity; +use http::{header::CONTENT_TYPE, HeaderMap, HeaderValue, Method, Request, StatusCode}; +use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; +use reqwest::{Client, Url}; +use serde::{Deserialize, Serialize}; +use thiserror::Error; +use utoipa::ToSchema; + +use crate::traits::{ + GuardrailCheckPayload, GuardrailMessage, GuardrailMessageContent, GuardrailMeta, + GuardrailOutcome, GuardrailRuntime, GuardrailStage, +};🤖 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 `@crates/aisix-guardrail/src/guardrails/bedrock.rs` around lines 1 - 20, The import block at the top is not ordered per project convention; reorder the use statements so std library imports come first (e.g., use std::{fmt, time::SystemTime};), then external crates in alphabetical order (async_trait, aws_credential_types, aws_sigv4, aws_smithy_runtime_api, http, percent_encoding, reqwest, serde, thiserror, utoipa), and finally local crate imports (crate::traits::{GuardrailCheckPayload, GuardrailMessage, GuardrailMessageContent, GuardrailMeta, GuardrailOutcome, GuardrailRuntime, GuardrailStage}); separate each group with a single blank line to match style guidelines.
🧹 Nitpick comments (5)
src/config/entities/guardrails.rs (3)
38-45: ⚡ Quick winAdd documentation for the public Guardrail struct.
The public struct lacks a doc comment explaining the guardrail entity structure. As per coding guidelines, use
///for doc comments on public items in Rust.📝 Proposed addition
+/// A guardrail entity combining metadata and runtime configuration. +/// +/// The `guardrail` field is flattened, so `type` and `config` appear at the top level in JSON. #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] pub struct Guardrail { + /// Human-readable name of the guardrail. pub name: String, #[serde(flatten)] #[schema(inline)] pub guardrail: GuardrailConfig, }🤖 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/config/entities/guardrails.rs` around lines 38 - 45, Add a Rust doc comment (using ///) above the public Guardrail struct describing its purpose and fields: explain that Guardrail represents a named guardrail entity with a human-readable name and an embedded GuardrailConfig (flattened into the struct) that contains the actual configuration options; mention serialization/deserialization behavior if relevant and that the config is inlined via serde(flatten) and schema(inline). Place the comment immediately above the pub struct Guardrail declaration to follow crate documentation standards.
68-88: ⚡ Quick winAdd documentation and tracing to public GuardrailsStore methods.
Public methods lack doc comments and
#[fastrace::trace]decorators. As per coding guidelines, use///for doc comments on public items and#[fastrace::trace]decorator for distributed tracing on public functions insrc/**/*.rs.📝 Proposed additions
+/// Store for guardrail configuration entities. +/// +/// Backed by an `EntityStore` with JSON Schema validation. #[derive(Clone)] pub struct GuardrailsStore { store: EntityStore<Guardrail>, } impl GuardrailsStore { + /// Creates a new guardrails store, loading existing entries from the provider. + #[fastrace::trace] pub async fn new(provider: Arc<dyn ConfigProvider + Send + Sync>) -> Self { Self { store: EntityStore::new(provider, "/guardrails/", "guardrails", Some(validate), &[]) .await, } } + /// Returns a snapshot of all guardrails indexed by ID. + #[fastrace::trace] pub fn list(&self) -> Arc<HashMap<String, ResourceEntry<Guardrail>>> { self.store.list() } + /// Retrieves a guardrail by its ID. + #[fastrace::trace] pub fn get_by_id(&self, id: &str) -> Option<ResourceEntry<Guardrail>> { self.store.get(id) } }🤖 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/config/entities/guardrails.rs` around lines 68 - 88, Add API docs and tracing to the public GuardrailsStore API: add /// doc comments describing the GuardrailsStore struct and each public method (new, list, get_by_id) and annotate the public functions with #[fastrace::trace] (i.e., #[fastrace::trace] pub async fn new(...), #[fastrace::trace] pub fn list(&self) -> ..., #[fastrace::trace] pub fn get_by_id(&self, id: &str) -> ...). Ensure the doc comments briefly explain purpose/behavior and add any required fastrace import or feature gate if missing so the attribute compiles.
23-28: ⚡ Quick winAdd documentation for the public GuardrailConfig enum.
The public enum lacks a doc comment explaining its purpose and variants. As per coding guidelines, use
///for doc comments on public items in Rust.📝 Proposed addition
+/// Configuration for a guardrail runtime. +/// +/// This enum is tagged by `type` and carries provider-specific configuration. #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] #[serde(tag = "type", content = "config")] pub enum GuardrailConfig { + /// AWS Bedrock guardrail configuration. #[serde(rename = "bedrock")] Bedrock(BedrockGuardrailConfig), }🤖 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/config/entities/guardrails.rs` around lines 23 - 28, The public enum GuardrailConfig lacks a doc comment; add a Rust doc comment (///) above the GuardrailConfig definition describing its purpose (a serializable enum selecting the guardrail backend/config) and briefly document each variant, e.g., that GuardrailConfig::Bedrock wraps BedrockGuardrailConfig for Bedrock-specific settings; update the comment to mention serialization via serde (tag="type") if helpful so readers know how to construct/select variants.crates/aisix-guardrail/src/guardrails/bedrock.rs (1)
96-105: ⚡ Quick winAdd documentation comments to public functions.
Public functions
new()andwith_client()lack doc comments. As per coding guidelines, use///for doc comments on public items in Rust.📝 Proposed addition
impl BedrockGuardrailRuntime { + /// Creates a new Bedrock guardrail runtime with a default HTTP client. pub fn new() -> Self { Self { client: Client::new(), } } + /// Creates a new Bedrock guardrail runtime with the provided HTTP client. pub fn with_client(client: Client) -> Self { Self { client } }🤖 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 `@crates/aisix-guardrail/src/guardrails/bedrock.rs` around lines 96 - 105, Add Rust doc comments (///) to the public constructors on BedrockGuardrailRuntime: document pub fn new() and pub fn with_client(client: Client) to explain what each does, their return value (a new BedrockGuardrailRuntime), and for with_client, describe the client parameter and ownership expectations; place the comments immediately above the respective methods in the impl block for BedrockGuardrailRuntime.src/config/entities/models.rs (1)
31-35: 💤 Low valueConsider tracking the temporary nature of guardrail_ids.
The comment indicates
guardrail_idsis a temporary binding surface until policy evaluation handles guardrails dynamically. To prevent this temporary solution from becoming permanent, consider creating a tracking issue for the future migration to dynamic policy-based guardrail attachment.🤖 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/config/entities/models.rs` around lines 31 - 35, Add a short-lived tracking artifact and explicit TODO to ensure guardrail_ids on the Model struct is not left permanent: open a tracking issue in the project tracker describing the migration to dynamic policy-based guardrail attachment, note the issue number, then update the comment above the pub guardrail_ids: Vec<String> field to include a TODO/FIXME with that issue number, an expected removal milestone, and a brief migration note so future maintainers can find the plan and timeline.
🤖 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/config/entities/guardrails-schema.json`:
- Around line 34-42: The JSON schema currently requires plaintext
"access_key_id" and "secret_access_key" (and similarly "session_token" if used);
change the schema for these properties so they can accept secret-manager
references (e.g., a string like "$secret://...") or an explicit secret reference
object rather than only a raw credential. Concretely, update the "properties"
entries for "access_key_id", "secret_access_key" (and "session_token" if
present) to allow either a non-empty string OR a secret reference (e.g., a
string matching a "^\\$secret://.+$" pattern or a union with an object form like
{"secret_ref":"<string>"}), and adjust any "minLength" constraints accordingly;
ensure the schema still lists these names in the required array only if truly
required, and add schema documentation so runtime code that reads these fields
(credential resolution logic) can detect and resolve "$secret://" references
before use.
---
Outside diff comments:
In `@crates/aisix-guardrail/src/guardrails/bedrock.rs`:
- Around line 1-20: The import block at the top is not ordered per project
convention; reorder the use statements so std library imports come first (e.g.,
use std::{fmt, time::SystemTime};), then external crates in alphabetical order
(async_trait, aws_credential_types, aws_sigv4, aws_smithy_runtime_api, http,
percent_encoding, reqwest, serde, thiserror, utoipa), and finally local crate
imports (crate::traits::{GuardrailCheckPayload, GuardrailMessage,
GuardrailMessageContent, GuardrailMeta, GuardrailOutcome, GuardrailRuntime,
GuardrailStage}); separate each group with a single blank line to match style
guidelines.
---
Nitpick comments:
In `@crates/aisix-guardrail/src/guardrails/bedrock.rs`:
- Around line 96-105: Add Rust doc comments (///) to the public constructors on
BedrockGuardrailRuntime: document pub fn new() and pub fn with_client(client:
Client) to explain what each does, their return value (a new
BedrockGuardrailRuntime), and for with_client, describe the client parameter and
ownership expectations; place the comments immediately above the respective
methods in the impl block for BedrockGuardrailRuntime.
In `@src/config/entities/guardrails.rs`:
- Around line 38-45: Add a Rust doc comment (using ///) above the public
Guardrail struct describing its purpose and fields: explain that Guardrail
represents a named guardrail entity with a human-readable name and an embedded
GuardrailConfig (flattened into the struct) that contains the actual
configuration options; mention serialization/deserialization behavior if
relevant and that the config is inlined via serde(flatten) and schema(inline).
Place the comment immediately above the pub struct Guardrail declaration to
follow crate documentation standards.
- Around line 68-88: Add API docs and tracing to the public GuardrailsStore API:
add /// doc comments describing the GuardrailsStore struct and each public
method (new, list, get_by_id) and annotate the public functions with
#[fastrace::trace] (i.e., #[fastrace::trace] pub async fn new(...),
#[fastrace::trace] pub fn list(&self) -> ..., #[fastrace::trace] pub fn
get_by_id(&self, id: &str) -> ...). Ensure the doc comments briefly explain
purpose/behavior and add any required fastrace import or feature gate if missing
so the attribute compiles.
- Around line 23-28: The public enum GuardrailConfig lacks a doc comment; add a
Rust doc comment (///) above the GuardrailConfig definition describing its
purpose (a serializable enum selecting the guardrail backend/config) and briefly
document each variant, e.g., that GuardrailConfig::Bedrock wraps
BedrockGuardrailConfig for Bedrock-specific settings; update the comment to
mention serialization via serde (tag="type") if helpful so readers know how to
construct/select variants.
In `@src/config/entities/models.rs`:
- Around line 31-35: Add a short-lived tracking artifact and explicit TODO to
ensure guardrail_ids on the Model struct is not left permanent: open a tracking
issue in the project tracker describing the migration to dynamic policy-based
guardrail attachment, note the issue number, then update the comment above the
pub guardrail_ids: Vec<String> field to include a TODO/FIXME with that issue
number, an expected removal milestone, and a brief migration note so future
maintainers can find the plan and timeline.
🪄 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: 3ab9a7f7-022f-4143-9ab2-41cd3159a513
📒 Files selected for processing (8)
crates/aisix-guardrail/src/guardrails/bedrock.rscrates/aisix-guardrail/src/guardrails/mod.rssrc/config/entities/guardrails-schema.jsonsrc/config/entities/guardrails.rssrc/config/entities/mod.rssrc/config/entities/models-schema.jsonsrc/config/entities/models.rssrc/lib.rs
Comment on lines
+34
to
+42
| "access_key_id", | ||
| "secret_access_key" | ||
| ], | ||
| "properties": { | ||
| "identifier": { "type": "string", "minLength": 1 }, | ||
| "version": { "type": "string", "minLength": 1 }, | ||
| "region": { "type": "string", "minLength": 1 }, | ||
| "access_key_id": { "type": "string", "minLength": 1 }, | ||
| "secret_access_key": { "type": "string", "minLength": 1 }, |
There was a problem hiding this comment.
Security concern: Credentials stored in plain text without secret manager support.
The schema requires access_key_id, secret_access_key, and optional session_token as plain strings, which means AWS credentials will be stored in clear text in the configuration backend (etcd). This creates significant security risks:
- Credentials are exposed in configuration snapshots, logs, and backups
- No rotation mechanism without config updates
- Violates least-privilege principle for config access
Consider supporting secret manager references (e.g., $secret://path/to/credential) in these fields so credentials can be resolved at runtime from a secure vault. As per coding guidelines, environment variable and secret manager references in configuration fields must be properly resolved before use, implying the schema should allow such references.
🤖 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/config/entities/guardrails-schema.json` around lines 34 - 42, The JSON
schema currently requires plaintext "access_key_id" and "secret_access_key" (and
similarly "session_token" if used); change the schema for these properties so
they can accept secret-manager references (e.g., a string like "$secret://...")
or an explicit secret reference object rather than only a raw credential.
Concretely, update the "properties" entries for "access_key_id",
"secret_access_key" (and "session_token" if present) to allow either a non-empty
string OR a secret reference (e.g., a string matching a "^\\$secret://.+$"
pattern or a union with an object form like {"secret_ref":"<string>"}), and
adjust any "minLength" constraints accordingly; ensure the schema still lists
these names in the required array only if truly required, and add schema
documentation so runtime code that reads these fields (credential resolution
logic) can detect and resolve "$secret://" references before use.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue view 58 --repo api7/aisix
Length of output: 1384
@bzp2010, thanks for the reference! The security concern raised here — storing AWS credentials as plain strings in the config backend — is already tracked in issue #58 ("feat: support secret references and secret provider integrations for provider configs"), which proposes support for $env:// and $secret:// reference syntax for runtime credential resolution. I'll note this as a known, tracked limitation for now.
✏️ Learnings added
Learnt from: bzp2010
Repo: api7/aisix PR: 103
File: src/config/entities/guardrails-schema.json:34-42
Timestamp: 2026-05-12T09:15:42.333Z
Learning: In the api7/aisix repository, plaintext credential storage for sensitive fields (e.g., `access_key_id`, `secret_access_key`, `api_key`) in provider/guardrail config schemas is a known, accepted limitation tracked in GitHub issue `#58`. The planned solution is to support `$env://` and `$secret://` reference syntax for runtime credential resolution via secret provider integrations. Do not raise this as a blocking concern in reviews; instead, reference issue `#58`.
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.
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
Refactor