Add openchoreo secret manager implementation#1327
Conversation
📝 WalkthroughWalkthroughThe service migrates secret management from OpenBao to OpenChoreo, adding OpenChoreo secret CRUD APIs, provider ownership handling, target-plane configuration, updated wiring, mock support, agent-secret cleanup, and promotion-time secret cloning. ChangesOpenChoreo secret contracts and API
OpenChoreo provider and integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentManager
participant SecretManagementClient
participant OpenChoreoProvider
participant OpenChoreoClient
AgentManager->>SecretManagementClient: CreateSecret or PatchSecret
SecretManagementClient->>OpenChoreoProvider: PushSecret or PatchSecret
OpenChoreoProvider->>OpenChoreoClient: Create, read, or update secret
OpenChoreoClient-->>OpenChoreoProvider: Secret result
OpenChoreoProvider-->>SecretManagementClient: Secret result
SecretManagementClient-->>AgentManager: Secret result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (3)
agent-manager-service/config/config.go (1)
140-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating
Provider/TargetPlaneKind/TargetPlaneNametogether at startup.These fields are co-dependent (target-plane fields are meaningless unless
Provider == "openchoreo"). As per coding guidelines, "Validate configuration at startup, including co-dependent values together, rather than on first use." Currently the only visible check (ValidateConfigin the OpenChoreo provider) runs lazily when the client is constructed, not as an explicit startup validation step alongside other config validation.🤖 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 `@agent-manager-service/config/config.go` around lines 140 - 148, Add startup validation for the co-dependent SecretManagerConfig fields Provider, TargetPlaneKind, and TargetPlaneName alongside the existing configuration validation flow, rather than relying only on the OpenChoreo provider’s lazy ValidateConfig check. Enforce that target-plane fields are required and meaningful when Provider is "openchoreo", and reject inconsistent combinations before client construction.Source: Coding guidelines
agent-manager-service/clients/secretmanagersvc/client.go (1)
316-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd correlation-context logging for secret mutations.
CreateSecret,PatchSecret, andDeleteSecretperform no logging at all. As per coding guidelines, "Log with correlation context including organization, resource ID, and request ID; use Debug for hot paths, Info for rare events, and Error for destructive operations." These are exactly the categories called out (rare-event creates/patches, destructive delete), but there's no visibility into org/resource identifiers when these operations succeed or fail — this will hamper incident triage for secret-related issues.🤖 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 `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 316 - 377, Add correlation-context logs to CreateSecret, PatchSecret, and DeleteSecret, including organization, resource ID, and request ID from the available context/location data. Log successful create and patch operations at Info (or the established rare-event level), destructive delete operations at Error per the project guideline, and include the operation error details in failure logs while preserving the existing returned errors and mutation behavior.Source: Coding guidelines
agent-manager-service/clients/secretmanagersvc/providers/openchoreo/provider.go (1)
60-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefensively validate the provider name.
While the configuration is correctly validated for required OpenChoreo properties, it is a good defensive practice to also verify that
config.Providermatches the expectedProviderName("openchoreo").♻️ Proposed refactor
func (p *Provider) ValidateConfig(config *secretmanagersvc.StoreConfig) error { if config == nil { return errors.New("config is required") } + if config.Provider != "" && config.Provider != ProviderName { + return fmt.Errorf("invalid provider %q, expected %q", config.Provider, ProviderName) + } if config.OpenChoreo == nil { return errors.New("openchoreo config is required")🤖 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 `@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/provider.go` around lines 60 - 69, Update Provider.ValidateConfig to validate config.Provider against the expected ProviderName value ("openchoreo") after the nil config check and return a clear error when it does not match, while preserving the existing OpenChoreo configuration validations.
🤖 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 `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 365-377: Update secretManagementClient.DeleteSecret to accept and
pass the supplied secretRefName to the lowLevelClient.DeleteSecret call instead
of discarding it and always deriving the target from location. Preserve the
existing metadata construction and error wrapping while ensuring persisted or
renamed secret references are deleted correctly.
In `@console/apps/web-ui/public/config.js`:
- Around line 21-69: Restore the environment-variable placeholders throughout
the runtime configuration instead of embedding locally evaluated values,
especially in the auth settings and fields such as disableAuth, rbacEnabled,
organizationHandle, scopes, tokenValidation, guardrailCapabilities, and
featureFlags. Use the corresponding injected variables (for example,
DISABLE_AUTH and RBAC_ENABLED) with the existing fallback and conversion
behavior preserved so envsubst can populate them at startup; leave ampVersion
unchanged.
---
Nitpick comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 316-377: Add correlation-context logs to CreateSecret,
PatchSecret, and DeleteSecret, including organization, resource ID, and request
ID from the available context/location data. Log successful create and patch
operations at Info (or the established rare-event level), destructive delete
operations at Error per the project guideline, and include the operation error
details in failure logs while preserving the existing returned errors and
mutation behavior.
In
`@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/provider.go`:
- Around line 60-69: Update Provider.ValidateConfig to validate config.Provider
against the expected ProviderName value ("openchoreo") after the nil config
check and return a clear error when it does not match, while preserving the
existing OpenChoreo configuration validations.
In `@agent-manager-service/config/config.go`:
- Around line 140-148: Add startup validation for the co-dependent
SecretManagerConfig fields Provider, TargetPlaneKind, and TargetPlaneName
alongside the existing configuration validation flow, rather than relying only
on the OpenChoreo provider’s lazy ValidateConfig check. Enforce that
target-plane fields are required and meaningful when Provider is "openchoreo",
and reject inconsistent combinations before client construction.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92c09b70-43e9-4657-a6c4-b3ffac45ca67
⛔ Files ignored due to path filters (2)
agent-manager-service/clients/openchoreosvc/gen/client.gen.gois excluded by!**/gen/**agent-manager-service/clients/openchoreosvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (27)
agent-manager-service/.env.exampleagent-manager-service/Makefileagent-manager-service/app/app.goagent-manager-service/clients/clientmocks/openchoreo_client_fake.goagent-manager-service/clients/clientmocks/secret_mgmt_client_fake.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/secrets.goagent-manager-service/clients/openchoreosvc/client/types.goagent-manager-service/clients/secretmanagersvc/client.goagent-manager-service/clients/secretmanagersvc/provider.goagent-manager-service/clients/secretmanagersvc/providers/openbao/client.goagent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.goagent-manager-service/clients/secretmanagersvc/providers/openchoreo/provider.goagent-manager-service/clients/secretmanagersvc/types.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/main.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/monitor_executor.goagent-manager-service/tests/apitestutils/mock_clients.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsdeployments/single-cluster/values-cp.yaml
💤 Files with no reviewable changes (2)
- agent-manager-service/tests/apitestutils/mock_clients.go
- agent-manager-service/clients/secretmanagersvc/providers/openbao/client.go
| // DeleteSecret deletes a secret and its provider-managed SecretReference. | ||
| // secretRefName is retained for interface compatibility; the provider derives | ||
| // the secret name from location. | ||
| func (c *secretManagementClient) DeleteSecret(ctx context.Context, location SecretLocation, _ string) error { | ||
| metadata := &SecretMetadata{ | ||
| ManagedBy: c.managedBy, | ||
| } | ||
| if err := c.lowLevelClient.DeleteSecret(ctx, location, metadata); err != nil { | ||
| return fmt.Errorf("failed to delete secret: %w", err) | ||
| } | ||
|
|
||
| // If ocClient is configured, also delete the SecretReference | ||
| if c.ocClient != nil { | ||
| if err := c.ocClient.DeleteSecretReference(ctx, location.OrgName, secretRefName); err != nil { | ||
| // Ignore not found errors - the SecretReference may not exist | ||
| if !errors.Is(err, utils.ErrNotFound) { | ||
| return fmt.Errorf("failed to delete SecretReference: %w", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect DeleteSecret call sites and check whether callers still pass/derive a distinct secretRefName
rg -n -B3 -A5 --type=go '\.DeleteSecret\(' agent-manager-service/servicesRepository: wso2/agent-manager
Length of output: 14043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate SecretLocation definition and SecretRefName derivation, then inspect related create/delete flows.
rg -n -B3 -A6 --type=go 'type SecretLocation|func \(.*SecretRefName\(|CreateSecret\(|DeleteSecret\(' agent-manager-service/clients/secretmanagersvc agent-manager-service/services
# Also inspect the specific service methods around the call sites found earlier.
for f in \
agent-manager-service/services/agent_manager.go \
agent-manager-service/services/llm_proxy_provisioner.go \
agent-manager-service/services/agent_configuration_service.go
do
echo "===== $f ====="
rg -n -B4 -A10 'DeleteSecret\(|SecretRefName\(|CreateSecret\(' "$f"
doneRepository: wso2/agent-manager
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how SecretRefName is derived and whether any legacy/alternate secret-ref naming is persisted or referenced.
sed -n '120,170p' agent-manager-service/clients/secretmanagersvc/client.go
printf '\n===== secretRefName references =====\n'
rg -n --type=go 'secretRef(Name|ForDelete)|SecretReference|legacy|migration|pre-migration|OpenChoreo' agent-manager-service/services agent-manager-service/clients/secretmanagersvc | head -n 200
printf '\n===== delete callers that pass empty or derived names =====\n'
rg -n --type=go 'DeleteSecret\(ctx, [^,]+, *(""|location\.SecretRefName\(\)|secretRefName|secretRefForDelete|res\.secretRefName)\)' agent-manager-service/servicesRepository: wso2/agent-manager
Length of output: 31791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific provider delete implementation and any helper used for secret-ref lookup.
sed -n '216,245p' agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go
printf '\n===== create/delete symmetry in secret management client =====\n'
sed -n '313,377p' agent-manager-service/clients/secretmanagersvc/client.goRepository: wso2/agent-manager
Length of output: 3614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any persisted secret-ref identifiers and search for tests/fixtures that assert alternate secret names.
rg -n --type=go 'secretRefName|SecretReference|DeleteSecret\(|CreateSecret\(' agent-manager-service | head -n 250
printf '\n===== tests mentioning secret locations =====\n'
rg -n --type=go 'SecretLocation|SecretRefName|DeleteSecret\(' agent-manager-service | rg '_test\.go|test'Repository: wso2/agent-manager
Length of output: 37089
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether create returns the same name derived from SecretLocation and whether any caller uses a persisted alternate name.
sed -n '68,190p' agent-manager-service/clients/openchoreosvc/client/secrets.go
printf '\n===== create/delete call paths that persist the returned secret ref =====\n'
rg -n --type=go 'secretRefName, err := .*CreateSecret|secretRefName := .*CreateSecret|secretRefForDelete|res\.secretRefName|SecretReference' agent-manager-service/services/agent_configuration_service.go agent-manager-service/services/agent_manager.go agent-manager-service/services/llm_proxy_provisioner.goRepository: wso2/agent-manager
Length of output: 19801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any legacy/migration-specific secret reference naming or backfill logic.
rg -n --type=go 'migration|migrate|legacy|pre-migration|old secret|alternate secret|secret ref.*old|SecretRefName\(\).*legacy|stored.*secretRef' agent-manager-serviceRepository: wso2/agent-manager
Length of output: 15337
Honor the supplied secret ref on delete. DeleteSecret ignores secretRefName and always derives the target from location, so cleanup paths that rely on a persisted ref for renamed or pre-migration secrets will miss the real secret and leave it behind.
🤖 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 `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 365 -
377, Update secretManagementClient.DeleteSecret to accept and pass the supplied
secretRefName to the lowLevelClient.DeleteSecret call instead of discarding it
and always deriving the target from location. Preserve the existing metadata
construction and error wrapping while ensuring persisted or renamed secret
references are deleted correctly.
The runtime-generated console config was unintentionally reformatted in 0500f01; restore it so the branch carries no changes to this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go (3)
92-124: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInconsistent label handling between the "already exists" and "conflict-fallback" update paths.
When the initial
GetSecretfinds the secret already exists (line 92), the subsequentUpdateSecretusesuserLabels(existing.Labels, metadata.ManagedBy)(line 99), silently discarding anymetadata.Labelsthe caller passed in for this push. But when aCreateSecretrace loses (ErrConflict, line 114) and falls back toUpdateSecret, it usesuserLabels(metadata.Labels, metadata.ManagedBy)(line 117) instead. Both branches are logically "update a pre-existing secret," yet they apply labels from different sources depending purely on race timing between the initialGetand theCreateattempt — this makes label-update behavior forPushSecretnon-deterministic and effectively drops caller-supplied labels in the common (non-race) case.🐛 Proposed fix — apply caller-supplied labels consistently on update
if err == nil { // Secret exists — verify ownership before replacing it if !isManagedBy(existing.Labels, metadata.ManagedBy) { return "", secretmanagersvc.ErrNotManaged } if _, err := c.oc.UpdateSecret(ctx, location.OrgName, name, occlient.UpdateSecretRequest{ Data: data, - Labels: userLabels(existing.Labels, metadata.ManagedBy), + Labels: userLabels(metadata.Labels, metadata.ManagedBy), }); err != nil {🤖 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 `@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go` around lines 92 - 124, Update the existing-secret branch in PushSecret to pass metadata.Labels through userLabels, matching the conflict-fallback UpdateSecret path. Replace the existing.Labels argument in the UpdateSecret call while preserving metadata.ManagedBy and the rest of both update flows.
174-200: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDestructive/ownership-mismatch operations complete silently, with no logging.
DeleteSecret, along withPushSecret/PatchSecretabove, perform destructive writes (create/update/delete) and ownership-mismatch skips without any log line. Per the repo's logging guideline, destructive operations should be logged at Error level with correlation context (organization, resource ID, request ID) — this file has none. In particular, the ownership-mismatch no-op at line 188-190 is indistinguishable in the logs from "already deleted," which will make debugging cross-owner conflicts and unexpected no-ops difficult.Based on coding guidelines: "Log with correlation context including organization, resource ID, and request ID; use Debug for hot paths, Info for rare events, and Error for destructive operations."
🤖 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 `@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go` around lines 174 - 200, Update DeleteSecret, PushSecret, and PatchSecret to log destructive create/update/delete operations and ownership-mismatch skips using the repository’s logger at the appropriate levels, including organization, resource ID, and request ID correlation context. Ensure the ownership-mismatch path is distinguishable from the already-deleted path while preserving existing return behavior.Source: Coding guidelines
150-166: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid stale read/merge/write in
PatchSecret. The currentSecretInfo/UpdateSecretRequestpath doesn’t carry any resourceVersion/etag, so concurrent patches can silently drop keys. Add conditional update support with retry-on-conflict, or move the merge server-side.🤖 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 `@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go` around lines 150 - 166, Update PatchSecret’s client-side merge flow around existing and c.oc.UpdateSecret to prevent lost updates: use conditional update support with the current resourceVersion/etag and retry the read/merge/update sequence on conflicts, or invoke a server-side merge operation if the OpenChoreo client provides one. Preserve the existing deletion and label semantics while ensuring concurrent patches cannot silently overwrite keys.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go`:
- Around line 92-124: Update the existing-secret branch in PushSecret to pass
metadata.Labels through userLabels, matching the conflict-fallback UpdateSecret
path. Replace the existing.Labels argument in the UpdateSecret call while
preserving metadata.ManagedBy and the rest of both update flows.
- Around line 174-200: Update DeleteSecret, PushSecret, and PatchSecret to log
destructive create/update/delete operations and ownership-mismatch skips using
the repository’s logger at the appropriate levels, including organization,
resource ID, and request ID correlation context. Ensure the ownership-mismatch
path is distinguishable from the already-deleted path while preserving existing
return behavior.
- Around line 150-166: Update PatchSecret’s client-side merge flow around
existing and c.oc.UpdateSecret to prevent lost updates: use conditional update
support with the current resourceVersion/etag and retry the read/merge/update
sequence on conflicts, or invoke a server-side merge operation if the OpenChoreo
client provides one. Preserve the existing deletion and label semantics while
ensuring concurrent patches cannot silently overwrite keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcaae5e9-7d92-4632-b9c6-469c5be24297
📒 Files selected for processing (3)
agent-manager-service/clients/secretmanagersvc/client.goagent-manager-service/clients/secretmanagersvc/provider.goagent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- agent-manager-service/clients/secretmanagersvc/client.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/config/config_loader.go (1)
207-212: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the secret-manager settings during startup.
Reject unsupported providers, missing target-plane fields, and invalid/non-positive refresh intervals before the service starts; otherwise misconfiguration surfaces only when secret operations execute.
As per coding guidelines, “Validate configuration at startup, including co-dependent values together, rather than on first use.”
🤖 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 `@agent-manager-service/config/config_loader.go` around lines 207 - 212, Validate the SecretManagerConfig values immediately after construction in the config-loading startup flow: allow only supported providers, require non-empty TargetPlaneKind and TargetPlaneName, and parse RefreshInterval as a positive duration. Return a configuration error before startup continues when any check fails, keeping the existing defaults for valid settings.Source: Coding guidelines
agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go (1)
80-128: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake secret mutations conditional or atomic.
The provider performs ownership/version checks separately from the mutation, allowing concurrent operations to overwrite updates or act on a replacement resource.
agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go#L80-L128: after create conflict, refetch and verify ownership before updating.agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go#L130-L170: use server-side patching or resource-version retries to preserve concurrent changes.agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go#L175-L198: delete with a resource-version or ownership precondition.As per coding guidelines, avoid read-then-write and TOCTOU mutations.
🤖 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 `@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go` around lines 80 - 128, Make secret mutations conditional or atomic to prevent TOCTOU overwrites: in agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go lines 80-128, update PushSecret’s create-conflict path to refetch the secret and verify ownership before updating; in lines 130-170, change the corresponding mutation flow to use server-side patching or resource-version retries that preserve concurrent changes; in lines 175-198, make deletion conditional on the current resource version or ownership precondition. Use the existing Client methods and ownership helpers, and avoid adding an unconditional read-then-write path.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go`:
- Around line 80-128: Make secret mutations conditional or atomic to prevent
TOCTOU overwrites: in
agent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.go
lines 80-128, update PushSecret’s create-conflict path to refetch the secret and
verify ownership before updating; in lines 130-170, change the corresponding
mutation flow to use server-side patching or resource-version retries that
preserve concurrent changes; in lines 175-198, make deletion conditional on the
current resource version or ownership precondition. Use the existing Client
methods and ownership helpers, and avoid adding an unconditional read-then-write
path.
In `@agent-manager-service/config/config_loader.go`:
- Around line 207-212: Validate the SecretManagerConfig values immediately after
construction in the config-loading startup flow: allow only supported providers,
require non-empty TargetPlaneKind and TargetPlaneName, and parse RefreshInterval
as a positive duration. Return a configuration error before startup continues
when any check fails, keeping the existing defaults for valid settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d3741d7-9ad1-4998-a726-e819983ac472
📒 Files selected for processing (12)
agent-manager-service/app/app.goagent-manager-service/clients/openchoreosvc/client/types.goagent-manager-service/clients/secretmanagersvc/client.goagent-manager-service/clients/secretmanagersvc/provider.goagent-manager-service/clients/secretmanagersvc/providers/openchoreo/client.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/main.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (6)
- agent-manager-service/app/app.go
- agent-manager-service/main.go
- agent-manager-service/services/agent_configuration_service.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/clients/secretmanagersvc/provider.go
1 participant
Purpose
This PR integrates OpenChoreo’s secret management capability with the Agent Manager, replacing the previous OpenBao-based secret management implementation.
resolves #1285
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit