From a7c43205fca7489c2a8f331625757e2f44dd4de5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 14:54:41 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20feat:=20delete=20the=20Coder=20M?= =?UTF-8?q?CP=20server=20config=20of=20a=20deleted=20CoderMCPServer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deleted CoderMCPServer now deletes the config that it created and then releases its finalizer (plan 4.10): - With a config ID, one DELETE by ID. Coder's own 404 also releases. Any other answer keeps the finalizer and retries every 60 s. - With only a create marker, the slug read decides: only the nonce proves that the config is this object's. A missing config waits until a late create cannot commit any more (2 minutes), and needs a site owner. - A deleted control plane releases the finalizer without a Coder request. A control plane that is being deleted gets one DELETE attempt bounded at 10 s, then the finalizer goes. - The scope gate never blocks the delete. - The released object's credential hashes leave the reconciler's map. Refs #155 Change-Id: Iaa3d096523bb12408656377958c20563409982f3 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 15 +- .../codermcpserver_controller_test.go | 20 +- internal/controller/codermcpserver_delete.go | 138 ++++++++++ .../controller/codermcpserver_delete_test.go | 242 ++++++++++++++++++ internal/controller/export_test.go | 8 + 5 files changed, 407 insertions(+), 16 deletions(-) create mode 100644 internal/controller/codermcpserver_delete.go create mode 100644 internal/controller/codermcpserver_delete_test.go diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 6a5c4a184..11723133b 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -163,15 +163,16 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !controllerutil.ContainsFinalizer(server, coderv1alpha1.CoderMCPServerCleanupFinalizer) { return ctrl.Result{}, nil } - if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder. + release, step, err := r.deleteStep(ctx, server) + if err != nil { + return ctrl.Result{}, err + } + if release { + r.sentCredentials.Delete(server.UID) return ctrl.Result{}, r.patchFinalizer(ctx, server, false) } - // Keep the finalizer: this controller version cannot prove what Coder - // holds, so it neither deletes nor forgets the config. - applyMCPServerStep(server, mcpServerWait(0, "DeletePending", - "MCP server config %s can exist in Coder. This controller version keeps the finalizer and deletes nothing.", server.Spec.Slug)) - return ctrl.Result{}, r.writeStatus(ctx, server, before) + applyMCPServerStep(server, step) + return ctrl.Result{RequeueAfter: step.requeue}, r.writeStatus(ctx, server, before) } if err := r.patchFinalizer(ctx, server, true); err != nil { diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index e07666d68..142f9c3aa 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -543,15 +543,17 @@ func TestCoderMCPServerFinalizer(t *testing.T) { require.NoError(t, err) require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got))) - // A config with a Coder ID keeps its finalizer: this version deletes nothing. - kept := env.create(ctx, t, nil) - kept, _ = env.reconcile(ctx, t, kept) - kept.Status.ConfigID = "5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11" - require.NoError(t, k8sClient.Status().Update(ctx, kept)) - require.NoError(t, k8sClient.Delete(ctx, kept)) - kept, _ = env.reconcile(ctx, t, kept) - require.Equal(t, []string{coderv1alpha1.CoderMCPServerCleanupFinalizer}, kept.Finalizers) - requireMCPServerStep(t, kept, false, "DeletePending") + // A config with a Coder ID whose control plane is gone: nothing can + // delete it any more, so the finalizer goes without a Coder request + // (plan 4.10 item 5). + gone := env.create(ctx, t, nil) + gone, _ = env.reconcile(ctx, t, gone) + gone.Status.ConfigID, gone.Status.OrganizationID = "5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11", env.orgID.String() + require.NoError(t, k8sClient.Status().Update(ctx, gone)) + require.NoError(t, k8sClient.Delete(ctx, gone)) + _, err = env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gone)}) + require.NoError(t, err) + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(gone), gone))) require.Zero(t, env.fake.totalRequests()) } diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go new file mode 100644 index 000000000..072364af8 --- /dev/null +++ b/internal/controller/codermcpserver_delete.go @@ -0,0 +1,138 @@ +package controller + +import ( + "context" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/log" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +const ( + // mcpServerDeletePoll is the retry while a delete cannot reach Coder + // (plan 4.10 item 4). + mcpServerDeletePoll = 60 * time.Second + // mcpServerBoundedDeleteTimeout bounds the one delete attempt while the + // control plane is being deleted (plan 4.10 item 5). + mcpServerBoundedDeleteTimeout = 10 * time.Second + // mcpServerCreateSettle is how long after a create request a missing + // config does not yet prove absence: the request can still be in flight + // in Coder. + mcpServerCreateSettle = 2 * time.Minute +) + +// deleteStep handles an object that is being deleted (plan 4.10). It +// deletes in Coder only a config that this object proved it created, and it +// returns release when the finalizer can go. Otherwise the step says why the +// finalizer stays. The scope gate never blocks it. +func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coderv1alpha1.CoderMCPServer) (release bool, step *mcpServerStep, err error) { + if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil { + // Nothing of this object can exist in Coder. + return true, nil, nil + } + orgID, err := uuid.Parse(server.Status.OrganizationID) + if err != nil || orgID == uuid.Nil { + // The organization is pinned before the first MCP write. + return false, nil, fmt.Errorf("assertion failed: MCP server %s/%s can exist in Coder but has status.organizationID %q", server.Namespace, server.Name, server.Status.OrganizationID) + } + ref := types.NamespacedName{Namespace: server.Namespace, Name: server.Spec.ControlPlaneRef.Name} + controlPlane := &coderv1alpha1.CoderControlPlane{} + if err := r.Get(ctx, ref, controlPlane); apierrors.IsNotFound(err) { + return r.releaseWithoutDelete(ctx, server, "CoderControlPlane is gone"), nil, nil + } else if err != nil { + return false, nil, err + } + deleting := !controlPlane.DeletionTimestamp.IsZero() + sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, deleting) + switch { + case err != nil && deleting: + // One attempt only, also when the access setup fails. + return r.releaseWithoutDelete(ctx, server, "the Coder access of CoderControlPlane, which is being deleted, failed: "+err.Error()), nil, nil + case err != nil: + return false, nil, err + case wait != nil && deleting: + return r.releaseWithoutDelete(ctx, server, "CoderControlPlane is being deleted without operator access"), nil, nil + case wait != nil: + return false, mcpServerWait(mcpServerDeletePoll, wait.reason, "%s The controller deletes MCP server config %s once it can call Coder.", wait.message, server.Spec.Slug), nil + } + if deleting { + // One bounded attempt, then release, so the namespace deletion + // finishes. + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, mcpServerBoundedDeleteTimeout) + defer cancel() + } + step, err = deleteMCPServerInCoder(ctx, sdk, server, orgID) + switch { + case err != nil && deleting: + // The attempt ended with an impossible answer. Log it, and still + // release: the one attempt is over. + log.FromContext(ctx).Error(err, "The one delete attempt while CoderControlPlane is being deleted got an impossible answer") + return r.releaseWithoutDelete(ctx, server, "the one delete attempt while CoderControlPlane is being deleted failed: "+err.Error()), nil, nil + case err != nil: + return false, nil, err + case step != nil && deleting: + return r.releaseWithoutDelete(ctx, server, "the one delete attempt while CoderControlPlane is being deleted failed: "+step.message), nil, nil + } + return step == nil, step, nil +} + +// deleteMCPServerInCoder deletes the config of this object in Coder and +// returns nil when Coder holds none. With only a create marker, a read +// decides first: only the nonce proves that the config is this object's. +// Only Coder's own 404 proves absence (#337). +func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID) (*mcpServerStep, error) { + var id uuid.UUID + if server.Status.ConfigID != "" { + parsed, err := uuid.Parse(server.Status.ConfigID) + if err != nil || parsed == uuid.Nil || parsed.String() != server.Status.ConfigID { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a canonical UUID", server.Namespace, server.Name, server.Status.ConfigID) + } + id = parsed + } else { + found, step := findMCPServerSlug(ctx, sdk, server, orgID) + switch { + case step != nil: + step.requeue = max(step.requeue, mcpServerDeletePoll) + return step, nil + case found == nil: + // A missing config proves absence only for a site owner. A role + // change in Coder has no watch, so the wait uses the delete poll. + if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { + step.requeue = mcpServerDeletePoll + return step, nil + } + if wait := mcpServerCreateSettle - time.Since(server.Status.CreateAttemptTime.Time); wait > 0 { + // A create sent moments ago can still commit after this read. + return mcpServerWait(wait, "ConfirmingCreate", + "MCP server config %s is not in Coder, but its create request was sent at %s and can still commit. The controller reads again before it releases the finalizer.", + server.Spec.Slug, server.Status.CreateAttemptTime.UTC().Format(time.RFC3339)), nil + } + return nil, nil + case found.DisplayName != mcpServerNonce(server): + // Someone else's config holds the slug, so no create of this + // object committed. The controller never deletes it. + return nil, nil + } + id = found.ID + } + if err := sdk.DeleteMCPServerConfig(ctx, orgID, id); err != nil && !isCoderResourceNotFound(err) { + return mcpServerWait(mcpServerDeletePoll, "CoderUnavailable", "delete MCP server config %s (ID %s): %s", server.Spec.Slug, id, coderErrorSummary(err)), nil + } + return nil, nil +} + +// releaseWithoutDelete logs what can stay in Coder, because nothing can +// delete it any more, and returns true (ControlPlaneGone). +func (r *CoderMCPServerReconciler) releaseWithoutDelete(ctx context.Context, server *coderv1alpha1.CoderMCPServer, why string) bool { + log.FromContext(ctx).Info("Releasing the finalizer without a delete in Coder: the MCP server config can stay in the Coder database", + "reason", "ControlPlaneGone", "why", why, "slug", server.Spec.Slug, "configID", server.Status.ConfigID, + "createPending", server.Status.CreateAttemptTime != nil) + return true +} diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go new file mode 100644 index 000000000..f6a3e76b7 --- /dev/null +++ b/internal/controller/codermcpserver_delete_test.go @@ -0,0 +1,242 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// deleteMCPServer deletes the object and runs one reconcile. It returns the +// object, or nil when the finalizer went and the object is gone. +func (env mcpServerEnv) deleteMCPServer(ctx context.Context, t *testing.T, server *coderv1alpha1.CoderMCPServer) (*coderv1alpha1.CoderMCPServer, ctrl.Result) { + t.Helper() + require.NoError(t, client.IgnoreNotFound(k8sClient.Delete(ctx, server))) + return env.reconcileDeleting(ctx, t, server) +} + +func (env mcpServerEnv) reconcileDeleting(ctx context.Context, t *testing.T, server *coderv1alpha1.CoderMCPServer) (*coderv1alpha1.CoderMCPServer, ctrl.Result) { + t.Helper() + result, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) + require.NoError(t, err) + got := &coderv1alpha1.CoderMCPServer{} + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(server), got) + if apierrors.IsNotFound(err) { + return nil, result + } + require.NoError(t, err) + return got, result +} + +// Plan 4.10 items 3 and 6: deleting the object deletes the config that it +// created, also when the control plane no longer allows the organization, +// and then releases the finalizer. +func TestCoderMCPServerDeleteDeletesTheConfig(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + withdraw bool + }{{name: "opted in"}, {name: "opt-in withdrawn", withdraw: true}} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + if tc.withdraw { + env.setOptIn(ctx, t, nil) + } + mark := len(env.writeRoutes()) + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "the finalizer went") + require.Empty(t, env.fake.mcpServers()) + require.Equal(t, 1, env.fake.requestCount(routeDeleteMCPServer)) + require.Empty(t, env.writesSince(mark), "no write other than the DELETE") + require.NotContains(t, configIDs(env), id) + }) + } +} + +// Plan 4.10 item 3: a config that is already gone (Coder's own 404) releases +// the finalizer. Any other 404 keeps it and retries (#337), as do other +// errors (plan 4.10 item 4). +func TestCoderMCPServerDeleteAnswers(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + prepare func(context.Context, *testing.T, mcpServerEnv, *coderv1alpha1.CoderMCPServer) + fault *fakeFault + release bool + }{ + {name: "gone in Coder", release: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, got *coderv1alpha1.CoderMCPServer) { + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, env.requireCreated(t, got).view.ID)) + }}, + {name: "proxy 404", fault: ptrTo(notCoder404s["HTML page"])}, + {name: "route not found", fault: ptrTo(notCoder404s["route not found"])}, + {name: "Coder unavailable", fault: &fakeFault{Status: http.StatusBadGateway}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + if tc.prepare != nil { + tc.prepare(ctx, t, env, got) + } + if tc.fault != nil { + env.fake.failNext(routeDeleteMCPServer, *tc.fault) + } + kept, result := env.deleteMCPServer(ctx, t, got) + if tc.release { + require.Nil(t, kept) + return + } + require.NotNil(t, kept) + require.Equal(t, []string{coderv1alpha1.CoderMCPServerCleanupFinalizer}, kept.Finalizers) + requireMCPServerStep(t, kept, false, "CoderUnavailable") + require.Equal(t, 60*time.Second, result.RequeueAfter) + require.Len(t, env.fake.mcpServers(), 1, "the config stays") + + gone, _ := env.reconcileDeleting(ctx, t, kept) + require.Nil(t, gone, "the retry deletes it") + require.Empty(t, env.fake.mcpServers()) + }) + } +} + +// Plan 4.10 item 2: with only a create marker, the slug read decides. A +// config with this object's nonce is deleted. Someone else's config with the +// slug is never touched. With no config, the controller waits until a late +// create cannot commit any more, then releases. +func TestCoderMCPServerDeletePendingCreate(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + create func(context.Context, *testing.T, mcpServerEnv, *coderv1alpha1.CoderMCPServer) // nil: no config in Coder. + age time.Duration // Age of the create marker. + release bool + deletes int + }{ + {name: "nonce found", create: createWithNonce, release: true, deletes: 1}, + {name: "someone else's config", create: createOther, release: true}, + {name: "no config, recent create", age: time.Second}, + {name: "no config, old create", age: 3 * time.Minute, release: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + server, _ = env.reconcile(ctx, t, server) + for _, s := range env.fake.mcpServers() { + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, s.view.ID)) + } + if tc.create != nil { + tc.create(ctx, t, env, server) + } + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + server.Status.ConfigID = "" + server.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-max(tc.age, time.Second))} + require.NoError(t, k8sClient.Status().Update(ctx, server)) + before, deletes := len(env.fake.mcpServers()), env.fake.requestCount(routeDeleteMCPServer) + + kept, result := env.deleteMCPServer(ctx, t, server) + require.Equal(t, tc.deletes, env.fake.requestCount(routeDeleteMCPServer)-deletes) + if tc.release { + require.Nil(t, kept) + require.Len(t, env.fake.mcpServers(), before-tc.deletes) + return + } + require.NotNil(t, kept) + requireMCPServerStep(t, kept, false, "ConfirmingCreate") + require.Positive(t, result.RequeueAfter) + require.LessOrEqual(t, result.RequeueAfter, 2*time.Minute) + }) + } +} + +func createWithNonce(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { + t.Helper() + req := apiKeyMCPRequest(server.Spec.Slug) + req.DisplayName = "coder-k8s:" + string(server.UID) + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) +} + +func createOther(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { + t.Helper() + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, apiKeyMCPRequest(server.Spec.Slug)) + require.NoError(t, err) +} + +// Plan 4.10 item 5: while the control plane is being deleted (for example +// with its namespace), the controller makes one bounded DELETE attempt and +// then releases the finalizer, also when that attempt fails. +func TestCoderMCPServerDeleteWhileControlPlaneDeletes(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fault *fakeFault + }{ + {name: "attempt succeeds"}, + {name: "attempt fails", fault: &fakeFault{Status: http.StatusBadGateway}}, + // The attempt is bounded at 10 s, below the 30 s request timeout. + {name: "attempt hangs", fault: &fakeFault{Hang: true}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + holdControlPlane(ctx, t, env.namespace) + if tc.fault != nil { + env.fake.failNext(routeDeleteMCPServer, *tc.fault) + } + start := time.Now() + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Less(t, time.Since(start), 20*time.Second, "the attempt is bounded") + require.Nil(t, gone, "released after one attempt") + require.Equal(t, 1, env.fake.requestCount(routeDeleteMCPServer)) + if tc.fault != nil && tc.fault.Hang { + return + } + require.Equal(t, tc.fault == nil, len(env.fake.mcpServers()) == 0) + }) + } +} + +// From the #329 final assessment: a released object's entry in the +// reconciler's credential hash map goes, so the map does not grow without +// bound. +func TestCoderMCPServerDeleteForgetsSentCredentials(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + require.True(t, controller.HasSentCredentials(env.r, got.UID)) + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.False(t, controller.HasSentCredentials(env.r, got.UID)) +} + +func configIDs(env mcpServerEnv) []uuid.UUID { + var out []uuid.UUID + for _, s := range env.fake.mcpServers() { + out = append(out, s.view.ID) + } + return out +} diff --git a/internal/controller/export_test.go b/internal/controller/export_test.go index 87d91a6eb..c57ffdf75 100644 --- a/internal/controller/export_test.go +++ b/internal/controller/export_test.go @@ -6,6 +6,7 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" + "k8s.io/apimachinery/pkg/types" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" ) @@ -32,3 +33,10 @@ func SyncMCPServerACL( } return MCPServerACLResult{Reason: step.reason, Stalled: step.stalled, Message: step.message, Requeue: step.requeue}, err } + +// HasSentCredentials reports whether the reconciler keeps credential hashes +// for the object uid. +func HasSentCredentials(r *CoderMCPServerReconciler, uid types.UID) bool { + _, ok := r.sentCredentials.Load(uid) + return ok +}