feat: add GCP Secret Manager provider#2403
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GCP Secret Manager as a secret provider with configuration, reference parsing, regional client caching, checksum validation, JSON field extraction, registry cleanup, dependencies, and tests. Notification monitor tests now use platform-aware timing and shutdown helpers. ChangesGCP Secret Manager support
Notification monitor test timing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SecretRegistry
participant GCPSecretManagerResolver
participant GCPSecretManager
Agent->>SecretRegistry: Resolve secret reference
SecretRegistry->>GCPSecretManagerResolver: Resolve
GCPSecretManagerResolver->>GCPSecretManager: AccessSecretVersion
GCPSecretManager-->>GCPSecretManagerResolver: Secret payload and checksum
GCPSecretManagerResolver-->>SecretRegistry: Extracted secret value
SecretRegistry-->>Agent: Resolved secret
Agent->>SecretRegistry: Close
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new GCP Secret Manager-backed secret provider to Dagu’s secret-resolution system, including global configuration defaults and lifecycle cleanup, aligning with the secure “load-at-runtime” secret management direction in #798.
Changes:
- Implement
gcp-secret-managersecret provider with support for short IDs, fully-qualified resource names (global/regional), version selection, regional endpoints, CRC32C verification, and optional JSON field extraction. - Add
secrets.gcp.{project_id,location}to config definition/loader/env bindings and extend the config JSON schema accordingly. - Ensure secret providers can be cleaned up via
Registry.Close()and add tests for the provider and JSON field extraction.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates documentation blurb to include GCP Secret Manager in secret management options. |
| internal/runtime/agent/agent.go | Closes the secrets registry after resolving secrets to release provider resources. |
| internal/cmn/secrets/resolver.go | Adds Registry.Close() to close any providers that implement Close() error. |
| internal/cmn/secrets/resolver_test.go | Updates built-in provider expectations to include gcp-secret-manager. |
| internal/cmn/secrets/gcp_secret_manager.go | Implements the GCP Secret Manager resolver and client lifecycle management. |
| internal/cmn/secrets/gcp_secret_manager_test.go | Adds tests for parsing, resolution behavior, error cases, and close semantics. |
| internal/cmn/secrets/field.go | Adds JSON field selection helper for secret payloads. |
| internal/cmn/secrets/field_test.go | Tests JSON field selection behavior and error hygiene. |
| internal/cmn/schema/config.schema.json | Extends config schema with secrets.gcp defaults. |
| internal/cmn/config/loader.go | Loads secrets.gcp defaults and adds env bindings. |
| internal/cmn/config/loader_test.go | Verifies YAML/env/config-scope loading for secrets.gcp. |
| internal/cmn/config/definition.go | Adds GCPSecretsDef to the config definition model. |
| internal/cmn/config/config.go | Adds GCPSecretsConfig to runtime config. |
| go.mod | Adds Secret Manager client dependency and related Google API dependency. |
| go.sum | Updates dependency checksums for newly introduced modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (1)
internal/cmn/secrets/resolver.go (1)
278-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider filtering out
nilerrors before appending.While
errors.Joinsafely ignoresnilvalues, it is slightly more idiomatic to only append actual errors to the slice.♻️ Proposed refactor
var errs []error for _, resolver := range resolvers { if closer, ok := resolver.(interface{ Close() error }); ok { - errs = append(errs, closer.Close()) + if err := closer.Close(); err != nil { + errs = append(errs, err) + } } } return errors.Join(errs...)🤖 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 `@internal/cmn/secrets/resolver.go` around lines 278 - 295, Update Registry.Close to append only non-nil errors returned by each resolver’s Close method, while preserving the existing resolver iteration and errors.Join aggregation.
🤖 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 `@internal/cmn/secrets/gcp_secret_manager_test.go`:
- Around line 131-163: Fix the ChecksumMismatch test case in
TestGCPSecretManagerResolverErrors by replacing the invalid new(int64(1))
expression with a valid *int64 value containing 1, while preserving the existing
CRC32C mismatch setup and assertion.
---
Nitpick comments:
In `@internal/cmn/secrets/resolver.go`:
- Around line 278-295: Update Registry.Close to append only non-nil errors
returned by each resolver’s Close method, while preserving the existing resolver
iteration and errors.Join aggregation.
🪄 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: 6279a178-4b9a-46d5-8e0a-111db8f5fc9a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
README.mdgo.modinternal/cmn/config/config.gointernal/cmn/config/definition.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/schema/config.schema.jsoninternal/cmn/secrets/field.gointernal/cmn/secrets/field_test.gointernal/cmn/secrets/gcp_secret_manager.gointernal/cmn/secrets/gcp_secret_manager_test.gointernal/cmn/secrets/resolver.gointernal/cmn/secrets/resolver_test.gointernal/runtime/agent/agent.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
internal/cmn/secrets/resolver_test.go:25
NewRegistry()now includes the GCP Secret Manager provider, but this test no longer asserts it (or the expected number of built-in providers). This weakens coverage and could allow accidental removal/renaming of the new provider without failing tests.
assert.Contains(t, providers, "env")
assert.Contains(t, providers, "file")
assert.Contains(t, providers, "vault")
assert.Contains(t, providers, "kubernetes")
}
internal/cmn/secrets/resolver_test.go:323
Registry.Providers()test no longer verifies the new built-in GCP Secret Manager provider is present (and no longer checks the expected built-in count). Adding an explicit assertion here would better protect against regressions in provider registration.
assert.Contains(t, providers, "env")
assert.Contains(t, providers, "file")
assert.Contains(t, providers, "vault")
assert.Contains(t, providers, "kubernetes")
internal/cmn/secrets/resolver_test.go:329
- After registering the custom provider, the test should also assert the updated provider count to ensure
Registeractually adds a new provider entry rather than replacing an existing one.
mock := &mockResolver{mockName: "custom"}
registry.Register("custom", mock)
providers = registry.Providers()
assert.Contains(t, providers, "custom")
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed the CodeRabbit nitpick from the review summary. Registry.Close now collects only non-nil close errors before errors.Join. The secrets and runtime-agent race tests pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Testing
Part of #798
Summary by cubic
Adds GCP Secret Manager as a secret provider using Application Default Credentials. Supports short IDs and full resource names (global or regional), versions, regional endpoints, CRC32C checks, and JSON field extraction that preserves values; aligns with Linear #798.
New Features
gcp-secret-managerprovider withversion,project_id,location, andfieldoptions; accepts short IDs or full resource names.locationis set and verifies payload CRC32C.secrets.gcp.project_id,secrets.gcp.location(env:DAGU_SECRETS_GCP_PROJECT_ID,DAGU_SECRETS_GCP_LOCATION).Bug Fixes
fieldfrom JSON secrets (keeps large numbers intact and returns compact JSON for objects).Written for commit f980d71. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests