Skip to content

Commit b14618f

Browse files
SarthakB11EItanya
andauthored
fix(cli): correct mcp secrets sync apply behavior and thread cobra ctx (#1814)
## summary three issues in `mcp secrets sync`: - `SyncSecretsMcp` and `applySecretToCluster` passed `context.TODO()` to every clientset Get/Create/Update call. ctrl-c during a slow apply (remote cluster, hung api-server connection) silently fell through to whatever the kubernetes client default did rather than honoring cancellation. fix: thread `cmd.Context()` from the cobra `RunE` through `SyncSecretsMcp` and `applySecretToCluster`. matches the existing kagent cli convention used by `InstallCmd`, `UninstallCmd`, `InvokeCmd`, `DashboardCmd`, and `DeployCmd`, which all take `ctx` as the first argument and are wired from `cmd.Context()` at the top level in `cmd/kagent/main.go`. - the Get error path was treated as "secret does not exist" without inspecting the error, so RBAC forbidden, network errors, and ctx-cancellation all surfaced as confusing `"failed to create secret"` messages instead of the real cause. fix: branch on `apierrors.IsNotFound(err)` so only true-NotFound triggers Create; any other Get error returns a wrapped message immediately. - the update path discarded the Get result, so the Secret being sent to Update had no `metadata.resourceVersion`. kubernetes Update requires `resourceVersion` and rejects optimistic-concurrency-violating writes, so updates against a populated cluster would fail with a validation error. fix: capture the live object from Get and copy its `resourceVersion` onto the secret built from `.env` before calling Update. the second and third items were flagged by copilot on the initial revision of this PR; addressed in the follow-up commit `cf8b5efd`. ## ai model disclosure used claude code (claude opus 4.7) to triage the `context.TODO()` call sites and draft the diff. self-reviewed against `go/core/cli/internal/cli/agent/deploy.go` to confirm the `apierrors` import alias and the ctx-as-first-arg pattern, and against `cmd/kagent/main.go` to confirm `cmd.Context()` is the established wiring point. verified locally via: ```bash go vet ./core/cli/... ./bin/golangci-lint run ./core/cli/internal/cli/mcp/... # 0 issues go build ./core/cli/... go test -race -count=1 ./core/cli/internal/cli/mcp/... # ok ``` --------- Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in> Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
1 parent 7fe81f3 commit b14618f

2 files changed

Lines changed: 21 additions & 13 deletions

File tree

go/core/cli/internal/cli/mcp/root.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,8 +393,8 @@ Examples:
393393
kagent mcp secrets sync production --dry-run
394394
`,
395395
Args: cobra.ExactArgs(1),
396-
RunE: func(_ *cobra.Command, args []string) error {
397-
return SyncSecretsMcp(cfg, args[0])
396+
RunE: func(cmd *cobra.Command, args []string) error {
397+
return SyncSecretsMcp(cmd.Context(), cfg, args[0])
398398
},
399399
}
400400

go/core/cli/internal/cli/mcp/secrets.go

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/kagent-dev/kagent/go/core/cli/internal/mcp/manifests"
1111
corev1 "k8s.io/api/core/v1"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1314
"k8s.io/client-go/kubernetes"
1415
"sigs.k8s.io/controller-runtime/pkg/client/config"
@@ -22,7 +23,7 @@ type SecretsCfg struct {
2223
ProjectDir string
2324
}
2425

25-
func SyncSecretsMcp(cfg *SecretsCfg, environment string) error {
26+
func SyncSecretsMcp(ctx context.Context, cfg *SecretsCfg, environment string) error {
2627
// Determine project root
2728
projectRoot := cfg.ProjectDir
2829
if projectRoot == "" {
@@ -109,10 +110,10 @@ func SyncSecretsMcp(cfg *SecretsCfg, environment string) error {
109110
}
110111

111112
// Apply to cluster
112-
return applySecretToCluster(secret)
113+
return applySecretToCluster(ctx, secret)
113114
}
114115

115-
func applySecretToCluster(secret *corev1.Secret) error {
116+
func applySecretToCluster(ctx context.Context, secret *corev1.Secret) error {
116117
// Get kubeconfig
117118
cfg, err := config.GetConfig()
118119
if err != nil {
@@ -125,18 +126,25 @@ func applySecretToCluster(secret *corev1.Secret) error {
125126
return fmt.Errorf("failed to create kubernetes clientset: %w", err)
126127
}
127128

128-
// Check if secret exists
129-
_, err = clientset.CoreV1().Secrets(secret.Namespace).Get(context.TODO(), secret.Name, metav1.GetOptions{})
130-
if err != nil {
131-
// Create if it doesn't exist
132-
_, err = clientset.CoreV1().Secrets(secret.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
129+
// Check if secret exists. Branch on IsNotFound so RBAC, network, or
130+
// context-cancellation failures from Get aren't silently treated as
131+
// "secret does not exist" and don't fall through to a Create that masks
132+
// the real error.
133+
existing, err := clientset.CoreV1().Secrets(secret.Namespace).Get(ctx, secret.Name, metav1.GetOptions{})
134+
switch {
135+
case apierrors.IsNotFound(err):
136+
_, err = clientset.CoreV1().Secrets(secret.Namespace).Create(ctx, secret, metav1.CreateOptions{})
133137
if err != nil {
134138
return fmt.Errorf("failed to create secret: %w", err)
135139
}
136140
fmt.Printf("✅ Secret '%s' created in namespace '%s'.\n", secret.Name, secret.Namespace)
137-
} else {
138-
// Update if it exists
139-
_, err = clientset.CoreV1().Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{})
141+
case err != nil:
142+
return fmt.Errorf("failed to get secret: %w", err)
143+
default:
144+
// Update requires the live resourceVersion from the existing object;
145+
// the Secret we built from .env has none.
146+
secret.ResourceVersion = existing.ResourceVersion
147+
_, err = clientset.CoreV1().Secrets(secret.Namespace).Update(ctx, secret, metav1.UpdateOptions{})
140148
if err != nil {
141149
return fmt.Errorf("failed to update secret: %w", err)
142150
}

0 commit comments

Comments
 (0)