From 0c5993eb4b46809a2728ad575c8318efda510059 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 11:39:27 +0000 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=A4=96=20feat:=20disable=20the=20Code?= =?UTF-8?q?rMCPServer=20config=20when=20the=20control=20plane=20withdraws?= =?UTF-8?q?=20its=20opt-in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When spec.mcpServers is removed, no longer lists the organization, or no longer allows force_on for a force_on server, the controller now disables a config that the object created, with one PATCH that holds only enabled: false. It reads no credential. The disable is a kill switch, so a restored opt-in needs a spec edit to enable the config. The read and the PATCH are shared with the consent disable. Fixes #336 Refs #155 Change-Id: Iebea23e388d9c43540daebf8380f256c59f282cf Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 4 + .../codermcpserver_controller_test.go | 17 ++- internal/controller/codermcpserver_disable.go | 102 +++++++++++---- .../controller/codermcpserver_optin_test.go | 120 ++++++++++++++++++ 4 files changed, 216 insertions(+), 27 deletions(-) create mode 100644 internal/controller/codermcpserver_optin_test.go diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index e4f5bf24e..846771b41 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -215,6 +215,10 @@ func (r *CoderMCPServerReconciler) check( } server.Status.OrganizationID = org.ID.String() if step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, org.ID); step != nil || err != nil { + if step != nil && step.stalled && server.Status.ConfigID != "" { + // Fail closed when the opt-in goes (#336). + return r.disableMCPServerWithoutOptIn(ctx, sdk, server, org.ID, step) + } return step, err } values, step, err := readMCPServerCredentials(ctx, r.Client, server) diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 099d6bf52..e07666d68 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -354,10 +354,17 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { require.NoError(t, k8sClient.Update(ctx, server)) mcp := env.mcpRequests() got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, true, "ForceOnNotAllowed") - require.Equal(t, mcp, env.mcpRequests()) + // The config exists, so the withdrawn opt-in disables it (#336): one read + // and one PATCH. + require.Contains(t, requireMCPServerStep(t, got, true, "ForceOnNotAllowed"), "The controller disabled") + require.Equal(t, mcp+2, env.mcpRequests()) env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}) got, _ = env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + server.Spec.Description = "re-armed" + require.NoError(t, k8sClient.Update(ctx, server)) + got, _ = env.reconcile(ctx, t, server) requireMCPServerStep(t, got, false, "Applied") } @@ -586,8 +593,8 @@ func TestCoderMCPServerWatches(t *testing.T) { }, 30*time.Second, 100*time.Millisecond, reason) } requireReason("Applied") - // A withdrawn consent disables the config in Coder (plan G8), so only a - // spec edit applies it again. + // A withdrawn consent (plan G8) or opt-in (#336) disables the config in + // Coder, so only a spec edit applies it again. rearm := func(description string) { t.Helper() requireReason("DisabledExternally") @@ -609,7 +616,7 @@ func TestCoderMCPServerWatches(t *testing.T) { env.setOptIn(ctx, t, nil) requireReason("MCPServersNotAllowed") env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) - requireReason("Applied") + rearm("opt-in back") // Another control plane that uses a credential Secret for its own access // makes it unusable. Deleting that control plane makes it usable again. diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index e2fe00964..a29caad84 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -45,20 +45,9 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( unchecked := func(format string, args ...any) *mcpServerStep { return mcpServerWait(mcpServerRetry, failed.reason, "Coder can still send the credentials of MCP server config %s (%s). %s", id, fmt.Sprintf(format, args...), failed.message) } - found, err := sdk.MCPServerConfigByID(ctx, orgID, id) - switch { - case isCoderResourceNotFound(err): - // Only Coder's own 404 proves that the config is gone (#337). - return stalled("MCP server config %s does not exist in Coder.", id), nil - case err != nil: - return unchecked("get MCP server config: %s", coderErrorSummary(err)), nil - } - if found.ID != id || found.OrganizationID != orgID || found.Slug != server.Spec.Slug { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", - "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id), nil - } - if !found.Enabled { - return stalled("MCP server config %s is disabled in Coder.", id), nil + found, step := readMCPServerToDisable(ctx, sdk, server, orgID, id, stalled, unchecked) + if step != nil { + return step, nil } // user_oidc sends each chat user's login token to url (plan F14). Without // a readable credential the controller cannot set the auth type back, so @@ -76,22 +65,91 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( return stalled("Coder sends each credential of MCP server config %s only to origins that its recorded Secret allows.", id), nil } + if step := sendMCPServerDisable(ctx, sdk, server, orgID, id, unchecked); step != nil { + return step, nil + } + // The reason stays the credential reason, so it does not flap between + // this reconcile and the next one, which finds the config disabled. + return stalled("The controller disabled MCP server config %s in Coder: a credential Secret no longer proves consent for a credential that Coder uses. After the Secret is fixed, a spec edit enables the config.", id), nil +} + +// readMCPServerToDisable reads the config of a fail-closed path. It returns +// a step when there is nothing to disable or the read failed: a config that +// is gone (only Coder's own 404 proves it, #337) or already disabled, an +// answer for another config, or an error. +func readMCPServerToDisable( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID, id uuid.UUID, + stalled, unchecked func(format string, args ...any) *mcpServerStep, +) (codersdk.MCPServerConfig, *mcpServerStep) { + found, err := sdk.MCPServerConfigByID(ctx, orgID, id) + switch { + case isCoderResourceNotFound(err): + return found, stalled("MCP server config %s does not exist in Coder.", id) + case err != nil: + return found, unchecked("get MCP server config: %s", coderErrorSummary(err)) + } + if found.ID != id || found.OrganizationID != orgID || found.Slug != server.Spec.Slug { + return found, mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", + "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id) + } + if !found.Enabled { + return found, stalled("MCP server config %s is disabled in Coder.", id) + } + return found, nil +} + +// sendMCPServerDisable sends the one PATCH of a fail-closed path. It holds +// only enabled false: no credential and no destination field. It returns a +// wait step when the PATCH failed or Coder answered for another or an +// enabled config. On success it sets appliedGeneration: the disable counts +// as an external disable of this generation, also of one that is not +// applied yet, so the DisabledExternally kill switch holds until the next +// spec edit. +func sendMCPServerDisable( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID, id uuid.UUID, + unchecked func(format string, args ...any) *mcpServerStep, +) *mcpServerStep { disabled := false updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, codersdk.UpdateMCPServerConfigRequest{Enabled: &disabled}) if err != nil { - return unchecked("the disable failed: %s", coderErrorSummary(err)), nil + return unchecked("the disable failed: %s", coderErrorSummary(err)) } if updated.ID != id || updated.OrganizationID != orgID || updated.Slug != server.Spec.Slug || updated.Enabled { return mcpServerWait(mcpServerRetry, "CoderAnswerMismatch", - "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id), nil + "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id) } - // The disable counts as an external disable of this generation, also of - // one that is not applied yet: the kill switch then holds until the next - // spec edit. server.Status.AppliedGeneration = server.Generation - // The reason stays the credential reason, so it does not flap between - // this reconcile and the next one, which finds the config disabled. - return stalled("The controller disabled MCP server config %s in Coder: a credential Secret no longer proves consent for a credential that Coder uses. After the Secret is fixed, a spec edit enables the config.", id), nil + return nil +} + +// disableMCPServerWithoutOptIn handles a config that this object created +// when the control plane withdraws its opt-in (#336): spec.mcpServers is +// removed, it no longer lists the organization, or it no longer allows +// force_on for a force_on server. The operator may no longer manage MCP +// servers there, so the controller disables the config with the one PATCH +// of sendMCPServerDisable. It reads no credential. The disable is a kill +// switch, as in disableMCPServerWithoutConsent: a restored opt-in needs a +// spec edit to enable the config again. +func (r *CoderMCPServerReconciler) disableMCPServerWithoutOptIn( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, failed *mcpServerStep, +) (*mcpServerStep, error) { + id, err := uuid.Parse(server.Status.ConfigID) + if err != nil || id == uuid.Nil || failed == nil || !failed.stalled { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs a config ID and a stalled opt-in failure to be disabled", server.Namespace, server.Name) + } + stalled := func(format string, args ...any) *mcpServerStep { + return mcpServerStalled(mcpServerResync, failed.reason, "%s %s", fmt.Sprintf(format, args...), failed.message) + } + unchecked := func(format string, args ...any) *mcpServerStep { + return mcpServerWait(mcpServerRetry, failed.reason, "Coder can still use MCP server config %s (%s). %s", id, fmt.Sprintf(format, args...), failed.message) + } + if _, step := readMCPServerToDisable(ctx, sdk, server, orgID, id, stalled, unchecked); step != nil { + return step, nil + } + if step := sendMCPServerDisable(ctx, sdk, server, orgID, id, unchecked); step != nil { + return step, nil + } + return stalled("The controller disabled MCP server config %s in Coder: the control plane no longer allows it. After the opt-in is back, a spec edit enables the config.", id), nil } // errMCPServerSecretChanged reports that a Secret changed between two reads diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go new file mode 100644 index 000000000..569e27025 --- /dev/null +++ b/internal/controller/codermcpserver_optin_test.go @@ -0,0 +1,120 @@ +package controller_test + +import ( + "context" + "net/http" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// #336: when the control plane withdraws its opt-in, the controller disables +// a config that this object created with one PATCH that holds only enabled +// false, and it reads no credential Secret. No other write follows while the +// opt-in stays withdrawn. The disable is a kill switch: the restored opt-in +// gives DisabledExternally, and a spec edit enables the config again. +func TestCoderMCPServerDisablesWithoutOptIn(t *testing.T) { + t.Parallel() + forceOn := func(s *coderv1alpha1.CoderMCPServerSpec) { s.Availability = "force_on" } + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + optIn *coderv1alpha1.MCPServersSpec // The opt-in before the withdrawal. + withdraw *coderv1alpha1.MCPServersSpec + reason string + }{ + {name: "opt-in removed", reason: "MCPServersNotAllowed"}, + {name: "organization unlisted", withdraw: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other"}}, reason: "MCPServersNotAllowed"}, + { + name: "force_on no longer allowed", spec: forceOn, reason: "ForceOnNotAllowed", + optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}, + withdraw: &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + optIn := tc.optIn + if optIn == nil { + optIn = &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}} + } + env.setOptIn(ctx, t, optIn) + got, _ := env.applied(ctx, t, tc.spec) + before := env.stored(t, got) + + // Count the reads of the credential Secret from here on. + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + var secretReads atomic.Int32 + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok && key.Name == "github-mcp" { + secretReads.Add(1) + } + return c.Get(ctx, key, obj, opts...) + }, + }) + env.setOptIn(ctx, t, tc.withdraw) + + // A failed disable is retried, and the message says that Coder + // can still use the config. + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, result := env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, tc.reason), "Coder can still use MCP server config") + require.Equal(t, 30*time.Second, result.RequeueAfter) + + mark := len(env.writeRoutes()) + got, result = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, tc.reason), "The controller disabled MCP server config") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServer, writes[0].route) + require.JSONEq(t, `{"enabled":false}`, string(writes[0].body)) + before.view.Enabled = false + require.Equal(t, before, env.stored(t, got), "only enabled changed") + + mark = len(env.writeRoutes()) + for range 2 { + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, tc.reason), "is disabled in Coder") + } + require.Empty(t, env.writesSince(mark), "no write while the opt-in stays withdrawn") + require.Zero(t, secretReads.Load(), "no credential read") + + env.setOptIn(ctx, t, optIn) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "re-armed" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.stored(t, got).view.Enabled) + }) + } +} + +// An object without a config ID has nothing in Coder to disable, so a +// withdrawn opt-in still sends no MCP request (plan S17). +func TestCoderMCPServerOptInWithdrawnWithoutConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.setOptIn(ctx, t, nil) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.Zero(t, env.mcpRequests()) +} From de723169c0bc16d0f41ac47427e1e6970a4f7119 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 12:09:56 +0000 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=A4=96=20fix:=20trust=20only=20Coder'?= =?UTF-8?q?s=20own=20404=20on=20the=20CoderMCPServer=20organization=20look?= =?UTF-8?q?up=20and=20resync=20MCPServersNotAllowed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The organization lookup treated any 404 as a missing organization. A proxy page or "Route not found." for a listed name then skipped that name, so the scope check stalled with MCPServersNotAllowed, which had no timed requeue. The lookup now uses isCoderResourceNotFound, so another 404 waits with CoderUnavailable, and MCPServersNotAllowed resyncs every 10 minutes. The fake answers an unknown organization like Coder. Fixes #358 Refs #155 Change-Id: I90fa60c56e9f867d4c7c53bce35c1be49be366a8 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 12 ++++-- .../controller/codermcpserver_optin_test.go | 38 ++++++++++++++++++- internal/controller/fakecoder_test.go | 6 ++- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 846771b41..a554dad8e 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -263,11 +263,13 @@ func (r *CoderMCPServerReconciler) check( } // resolveMCPServerOrganization looks up a Coder organization by name. Coder -// compares names without case. +// compares names without case. Only Coder's own 404 proves that the +// organization does not exist: another 404 (a proxy page, "Route not +// found.") waits like any failed read (#358). func resolveMCPServerOrganization(ctx context.Context, sdk *codersdk.Client, name string) (codersdk.Organization, *mcpServerStep, error) { org, err := sdk.OrganizationByName(ctx, name) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): return org, mcpServerWait(mcpServerResync, "OrganizationNotFound", "Coder organization %s does not exist.", name), nil case err != nil: return org, mcpServerWait(mcpServerRetry, "CoderUnavailable", "get organization %s: %s", name, coderErrorSummary(err)), nil @@ -290,7 +292,7 @@ func mcpServerScopeGate( ) (*mcpServerStep, error) { optIn := controlPlane.Spec.MCPServers if optIn == nil { - return mcpServerStalled(0, "MCPServersNotAllowed", "CoderControlPlane %s does not set spec.mcpServers.", controlPlane.Name), nil + return mcpServerStalled(mcpServerResync, "MCPServersNotAllowed", "CoderControlPlane %s does not set spec.mcpServers.", controlPlane.Name), nil } allowed := false for _, name := range optIn.Organizations { @@ -314,7 +316,9 @@ func mcpServerScopeGate( } } if !allowed { - return mcpServerStalled(0, "MCPServersNotAllowed", + // A listed name can resolve later, when it is created or renamed in + // Coder, and no Kubernetes event follows (#358). + return mcpServerStalled(mcpServerResync, "MCPServersNotAllowed", "CoderControlPlane %s does not list Coder organization %s in spec.mcpServers.organizations.", controlPlane.Name, spec.Organization), nil } if spec.Availability == "force_on" && !optIn.AllowForceOn { diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index 569e27025..bfc46ddd4 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/internal/controller/codermcpserver_optin_test.go @@ -114,7 +114,43 @@ func TestCoderMCPServerOptInWithdrawnWithoutConfig(t *testing.T) { env := newMCPServerEnv(ctx, t, true) env.keySecret(ctx, t, testMCPKey) env.setOptIn(ctx, t, nil) - got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + got, result := env.reconcile(ctx, t, env.create(ctx, t, nil)) requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.Equal(t, 10*time.Minute, result.RequeueAfter, "a timed resync (#358)") require.Zero(t, env.mcpRequests()) } + +// #358: only Coder's own 404 proves that an organization does not exist. A +// proxy page or "Route not found." on the lookup of spec.organization, or of +// a listed name, waits with CoderUnavailable instead of OrganizationNotFound +// or MCPServersNotAllowed. An unlisted organization resyncs on a timer, +// because a listed name can resolve later without a Kubernetes event. +func TestCoderMCPServerOrganizationNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + for _, lookup := range []string{"own", "listed"} { + t.Run(name+"/"+lookup, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.addOrganization("alias") + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"alias"}}) + server := env.create(ctx, t, nil) + if lookup == "listed" { + // The object's own organization resolves first. + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { return answer }}) + } + env.fake.failNext(routeOrganization, fault) + got, result := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Zero(t, env.mcpRequests()) + + got, result = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + }) + } + } +} diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 91468374f..3c53d4441 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -115,7 +115,11 @@ func newFakeCoder(t *testing.T) *fakeCoder { return found(f.userByIdent(r.PathValue("user"))) }) f.route(mux, "GET /api/v2/organizations/{org}", routeOrganization, func(r *http.Request) (int, any) { - return found(f.orgByIdent(r.PathValue("org"))) + org, ok := f.orgByIdent(r.PathValue("org")) + if !ok { // coderd/httpmw/organizationparam.go:94 answers httpapi.ResourceNotFound. + return http.StatusNotFound, fakeResourceNotFound + } + return http.StatusOK, org }) f.route(mux, "GET /api/v2/organizations/{org}/members/{user}", routeOrgMember, f.getOrgMember) f.route(mux, "GET /api/v2/organizations/{org}/templates/{name}", routeTemplateByName, func(r *http.Request) (int, any) { From a5dd49ac2b34805e0b95d9561d4dd8917d529cdd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 12:25:06 +0000 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=A4=96=20fix:=20record=20the=20genera?= =?UTF-8?q?tion=20of=20a=20disabled=20CoderMCPServer=20config,=20limit=20t?= =?UTF-8?q?he=20opt-in=20disable=20to=20opt-in=20stalls,=20and=20document?= =?UTF-8?q?=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A read that finds the config already disabled now records the generation, so a lost disable answer and a restored opt-in do not apply a pending generation that enables the config. - Only MCPServersNotAllowed and ForceOnNotAllowed run the opt-in disable. Another stall of the scope check, such as a mismatched Coder answer, keeps the config. - The spec.mcpServers field docs describe the fail-closed disable. Refs #155 Change-Id: If9ea006bcd30e178dc579172f03ddce31b0b3923 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/codercontrolplane_types.go | 5 +- api/v1alpha1/codermcpserver_types.go | 10 ++- .../bases/coder.com_codercontrolplanes.yaml | 15 +++-- dist/install.yaml | 15 +++-- docs/reference/api/codercontrolplane.md | 6 +- .../controller/codermcpserver_controller.go | 5 +- internal/controller/codermcpserver_disable.go | 5 ++ .../controller/codermcpserver_optin_test.go | 64 +++++++++++++++++++ 8 files changed, 105 insertions(+), 20 deletions(-) diff --git a/api/v1alpha1/codercontrolplane_types.go b/api/v1alpha1/codercontrolplane_types.go index 3f20088ea..1c72e7a1a 100644 --- a/api/v1alpha1/codercontrolplane_types.go +++ b/api/v1alpha1/codercontrolplane_types.go @@ -126,8 +126,9 @@ type CoderControlPlaneSpec struct { TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"` // MCPServers configures CoderMCPServer objects that use this control - // plane. It has no effect yet: the CoderMCPServer controller is not - // shipped. + // plane. Removing it disables in Coder each server config that a + // CoderMCPServer created through this control plane. It has no effect + // yet: the CoderMCPServer controller is not shipped. // +optional MCPServers *MCPServersSpec `json:"mcpServers,omitempty"` diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index 7ccbd6cdd..6ebc77337 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -28,8 +28,11 @@ type MCPServersSpec struct { // control plane's operator token. This delegates Coder organization-admin // power over MCP servers in these organizations to everyone who can // create CoderMCPServer objects here. A CoderMCPServer for an - // organization outside this list sends no MCP request. Removing an - // organization stops further changes to its servers. Deleting a + // organization outside this list creates and changes nothing. Removing + // an organization disables in Coder each server config that a + // CoderMCPServer created there, with one request that holds only + // enabled: false, and then changes nothing else. After the organization + // is listed again, a spec edit enables the server. Deleting a // CoderMCPServer still deletes the config it created. // // This field has no effect yet: the CoderMCPServer controller is not @@ -45,7 +48,8 @@ type MCPServersSpec struct { // AllowForceOn lets CoderMCPServer objects use availability: force_on, // which adds a server to every new chat of each user who can read it, // without that user's choice. When false (the default), such objects - // send no MCP request. + // create and change nothing, and a server config that one of them + // created is disabled in Coder, as for a removed organization. // // This field has no effect yet: the CoderMCPServer controller is not // shipped. diff --git a/config/crd/bases/coder.com_codercontrolplanes.yaml b/config/crd/bases/coder.com_codercontrolplanes.yaml index 87d0c56cd..fe490c525 100644 --- a/config/crd/bases/coder.com_codercontrolplanes.yaml +++ b/config/crd/bases/coder.com_codercontrolplanes.yaml @@ -1403,15 +1403,17 @@ spec: mcpServers: description: |- MCPServers configures CoderMCPServer objects that use this control - plane. It has no effect yet: the CoderMCPServer controller is not - shipped. + plane. Removing it disables in Coder each server config that a + CoderMCPServer created through this control plane. It has no effect + yet: the CoderMCPServer controller is not shipped. properties: allowForceOn: description: |- AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects - send no MCP request. + create and change nothing, and a server config that one of them + created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. @@ -1423,8 +1425,11 @@ spec: control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an - organization outside this list sends no MCP request. Removing an - organization stops further changes to its servers. Deleting a + organization outside this list creates and changes nothing. Removing + an organization disables in Coder each server config that a + CoderMCPServer created there, with one request that holds only + enabled: false, and then changes nothing else. After the organization + is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not diff --git a/dist/install.yaml b/dist/install.yaml index b8e7b64ea..88c9b4c53 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -1678,15 +1678,17 @@ spec: mcpServers: description: |- MCPServers configures CoderMCPServer objects that use this control - plane. It has no effect yet: the CoderMCPServer controller is not - shipped. + plane. Removing it disables in Coder each server config that a + CoderMCPServer created through this control plane. It has no effect + yet: the CoderMCPServer controller is not shipped. properties: allowForceOn: description: |- AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects - send no MCP request. + create and change nothing, and a server config that one of them + created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. @@ -1698,8 +1700,11 @@ spec: control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an - organization outside this list sends no MCP request. Removing an - organization stops further changes to its servers. Deleting a + organization outside this list creates and changes nothing. Removing + an organization disables in Coder each server config that a + CoderMCPServer created there, with one request that holds only + enabled: false, and then changes nothing else. After the organization + is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not diff --git a/docs/reference/api/codercontrolplane.md b/docs/reference/api/codercontrolplane.md index ab5aecd7b..41dc8a975 100644 --- a/docs/reference/api/codercontrolplane.md +++ b/docs/reference/api/codercontrolplane.md @@ -40,7 +40,7 @@ | `tolerations` | [Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#toleration-v1-core) array | Tolerations are applied to the control plane pod. | | `affinity` | [Affinity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#affinity-v1-core) | Affinity configures pod affinity/anti-affinity rules. | | `topologySpreadConstraints` | [TopologySpreadConstraint](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#topologyspreadconstraint-v1-core) array | TopologySpreadConstraints control pod topology spread. | -| `mcpServers` | [MCPServersSpec](#mcpserversspec) | MCPServers configures CoderMCPServer objects that use this control plane. It has no effect yet: the CoderMCPServer controller is not shipped. | +| `mcpServers` | [MCPServersSpec](#mcpserversspec) | MCPServers configures CoderMCPServer objects that use this control plane. Removing it disables in Coder each server config that a CoderMCPServer created through this control plane. It has no effect yet: the CoderMCPServer controller is not shipped. | | `aiGateway` | [AIGatewaySpec](#aigatewayspec) | AIGateway configures CoderAIGatewayKey objects that use this control plane. It has no effect yet: the CoderAIGatewayKey controller is not shipped. | | `workspacePools` | [WorkspacePoolsSpec](#workspacepoolsspec) | WorkspacePools configures CoderWorkspacePool objects that use this control plane. It has no effect yet: the CoderWorkspacePool controller is not shipped. | | `templateTests` | [TemplateTestsSpec](#templatetestsspec) | TemplateTests configures CoderTemplateTest runs against this control plane. | @@ -157,8 +157,8 @@ CoderControlPlane (CoderControlPlane.spec.mcpServers). | Field | Type | Description | | --- | --- | --- | -| `organizations` | string array | Organizations are the names of the Coder organizations whose MCP server configs CoderMCPServer objects can create, change and delete with this control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an organization outside this list sends no MCP request. Removing an organization stops further changes to its servers. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not shipped. | -| `allowForceOn` | boolean | AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects send no MCP request. This field has no effect yet: the CoderMCPServer controller is not shipped. | +| `organizations` | string array | Organizations are the names of the Coder organizations whose MCP server configs CoderMCPServer objects can create, change and delete with this control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an organization outside this list creates and changes nothing. Removing an organization disables in Coder each server config that a CoderMCPServer created there, with one request that holds only enabled: false, and then changes nothing else. After the organization is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not shipped. | +| `allowForceOn` | boolean | AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects create and change nothing, and a server config that one of them created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. | ### OperatorAccessSpec diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index a554dad8e..30ab4723e 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -215,8 +215,9 @@ func (r *CoderMCPServerReconciler) check( } server.Status.OrganizationID = org.ID.String() if step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, org.ID); step != nil || err != nil { - if step != nil && step.stalled && server.Status.ConfigID != "" { - // Fail closed when the opt-in goes (#336). + if step != nil && (step.reason == "MCPServersNotAllowed" || step.reason == "ForceOnNotAllowed") && server.Status.ConfigID != "" { + // Fail closed when the opt-in goes (#336). Any other step, such + // as a mismatched Coder answer, keeps the config as it is. return r.disableMCPServerWithoutOptIn(ctx, sdk, server, org.ID, step) } return step, err diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index a29caad84..da5785a20 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -93,6 +93,11 @@ func readMCPServerToDisable( "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id) } if !found.Enabled { + // The disable answer can be lost after Coder committed it. Whoever + // disabled the config, the kill switch now holds this generation too, + // so an opt-in or consent that comes back does not apply a pending + // generation that enables the config. + server.Status.AppliedGeneration = server.Generation return found, stalled("MCP server config %s is disabled in Coder.", id) } return found, nil diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index bfc46ddd4..7def1878e 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/internal/controller/codermcpserver_optin_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/coder/coder/v2/codersdk" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -154,3 +155,66 @@ func TestCoderMCPServerOrganizationNotCoder404(t *testing.T) { } } } + +// A disable answer can be lost after Coder committed it. The next reconcile +// then finds the config disabled and records the generation, so the +// restored opt-in does not apply a pending generation that enables it. +func TestCoderMCPServerOptInFoundDisabledHolds(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) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + + env.setOptIn(ctx, t, nil) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, "MCPServersNotAllowed"), "the disable failed") + require.False(t, env.stored(t, got).view.Enabled, "the PATCH committed") + + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "is disabled in Coder") + require.Equal(t, got.Generation, got.Status.AppliedGeneration) + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + stored := env.stored(t, got) + require.False(t, stored.view.Enabled) + require.Empty(t, stored.view.Description, "the pending generation is not applied") + require.Equal(t, id, stored.view.ID) +} + +// Only a withdrawn opt-in disables the config. Another stall of the scope +// check, such as a mismatched Coder answer for a listed name, keeps the +// config as it is and sends no MCP request. +func TestCoderMCPServerScopeMismatchKeepsConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.addOrganization("alias") + got, _ := env.applied(ctx, t, nil) + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"alias"}}) + // The object's own organization resolves, then the listed name gets an + // answer for another organization. + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { return answer }}) + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { + org := answer.(codersdk.Organization) + org.Name = "other" + return org + }}) + mcp := env.mcpRequests() + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "CoderAnswerMismatch") + require.Equal(t, mcp, env.mcpRequests()) + require.True(t, env.stored(t, got).view.Enabled) +} From 7c187d5cca80c8c35a48792bdeb9d6156e95f007 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 12:46:17 +0000 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=A4=96=20fix:=20write=20the=20CoderMC?= =?UTF-8?q?PServer=20disable=20marker=20before=20the=20PATCH=20and=20disab?= =?UTF-8?q?le=20through=20the=20pinned=20organization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The opt-in and consent disables write appliedGeneration to status before the disable PATCH, and a failed status write sends no PATCH. A lost disable answer followed by a restored opt-in then gives DisabledExternally instead of applying a pending generation. - When the organization name no longer resolves (OrganizationNotFound) or resolves to another ID (OrganizationChanged), a withdrawn opt-in still disables the config through the pinned organization ID. Refs #155 Change-Id: Ie54facd7656489a5436d639734912e3c5c26050b Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 45 +++++-- internal/controller/codermcpserver_disable.go | 51 +++++--- .../controller/codermcpserver_optin_test.go | 111 ++++++++++++++++++ 3 files changed, 177 insertions(+), 30 deletions(-) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 30ab4723e..6a5c4a184 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -206,26 +206,32 @@ func (r *CoderMCPServerReconciler) check( return mcpServerWait(mcpServerRetry, wait.reason, "%s", wait.message), nil } org, step, err := resolveMCPServerOrganization(ctx, sdk, server.Spec.Organization) - if step != nil || err != nil { - return step, err + if err != nil { + return nil, err } - if pinned := server.Status.OrganizationID; pinned != "" && pinned != org.ID.String() { - return mcpServerStalled(mcpServerResync, "OrganizationChanged", - "Coder organization %s now has ID %s, not the pinned ID %s. Re-create this object to use the new organization.", server.Spec.Organization, org.ID, pinned), nil + pinned := server.Status.OrganizationID + if step == nil && pinned != "" && pinned != org.ID.String() { + step = mcpServerStalled(mcpServerResync, "OrganizationChanged", + "Coder organization %s now has ID %s, not the pinned ID %s. Re-create this object to use the new organization.", server.Spec.Organization, org.ID, pinned) } - server.Status.OrganizationID = org.ID.String() - if step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, org.ID); step != nil || err != nil { - if step != nil && (step.reason == "MCPServersNotAllowed" || step.reason == "ForceOnNotAllowed") && server.Status.ConfigID != "" { - // Fail closed when the opt-in goes (#336). Any other step, such - // as a mismatched Coder answer, keeps the config as it is. - return r.disableMCPServerWithoutOptIn(ctx, sdk, server, org.ID, step) + if step != nil { + // The config of a renamed, deleted or replaced organization stays in + // the pinned organization, and a withdrawn opt-in still disables it. + if pinnedID, parseErr := uuid.Parse(pinned); (step.reason == "OrganizationNotFound" || step.reason == "OrganizationChanged") && parseErr == nil && server.Status.ConfigID != "" { + if optInStep, err := r.optInGate(ctx, sdk, controlPlane, server, before, pinnedID); optInStep != nil || err != nil { + return optInStep, err + } } + return step, nil + } + server.Status.OrganizationID = org.ID.String() + if step, err := r.optInGate(ctx, sdk, controlPlane, server, before, org.ID); step != nil || err != nil { return step, err } values, step, err := readMCPServerCredentials(ctx, r.Client, server) if step != nil && step.stalled && server.Status.ConfigID != "" { // Fail closed when consent goes (plan G8). - step, err = r.disableMCPServerWithoutConsent(ctx, sdk, server, org.ID, step) + step, err = r.disableMCPServerWithoutConsent(ctx, sdk, server, before, org.ID, step) } if step != nil || err != nil { return step, err @@ -263,6 +269,21 @@ func (r *CoderMCPServerReconciler) check( return step, nil } +// optInGate applies the control plane's opt-in for the organization orgID. +// When the opt-in is withdrawn for a config that this object created, it +// disables the config (#336). Any other stall of the scope check, such as a +// mismatched Coder answer, keeps the config as it is. +func (r *CoderMCPServerReconciler) optInGate( + ctx context.Context, sdk *codersdk.Client, controlPlane *coderv1alpha1.CoderControlPlane, + server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, +) (*mcpServerStep, error) { + step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, orgID) + if step != nil && (step.reason == "MCPServersNotAllowed" || step.reason == "ForceOnNotAllowed") && server.Status.ConfigID != "" { + return r.disableMCPServerWithoutOptIn(ctx, sdk, server, before, orgID, step) + } + return step, err +} + // resolveMCPServerOrganization looks up a Coder organization by name. Coder // compares names without case. Only Coder's own 404 proves that the // organization does not exist: another 404 (a proxy page, "Route not diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index da5785a20..d7d10c613 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -32,7 +32,7 @@ import ( // appliedGeneration, so a later sync reports DisabledExternally and only a // spec edit after the disable enables the config again. func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( - ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, failed *mcpServerStep, + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, failed *mcpServerStep, ) (*mcpServerStep, error) { id, err := uuid.Parse(server.Status.ConfigID) if err != nil || id == uuid.Nil || failed == nil || !failed.stalled { @@ -65,8 +65,8 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( return stalled("Coder sends each credential of MCP server config %s only to origins that its recorded Secret allows.", id), nil } - if step := sendMCPServerDisable(ctx, sdk, server, orgID, id, unchecked); step != nil { - return step, nil + if step, err := r.sendMCPServerDisable(ctx, sdk, server, before, orgID, id, failed.reason, unchecked); step != nil || err != nil { + return step, err } // The reason stays the credential reason, so it does not flap between // this reconcile and the next one, which finds the config disabled. @@ -106,25 +106,40 @@ func readMCPServerToDisable( // sendMCPServerDisable sends the one PATCH of a fail-closed path. It holds // only enabled false: no credential and no destination field. It returns a // wait step when the PATCH failed or Coder answered for another or an -// enabled config. On success it sets appliedGeneration: the disable counts -// as an external disable of this generation, also of one that is not -// applied yet, so the DisabledExternally kill switch holds until the next -// spec edit. -func sendMCPServerDisable( - ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID, id uuid.UUID, - unchecked func(format string, args ...any) *mcpServerStep, -) *mcpServerStep { +// enabled config. +// +// The disable counts as an external disable of this generation, also of one +// that is not applied yet, so the DisabledExternally kill switch holds until +// the next spec edit. The controller writes appliedGeneration to status +// before the PATCH: Coder can commit the PATCH while its answer or the next +// status write is lost, and a pending generation must not enable the config +// once consent or the opt-in comes back. A failed status write sends no +// PATCH. +func (r *CoderMCPServerReconciler) sendMCPServerDisable( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, + orgID, id uuid.UUID, reason string, unchecked func(format string, args ...any) *mcpServerStep, +) (*mcpServerStep, error) { + if before == nil { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs the status before the reconcile to be disabled", server.Namespace, server.Name) + } + if server.Status.AppliedGeneration != server.Generation { + server.Status.AppliedGeneration = server.Generation + applyMCPServerStep(server, mcpServerWait(0, reason, "Disabling MCP server config %s in Coder.", id)) + if err := r.writeStatus(ctx, server, before); err != nil { + return nil, err + } + *before = *server.Status.DeepCopy() + } disabled := false updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, codersdk.UpdateMCPServerConfigRequest{Enabled: &disabled}) if err != nil { - return unchecked("the disable failed: %s", coderErrorSummary(err)) + return unchecked("the disable failed: %s", coderErrorSummary(err)), nil } if updated.ID != id || updated.OrganizationID != orgID || updated.Slug != server.Spec.Slug || updated.Enabled { return mcpServerWait(mcpServerRetry, "CoderAnswerMismatch", - "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id) + "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id), nil } - server.Status.AppliedGeneration = server.Generation - return nil + return nil, nil } // disableMCPServerWithoutOptIn handles a config that this object created @@ -136,7 +151,7 @@ func sendMCPServerDisable( // switch, as in disableMCPServerWithoutConsent: a restored opt-in needs a // spec edit to enable the config again. func (r *CoderMCPServerReconciler) disableMCPServerWithoutOptIn( - ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, failed *mcpServerStep, + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, failed *mcpServerStep, ) (*mcpServerStep, error) { id, err := uuid.Parse(server.Status.ConfigID) if err != nil || id == uuid.Nil || failed == nil || !failed.stalled { @@ -151,8 +166,8 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutOptIn( if _, step := readMCPServerToDisable(ctx, sdk, server, orgID, id, stalled, unchecked); step != nil { return step, nil } - if step := sendMCPServerDisable(ctx, sdk, server, orgID, id, unchecked); step != nil { - return step, nil + if step, err := r.sendMCPServerDisable(ctx, sdk, server, before, orgID, id, failed.reason, unchecked); step != nil || err != nil { + return step, err } return stalled("The controller disabled MCP server config %s in Coder: the control plane no longer allows it. After the opt-in is back, a spec edit enables the config.", id), nil } diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index 7def1878e..a3a31a236 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/internal/controller/codermcpserver_optin_test.go @@ -2,18 +2,22 @@ package controller_test import ( "context" + "errors" "net/http" "sync/atomic" "testing" "time" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" ) // #336: when the control plane withdraws its opt-in, the controller disables @@ -218,3 +222,110 @@ func TestCoderMCPServerScopeMismatchKeepsConfig(t *testing.T) { require.Equal(t, mcp, env.mcpRequests()) require.True(t, env.stored(t, got).view.Enabled) } + +// A disable PATCH can commit while its answer is lost, and the opt-in can +// come back before the next reconcile. The controller wrote +// appliedGeneration before the PATCH, so the sync then finds +// DisabledExternally and does not apply a pending generation. When that +// status write fails, the controller sends no PATCH. +func TestCoderMCPServerOptInDisableMarkerFirst(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + lostStatus bool // The status write before the PATCH fails. + }{{name: "answer lost, opt-in back"}, {name: "marker write fails", lostStatus: 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) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + env.setOptIn(ctx, t, nil) + + if tc.lostStatus { + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + r := &controller.CoderMCPServerReconciler{Client: interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { + return errors.New("lost status write") + }, + }), Scheme: scheme} + mark := len(env.writeRoutes()) + _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.ErrorContains(t, err, "lost status write") + require.Empty(t, env.writesSince(mark), "no PATCH without the marker") + require.True(t, env.stored(t, got).view.Enabled) + return + } + + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, "MCPServersNotAllowed"), "the disable failed") + require.False(t, env.stored(t, got).view.Enabled, "the PATCH committed") + require.Equal(t, got.Generation, got.Status.AppliedGeneration, "written before the PATCH") + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + require.Empty(t, env.stored(t, got).view.Description, "the pending generation is not applied") + }) + } +} + +// A renamed, deleted or replaced organization keeps the config in the +// pinned organization. A withdrawn opt-in still disables it there, while a +// name that is still listed changes nothing. +func TestCoderMCPServerOptInUsesPinnedOrganization(t *testing.T) { + t.Parallel() + gone := fakeFault{Status: http.StatusNotFound, Message: coderResourceNotFound} + replaced := fakeFault{Rewrite: func(answer any) any { + org := answer.(codersdk.Organization) + org.ID = uuid.New() + return org + }} + for _, tc := range []struct { + name string + fault fakeFault + reason string + withdraw bool + }{ + {name: "not found, opt-in removed", fault: gone, reason: "OrganizationNotFound", withdraw: true}, + {name: "replaced, opt-in removed", fault: replaced, reason: "OrganizationChanged", withdraw: true}, + {name: "not found, still listed", fault: gone, reason: "OrganizationNotFound"}, + {name: "replaced, still listed", fault: replaced, reason: "OrganizationChanged"}, + } { + 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.withdraw { + env.setOptIn(ctx, t, nil) + } + env.fake.failNext(routeOrganization, tc.fault) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + if !tc.withdraw { + message := requireMCPServerStep(t, got, tc.reason == "OrganizationChanged", tc.reason) + require.NotContains(t, message, "disabled") + require.Empty(t, env.writesSince(mark)) + require.True(t, env.stored(t, got).view.Enabled) + return + } + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "The controller disabled MCP server config") + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"enabled":false}`, string(writes[0].body)) + require.False(t, env.stored(t, got).view.Enabled) + }) + } +} From 1dacbde9c352c082610a007be35a1489c7788bde Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 13:07:48 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=A4=96=20fix:=20hold=20a=20gone=20Cod?= =?UTF-8?q?erMCPServer=20config=20with=20the=20disable=20kill=20switch=20a?= =?UTF-8?q?nd=20document=20appliedGeneration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A fail-closed disable that finds the config gone (Coder's own 404) now records the generation, so a restored opt-in or consent gives DeletedExternally instead of creating an enabled config before a spec edit. - The status.appliedGeneration doc now says that a kill switch also sets it, before the disable is sent. The dormant CRD is regenerated. Refs #155 Change-Id: I781f44dfea54d7834faf712e75d4fe764f65323f Signed-off-by: Thomas Kosiewski --- api/v1alpha1/codermcpserver_types.go | 6 ++- .../dormant/coder.com_codermcpservers.yaml | 8 +++- internal/controller/codermcpserver_disable.go | 4 ++ .../controller/codermcpserver_optin_test.go | 38 +++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index 6ebc77337..8d4478c0b 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -297,7 +297,11 @@ type CoderMCPServerStatus struct { // ObservedGeneration is the generation the controller last processed. // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // AppliedGeneration is the last generation whose spec Coder fully holds. + // AppliedGeneration is the last generation whose spec Coder fully holds, + // or that a kill switch holds: when the controller disables the config + // because a credential Secret or the control plane withdrew consent, it + // sets this field before it sends the disable, so only a later spec edit + // enables the config again. Ready tells whether Coder holds the spec. // +optional AppliedGeneration int64 `json:"appliedGeneration,omitempty"` // OrganizationID is the Coder organization ID that spec.organization diff --git a/config/crd/dormant/coder.com_codermcpservers.yaml b/config/crd/dormant/coder.com_codermcpservers.yaml index 334fcfd38..47f1654d7 100644 --- a/config/crd/dormant/coder.com_codermcpservers.yaml +++ b/config/crd/dormant/coder.com_codermcpservers.yaml @@ -421,8 +421,12 @@ spec: description: CoderMCPServerStatus defines the observed state of a CoderMCPServer. properties: appliedGeneration: - description: AppliedGeneration is the last generation whose spec Coder - fully holds. + description: |- + AppliedGeneration is the last generation whose spec Coder fully holds, + or that a kill switch holds: when the controller disables the config + because a credential Secret or the control plane withdrew consent, it + sets this field before it sends the disable, so only a later spec edit + enables the config again. Ready tells whether Coder holds the spec. format: int64 type: integer conditions: diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index d7d10c613..3a4b9e626 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -84,6 +84,10 @@ func readMCPServerToDisable( found, err := sdk.MCPServerConfigByID(ctx, orgID, id) switch { case isCoderResourceNotFound(err): + // The kill switch holds a gone config too: a pending generation must + // not create an enabled config again once consent or the opt-in comes + // back, before a spec edit. + server.Status.AppliedGeneration = server.Generation return found, stalled("MCP server config %s does not exist in Coder.", id) case err != nil: return found, unchecked("get MCP server config: %s", coderErrorSummary(err)) diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index a3a31a236..1ca2172b3 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/internal/controller/codermcpserver_optin_test.go @@ -329,3 +329,41 @@ func TestCoderMCPServerOptInUsesPinnedOrganization(t *testing.T) { }) } } + +// A config that is gone while a generation is pending and the opt-in is +// withdrawn: the kill switch holds it too. The restored opt-in gives +// DeletedExternally and no create, and only a spec edit creates the config +// again. +func TestCoderMCPServerOptInGoneConfigHolds(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) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, id)) + env.setOptIn(ctx, t, nil) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "does not exist in Coder") + require.Equal(t, got.Generation, got.Status.AppliedGeneration) + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DeletedExternally") + require.Empty(t, env.writesSince(mark), "no create before a spec edit") + require.Equal(t, 1, env.posts()) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "after the disable" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 2, env.posts()) +} From 32151aeff9b5b33ffd163447ad16f19e966d1a6d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 12:54:26 +0000 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A4=96=20fix:=20require=20https=20for?= =?UTF-8?q?=20Coder's=20revocation=20URL=20in=20the=20CoderMCPServer=20con?= =?UTF-8?q?sent=20check,=20and=20test=20the=20consent=20disable=20marker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The consent check now requires https for the OAuth2 revocation URL that Coder holds, as the spec does. Coder accepts an http loopback revocation URL, so a Secret that lists that origin still does not prove consent for it. - Tests for the consent path of the disable marker: a lost disable answer followed by restored consent, a failed marker write, and a config found disabled with a pending generation. Fixes #311 Fixes #334 Refs #155 Change-Id: I15caebdca4e137372ad3171e0f9007b207703413 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_disable.go | 4 + .../controller/codermcpserver_marker_test.go | 154 ++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 internal/controller/codermcpserver_marker_test.go diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index 3a4b9e626..521e9930f 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -230,6 +230,10 @@ func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, se if err != nil { return false, nil //nolint:nilerr // An origin that does not parse has no proof of consent. } + if raw == found.OAuth2RevocationURL && destination.Scheme != "https" { + // The spec allows only an https revocation URL (#334). + return false, nil + } value, err := credentialsecret.Read(ctx, r.Client, server.Namespace, coderv1alpha1.SecretKeySelector{Name: c.SecretName, Key: c.SecretKey}, destination) if credErr := (*credentialsecret.Error)(nil); errors.As(err, &credErr) { return false, nil diff --git a/internal/controller/codermcpserver_marker_test.go b/internal/controller/codermcpserver_marker_test.go new file mode 100644 index 000000000..1c35af712 --- /dev/null +++ b/internal/controller/codermcpserver_marker_test.go @@ -0,0 +1,154 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// pendingMCPServer applies a server, then makes a spec edit whose PATCH +// fails, so the object has a generation that is not applied yet. +func (env mcpServerEnv) pendingMCPServer(ctx context.Context, t *testing.T) (*coderv1alpha1.CoderMCPServer, *corev1.Secret) { + t.Helper() + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + return got, secret +} + +// restoreSecret creates the key Secret again with its consent. +func restoreSecret(ctx context.Context, t *testing.T, consented *corev1.Secret) { + t.Helper() + s := consented.DeepCopy() + s.ResourceVersion, s.UID = "", "" + createSecret(ctx, t, s) +} + +// #311: the disable PATCH commits in Coder but its answer is lost, while a +// generation is pending, and consent comes back before any other reconcile. +// The controller wrote appliedGeneration before the PATCH, so the next sync +// finds DisabledExternally and does not apply the pending generation. Only a +// spec edit after the disable enables the config. +func TestCoderMCPServerLostDisableAnswerHolds(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + got, secret := env.pendingMCPServer(ctx, t) + consented := secret.DeepCopy() + require.NoError(t, k8sClient.Delete(ctx, secret)) + + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, credentialsecret.ReasonSecretNotFound), "the disable failed") + require.False(t, env.stored(t, got).view.Enabled, "the PATCH committed") + require.Equal(t, got.Generation, got.Status.AppliedGeneration, "written before the PATCH") + + restoreSecret(ctx, t, consented) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + stored := env.stored(t, got) + require.False(t, stored.view.Enabled) + require.Empty(t, stored.view.Description, "the pending generation is not applied") + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "after the disable" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.stored(t, got).view.Enabled) +} + +// #311: when the status write before the disable PATCH fails, the +// controller sends no PATCH, and the reconcile returns the error. +func TestCoderMCPServerDisableNeedsTheMarker(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + got, secret := env.pendingMCPServer(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, secret)) + + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + r := &controller.CoderMCPServerReconciler{Client: interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { + return errors.New("lost status write") + }, + }), Scheme: scheme} + mark := len(env.writeRoutes()) + _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.ErrorContains(t, err, "lost status write") + require.Empty(t, env.writesSince(mark), "no PATCH without the marker") + require.True(t, env.stored(t, got).view.Enabled) +} + +// #311: the disable answer and the status write after it are both lost, so +// the next disable reconcile finds the config disabled. It records the +// generation as applied, so consent that comes back later does not apply +// the pending generation either. +func TestCoderMCPServerFoundDisabledHolds(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + got, secret := env.pendingMCPServer(ctx, t) + consented := secret.DeepCopy() + require.NoError(t, k8sClient.Delete(ctx, secret)) + // Coder holds the disable, but status does not know it. + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, env.stored(t, got).view.ID, codersdk.UpdateMCPServerConfigRequest{Enabled: ptrTo(false)}) + require.NoError(t, err) + + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, credentialsecret.ReasonSecretNotFound), "is disabled in Coder") + require.Equal(t, got.Generation, got.Status.AppliedGeneration) + require.Empty(t, env.writesSince(mark)) + + restoreSecret(ctx, t, consented) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + require.Empty(t, env.stored(t, got).view.Description) +} + +// #334: the consent check requires https for the revocation URL that Coder +// holds, as the spec does. Coder accepts an http loopback revocation URL, +// so a Secret that lists that origin still does not prove consent for it. +func TestCoderMCPServerConsentCheckNeedsHTTPSRevocation(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + const loopback = "http://127.0.0.1:8080" + env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin, loopback) + got, id := env.applied(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { useOAuth2(s, nil) }) + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{OAuth2RevocationURL: ptrTo(loopback + "/revoke")}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.URL = "https://other.example.com/mcp" + require.NoError(t, k8sClient.Update(ctx, got)) + + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, credentialsecret.ReasonDestinationNotAllowed), "The controller disabled MCP server config") + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"enabled":false}`, string(writes[0].body)) +}