Refactor secret management client methods and update configurations#680
Merged
Refactor secret management client methods and update configurations#680
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughSecret management APIs and implementations were redesigned: raw KV key strings replaced by structured Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Agent Configuration Service
participant SMClient as Secret Management Client
participant Provider as Secrets Provider (OpenBao)
participant OC as OpenChoreo Client
Note over Service,SMClient: Create / Upsert Secret
Service->>SMClient: CreateSecret(ctx, location, data)
SMClient->>Provider: PushSecret(ctx, location, value, metadata)
Provider-->>SMClient: kvPath / secretRef, err
alt ocClient configured
SMClient->>OC: GetSecretReference(secretRefName)
alt not exists
SMClient->>OC: CreateSecretReference(crd)
else
SMClient->>OC: UpdateSecretReference(crd)
end
end
SMClient-->>Service: secretRefName / err
sequenceDiagram
participant Service as Gateway / Consumer
participant SMClient as Secret Management Client
participant Provider as Secrets Provider (OpenBao)
Note over Service,SMClient: Read-with-value Flow
Service->>SMClient: GetSecretWithValue(ctx, kvPath)
SMClient->>SMClient: ParseKVPath(kvPath) -> SecretLocation
SMClient->>Provider: GetSecretWithValue(ctx, location)
Provider-->>SMClient: value ([]byte) / ErrNotSupported / err
SMClient-->>Service: value / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/services/agent_configuration_service.go (1)
1856-1873:⚠️ Potential issue | 🔴 CriticalKeep
UpstreamAuth.SecretRefas the KV path.
secretClient.CreateSecret()now returns the OpenChoreo SecretReference name whenocClientis configured. Persisting that value inUpstreamAuth.SecretRefturns a KV-path field into a Kubernetes resource name, so secured providers will no longer be able to resolve their upstream credential.Based on learnings, `UpstreamAuth.SecretRef` in `agent-manager-service/models/upstream.go` is a KV path reference managed exclusively by the secret management integration layer.Suggested fix
- kvPath, err := s.secretClient.CreateSecret(ctx, *providerSecretLoc, + kvPath, err := providerSecretLoc.KVPath() + if err != nil { + return nil, "", "", nil, fmt.Errorf("failed to derive provider secret KV path: %w", err) + } + _, err = s.secretClient.CreateSecret(ctx, *providerSecretLoc, map[string]string{secretmanagersvc.SecretKeyAPIKey: apiKey.APIKey}) if err != nil { // revoke created api key if err := s.llmProviderAPIKeyService.RevokeAPIKey(ctx, config.OrganizationName, provider.UUID.String(), proxyName); err != nil { s.logger.Error("Failed to revoke provider API key during creation",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 1856 - 1873, The code is storing the value returned by secretClient.CreateSecret (kvPath) into UpstreamAuth.SecretRef, but that return can be an OpenChoreo SecretReference rather than the KV path that UpstreamAuth.SecretRef expects; change the assignment so UpstreamAuth.SecretRef keeps the original KV path (the providerSecretLoc value) instead of kvPath. Update the block around secretClient.CreateSecret (references: CreateSecret, providerSecretLoc, kvPath, upstreamAuthConfig.SecretRef, UpstreamAuth) to assign the KV-path string (or pointer to it) used to create the secret to upstreamAuthConfig.SecretRef and leave kvPath for any oc-specific handling, ensuring the stored type matches models/upstream.go expectations.
🧹 Nitpick comments (2)
agent-manager-service/config/config_loader.go (1)
180-185: Consider adding validation forsecret-manager-apiprovider configuration.The new
BaseURLfield has an empty default, which is fine for other providers. However, whenProvideris set to"secret-manager-api", a missingBaseURLwill likely cause runtime failures when the client attempts to make HTTP requests.Consider adding validation in
loadEnvs()or a dedicated validation function:if cfg.SecretManager.Provider == "secret-manager-api" && cfg.SecretManager.BaseURL == "" { r.errors = append(r.errors, fmt.Errorf("SECRET_MANAGER_API_URL is required when SECRET_MANAGER_PROVIDER is 'secret-manager-api'")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/config/config_loader.go` around lines 180 - 185, Add validation to ensure SecretManagerConfig.BaseURL is provided when SecretManagerConfig.Provider == "secret-manager-api": in loadEnvs() (or a dedicated validateConfig function) check cfg.SecretManager.Provider (or r.config.SecretManager) and if it equals "secret-manager-api" and SecretManager.BaseURL (set via r.readOptionalString("SECRET_MANAGER_API_URL", "")) is empty, append an error to r.errors (or return/collect an error) indicating SECRET_MANAGER_API_URL is required; this prevents runtime failures when the secret-manager-api client (the SecretManagerConfig used by the client constructor) attempts HTTP requests.agent-manager-service/services/llm_provider_service.go (1)
249-256: Consider handlingKVPath()error for more accurate logging.The
KVPath()method returns(string, error), but the error is discarded with_. IfKVPath()fails (e.g., due to an emptyOrgName), the loggedkvPathcould be empty or misleading, making manual cleanup harder.🔧 Suggested improvement
if delErr := s.secretClient.DeleteSecret(ctx, *secretLoc, secretLoc.SecretRefName()); delErr != nil { - kvPath, _ := secretLoc.KVPath() + kvPath, kvErr := secretLoc.KVPath() + if kvErr != nil { + kvPath = fmt.Sprintf("(failed to compute: %v)", kvErr) + } slog.Error("LLMProviderService.Create: failed to delete orphaned secret after DB failure — manual cleanup required",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/llm_provider_service.go` around lines 249 - 256, The log call in LLMProviderService.Create discards the error from secretLoc.KVPath(), which can produce an empty/misleading kvPath; call secretLoc.KVPath() and capture its (path, err) before logging, and if err != nil include a clear fallback or the error text in the slog.Error fields (e.g., "kvPathError": err) so the log contains either the actual path or the KVPath error alongside orgName, handle, action and delErr; update the block that calls s.secretClient.DeleteSecret and the subsequent slog.Error to use these captured values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 144-149: SecretRefName() currently builds the secretref name using
only ConfigName and EnvironmentName when ConfigName is set, causing distinct
secrets (e.g., provider vs proxy) that share the same config/env to collide;
update SecretLocation.SecretRefName to include EntityName as part of the
generated Kubernetes-safe name whenever ConfigName is present (so the format
uses sanitizeForK8sName(l.ConfigName), sanitizeForK8sName(l.EnvironmentName) and
sanitizeForK8sName(l.EntityName) in the name) to ensure CreateSecret() produces
unique SecretReference names per KV path and avoids upserting one reference over
another.
- Around line 383-396: The branch that runs when c.ocClient != nil calls
c.lowLevelClient.GetSecret and then c.upsertSecretReference but still returns
kvPath, causing PatchSecret callers to receive the remote KV path instead of the
updated SecretReference name; change the return to return the SecretReference
name produced by upsertSecretReference (use the value returned by
upsertSecretReference instead of kvPath) so PatchSecret returns the Kubernetes
Secret name after a successful upsert; locate this logic around ocClient,
lowLevelClient.GetSecret, upsertSecretReference and PatchSecret and replace the
final returned value accordingly.
- Around line 171-195: ParseKVPath only accepts 2 or 4 segments but
SecretLocation.KVPath() can produce longer paths including AgentName,
ConfigName, and SecretKey, causing GetSecret/GetSecretWithValue to fail for
provider secrets created in buildLLMProxyConfig. Update ParseKVPath to accept
and parse the full KVPath shape: detect and handle lengths for 2 (org/entity), 4
(org/project/env/entity) and longer forms (e.g.,
org/project/env/entity/agent/config/key) by mapping parts[0]→OrgName,
parts[1]→ProjectName (if present), parts[2]→EnvironmentName (if present),
parts[3]→EntityName, and assign subsequent segments to AgentName, ConfigName,
and SecretKey respectively so ParseKVPath returns a complete SecretLocation
matching SecretLocation.KVPath().
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 397-405: The proxy secret is being stored under agent-scoped
EntityName (agentID) causing a mismatch with Delete() which reconstructs the
secret location using proxy.Handle; update the SecretLocation construction (the
proxySecretLoc variable where you instantiate secretmanagersvc.SecretLocation)
to set EntityName: proxy.Handle (instead of agentID) so create and delete paths
match, and make the same change for the other occurrences that construct proxy
secret locations (the other proxySecretLoc instances around the areas noted) to
use proxy.Handle.
In `@agent-manager-service/services/agent_manager.go`:
- Around line 1814-1845: The code currently always calls
s.secretMgmtClient.PatchSecret even when existingInfo is nil; change the flow so
that when GetSecret returns nil (and preservedSecretKeys is empty) you call
s.secretMgmtClient.CreateSecret(ctx, location, newSecretData) to create the
secret and set secretRefName, and only call s.secretMgmtClient.PatchSecret(ctx,
location, newSecretData, keysToDelete) when existingInfo != nil (i.e., when you
computed keysToDelete). Keep the existing error handling for ErrSecretNotFound
and for the preserved-key error path, and ensure you return/create errors from
CreateSecret the same way you handle PatchSecret errors.
---
Outside diff comments:
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 1856-1873: The code is storing the value returned by
secretClient.CreateSecret (kvPath) into UpstreamAuth.SecretRef, but that return
can be an OpenChoreo SecretReference rather than the KV path that
UpstreamAuth.SecretRef expects; change the assignment so UpstreamAuth.SecretRef
keeps the original KV path (the providerSecretLoc value) instead of kvPath.
Update the block around secretClient.CreateSecret (references: CreateSecret,
providerSecretLoc, kvPath, upstreamAuthConfig.SecretRef, UpstreamAuth) to assign
the KV-path string (or pointer to it) used to create the secret to
upstreamAuthConfig.SecretRef and leave kvPath for any oc-specific handling,
ensuring the stored type matches models/upstream.go expectations.
---
Nitpick comments:
In `@agent-manager-service/config/config_loader.go`:
- Around line 180-185: Add validation to ensure SecretManagerConfig.BaseURL is
provided when SecretManagerConfig.Provider == "secret-manager-api": in
loadEnvs() (or a dedicated validateConfig function) check
cfg.SecretManager.Provider (or r.config.SecretManager) and if it equals
"secret-manager-api" and SecretManager.BaseURL (set via
r.readOptionalString("SECRET_MANAGER_API_URL", "")) is empty, append an error to
r.errors (or return/collect an error) indicating SECRET_MANAGER_API_URL is
required; this prevents runtime failures when the secret-manager-api client (the
SecretManagerConfig used by the client constructor) attempts HTTP requests.
In `@agent-manager-service/services/llm_provider_service.go`:
- Around line 249-256: The log call in LLMProviderService.Create discards the
error from secretLoc.KVPath(), which can produce an empty/misleading kvPath;
call secretLoc.KVPath() and capture its (path, err) before logging, and if err
!= nil include a clear fallback or the error text in the slog.Error fields
(e.g., "kvPathError": err) so the log contains either the actual path or the
KVPath error alongside orgName, handle, action and delErr; update the block that
calls s.secretClient.DeleteSecret and the subsequent slog.Error to use these
captured values.
🪄 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: f7e264c4-acad-4471-996b-29edbda6be6d
⛔ Files ignored due to path filters (1)
agent-manager-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
agent-manager-service/clients/clientmocks/secret_mgmt_client_fake.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/types.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/gateway_internal_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/tests/apitestutils/mock_clients.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
Contributor
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 (2)
agent-manager-service/services/agent_configuration_service.go (2)
1844-1861:⚠️ Potential issue | 🔴 CriticalDon't persist the returned SecretReference name as
UpstreamAuth.SecretRef.Per
agent-manager-service/clients/secretmanagersvc/client.go:1-210,SecretManagementClient.CreateSecretnow returns the OpenChoreosecretRefName, whileGetSecretWithValuestill resolves secrets by KV path. Persisting theCreateSecretreturn value here will store something likeconfig-env-secretsinstead of the provider secret location, so secured upstream auth can no longer resolve the provider API key.Suggested fix
- kvPath, err := s.secretClient.CreateSecret(ctx, *providerSecretLoc, - map[string]string{secretmanagersvc.SecretKeyAPIKey: apiKey.APIKey}) - if err != nil { + if _, err := s.secretClient.CreateSecret(ctx, *providerSecretLoc, + map[string]string{secretmanagersvc.SecretKeyAPIKey: apiKey.APIKey}); err != nil { // revoke created api key if err := s.llmProviderAPIKeyService.RevokeAPIKey(ctx, config.OrganizationName, provider.UUID.String(), proxyName); err != nil { s.logger.Error("Failed to revoke provider API key during creation", "providerUUID", provider.UUID.String(), "providerKeyName", proxyName, "error", err, ) } return nil, "", "", nil, fmt.Errorf("failed to store provider API key in KV: %w", err) } + kvPath, err := providerSecretLoc.KVPath() + if err != nil { + return nil, "", "", nil, fmt.Errorf("failed to derive provider KV path: %w", err) + } upstreamAuthConfig.Type = utils.StrAsStrPointer(models.AuthTypeAPIKey) upstreamAuthConfig.Header = utils.StrAsStrPointer(providerApiKeyConfig.Key) upstreamAuthConfig.SecretRef = &kvPath // Store KV path instead of plaintext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 1844 - 1861, The SecretManagementClient.CreateSecret returns an OpenChoreo secretRefName while GetSecretWithValue expects a KV path, so don't persist the CreateSecret return (kvPath) into upstreamAuthConfig.SecretRef; instead persist the original KV path (providerSecretLoc) used to store the secret so downstream resolution works. Update the code around CreateSecret / upstreamAuthConfig.SecretRef to assign providerSecretLoc (or its dereferenced value) to upstreamAuthConfig.SecretRef rather than kvPath, keeping the revoke-on-error logic and niling upstreamAuthConfig.Value as-is.
725-737:⚠️ Potential issue | 🟠 MajorPatch the per-environment ReleaseBinding here, not the shared Component CR.
This block is inverted relative to
Create()andprocessNewEnv(): it updates the global Component on every env change and only updates the ReleaseBinding for the first env. Changing a non-first environment will leave that env’s ReleaseBinding stale and overwrite the shared Component defaults with the wrong URL/API key.Suggested fix
if !isExternalAgent { proxyURL := buildProxyURL(gateway.Vhost, proxy.Configuration.Context) envVarsToInject := buildLLMEnvVars(envConfigTemplates, proxyURL, proxySecretLoc.SecretRefName()) - if uvErr := s.ocClient.UpdateComponentEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envVarsToInject); uvErr != nil { - s.logger.Error("failed to update Component CR env vars in Scenario A — Component CR in inconsistent state", "env", envName, "err", uvErr) - } - if firstEnvName != "" && envName == firstEnvName { - if rbErr := s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName, config.AgentID, firstEnvName, envVarsToInject); rbErr != nil { + if rbErr := s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envName, envVarsToInject); rbErr != nil { + s.logger.Warn("failed to patch ReleaseBinding in Scenario A", "env", envName, "err", rbErr) + } + if firstEnvName != "" && envName == firstEnvName { + if uvErr := s.ocClient.UpdateComponentEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envVarsToInject); uvErr != nil { - s.logger.Warn("failed to patch ReleaseBinding in Scenario A", "env", envName, "err", rbErr) + s.logger.Error("failed to update Component CR env vars in Scenario A — Component CR in inconsistent state", "env", envName, "err", uvErr) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 725 - 737, The code currently updates the shared Component CR for every environment change and only patches the ReleaseBinding for the first env, which is inverted; instead, always patch the per-environment ReleaseBinding for the current envName and only update the shared Component CR when this is the first environment (firstEnvName). Concretely: keep building proxyURL and envVarsToInject via buildProxyURL and buildLLMEnvVars, call s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envName, envVarsToInject) for all non-external agents (remove the firstEnvName guard), and only call s.ocClient.UpdateComponentEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envVarsToInject) when envName == firstEnvName; adjust the log messages accordingly.
♻️ Duplicate comments (1)
agent-manager-service/services/agent_manager.go (1)
1814-1849:⚠️ Potential issue | 🟠 MajorCall
CreateSecretwhen the metadata probe finds nothing.The
ErrSecretNotFoundpath still falls through toPatchSecret, so the first deploy/update that introduces sensitive env vars will fail instead of creating the backing secret.Suggested fix
- // Use PatchSecret for server-side merge: adds/updates newSecretData, deletes keysToDelete - // Preserved keys are implicitly kept (not in keysToDelete, not overwritten) - secretRefName, err = s.secretMgmtClient.PatchSecret(ctx, location, newSecretData, keysToDelete) + if existingInfo == nil { + secretRefName, err = s.secretMgmtClient.CreateSecret(ctx, location, newSecretData) + } else { + // Use PatchSecret for server-side merge: adds/updates newSecretData, deletes keysToDelete + // Preserved keys are implicitly kept (not in keysToDelete, not overwritten) + secretRefName, err = s.secretMgmtClient.PatchSecret(ctx, location, newSecretData, keysToDelete) + } if err != nil { if errors.Is(err, secretmanagersvc.ErrNotManaged) { return "", fmt.Errorf("secret path %q is already owned by another system and cannot be overwritten; manual cleanup may be required: %w", kvPath, utils.ErrSecretPathConflict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_manager.go` around lines 1814 - 1849, GetSecret currently falls through to PatchSecret when no metadata is found, causing first-time deploys to fail; modify the code so that when existingInfo is nil (or readErr is ErrSecretNotFound) and preservedSecretKeys is empty you call s.secretMgmtClient.CreateSecret(ctx, location, newSecretData) instead of PatchSecret. Specifically, after the GetSecret block and the branch that returns an error if preservedSecretKeys > 0, add logic to call CreateSecret, handle ErrNotManaged the same way you handle it for PatchSecret (wrap with utils.ErrSecretPathConflict), set secretRefName from the CreateSecret result, and return or continue only on success; keep the existing PatchSecret path for cases where existingInfo != nil (i.e., update/merge) and preserve the keysToDelete logic and error wrapping for PatchSecret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 2041-2055: When rolling back, ensure DeleteSecret gets the correct
SecretReference name even if res.secretRefName is empty by deriving it from the
secret location; for each block that calls s.secretClient.DeleteSecret(ctx,
*res.providerSecretLoc, res.secretRefName) or s.secretClient.DeleteSecret(ctx,
*res.proxySecretLoc, res.secretRefName) check if res.secretRefName == "" and
compute the name from the location (use the location helper such as
res.providerSecretLoc.KVPath() or an equivalent method on the location to derive
the secret reference name) and pass that derived name to DeleteSecret so
providerSecretLoc deletes also remove their SecretReference during rollback.
---
Outside diff comments:
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 1844-1861: The SecretManagementClient.CreateSecret returns an
OpenChoreo secretRefName while GetSecretWithValue expects a KV path, so don't
persist the CreateSecret return (kvPath) into upstreamAuthConfig.SecretRef;
instead persist the original KV path (providerSecretLoc) used to store the
secret so downstream resolution works. Update the code around CreateSecret /
upstreamAuthConfig.SecretRef to assign providerSecretLoc (or its dereferenced
value) to upstreamAuthConfig.SecretRef rather than kvPath, keeping the
revoke-on-error logic and niling upstreamAuthConfig.Value as-is.
- Around line 725-737: The code currently updates the shared Component CR for
every environment change and only patches the ReleaseBinding for the first env,
which is inverted; instead, always patch the per-environment ReleaseBinding for
the current envName and only update the shared Component CR when this is the
first environment (firstEnvName). Concretely: keep building proxyURL and
envVarsToInject via buildProxyURL and buildLLMEnvVars, call
s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName,
config.AgentID, envName, envVarsToInject) for all non-external agents (remove
the firstEnvName guard), and only call s.ocClient.UpdateComponentEnvVars(ctx,
orgName, config.ProjectName, config.AgentID, envVarsToInject) when envName ==
firstEnvName; adjust the log messages accordingly.
---
Duplicate comments:
In `@agent-manager-service/services/agent_manager.go`:
- Around line 1814-1849: GetSecret currently falls through to PatchSecret when
no metadata is found, causing first-time deploys to fail; modify the code so
that when existingInfo is nil (or readErr is ErrSecretNotFound) and
preservedSecretKeys is empty you call s.secretMgmtClient.CreateSecret(ctx,
location, newSecretData) instead of PatchSecret. Specifically, after the
GetSecret block and the branch that returns an error if preservedSecretKeys > 0,
add logic to call CreateSecret, handle ErrNotManaged the same way you handle it
for PatchSecret (wrap with utils.ErrSecretPathConflict), set secretRefName from
the CreateSecret result, and return or continue only on success; keep the
existing PatchSecret path for cases where existingInfo != nil (i.e.,
update/merge) and preserve the keysToDelete logic and error wrapping for
PatchSecret.
🪄 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: 911cb56c-3080-411b-8b13-4748dfaeb48e
⛔ Files ignored due to path filters (1)
agent-manager-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
agent-manager-service/clients/clientmocks/secret_mgmt_client_fake.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/types.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.goagent-manager-service/services/gateway_internal_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/tests/apitestutils/mock_clients.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.go
✅ Files skipped from review due to trivial changes (2)
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/clients/secretmanagersvc/client.go
🚧 Files skipped from review as they are similar to previous changes (4)
- agent-manager-service/clients/secretmanagersvc/types.go
- agent-manager-service/services/gateway_internal_service.go
- agent-manager-service/config/config_loader.go
- agent-manager-service/tests/apitestutils/mock_clients.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/services/agent_configuration_service.go (1)
724-735:⚠️ Potential issue | 🟠 MajorUpdate the changed environment's ReleaseBinding here, not the global Component.
Create and Scenario C patch
ReleaseBindingper environment and only bootstrap theComponentfor the first environment. This block does the opposite, so changing a non-first environment's provider leaves that environment on the old URL/secretRef and can overwrite the Component defaults with the wrong environment.Suggested fix
if !isExternalAgent { proxyURL := buildProxyURL(gateway.Vhost, proxy.Configuration.Context) envVarsToInject := buildLLMEnvVars(envConfigTemplates, proxyURL, proxySecretLoc.SecretRefName()) - if uvErr := s.ocClient.UpdateComponentEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envVarsToInject); uvErr != nil { - s.logger.Error("failed to update Component CR env vars in Scenario A — Component CR in inconsistent state", "env", envName, "err", uvErr) - } - if firstEnvName != "" && envName == firstEnvName { - if rbErr := s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName, config.AgentID, firstEnvName, envVarsToInject); rbErr != nil { + if rbErr := s.ocClient.UpdateReleaseBindingEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envName, envVarsToInject); rbErr != nil { + s.logger.Warn("failed to patch ReleaseBinding in Scenario A", "env", envName, "err", rbErr) + } + if firstEnvName != "" && envName == firstEnvName { + if uvErr := s.ocClient.UpdateComponentEnvVars(ctx, orgName, config.ProjectName, config.AgentID, envVarsToInject); uvErr != nil { - s.logger.Warn("failed to patch ReleaseBinding in Scenario A", "env", envName, "err", rbErr) + s.logger.Error("failed to update Component CR env vars in Scenario A — Component CR in inconsistent state", "env", envName, "err", uvErr) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 724 - 735, The current logic updates the global Component CR for non-first environments and only patches the ReleaseBinding for the first env, which causes non-first environments to keep old proxy URL/secret and overwrites Component defaults incorrectly; change the order so that for non-external agents you always build proxyURL via buildProxyURL and envVarsToInject via buildLLMEnvVars, then call s.ocClient.UpdateReleaseBindingEnvVars for the current envName (creating the Scenario C per-environment patch), and only when envName == firstEnvName call s.ocClient.UpdateComponentEnvVars to bootstrap the Component defaults; keep error handling around s.ocClient.UpdateReleaseBindingEnvVars and s.ocClient.UpdateComponentEnvVars as before and reference isExternalAgent, envName and firstEnvName to gate which update runs.
♻️ Duplicate comments (4)
agent-manager-service/clients/secretmanagersvc/client.go (2)
145-149:⚠️ Potential issue | 🟠 MajorInclude
EntityNamein config-scoped SecretReference names.When
ConfigNameis set, this still aliases distinct secrets in the same config/environment.agent_configuration_service.gonow writes both the provider secret and the proxy secret with the sameConfigName/EnvironmentName, so the later upsert repoints one SecretReference at the other KV path.Suggested fix
func (l SecretLocation) SecretRefName() string { var name string if l.ConfigName != "" && l.EnvironmentName != "" { - name = fmt.Sprintf("%s-%s-secrets", sanitizeForK8sName(l.ConfigName), sanitizeForK8sName(l.EnvironmentName)) + name = fmt.Sprintf( + "%s-%s-%s-secrets", + sanitizeForK8sName(l.ConfigName), + sanitizeForK8sName(l.EnvironmentName), + sanitizeForK8sName(l.EntityName), + ) } else { name = fmt.Sprintf("%s-secrets", sanitizeForK8sName(l.EntityName)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 145 - 149, SecretRefName currently builds a config-scoped secret name using ConfigName and EnvironmentName which causes collisions; update SecretLocation.SecretRefName to include the EntityName when ConfigName is present so config-scoped provider vs proxy secrets are distinct. In the branch where l.ConfigName != "" && l.EnvironmentName != "", append sanitizeForK8sName(l.EntityName) into the fmt.Sprintf pattern (using the existing sanitizeForK8sName helper) so the generated string includes entity identity alongside config and environment names.
176-195:⚠️ Potential issue | 🟠 MajorParse the full agent-scoped KV path shape.
KVPath()can emitorg/project/env/agent/config/entity/api-key, but this parser still rejects anything except 2 or 4 segments.cleanupSecretReference()now feeds storedRemoteRef.Keyvalues back through this helper during delete, so provider/proxy secret cleanup fails withunrecognized KV path format.Suggested fix
func ParseKVPath(kvPath string) (SecretLocation, error) { parts := strings.Split(kvPath, "/") switch len(parts) { case 2: // org/entity (org-level secret) return SecretLocation{ OrgName: parts[0], EntityName: parts[1], }, nil case 4: // org/project/env/entity (agent secret) return SecretLocation{ OrgName: parts[0], ProjectName: parts[1], EnvironmentName: parts[2], EntityName: parts[3], }, nil + case 7: + return SecretLocation{ + OrgName: parts[0], + ProjectName: parts[1], + EnvironmentName: parts[2], + AgentName: parts[3], + ConfigName: parts[4], + EntityName: parts[5], + SecretKey: parts[6], + }, nil default: - return SecretLocation{}, fmt.Errorf("unrecognized KV path format: %s (expected 2 or 4 segments, got %d)", kvPath, len(parts)) + return SecretLocation{}, fmt.Errorf("unrecognized KV path format: %s (expected 2, 4, or 7 segments, got %d)", kvPath, len(parts)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 176 - 195, ParseKVPath currently only accepts exactly 2 or 4 segments, which breaks deletes when KV paths include extra agent-scoped segments (e.g. org/project/env/agent/config/entity/api-key). Change ParseKVPath to accept variable-length paths: always set OrgName = parts[0]; if len(parts) >= 4 set ProjectName = parts[1] and EnvironmentName = parts[2]; set EntityName = parts[1] for the 2-segment case or for longer paths set EntityName = parts[len(parts)-2] (so the entity is taken from the second-to-last segment, matching returned KV shapes); update the error branch only for len(parts) < 2 and reference ParseKVPath and cleanupSecretReference/RemoteRef.Key in the change.agent-manager-service/services/agent_configuration_service.go (1)
2040-2050:⚠️ Potential issue | 🟠 MajorDerive the SecretReference name from each location during rollback.
res.secretRefNameis only filled after the proxy secret is created. Any earlier failure still hasproviderSecretLocpopulated, so these deletes pass an empty name and leave the SecretReference behind. Using the shared field also breaks once provider and proxy secrets get distinct names.Suggested fix
// Delete provider API key from KV and SecretReference if res.providerSecretLoc != nil { - if err := s.secretClient.DeleteSecret(ctx, *res.providerSecretLoc, res.secretRefName); err != nil { + if err := s.secretClient.DeleteSecret(ctx, *res.providerSecretLoc, res.providerSecretLoc.SecretRefName()); err != nil { kvPath, _ := res.providerSecretLoc.KVPath() s.logger.Error("Failed to delete provider API key during rollback", "kvPath", kvPath, "error", err) } } // Delete proxy API key from KV and SecretReference if res.proxySecretLoc != nil { - if err := s.secretClient.DeleteSecret(ctx, *res.proxySecretLoc, res.secretRefName); err != nil { + if err := s.secretClient.DeleteSecret(ctx, *res.proxySecretLoc, res.proxySecretLoc.SecretRefName()); err != nil { kvPath, _ := res.proxySecretLoc.KVPath() s.logger.Error("Failed to delete proxy API key during rollback", "kvPath", kvPath, "error", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_configuration_service.go` around lines 2040 - 2050, During rollback use the secret reference name derived from each SecretLocation instead of the shared res.secretRefName; for the provider delete call use the name derived from res.providerSecretLoc and for the proxy delete use the name derived from res.proxySecretLoc (e.g. call a method or helper like providerSecretLoc.SecretRefName()/deriveSecretRefName(res.providerSecretLoc) and the equivalent for proxy) when invoking s.secretClient.DeleteSecret(ctx, *location, derivedName) so each DeleteSecret passes the correct per-location SecretReference name.agent-manager-service/services/agent_manager.go (1)
1831-1862:⚠️ Potential issue | 🟠 MajorCreate the secret when the metadata probe finds nothing.
This branch explicitly tolerates
ErrSecretNotFound, but still falls through toPatchSecret. On the first deploy/update that introduces sensitive env vars, providers that treat patch-on-missing as not found will fail instead of creating the backing secret.Suggested fix
- // Use PatchSecret for server-side merge: adds/updates newSecretData, deletes keysToDelete - // Preserved keys are implicitly kept (not in keysToDelete, not overwritten) - secretRefName, err = s.secretMgmtClient.PatchSecret(ctx, location, newSecretData, keysToDelete) + if existingInfo == nil { + secretRefName, err = s.secretMgmtClient.CreateSecret(ctx, location, newSecretData) + } else { + // Use PatchSecret for server-side merge: adds/updates newSecretData, deletes keysToDelete + // Preserved keys are implicitly kept (not in keysToDelete, not overwritten) + secretRefName, err = s.secretMgmtClient.PatchSecret(ctx, location, newSecretData, keysToDelete) + } if err != nil { if errors.Is(err, secretmanagersvc.ErrNotManaged) { return "", fmt.Errorf("secret path %q is already owned by another system and cannot be overwritten; manual cleanup may be required: %w", kvPath, utils.ErrSecretPathConflict) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_manager.go` around lines 1831 - 1862, The code currently tolerates secretmanagersvc.ErrSecretNotFound but still calls PatchSecret, which fails on providers that don't allow patching non-existent secrets; modify the logic so that when s.secretMgmtClient.GetSecret returns nil (existingInfo == nil) you create the secret instead of patching: call s.secretMgmtClient.CreateSecret(ctx, location, newSecretData) (or the appropriate create method on secretMgmtClient) to create and set secretRefName, keeping the existing error path that disallows preserving keys when preservedSecretKeys is non-empty, and only fall back to PatchSecret when existingInfo != nil; reference GetSecret, ErrSecretNotFound, CreateSecret (or the client’s create method), PatchSecret, preservedSecretKeys, keysToDelete, and secretRefName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 632-638: The rollback state currently overwrites credential
metadata with the new provider values and thus loses the replaced (old) API key
and secret location; update rollbackResource to include fields for the replaced
provider credentials (e.g., replacedProviderAPIKeyID, replacedProviderUUID,
replacedProviderSecretLoc) and populate those from existingMapping (or the
mapping's credential fields) before you assign rbRes with the new
providerAPIKeyID/providerUUID/providerSecretLoc; ensure rbRes captures both the
new and the old credential identifiers so cleanup can revoke/delete the replaced
secret later.
- Around line 1035-1039: The current Scenario D only removes the SecretReference
CR via secretmanagersvc.SecretLocation{ConfigName: configName, EnvironmentName:
envName}.SecretRefName() and s.ocClient.DeleteSecretReference, but does not
revoke provider/proxy API keys or delete the backing KV secrets; update the
cleanup to (1) call your secret-management/credential-revocation routine for the
given SecretLocation (e.g., invoke the service that revokes provider or proxy
API keys for configName/envName) and (2) delete the backing KV/store entries
that hold those credentials before/after calling
s.ocClient.DeleteSecretReference; ensure you use the same
secretmanagersvc.SecretLocation and secretRefName values and log errors via
s.logger.Warn similarly if either revocation or KV deletion fails.
---
Outside diff comments:
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 724-735: The current logic updates the global Component CR for
non-first environments and only patches the ReleaseBinding for the first env,
which causes non-first environments to keep old proxy URL/secret and overwrites
Component defaults incorrectly; change the order so that for non-external agents
you always build proxyURL via buildProxyURL and envVarsToInject via
buildLLMEnvVars, then call s.ocClient.UpdateReleaseBindingEnvVars for the
current envName (creating the Scenario C per-environment patch), and only when
envName == firstEnvName call s.ocClient.UpdateComponentEnvVars to bootstrap the
Component defaults; keep error handling around
s.ocClient.UpdateReleaseBindingEnvVars and s.ocClient.UpdateComponentEnvVars as
before and reference isExternalAgent, envName and firstEnvName to gate which
update runs.
---
Duplicate comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 145-149: SecretRefName currently builds a config-scoped secret
name using ConfigName and EnvironmentName which causes collisions; update
SecretLocation.SecretRefName to include the EntityName when ConfigName is
present so config-scoped provider vs proxy secrets are distinct. In the branch
where l.ConfigName != "" && l.EnvironmentName != "", append
sanitizeForK8sName(l.EntityName) into the fmt.Sprintf pattern (using the
existing sanitizeForK8sName helper) so the generated string includes entity
identity alongside config and environment names.
- Around line 176-195: ParseKVPath currently only accepts exactly 2 or 4
segments, which breaks deletes when KV paths include extra agent-scoped segments
(e.g. org/project/env/agent/config/entity/api-key). Change ParseKVPath to accept
variable-length paths: always set OrgName = parts[0]; if len(parts) >= 4 set
ProjectName = parts[1] and EnvironmentName = parts[2]; set EntityName = parts[1]
for the 2-segment case or for longer paths set EntityName = parts[len(parts)-2]
(so the entity is taken from the second-to-last segment, matching returned KV
shapes); update the error branch only for len(parts) < 2 and reference
ParseKVPath and cleanupSecretReference/RemoteRef.Key in the change.
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 2040-2050: During rollback use the secret reference name derived
from each SecretLocation instead of the shared res.secretRefName; for the
provider delete call use the name derived from res.providerSecretLoc and for the
proxy delete use the name derived from res.proxySecretLoc (e.g. call a method or
helper like
providerSecretLoc.SecretRefName()/deriveSecretRefName(res.providerSecretLoc) and
the equivalent for proxy) when invoking s.secretClient.DeleteSecret(ctx,
*location, derivedName) so each DeleteSecret passes the correct per-location
SecretReference name.
In `@agent-manager-service/services/agent_manager.go`:
- Around line 1831-1862: The code currently tolerates
secretmanagersvc.ErrSecretNotFound but still calls PatchSecret, which fails on
providers that don't allow patching non-existent secrets; modify the logic so
that when s.secretMgmtClient.GetSecret returns nil (existingInfo == nil) you
create the secret instead of patching: call s.secretMgmtClient.CreateSecret(ctx,
location, newSecretData) (or the appropriate create method on secretMgmtClient)
to create and set secretRefName, keeping the existing error path that disallows
preserving keys when preservedSecretKeys is non-empty, and only fall back to
PatchSecret when existingInfo != nil; reference GetSecret, ErrSecretNotFound,
CreateSecret (or the client’s create method), PatchSecret, preservedSecretKeys,
keysToDelete, and secretRefName.
🪄 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: d5e8dc1c-4eb4-444f-b737-dacfeca0cac2
📒 Files selected for processing (3)
agent-manager-service/clients/secretmanagersvc/client.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.go
Comment on lines
1035
to
1039
| // Delete SecretReference CR after consumer refs have been cleaned up (best-effort). | ||
| secretRefName := buildSecretRefName(configName, envName) | ||
| secretRefName := secretmanagersvc.SecretLocation{ConfigName: configName, EnvironmentName: envName}.SecretRefName() | ||
| if delErr := s.ocClient.DeleteSecretReference(ctx, orgName, secretRefName); delErr != nil { | ||
| s.logger.Warn("failed to delete SecretReference in Scenario D", "name", secretRefName, "err", delErr) | ||
| } |
Contributor
There was a problem hiding this comment.
Scenario D only deletes the CR, not the actual credentials.
This removes the SecretReference, but it never revokes the provider/proxy API keys or deletes the backing KV secrets for the removed environment. The later cleanup only tears down proxy objects, so environment removal leaks credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent-manager-service/services/agent_configuration_service.go` around lines
1035 - 1039, The current Scenario D only removes the SecretReference CR via
secretmanagersvc.SecretLocation{ConfigName: configName, EnvironmentName:
envName}.SecretRefName() and s.ocClient.DeleteSecretReference, but does not
revoke provider/proxy API keys or delete the backing KV secrets; update the
cleanup to (1) call your secret-management/credential-revocation routine for the
given SecretLocation (e.g., invoke the service that revokes provider or proxy
API keys for configName/envName) and (2) delete the backing KV/store entries
that hold those credentials before/after calling
s.ocClient.DeleteSecretReference; ensure you use the same
secretmanagersvc.SecretLocation and secretRefName values and log errors via
s.logger.Warn similarly if either revocation or KV deletion fails.
Contributor
Author
There was a problem hiding this comment.
pre-existing design issues, not regressions from the secret management refactor
Contributor
There was a problem hiding this comment.
@hanzjk, understood — the credential cleanup gap in Scenario D is a pre-existing design limitation and not introduced by this refactor. The diff here is purely a rename (swapping buildSecretRefName for SecretLocation{}.SecretRefName()), so the behavior is unchanged.
Would you like me to open a follow-up GitHub issue to track the Scenario D credential leak (missing KV secret deletion and API key revocation on environment removal) so it can be addressed in a dedicated PR?
🧠 Learnings used
Learnt from: hanzjk
Repo: wso2/ai-agent-management-platform PR: 70
File: agent-manager-service/services/agent_manager.go:978-980
Timestamp: 2025-12-18T16:41:56.601Z
Learning: In the agent-manager-service, enforce that external agents cannot have a SubType; only internal/managed agents may have SubType (e.g., chat-api, custom-api). When converting external agents to AgentResponse, the Type payload should include only the Type field and should not expose SubType. Implement this in the conversion/serialization path (likely in services/agent_manager.go) and update related DTO mappings or serializers. Add unit tests to verify internal vs external agent handling and ensure SubType is omitted for external agents.
Learnt from: hanzjk
Repo: wso2/ai-agent-management-platform PR: 244
File: agent-manager-service/utils/agent_types.go:31-31
Timestamp: 2026-01-28T04:44:15.667Z
Learning: Do not persist AgentType constant values (e.g., AgentTypeAPI) to the database in agent-manager-service. Changes to these constants should not trigger database migrations. When reviewing related code, verify that agent type values are only defined in code and not stored, and avoid adding schema migrations or altered migrations for changes to these constants. If new constants affect persistence in any way, flag for clarification and add a note to the migration policy.
Learnt from: nadheesh
Repo: wso2/agent-manager PR: 397
File: agent-manager-service/spec/api_default.go:3697-3704
Timestamp: 2026-02-19T11:51:13.017Z
Learning: Enforce camelCase for query parameter names across the agent-manager-service API surface. When adding new endpoints or updating OpenAPI specs, use camelCase (e.g., startTime, endTime) to align with existing endpoints like /traces and /traces/export. Apply this rule to all Go API definitions under agent-manager-service, not just a single file.
Learnt from: nadheesh
Repo: wso2/agent-manager PR: 493
File: agent-manager-service/controllers/monitor_scores_controller.go:334-337
Timestamp: 2026-03-05T05:50:42.812Z
Learning: Enforce camelCase for all query parameter names across the agent-manager-service API. Do not accept snake_case variants (e.g., start_time, end_time). This convention applies to all endpoints, including new ones like GetAgentTraceScores, and should be consistently implemented across all routes and controllers.
Learnt from: hanzjk
Repo: wso2/agent-manager PR: 501
File: agent-manager-service/clients/openchoreosvc/client/secret_references.go:35-43
Timestamp: 2026-03-06T10:39:53.140Z
Learning: In the wso2/agent-manager repo (agent-manager-service), Go modules show version 1.25.0 (go.mod). Do not flag range loop variable pointer aliasing (e.g., &key in a for i, key := range ...) as a bug — Go 1.22+ gives each iteration its own loop variable, making this a non-issue. Apply this guidance to all Go files in the repository (pattern **/*.go). If a static analyzer flags it anyway, document that it’s version-enabled and suppress the warning for Go >= 1.22.
Learnt from: hanzjk
Repo: wso2/agent-manager PR: 501
File: agent-manager-service/clients/openchoreosvc/client/deployments.go:49-55
Timestamp: 2026-03-06T13:28:35.328Z
Learning: In the wso2/agent-manager repository, there is an architectural invariant of exactly one workload per component for the agent-manager-service. During code reviews of files under agent-manager-service/clients/openchoreosvc/client (e.g., deployments.go and related files) where the code filters workloads by component name and then uses Items[0], do not flag it as an ambiguous-match issue. The 1:1 workload-to-component relationship means this usage is intentional and correct; treat such patterns as valid rather than ambiguous.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
agent-manager-service/clients/secretmanagersvc/client.go (1)
145-155:⚠️ Potential issue | 🟠 Major
SecretLocationstill has ambiguous serializations.
SecretRefName()only keepsEnvironmentNamewhenConfigNameis present, andParseKVPath()hard-codes 4/5/6-segment paths to one layout even thoughKVPath()can emit other combinations from the same optional fields. For example, the documentedorg/env/provider-handle/api-keyshape parses asproject/env/entity, andorg/project/env/entity/api-keyparses asagent/entity. That leaves some locations impossible to round-trip and others non-unique inside the org namespace. Please either validate a small set of supported field combinations up front or change the encoding so each scope is unambiguous.Also applies to: 182-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 145 - 155, SecretLocation serialization is ambiguous: SecretRefName() drops EnvironmentName unless ConfigName is present and ParseKVPath() assumes one fixed segment layout while KVPath() can emit multiple combinations, causing non-unique and non-roundtrip paths; fix by choosing one of two approaches and applying it consistently across SecretLocation.KVPath(), SecretLocation.ParseKVPath(), and SecretLocation.SecretRefName(): either validate and reject any unsupported field combinations up-front (e.g., allow only {org/project/env/entity} or {org/env/entity} and return an error from KVPath()/ParseKVPath()/SecretRefName() when inputs are invalid), or change the encoding to be unambiguous (e.g., include explicit field prefixes or fixed-position markers so all optional fields are represented uniquely), then update ParseKVPath() to parse the new unambiguous format and update SecretRefName() to derive names deterministically from the same normalized representation (do not drop EnvironmentName conditionally).
🧹 Nitpick comments (1)
agent-manager-service/clients/secretmanagersvc/client.go (1)
247-269: The public interface now makes callers juggle two identifiers.
CreateSecret()/PatchSecret()return SecretReference names, butGetSecret*()need KV paths andDeleteSecret()needs bothSecretLocationand the ref name. That means every caller has to cache extra state outside the API to do a simple write→read/delete flow. Returning a small result struct with bothsecretRefNameandkvPath, or moving the read/delete APIs toSecretLocation, would make this contract much harder to misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 247 - 269, The API forces callers to track two IDs; change CreateSecret and PatchSecret to return a small result struct that includes both the SecretReference name and the KV path (e.g., SecretResult with SecretRefName and KVPath) instead of just string, update their signatures in client.go (CreateSecret, PatchSecret) and adjust their doc comments to describe the new return type; then update any internal usages/call sites to consume the new struct so callers no longer need external state (no need to change GetSecret*/DeleteSecret signatures unless you prefer the alternate approach of accepting SecretLocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 339-354: The upsertSecretReference() path is a race: two callers
can both miss GetSecretReference() and one CreateSecretReference() will fail
with an "already exists" conflict; change the create branch to detect a create
conflict (e.g., the client/k8s "already exists" error or utils.ErrAlreadyExists)
and in that case call UpdateSecretReference(ctx, location.OrgName,
secretRefName, secretRefReq) instead of returning an error; otherwise return the
create error as before. Ensure you reference ocClient.CreateSecretReference and
ocClient.UpdateSecretReference and handle/propagate other errors unchanged.
- Around line 451-468: In DeleteSecret, do not trust the arbitrary secretRefName
when c.ocClient is present: derive the expected SecretReference name from the
provided SecretLocation (the same deterministic naming logic used elsewhere) and
either (1) verify the supplied secretRefName matches that derived name and
return an error if it doesn't, or (2) ignore the supplied name and call
c.ocClient.DeleteSecretReference with the derived name instead; perform this
check/derivation before calling c.lowLevelClient.DeleteSecret so you never
delete the KV secret for one resource while removing a different CR (refer to
DeleteSecret, c.lowLevelClient.DeleteSecret and c.ocClient.DeleteSecretReference
to locate the code).
---
Duplicate comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 145-155: SecretLocation serialization is ambiguous:
SecretRefName() drops EnvironmentName unless ConfigName is present and
ParseKVPath() assumes one fixed segment layout while KVPath() can emit multiple
combinations, causing non-unique and non-roundtrip paths; fix by choosing one of
two approaches and applying it consistently across SecretLocation.KVPath(),
SecretLocation.ParseKVPath(), and SecretLocation.SecretRefName(): either
validate and reject any unsupported field combinations up-front (e.g., allow
only {org/project/env/entity} or {org/env/entity} and return an error from
KVPath()/ParseKVPath()/SecretRefName() when inputs are invalid), or change the
encoding to be unambiguous (e.g., include explicit field prefixes or
fixed-position markers so all optional fields are represented uniquely), then
update ParseKVPath() to parse the new unambiguous format and update
SecretRefName() to derive names deterministically from the same normalized
representation (do not drop EnvironmentName conditionally).
---
Nitpick comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 247-269: The API forces callers to track two IDs; change
CreateSecret and PatchSecret to return a small result struct that includes both
the SecretReference name and the KV path (e.g., SecretResult with SecretRefName
and KVPath) instead of just string, update their signatures in client.go
(CreateSecret, PatchSecret) and adjust their doc comments to describe the new
return type; then update any internal usages/call sites to consume the new
struct so callers no longer need external state (no need to change
GetSecret*/DeleteSecret signatures unless you prefer the alternate approach of
accepting SecretLocation).
🪄 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: 03f9abf4-8bf6-4dc6-9e6d-24676f5395c9
📒 Files selected for processing (3)
agent-manager-service/clients/secretmanagersvc/client.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/agent_manager.go
🚧 Files skipped from review as they are similar to previous changes (2)
- agent-manager-service/services/agent_manager.go
- agent-manager-service/services/agent_configuration_service.go
Contributor
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 (1)
agent-manager-service/clients/secretmanagersvc/client.go (1)
73-75:⚠️ Potential issue | 🟡 MinorMinor: Error message references wrong field name.
The error message says
"SecretLocation.ComponentName is required"but the actual field being checked isEntityName.📝 Suggested fix
if strings.TrimSpace(l.EntityName) == "" { - return "", fmt.Errorf("SecretLocation.ComponentName is required") + return "", fmt.Errorf("SecretLocation.EntityName is required") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 73 - 75, The error message incorrectly references "SecretLocation.ComponentName" while the code checks l.EntityName; update the fmt.Errorf call in the client.go check that validates l.EntityName to return a message mentioning EntityName (e.g., "SecretLocation.EntityName is required" or "EntityName is required") so the error accurately reflects the validated field (locate the validation for l.EntityName in the client.go method where this check occurs).
♻️ Duplicate comments (1)
agent-manager-service/clients/secretmanagersvc/client.go (1)
458-478:⚠️ Potential issue | 🟠 MajorValidate
secretRefNamematches the expected value fromlocation.The function accepts an arbitrary
secretRefNamebut the SecretReference name should be deterministic fromlocation. If a stale or incorrectsecretRefNameis passed, this could delete the KV secret successfully but then remove the wrong SecretReference CR (or fail to remove the correct one), leaving orphaned resources or breaking another secret.Based on the relevant code snippets, callers currently pass
location.SecretRefName(), but the interface doesn't enforce this invariant.🛡️ Suggested defensive check
func (c *secretManagementClient) DeleteSecret(ctx context.Context, location SecretLocation, secretRefName string) error { + // Validate secretRefName matches location when ocClient is configured + if c.ocClient != nil { + expected := location.SecretRefName() + if secretRefName != expected { + return fmt.Errorf("secretRefName %q does not match expected %q from location", secretRefName, expected) + } + } + // Delete the KV secret - provider derives path from location metadata := &SecretMetadata{Alternatively, consider removing the
secretRefNameparameter entirely and always deriving it fromlocation.SecretRefName()within the method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/clients/secretmanagersvc/client.go` around lines 458 - 478, The DeleteSecret method currently accepts an external secretRefName which can diverge from the deterministic name derived from location and cause wrong CR deletions; change DeleteSecret to derive the SecretReference name from the provided location by calling location.SecretRefName() (or validate that the passed secretRefName equals location.SecretRefName()) before calling ocClient.DeleteSecretReference, and if they do not match return an error; update uses of DeleteSecret accordingly (or remove the secretRefName parameter and use location.SecretRefName() inside DeleteSecret) and keep existing behavior around calling lowLevelClient.DeleteSecret and ocClient.DeleteSecretReference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 73-75: The error message incorrectly references
"SecretLocation.ComponentName" while the code checks l.EntityName; update the
fmt.Errorf call in the client.go check that validates l.EntityName to return a
message mentioning EntityName (e.g., "SecretLocation.EntityName is required" or
"EntityName is required") so the error accurately reflects the validated field
(locate the validation for l.EntityName in the client.go method where this check
occurs).
---
Duplicate comments:
In `@agent-manager-service/clients/secretmanagersvc/client.go`:
- Around line 458-478: The DeleteSecret method currently accepts an external
secretRefName which can diverge from the deterministic name derived from
location and cause wrong CR deletions; change DeleteSecret to derive the
SecretReference name from the provided location by calling
location.SecretRefName() (or validate that the passed secretRefName equals
location.SecretRefName()) before calling ocClient.DeleteSecretReference, and if
they do not match return an error; update uses of DeleteSecret accordingly (or
remove the secretRefName parameter and use location.SecretRefName() inside
DeleteSecret) and keep existing behavior around calling
lowLevelClient.DeleteSecret and ocClient.DeleteSecretReference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2252e7c4-3807-4bb6-8f73-5e6c91d7375b
📒 Files selected for processing (1)
agent-manager-service/clients/secretmanagersvc/client.go
menakaj
approved these changes
Apr 9, 2026
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
2 participants
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.
Purpose
Resolves #685
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
New Features
Improvements
Tests