Open
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughAdds a built-in AWS Secrets Manager backend: code, tests, CLI registration, module dependencies, docs (usage, URI/key format, ESO notes), and a CHANGELOG entry. Backend supports region, assume-role, configurable cache TTL, JSON-field extraction, and in-memory caching. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend as AWS SM Backend
participant Cache as In-Memory Cache
participant AWS as AWS Secrets Manager
participant Parser as JSON Parser
Client->>Backend: GetSecret(ctx, "secret-name#json-field")
Backend->>Cache: Check cache for secret-name
alt cache hit (valid TTL)
Cache-->>Backend: cached SecretString
Backend->>Parser: extract json-field (if present)
Parser-->>Backend: field value
Backend-->>Client: return value
else cache miss or expired
Backend->>AWS: GetSecretValue(SecretId=secret-name)
AWS-->>Backend: SecretString
Backend->>Parser: extract json-field (if present)
Parser-->>Backend: field value
Backend->>Cache: store SecretString with TTL
Backend-->>Client: return value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 94-95: Remove "AWS SM" / "AWS Secrets Manager" from the list of
backends described as "not implemented" in the changelog entry and ensure the
list now only names the actual unimplemented backends (e.g., 1Password and Azure
KV); keep the existing note that a built-in Vault backend was added and that AWS
Secrets Manager was added elsewhere in the release notes so the changelog no
longer contradicts itself.
In `@docs/plugins/secrets.md`:
- Around line 255-258: Add a language tag to the fenced code block that shows
the AWS Secrets Manager URIs (the block containing
"aws-sm://myapp/api-keys#stripe_key" and
"aws-sm://arn:aws:secretsmanager:eu-west-1:123456789:secret:myapp/config#db_host")
so markdownlint MD040 is satisfied; use a neutral tag such as "text" (i.e.,
change the opening ``` to ```text) to mark the block as plain text.
- Around line 188-199: The examples still use the retired config.secrets.backend
layout; update them to the current secret_resolver.backends[] schema by
replacing config.secrets.backend entries with a secret_resolver.backends: -
name: <id> entry that sets scheme: "vault" (or "k8s"), backend: "vault" (or
"k8s") and moves the previous config block under that backend's config key
(e.g., address, role, auth_method, secrets_path for Vault); make the same
nesting change for the Kubernetes example (scheme: "k8s", backend: "k8s") and
also fix the other occurrence noted (lines 342-348) so all examples use
secret_resolver.backends[].
- Around line 214-223: Add a short "IRSA prerequisites" paragraph before the
Setup or inside the Authentication section that explains the required EKS setup:
annotate the ServiceAccount referenced in the ClusterSecretStore example with
the IAM role ARN (e.g., eks.amazonaws.com/role-arn: arn:aws:iam::...),
create/configure the IAM role with the necessary Secrets Manager permissions,
and for cross-account access ensure the source role has sts:AssumeRole
permission and the target role's trust policy allows the source to assume it;
also note that although the AWS SDK credential chain is used (and no custom auth
code is needed), IRSA still requires this prior EKS/IAM configuration and
reference the assume_role_arn setting for STS use.
In `@pkg/plugin/secrets/awssm/awssm.go`:
- Around line 121-150: The ensureClient function currently uses sync.Once
(initOnce) so any transient LoadDefaultConfig error is stored in initErr
forever; replace the sync.Once memoization with explicit guarded initialization
that retries/recovers: remove initOnce.Do and instead use a short critical
section guarded by a mutex (e.g., add initMu sync.Mutex) and in ensureClient
lock initMu, return early if b.smClient != nil and initErr == nil, otherwise
call awsconfig.LoadDefaultConfig (optionally with a small retry loop or single
retry) and on success clear b.initErr, set b.smClient (and configure sts creds
and logger as currently done), then unlock; ensure transient LoadDefaultConfig
errors are not permanently persisted by only setting b.initErr on failure and
allowing subsequent calls to reattempt initialization.
- Around line 139-142: The code assigns a raw stscreds.NewAssumeRoleProvider to
cfg.Credentials (created after sts.NewFromConfig using b.assumeRole), which
bypasses caching; wrap the assume-role provider with aws.NewCredentialsCache
before assigning to cfg.Credentials so assumed-role credentials are cached
(i.e., create the provider via stscreds.NewAssumeRoleProvider(...), then wrap it
with aws.NewCredentialsCache(...) and set cfg.Credentials to that cached
provider), leaving the rest of the sts.NewFromConfig and logging logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 603483e6-b92d-42e5-8df4-d79ad0407372
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
CHANGELOG.mdcmd/robodev/main.godocs/plugins/secrets.mddocs/roadmap.mdgo.modpkg/plugin/secrets/awssm/awssm.gopkg/plugin/secrets/awssm/awssm_test.go
Add a native aws-secrets-manager secrets backend that reads secrets from AWS Secrets Manager via the AWS SDK v2 default credential chain. On EKS with IRSA, authentication is automatic with no static credentials. The backend supports: - URI scheme aws-sm://secret-name#json-field - Secret-name-level caching with configurable TTL (default 5m) - Cross-account access via STS AssumeRole - Both secret names and full ARNs as identifiers - JSON field extraction from JSON-valued secrets Wired into the controller via the existing secret_resolver.backends config with backend type "aws-secrets-manager". 12 unit tests covering retrieval, caching, batch deduplication, ARN support, error cases, and key parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace sync.Once with mutex-guarded init so transient AWS config failures don't permanently disable the backend - Wrap stscreds.NewAssumeRoleProvider with aws.NewCredentialsCache so assumed-role credentials are cached across requests - Fix retired config schema in K8s and Vault docs examples to use secret_resolver.backends[] format - Add IRSA prerequisites section to AWS SM authentication docs - Fix CHANGELOG contradiction (AWS SM listed as both added and not implemented) - Add language tag to URI format fenced code block (MD040) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mattcamp
force-pushed
the
feat/aws-secrets-manager-integration
branch
from
4d83f5c to
d084045
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/plugins/secrets.md (1)
312-329:⚠️ Potential issue | 🟠 MajorShow the IRSA annotation step for
external-secrets-sa.This flow says the
ClusterSecretStoreauthenticates via IRSA, but the example only referencesexternal-secrets-sa; it never shows the requiredeks.amazonaws.com/role-arnannotation or clarifies that the IAM role belongs to ESO’s ServiceAccount here, not the RoboDev controller’s. Copy-pasting these steps still leaves ESO without AWS credentials.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plugins/secrets.md` around lines 312 - 329, The example ClusterSecretStore references the ServiceAccount external-secrets-sa but doesn’t show the required IRSA annotation or clarify which IAM role to use; update the docs to add the eks.amazonaws.com/role-arn annotation to the external-secrets-sa ServiceAccount (and state that the referenced IAM role should be created with the SecretsManager permissions and be bound to ESO’s ServiceAccount, not the RoboDev controller’s), and include a brief note that this ServiceAccount is the one used by the external-secrets operator (external-secrets-sa) so AWS credentials are provided via IRSA.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/plugins/secrets.md`:
- Around line 312-329: The example ClusterSecretStore references the
ServiceAccount external-secrets-sa but doesn’t show the required IRSA annotation
or clarify which IAM role to use; update the docs to add the
eks.amazonaws.com/role-arn annotation to the external-secrets-sa ServiceAccount
(and state that the referenced IAM role should be created with the
SecretsManager permissions and be bound to ESO’s ServiceAccount, not the RoboDev
controller’s), and include a brief note that this ServiceAccount is the one used
by the external-secrets operator (external-secrets-sa) so AWS credentials are
provided via IRSA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c261b94-4344-4f74-b86f-68e455cc5eb4
📒 Files selected for processing (3)
CHANGELOG.mddocs/plugins/secrets.mdpkg/plugin/secrets/awssm/awssm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Comment on lines
+193
to
+199
| result := make(map[string]string, len(keys)) | ||
| for _, key := range keys { | ||
| value, err := b.GetSecret(ctx, key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result[key] = value |
There was a problem hiding this comment.
Don’t fail the whole batch on one missing secret.
This returns on the first GetSecret error, so one absent key aborts the entire batch. That conflicts with the documented GetSecrets contract, which says missing keys should be omitted so the caller can decide which omissions are critical.
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
aws-secrets-managersecrets backend (pkg/plugin/secrets/awssm/) that reads secrets from AWS Secrets Manager via the AWS SDK v2 default credential chainaws-sm://secret-name#json-fieldURI scheme, secret-name-level caching (configurable TTL, default 5m), cross-account STS AssumeRole, and both secret names and full ARNsinitSecretsResolveralongside the existing K8s and Vault backendsdocs/plugins/secrets.mdwith configuration, key format, IAM permissions, and ESO integration guideTest plan
go test -v ./pkg/plugin/secrets/awssm/...— 12/12 passgo build ./cmd/robodev/— compiles cleangolangci-lint run ./pkg/plugin/secrets/awssm/...— no issuesgo test ./...— full suite green, zero regressionsaws-sm://URIs resolve correctly (post-merge)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests