From daedf3bca98d9e9fef008fb05a121f1682a18709 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 11:34:02 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20trust=20only=20Coder's=20?= =?UTF-8?q?own=20404=20before=20a=20CoderMCPServer=20config=20counts=20as?= =?UTF-8?q?=20gone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sync read and the disable read treated any 404 as a deleted config, including a proxy HTML page and Coder's "Route not found." for a wrong URL. After a spec edit that cleared status.configID, so the create path then reported SlugConflict and the config became unmanaged. The disable read skipped the fail-closed PATCH. Both reads now use isCoderResourceNotFound, and any other 404 waits and changes nothing. Fixes #337 Refs #155 Change-Id: I734fed2d39abb89dbbb95d055b489f9a65d6b803 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 6 +- internal/controller/codermcpserver_disable.go | 3 +- .../codermcpserver_notfound_test.go | 98 +++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 internal/controller/codermcpserver_notfound_test.go diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index e4bcb1980..1493fa50f 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -46,10 +46,12 @@ func (r *CoderMCPServerReconciler) syncMCPServer( applied := server.Status.AppliedGeneration == server.Generation found, err := sdk.MCPServerConfigByID(ctx, orgID, id) switch { - case isCoderNotFound(err) && applied: + // Only Coder's own 404 proves that the config is gone. A proxy page or + // "Route not found." waits below and changes nothing (#337). + case isCoderResourceNotFound(err) && applied: return mcpServerStalled(mcpServerResync, "DeletedExternally", "MCP server config %s was deleted in Coder. The controller does not create it again until the spec changes.", id), false, nil - case isCoderNotFound(err): + case isCoderResourceNotFound(err): server.Status.ConfigID, server.Status.Credentials = "", nil r.sentCredentials.Delete(server.UID) return nil, true, nil diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index 5da84c7f2..e2fe00964 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -47,7 +47,8 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( } found, err := sdk.MCPServerConfigByID(ctx, orgID, id) switch { - case isCoderNotFound(err): + 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 diff --git a/internal/controller/codermcpserver_notfound_test.go b/internal/controller/codermcpserver_notfound_test.go new file mode 100644 index 000000000..3d55ee855 --- /dev/null +++ b/internal/controller/codermcpserver_notfound_test.go @@ -0,0 +1,98 @@ +package controller_test + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// #337: in the sync, only Coder's own 404 proves that the config is gone. A +// proxy page or "Route not found." waits with CoderUnavailable and changes +// nothing, both for an applied config (no DeletedExternally kill switch) and +// after a spec edit (no cleared config ID, no create). +func TestCoderMCPServerSyncNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + for _, edited := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/edited=%v", name, edited), 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 edited { + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "edited" + require.NoError(t, k8sClient.Update(ctx, got)) + } + before := got.Status.DeepCopy() + mark := len(env.writeRoutes()) + env.fake.failNext(routeMCPServer, fault) + got, result := env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Equal(t, before.ConfigID, got.Status.ConfigID, "ownership is kept") + require.Equal(t, before.Credentials, got.Status.Credentials) + require.Empty(t, env.writesSince(mark), "no Coder write") + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.posts(), "the only create is the first one") + }) + } + } +} + +// #337: the read of the disable path also needs Coder's own 404 before the +// config counts as gone. Another 404 waits, keeps the config ID, and the next +// reconcile disables the config. +func TestCoderMCPServerDisableNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Delete(ctx, secret)) + before := got.Status.DeepCopy() + mark := len(env.writeRoutes()) + env.fake.failNext(routeMCPServer, fault) + got, result := env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, credentialsecret.ReasonSecretNotFound), "Coder can still send the credentials") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Equal(t, before.ConfigID, got.Status.ConfigID, "ownership is kept") + require.Empty(t, env.writesSince(mark), "no Coder write") + require.True(t, env.stored(t, got).view.Enabled) + + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, credentialsecret.ReasonSecretNotFound), "The controller disabled MCP server config") + require.False(t, env.stored(t, got).view.Enabled) + }) + } +} + +// Coder's own 404 still counts as gone in the disable read, so the test +// above cannot pass by never treating a 404 as gone. The sync read is +// covered by TestCoderMCPServerSyncDeletedConfig. +func TestCoderMCPServerDisableCoder404(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, id)) + require.NoError(t, k8sClient.Delete(ctx, secret)) + mark := len(env.writeRoutes()) + got, result := env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, credentialsecret.ReasonSecretNotFound), "does not exist in Coder") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Empty(t, env.writesSince(mark)) +}