From 0507afef8949f5cb53f447b8d0e1bee44aabca6f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:17:26 +0000 Subject: [PATCH 1/7] =?UTF-8?q?=F0=9F=A4=96=20feat:=20sync=20the=20CoderMC?= =?UTF-8?q?PServer=20config=20with=20the=20spec=20on=20every=20reconcile?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconciler now reads the config on every reconcile and sets back any field that differs with one sparse PATCH. A PATCH that sets a URL or the auth type carries every credential of the auth type. Credentials of other auth types are cleared and checked on every apply (#314). The ACL sync also runs every reconcile, so a user grant added in the dashboard goes. A config deleted before its generation is applied is created again. Refs #155 Change-Id: Iecb93e07d2fe2d13630dd026827c948afa0c6a73 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 293 ++++++++++++------ .../controller/codermcpserver_apply_test.go | 24 +- .../controller/codermcpserver_controller.go | 39 ++- .../controller/codermcpserver_create_test.go | 4 +- .../controller/codermcpserver_sync_test.go | 176 +++++++++++ 5 files changed, 411 insertions(+), 125 deletions(-) create mode 100644 internal/controller/codermcpserver_sync_test.go diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index d023c0db7..0614079b3 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -1,9 +1,11 @@ package controller import ( + "cmp" "context" "fmt" "net/http" + "slices" "strings" "github.com/coder/coder/v2/codersdk" @@ -13,133 +15,236 @@ import ( "github.com/coder/coder-k8s/internal/credentialsecret" ) -// applyMCPServer sets the spec on the config that this object created (plan -// 4.5 create steps 5 and 6). It reads the config, removes the ACL entries -// that spec.access does not list, sends one PATCH with every spec field and -// every credential of the auth type, and only then adds the missing groups. -// So while the PATCH changes or enables the config, the ACL holds only groups -// that both the old and the new access list allow (plan S20), and a failed -// PATCH leaves the ACL narrower, never broader. Every PATCH sets url, so it -// always carries the credentials (plan 4.6 item 5). It returns a ready step -// and sets status.appliedGeneration on success. -func applyMCPServer( +// syncMCPServer reads the config that this object created and makes it match +// the spec (plan 4.5 create steps 5 and 6, 4.7). It runs on every reconcile, +// so a dashboard edit is set back at the latest on the next resync. +// +// One sparse PATCH sends every field that differs. A PATCH that sets a URL or +// the auth type also carries every credential of the auth type (plan 4.6 item +// 5), and a credential also goes when its Secret changed or Coder lost it. +// Credentials of the other auth types are cleared. The ACL entries that +// spec.access does not list go before the PATCH and the missing groups after +// it, so the ACL never holds a group that neither generation allows (plan +// S20). +// +// A dashboard delete after a full apply of this generation is a kill switch: +// the controller reports it and writes nothing until the spec changes (plan +// 4.7 item 2). The other kill switches come in the next change. recreate is +// true when the config is gone before a full apply of this generation: the +// caller then creates it again, if the slug is free. +func syncMCPServer( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, -) (*mcpServerStep, error) { +) (step *mcpServerStep, recreate bool, err error) { id, err := uuid.Parse(server.Status.ConfigID) if err != nil || id == uuid.Nil { - return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a UUID", server.Namespace, server.Name, server.Status.ConfigID) + return nil, false, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a UUID", server.Namespace, server.Name, server.Status.ConfigID) } - current, err := sdk.MCPServerConfigByID(ctx, orgID, id) + applied := server.Status.AppliedGeneration == server.Generation + found, err := sdk.MCPServerConfigByID(ctx, orgID, id) switch { + case isCoderNotFound(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): - // Re-creating it after a spec change comes with the update path. - return mcpServerStalled(0, "DeletedExternally", "MCP server config %s no longer exists in Coder. This controller version does not create it again.", id), nil + server.Status.ConfigID, server.Status.Credentials = "", nil + return nil, true, nil case err != nil: - return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), nil + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), false, nil } - if current.ID != id || current.OrganizationID != orgID || current.Slug != server.Spec.Slug { + 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", current.ID, current.Slug, current.OrganizationID, id), nil + "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), false, nil } - if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { - return step, err - } - req, err := mcpServerUpdateRequest(server, values) + + full, err := mcpServerUpdateRequest(server, values) if err != nil { - return nil, err - } - updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) - switch status := coderStatus(err); { - case status == http.StatusBadRequest: - return mcpServerStalled(0, "CoderRejected", "Coder rejected the update: %s", coderErrorSummary(err)), nil - case err != nil: - // The PATCH sets absolute values, so sending it again is safe. This - // includes a 404: a proxy can put one in place of a committed answer, - // so the GET of the next reconcile decides whether the config is gone. - return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update MCP server config %s: %s", id, coderErrorSummary(err)), nil + return nil, false, err } - if field := mcpServerUpdateMismatch(id, current.AuthType, req, updated); field != "" { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with another %s", id, field), nil + if req := mcpServerSparseRequest(server, full, found, values); req != (codersdk.UpdateMCPServerConfigRequest{}) { + if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, false, err + } + updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) + switch status := coderStatus(err); { + case status == http.StatusBadRequest: + return mcpServerStalled(0, "CoderRejected", "Coder rejected the update: %s", coderErrorSummary(err)), false, nil + case err != nil: + // The PATCH sets absolute values, so sending it again is safe. This + // includes a 404: a proxy can put one in place of a committed answer, + // so the GET of the next reconcile decides whether the config is gone. + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update MCP server config %s: %s", id, coderErrorSummary(err)), false, nil + } + if field := mcpServerMismatch(server.Spec.Auth, full, updated); field != "" || updated.ID != id { + return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with another %s", id, cmp.Or(field, "ID")), false, nil + } + } else if field := mcpServerMismatch(server.Spec.Auth, full, found); field != "" { + return nil, false, fmt.Errorf("assertion failed: MCP server %s/%s differs in %s, but the sync request is empty", server.Namespace, server.Name, field) } if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { - return step, err + return step, false, err } server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) server.Status.AppliedGeneration = server.Generation - return mcpServerReady(server), nil + return mcpServerReady(server), false, nil +} + +// mcpServerCredentialSet says which credential fields an auth type uses. +type mcpServerCredentialSet struct{ apiKey, headers, secret bool } + +func mcpServerActiveCredentials(auth coderv1alpha1.CoderMCPServerAuth) mcpServerCredentialSet { + return mcpServerCredentialSet{apiKey: auth.Type == "api_key", headers: auth.Type == "custom_headers", secret: auth.Type == "oauth2"} +} + +// mcpServerSparseRequest drops every field of full that found already holds. +// It keeps a credential when a URL or the auth type changes, when its change +// token changed, or when Coder lost it. It clears credentials of the other +// auth types that Coder holds (#314). +func mcpServerSparseRequest( + server *coderv1alpha1.CoderMCPServer, full codersdk.UpdateMCPServerConfigRequest, found codersdk.MCPServerConfig, values map[string]credentialsecret.Value, +) codersdk.UpdateMCPServerConfigRequest { + req := full + authChanged := found.AuthType != *full.AuthType + for _, f := range mcpServerStringFields(&req, found) { + if *f.want != nil && (!authChanged || !f.auth) && mcpServerSameString(f.name, **f.want, f.got) { + *f.want = nil + } + } + for _, f := range mcpServerBoolFields(&req, found) { + if *f.want != nil && **f.want == f.got { + *f.want = nil + } + } + destinationChanged := req.URL != nil || req.AuthType != nil || req.OAuth2AuthURL != nil || req.OAuth2TokenURL != nil || req.OAuth2RevocationURL != nil + recorded := map[string]coderv1alpha1.CoderMCPServerCredentialStatus{} + for _, c := range server.Status.Credentials { + recorded[c.Field] = c + } + needs := func(field string, held bool) bool { + if destinationChanged || !held { + return true + } + token := values[field].Token + c, ok := recorded[field] + return !ok || c.SecretName != token.SecretName || c.SecretKey != token.SecretKey || c.SecretUID != string(token.SecretUID) || c.SecretResourceVersion != token.ResourceVersion + } + var sent []string + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + held := map[string]bool{"apiKey": found.HasAPIKey, "oauth2ClientSecret": found.HasOAuth2Secret}[c.field] || strings.HasPrefix(c.field, "header:") && found.HasCustomHeaders + if needs(c.field, held) { + sent = append(sent, c.field) + } + } + active := mcpServerActiveCredentials(server.Spec.Auth) + sends := func(prefix string) bool { + return slices.ContainsFunc(sent, func(f string) bool { return strings.HasPrefix(f, prefix) }) + } + // Custom headers are one Coder field, so they all go together. + if !active.headers || !sends("header:") { + req.CustomHeaders = nil + } + if !active.apiKey || !sends("apiKey") { + req.APIKeyValue = nil + } + if !active.secret || !sends("oauth2ClientSecret") { + req.OAuth2ClientSecret = nil + } + // Coder keeps other types' credentials while the auth type stays. + empty, none := "", map[string]string{} + if !active.apiKey && found.HasAPIKey { + req.APIKeyValue = &empty + } + if !active.headers && found.HasCustomHeaders { + req.CustomHeaders = &none + } + if !active.secret && found.HasOAuth2Secret { + req.OAuth2ClientSecret = &empty + } + return req +} + +type mcpServerStringField struct { + name string + want **string + got string + auth bool +} + +func mcpServerStringFields(req *codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) []mcpServerStringField { + return []mcpServerStringField{ + {"display_name", &req.DisplayName, got.DisplayName, false}, + {"description", &req.Description, got.Description, false}, + {"icon_url", &req.IconURL, got.IconURL, false}, + {"transport", &req.Transport, got.Transport, false}, + {"url", &req.URL, got.URL, false}, + {"availability", &req.Availability, got.Availability, false}, + {"auth_type", &req.AuthType, got.AuthType, true}, + {"api_key_header", &req.APIKeyHeader, got.APIKeyHeader, true}, + {"oauth2_client_id", &req.OAuth2ClientID, got.OAuth2ClientID, true}, + {"oauth2_auth_url", &req.OAuth2AuthURL, got.OAuth2AuthURL, true}, + {"oauth2_token_url", &req.OAuth2TokenURL, got.OAuth2TokenURL, true}, + {"oauth2_revocation_url", &req.OAuth2RevocationURL, got.OAuth2RevocationURL, true}, + {"oauth2_scopes", &req.OAuth2Scopes, got.OAuth2Scopes, true}, + } } -// mcpServerUpdateMismatch returns the first field of the update answer that -// differs from the request, or "". Coder returns every field the request -// sets, except the credential values, for which it returns a has_* flag. A -// changed auth type makes Coder clear the credentials of the other types -// (plan F9), so their flags must then be false. -func mcpServerUpdateMismatch(id uuid.UUID, oldAuthType string, req codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) string { - if got.ID != id { - return "ID" - } - strs := []struct { - name string - want *string - got string - }{ - {"display_name", req.DisplayName, got.DisplayName}, - {"description", req.Description, got.Description}, - {"icon_url", req.IconURL, got.IconURL}, - {"transport", req.Transport, got.Transport}, - {"url", req.URL, got.URL}, - {"auth_type", req.AuthType, got.AuthType}, - {"availability", req.Availability, got.Availability}, - {"api_key_header", req.APIKeyHeader, got.APIKeyHeader}, - {"oauth2_client_id", req.OAuth2ClientID, got.OAuth2ClientID}, - {"oauth2_auth_url", req.OAuth2AuthURL, got.OAuth2AuthURL}, - {"oauth2_token_url", req.OAuth2TokenURL, got.OAuth2TokenURL}, - {"oauth2_revocation_url", req.OAuth2RevocationURL, got.OAuth2RevocationURL}, - {"oauth2_scopes", req.OAuth2Scopes, got.OAuth2Scopes}, - } - for _, f := range strs { - if f.want != nil && *f.want != f.got { +type mcpServerBoolField struct { + name string + want **bool + got bool +} + +func mcpServerBoolFields(req *codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) []mcpServerBoolField { + return []mcpServerBoolField{ + {"enabled", &req.Enabled, got.Enabled}, + {"model_intent", &req.ModelIntent, got.ModelIntent}, + {"allow_in_plan_mode", &req.AllowInPlanMode, got.AllowInPlanMode}, + {"forward_coder_headers", &req.ForwardCoderHeaders, got.ForwardCoderHeaders}, + } +} + +// mcpServerSameString compares one string field. Scopes are a set of tokens +// separated by spaces (plan 4.7 item 5). +func mcpServerSameString(name, want, got string) bool { + if name != "oauth2_scopes" { + return want == got + } + a, b := strings.Fields(want), strings.Fields(got) + slices.Sort(a) + slices.Sort(b) + return slices.Equal(slices.Compact(a), slices.Compact(b)) +} + +// mcpServerMismatch returns the first field in which got differs from want, +// or "". It checks every field that want sets and, on every apply, that Coder +// holds exactly the credentials of the auth type (#314). +func mcpServerMismatch(auth coderv1alpha1.CoderMCPServerAuth, want codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) string { + for _, f := range mcpServerStringFields(&want, got) { + if *f.want != nil && !mcpServerSameString(f.name, **f.want, f.got) { return f.name } } - bools := []struct { - name string - want *bool - got bool - }{ - {"enabled", req.Enabled, got.Enabled}, - {"model_intent", req.ModelIntent, got.ModelIntent}, - {"allow_in_plan_mode", req.AllowInPlanMode, got.AllowInPlanMode}, - {"forward_coder_headers", req.ForwardCoderHeaders, got.ForwardCoderHeaders}, - } - for _, f := range bools { - if f.want != nil && *f.want != f.got { + for _, f := range mcpServerBoolFields(&want, got) { + if *f.want != nil && **f.want != f.got { return f.name } } + if want.AuthType == nil { + // An auth-only check without the auth type cannot happen. + return "" + } + active := mcpServerActiveCredentials(auth) switch { - case req.APIKeyValue != nil && !got.HasAPIKey: + case active.apiKey != got.HasAPIKey: return "has_api_key" - case req.CustomHeaders != nil && !got.HasCustomHeaders: + case active.headers != got.HasCustomHeaders: return "has_custom_headers" - case req.OAuth2ClientSecret != nil && !got.HasOAuth2Secret: + case active.secret != got.HasOAuth2Secret: return "has_oauth2_secret" } - if req.AuthType != nil && *req.AuthType != oldAuthType { - switch { - case req.APIKeyValue == nil && got.HasAPIKey: - return "has_api_key" - case req.CustomHeaders == nil && got.HasCustomHeaders: - return "has_custom_headers" - case req.OAuth2ClientSecret == nil && got.HasOAuth2Secret: - return "has_oauth2_secret" - } - } return "" } -// mcpServerReady reports a config that matched the spec when this generation -// was applied. This version reads Coder only to apply a new generation. +// mcpServerReady reports a config that matches the spec. func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { return &mcpServerStep{ ready: true, reason: "Applied", requeue: mcpServerResync, diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index 16b35f3e7..8e5af3731 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -30,6 +30,13 @@ func (env mcpServerEnv) writeRoutes() []string { return out } +func (env mcpServerEnv) lastUpdateBody(t *testing.T) []byte { + t.Helper() + bodies := env.fake.mcpRequestBodies(routeUpdateMCPServer) + require.NotEmpty(t, bodies) + return bodies[len(bodies)-1] +} + func (env mcpServerEnv) lastUpdate(t *testing.T) codersdk.UpdateMCPServerConfigRequest { t.Helper() bodies := env.fake.mcpRequestBodies(routeUpdateMCPServer) @@ -68,14 +75,13 @@ func TestCoderMCPServerApplyOrder(t *testing.T) { groups, users := env.fake.mcpACL(stored.view.ID) require.Equal(t, []uuid.UUID{data}, groups) require.Empty(t, users) - req := env.lastUpdate(t) - require.Equal(t, testMCPKey, *req.APIKeyValue, "a PATCH that sets url carries the credential") - require.Equal(t, "https://mcp.example.com/github/mcp", *req.URL) + require.JSONEq(t, `{"display_name":"GitHub","enabled":true}`, string(env.lastUpdateBody(t)), + "the create sent every other field and the key, so the enabling PATCH is sparse") require.Len(t, got.Status.Credentials, 1) - before := env.mcpRequests() + writes := len(env.writeRoutes()) again, _ := env.reconcile(ctx, t, got) - require.Equal(t, before, env.mcpRequests(), "an applied generation sends nothing") + require.Len(t, env.writeRoutes(), writes, "a config that matches the spec gets no write") require.Equal(t, got.ResourceVersion, again.ResourceVersion) } @@ -203,7 +209,6 @@ func TestCoderMCPServerApplyFailures(t *testing.T) { stalled bool retry bool // The next reconcile applies the spec. }{ - {name: "config gone", route: routeMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "DeletedExternally", stalled: true}, {name: "read fails", route: routeMCPServer, fault: fakeFault{Status: http.StatusServiceUnavailable}, reason: "CoderUnavailable", retry: true}, {name: "read answers another slug", route: routeMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: fakeFault{Rewrite: func(answer any) any { c := answer.(codersdk.MCPServerConfig) @@ -218,6 +223,7 @@ func TestCoderMCPServerApplyFailures(t *testing.T) { {name: "update answers another availability", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Availability = "force_on" })}, {name: "update answers another flag", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.ForwardCoderHeaders = true })}, {name: "update answers another header", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.APIKeyHeader = "X-Other" })}, + {name: "update answers another type's credential", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.HasCustomHeaders = true })}, {name: "update answers no key", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.HasAPIKey = false })}, } { t.Run(tc.name, func(t *testing.T) { @@ -310,7 +316,7 @@ func TestCoderMCPServerApplyLostStatusWrite(t *testing.T) { got, _ := env.reconcile(ctx, t, server) requireMCPServerApplied(t, got) require.Equal(t, 1, env.posts()) - require.Equal(t, 2, env.fake.requestCount(routeUpdateMCPServer)) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer), "the config already matches the spec") } // A Secret change moves its change token but not the generation. The next @@ -335,9 +341,9 @@ func TestCoderMCPServerApplySecretRotation(t *testing.T) { require.Equal(t, rotated, env.requireCreated(t, got).apiKeyValue) require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) - before = env.mcpRequests() + writes := len(env.writeRoutes()) again, _ := env.reconcile(ctx, t, got) - require.Equal(t, before, env.mcpRequests(), "a recorded token sends nothing") + require.Len(t, env.writeRoutes(), writes, "a recorded token sends nothing") require.Equal(t, got.ResourceVersion, again.ResourceVersion) } diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 74cfdea3a..cffc297a7 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/url" - "slices" "strings" "time" "unicode/utf8" @@ -48,8 +47,8 @@ const ( // CoderMCPServerReconciler reconciles CoderMCPServer objects (#155). It is not // registered yet. It checks the control plane, the organization and its -// opt-in, and the credential Secrets, and it reports a slug that a config of -// someone else holds. It writes nothing to Coder. +// opt-in, and the credential Secrets, reports a slug that a config of +// someone else holds, creates the config, and syncs it on every reconcile. type CoderMCPServerReconciler struct { client.Client Scheme *runtime.Scheme @@ -217,30 +216,30 @@ func (r *CoderMCPServerReconciler) check( if step != nil || err != nil { return step, err } - if server.Status.ConfigID == "" { - if step, err = r.createOrRecord(ctx, sdk, server, org.ID, values, before); err != nil { - return nil, err - } - if step == nil { + // A config that is gone before a full apply is created again once. + for range 2 { + if server.Status.ConfigID == "" { + if step, err = r.createOrRecord(ctx, sdk, server, org.ID, values, before); err != nil || step != nil { + break + } // Store the ID before anything replaces the nonce: after that, // only the stored ID proves that this object created the config. - if err := r.writeStatus(ctx, server, before); err != nil { - return nil, err + if err = r.writeStatus(ctx, server, before); err != nil { + break } *before = *server.Status.DeepCopy() } - } - switch { - case step != nil: - case server.Status.AppliedGeneration == server.Generation && - slices.Equal(server.Status.Credentials, mcpServerCredentialStatus(server.Spec.Auth, values)): - // A Secret change moves its change token without a new generation. - step = mcpServerReady(server) - default: - if step, err = applyMCPServer(ctx, sdk, server, org.ID, values); err != nil { - return nil, err + var recreate bool + if step, recreate, err = syncMCPServer(ctx, sdk, server, org.ID, values); err != nil || !recreate { + break } } + if err != nil { + return nil, err + } + if step == nil { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s was created again twice in one reconcile", server.Namespace, server.Name) + } // Coder messages can echo the request, so scrub the credentials from them. all := make([]credentialsecret.Value, 0, len(values)) for _, v := range values { diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go index 0832f63ab..82d21d96d 100644 --- a/internal/controller/codermcpserver_create_test.go +++ b/internal/controller/codermcpserver_create_test.go @@ -142,9 +142,9 @@ func TestCoderMCPServerCreate(t *testing.T) { require.Equal(t, s.ResourceVersion, c.SecretResourceVersion) } - before := env.mcpRequests() + writes := len(env.writeRoutes()) again, _ := env.reconcile(ctx, t, got) - require.Equal(t, before, env.mcpRequests(), "an applied generation needs no MCP request in this version") + require.Len(t, env.writeRoutes(), writes, "a config that matches the spec gets no write") require.Equal(t, got.ResourceVersion, again.ResourceVersion) }) } diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go new file mode 100644 index 000000000..217b3e4ae --- /dev/null +++ b/internal/controller/codermcpserver_sync_test.go @@ -0,0 +1,176 @@ +package controller_test + +import ( + "context" + "slices" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// dashboard is a Coder client for the edits that an admin makes in the +// dashboard. The fake records its writes like the controller's, so take a +// write mark after the edit. +func (env mcpServerEnv) dashboard(t *testing.T) *codersdk.Client { + t.Helper() + return env.fake.client(t, 2*time.Second) +} + +// applied creates the server of validMCPServerSpec with mutate applied and +// applies it. +func (env mcpServerEnv) applied(ctx context.Context, t *testing.T, mutate func(*coderv1alpha1.CoderMCPServerSpec)) (*coderv1alpha1.CoderMCPServer, uuid.UUID) { + t.Helper() + got, _ := env.reconcile(ctx, t, env.create(ctx, t, mutate)) + requireMCPServerApplied(t, got) + return got, env.requireCreated(t, got).view.ID +} + +// writesSince returns the recorded writes after the first mark writes. +func (env mcpServerEnv) writesSince(mark int) []fakeMCPBody { + env.fake.mu.Lock() + defer env.fake.mu.Unlock() + return slices.Clone(env.fake.mcp.bodies[mark:]) +} + +// stored returns what Coder holds for the server, without updated_at, which +// every PATCH moves (plan F11). +func (env mcpServerEnv) stored(t *testing.T, server *coderv1alpha1.CoderMCPServer) fakeMCPServer { + t.Helper() + s := env.requireCreated(t, server) + s.view.UpdatedAt = time.Time{} + return s +} + +func oauth2MCPSecret(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin) +} + +// Plan 4.7 item 5 and S13: the controller reads the config on every +// reconcile, so a dashboard edit is set back by one sparse PATCH without a +// spec change. A PATCH that sets url carries the credential (plan 4.6 item +// 5). A credential of another auth type is cleared (#314). Scopes compare as +// a set, so a reordered list gets no write. +func TestCoderMCPServerSyncDrift(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + secrets func(context.Context, *testing.T, mcpServerEnv) + edit codersdk.UpdateMCPServerConfigRequest + want string // The body of the controller's PATCH, or "" for no write. + }{ + {name: "description", edit: codersdk.UpdateMCPServerConfigRequest{Description: ptrTo("edited")}, want: `{"description":""}`}, + { + name: "availability and a flag", edit: codersdk.UpdateMCPServerConfigRequest{Availability: ptrTo("default_on"), ForwardCoderHeaders: ptrTo(true)}, + want: `{"availability":"default_off","forward_coder_headers":false}`, + }, + {name: "url", edit: codersdk.UpdateMCPServerConfigRequest{URL: ptrTo("https://evil.example.net/mcp")}, want: `{"url":"https://mcp.example.com/github/mcp","api_key_value":"` + testMCPKey + `"}`}, + {name: "api key header", edit: codersdk.UpdateMCPServerConfigRequest{APIKeyHeader: ptrTo("X-Other")}, want: `{"api_key_header":"Authorization"}`}, + {name: "credential of another auth type", edit: codersdk.UpdateMCPServerConfigRequest{CustomHeaders: &map[string]string{"X-Extra": "v"}}, want: `{"custom_headers":{}}`}, + { + name: "api key on a none config", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} }, + edit: codersdk.UpdateMCPServerConfigRequest{APIKeyValue: ptrTo("dashboard-key")}, want: `{"api_key_value":""}`, + }, + { + name: "scopes reordered", secrets: oauth2MCPSecret, + spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + useOAuth2(s, func(o *coderv1alpha1.CoderMCPServerOAuth2Auth) { o.Scopes = []string{"read", "write"} }) + }, + edit: codersdk.UpdateMCPServerConfigRequest{OAuth2Scopes: ptrTo("write read")}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + if tc.secrets == nil { + tc.secrets = func(ctx context.Context, t *testing.T, env mcpServerEnv) { env.keySecret(ctx, t, testMCPKey) } + } + tc.secrets(ctx, t, env) + got, id := env.applied(ctx, t, tc.spec) + before := env.stored(t, got) + + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, tc.edit) + require.NoError(t, err) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + writes := env.writesSince(mark) + if tc.want == "" { + require.Empty(t, writes) + return + } + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServer, writes[0].route) + require.JSONEq(t, tc.want, string(writes[0].body)) + require.Equal(t, before, env.stored(t, got), "Coder holds the applied state again") + + mark = len(env.writeRoutes()) + env.reconcile(ctx, t, got) + require.Empty(t, env.writesSince(mark)) + }) + } +} + +// The spec.access field doc says that every sync removes user grants: a +// grant added in the dashboard goes on the next reconcile, without a spec +// change, and the config itself gets no PATCH. +func TestCoderMCPServerSyncRemovesUserGrant(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + data := env.fake.addGroup(env.orgID, "data", "") + alice := env.fake.addUser("alice", codersdk.LoginTypePassword) + env.fake.addMember(env.orgID, alice) + got, id := env.applied(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data"}} + }) + + require.NoError(t, env.dashboard(t).UpdateMCPServerConfigACL(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigACLRequest{ + UserRoles: map[string]codersdk.MCPServerConfigRole{alice.String(): codersdk.MCPServerConfigRoleRead}, + })) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + groups, users := env.fake.mcpACL(id) + require.Equal(t, []uuid.UUID{data}, groups) + require.Empty(t, users) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServerACL, writes[0].route) +} + +// Plan 4.7 items 2 and 3: a dashboard delete after the apply is a kill +// switch with no write. After a spec edit, the controller creates the config +// again, because the slug is free. +func TestCoderMCPServerSyncDeletedConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, gone := env.applied(ctx, t, nil) + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, gone)) + mark := len(env.writeRoutes()) + got, result := env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DeletedExternally") + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no Kubernetes event follows a dashboard delete") + 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.Equal(t, 2, env.posts()) + require.NotEqual(t, gone.String(), got.Status.ConfigID) + stored := env.stored(t, got) + require.True(t, stored.view.Enabled) + require.Equal(t, testMCPKey, stored.apiKeyValue) +} From 0ea11aa97b7632da5f6bbdfb985e15ada6d30090 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:18:38 +0000 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=A4=96=20feat:=20add=20the=20CoderMCP?= =?UTF-8?q?Server=20kill=20switches=20and=20the=20user=5Foidc=20correction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a full apply, a dashboard disable, credential clear or auth type change now stops every write until the spec changes. An observed user_oidc auth type is always set back with a PATCH that holds only auth fields, so a disable stays. An in-memory SHA-256 of each sent credential skips the resend after a metadata-only Secret change. Refs #155 Change-Id: I0ded9edf8413c7dff9c7cb3aaf1186ccf2681188 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 118 +++++++++++-- .../controller/codermcpserver_controller.go | 7 +- internal/controller/codermcpserver_create.go | 5 + .../controller/codermcpserver_sync_test.go | 158 ++++++++++++++++++ 4 files changed, 269 insertions(+), 19 deletions(-) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index 0614079b3..e9eb44c3e 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -3,7 +3,9 @@ package controller import ( "cmp" "context" + "crypto/sha256" "fmt" + "maps" "net/http" "slices" "strings" @@ -27,12 +29,14 @@ import ( // it, so the ACL never holds a group that neither generation allows (plan // S20). // -// A dashboard delete after a full apply of this generation is a kill switch: -// the controller reports it and writes nothing until the spec changes (plan -// 4.7 item 2). The other kill switches come in the next change. recreate is -// true when the config is gone before a full apply of this generation: the -// caller then creates it again, if the slug is free. -func syncMCPServer( +// After a full apply of this generation, a dashboard delete, disable or +// credential clear is a kill switch: the controller reports it and writes +// nothing until the spec changes (plan 4.7 item 2). An auth type user_oidc +// is always set back, with only the auth fields, so a disable stays. +// +// recreate is true when the config is gone before a full apply of this +// generation: the caller then creates it again, if the slug is free. +func (r *CoderMCPServerReconciler) syncMCPServer( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, ) (step *mcpServerStep, recreate bool, err error) { id, err := uuid.Parse(server.Status.ConfigID) @@ -47,6 +51,7 @@ func syncMCPServer( "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): server.Status.ConfigID, server.Status.Credentials = "", nil + r.sentCredentials.Delete(server.UID) return nil, true, nil case err != nil: return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), false, nil @@ -56,13 +61,38 @@ func syncMCPServer( "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), false, nil } + // user_oidc sends each chat user's login token to url (plan F14), so it is + // set back even when a kill switch holds every other write. + oidc := found.AuthType == "user_oidc" + enabled := server.Spec.Enabled == nil || *server.Spec.Enabled + var kill *mcpServerStep + switch active := mcpServerActiveCredentials(server.Spec.Auth); { + case applied && enabled && !found.Enabled: + kill = mcpServerStalled(mcpServerResync, "DisabledExternally", + "MCP server config %s was disabled in Coder. The controller does not enable it again until the spec changes.", id) + case applied && !oidc && (active.apiKey && !found.HasAPIKey || active.headers && !found.HasCustomHeaders || active.secret && !found.HasOAuth2Secret): + // A dashboard change of the auth type lands here too: Coder clears + // the credentials of the old type (plan F9). + kill = mcpServerStalled(mcpServerResync, "CredentialClearedExternally", + "A credential of MCP server config %s was cleared in Coder (auth type %s). The controller does not send it again until the spec changes.", id, found.AuthType) + } + if kill != nil && !oidc { + return kill, false, nil + } + full, err := mcpServerUpdateRequest(server, values) if err != nil { return nil, false, err } - if req := mcpServerSparseRequest(server, full, found, values); req != (codersdk.UpdateMCPServerConfigRequest{}) { - if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { - return step, false, err + req, sent := r.mcpServerSparseRequest(server, full, found, values) + if kill != nil { + req = mcpServerAuthOnly(req) + } + if req != (codersdk.UpdateMCPServerConfigRequest{}) { + if kill == nil { + if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, false, err + } } updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) switch status := coderStatus(err); { @@ -74,12 +104,21 @@ func syncMCPServer( // so the GET of the next reconcile decides whether the config is gone. return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update MCP server config %s: %s", id, coderErrorSummary(err)), false, nil } - if field := mcpServerMismatch(server.Spec.Auth, full, updated); field != "" || updated.ID != id { + check := full + if kill != nil { + check = mcpServerAuthOnly(full) + } + if field := mcpServerMismatch(server.Spec.Auth, check, updated); field != "" || updated.ID != id { return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with another %s", id, cmp.Or(field, "ID")), false, nil } + r.rememberSentCredentials(server, sent, values) } else if field := mcpServerMismatch(server.Spec.Auth, full, found); field != "" { return nil, false, fmt.Errorf("assertion failed: MCP server %s/%s differs in %s, but the sync request is empty", server.Namespace, server.Name, field) } + if kill != nil { + kill.message = "The controller set the auth type back from user_oidc. " + kill.message + return kill, false, nil + } if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { return step, false, err } @@ -97,11 +136,12 @@ func mcpServerActiveCredentials(auth coderv1alpha1.CoderMCPServerAuth) mcpServer // mcpServerSparseRequest drops every field of full that found already holds. // It keeps a credential when a URL or the auth type changes, when its change -// token changed, or when Coder lost it. It clears credentials of the other -// auth types that Coder holds (#314). -func mcpServerSparseRequest( +// token changed and Coder does not hold the value already, or when Coder lost +// it. It clears credentials of the other auth types that Coder holds (#314). +// It returns the credential fields that the request sends. +func (r *CoderMCPServerReconciler) mcpServerSparseRequest( server *coderv1alpha1.CoderMCPServer, full codersdk.UpdateMCPServerConfigRequest, found codersdk.MCPServerConfig, values map[string]credentialsecret.Value, -) codersdk.UpdateMCPServerConfigRequest { +) (codersdk.UpdateMCPServerConfigRequest, []string) { req := full authChanged := found.AuthType != *full.AuthType for _, f := range mcpServerStringFields(&req, found) { @@ -125,7 +165,11 @@ func mcpServerSparseRequest( } token := values[field].Token c, ok := recorded[field] - return !ok || c.SecretName != token.SecretName || c.SecretKey != token.SecretKey || c.SecretUID != string(token.SecretUID) || c.SecretResourceVersion != token.ResourceVersion + if ok && c.SecretName == token.SecretName && c.SecretKey == token.SecretKey && c.SecretUID == string(token.SecretUID) && c.SecretResourceVersion == token.ResourceVersion { + return false + } + // Only the Secret's metadata changed when Coder holds this value. + return !r.sentCredentialMatches(server, field, values[field]) } var sent []string for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { @@ -138,8 +182,13 @@ func mcpServerSparseRequest( sends := func(prefix string) bool { return slices.ContainsFunc(sent, func(f string) bool { return strings.HasPrefix(f, prefix) }) } - // Custom headers are one Coder field, so they all go together. - if !active.headers || !sends("header:") { + if active.headers && sends("header:") { + // Custom headers are one Coder field, so they all go together. + sent = slices.DeleteFunc(sent, func(f string) bool { return strings.HasPrefix(f, "header:") }) + for _, h := range server.Spec.Auth.CustomHeaders { + sent = append(sent, "header:"+h.Name) + } + } else { req.CustomHeaders = nil } if !active.apiKey || !sends("apiKey") { @@ -159,7 +208,16 @@ func mcpServerSparseRequest( if !active.secret && found.HasOAuth2Secret { req.OAuth2ClientSecret = &empty } - return req + return req, sent +} + +// mcpServerAuthOnly keeps only the auth fields of req. +func mcpServerAuthOnly(req codersdk.UpdateMCPServerConfigRequest) codersdk.UpdateMCPServerConfigRequest { + return codersdk.UpdateMCPServerConfigRequest{ + AuthType: req.AuthType, APIKeyHeader: req.APIKeyHeader, APIKeyValue: req.APIKeyValue, CustomHeaders: req.CustomHeaders, + OAuth2ClientID: req.OAuth2ClientID, OAuth2ClientSecret: req.OAuth2ClientSecret, OAuth2AuthURL: req.OAuth2AuthURL, + OAuth2TokenURL: req.OAuth2TokenURL, OAuth2RevocationURL: req.OAuth2RevocationURL, OAuth2Scopes: req.OAuth2Scopes, + } } type mcpServerStringField struct { @@ -244,6 +302,30 @@ func mcpServerMismatch(auth coderv1alpha1.CoderMCPServerAuth, want codersdk.Upda return "" } +// rememberSentCredentials keeps a SHA-256 of each credential value that Coder +// now holds, in process memory only (plan 4.6 item 4), so a metadata-only +// Secret change sends nothing. After a restart the map is empty, so one +// resend can happen. +func (r *CoderMCPServerReconciler) rememberSentCredentials(server *coderv1alpha1.CoderMCPServer, sent []string, values map[string]credentialsecret.Value) { + sums := map[string][sha256.Size]byte{} + if old, ok := r.sentCredentials.Load(server.UID); ok { + maps.Copy(sums, old.(map[string][sha256.Size]byte)) + } + for _, field := range sent { + sums[field] = sha256.Sum256([]byte(values[field].Reveal())) + } + r.sentCredentials.Store(server.UID, sums) +} + +func (r *CoderMCPServerReconciler) sentCredentialMatches(server *coderv1alpha1.CoderMCPServer, field string, value credentialsecret.Value) bool { + sums, ok := r.sentCredentials.Load(server.UID) + if !ok { + return false + } + sum, ok := sums.(map[string][sha256.Size]byte)[field] + return ok && sum == sha256.Sum256([]byte(value.Reveal())) +} + // mcpServerReady reports a config that matches the spec. func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { return &mcpServerStep{ diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index cffc297a7..cb4a4c942 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -6,6 +6,7 @@ import ( "fmt" "net/url" "strings" + "sync" "time" "unicode/utf8" @@ -52,6 +53,10 @@ const ( type CoderMCPServerReconciler struct { client.Client Scheme *runtime.Scheme + + // sentCredentials maps types.UID -> credential field -> SHA-256 of the + // value that Coder holds. Memory only (plan 4.6 item 4). + sentCredentials sync.Map } // mcpServerStep is the outcome of one reconcile. A ready step reports that @@ -230,7 +235,7 @@ func (r *CoderMCPServerReconciler) check( *before = *server.Status.DeepCopy() } var recreate bool - if step, recreate, err = syncMCPServer(ctx, sdk, server, org.ID, values); err != nil || !recreate { + if step, recreate, err = r.syncMCPServer(ctx, sdk, server, org.ID, values); err != nil || !recreate { break } } diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index de5fd6bc8..04f1d49ff 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -141,6 +141,11 @@ func (r *CoderMCPServerReconciler) createMCPServer( "The create answer does not match the request. The controller reads Coder instead of sending it again."), nil } server.Status.ConfigID, server.Status.CreateAttemptTime = created.ID.String(), nil + var sent []string + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + sent = append(sent, c.field) + } + r.rememberSentCredentials(server, sent, values) return nil, nil case status == 0 && createHadNoEffect(err): // A failed dial or TLS handshake never sent the request. diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index 217b3e4ae..8d3db7d56 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -2,6 +2,8 @@ package controller_test import ( "context" + "encoding/json" + "net/http" "slices" "testing" "time" @@ -9,9 +11,11 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" "github.com/stretchr/testify/require" + 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" ) // dashboard is a Coder client for the edits that an admin makes in the @@ -47,6 +51,10 @@ func (env mcpServerEnv) stored(t *testing.T, server *coderv1alpha1.CoderMCPServe return s } +func headerMCPSecret(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.secret(ctx, t, "tracker-mcp", map[string]string{"x-api-key": testMCPKey}, testMCPOrigin) +} + func oauth2MCPSecret(ctx context.Context, t *testing.T, env mcpServerEnv) { env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin) } @@ -174,3 +182,153 @@ func TestCoderMCPServerSyncDeletedConfig(t *testing.T) { require.True(t, stored.view.Enabled) require.Equal(t, testMCPKey, stored.apiKeyValue) } + +// Plan S14: after a full apply, a dashboard disable, credential clear or +// auth type change is a kill switch, like the delete of +// TestCoderMCPServerSyncDeletedConfig. It gives Stalled=True and no write +// until the spec changes. A spec edit re-arms the object, and an apply that +// fails keeps the old applied generation. +func TestCoderMCPServerSyncKillSwitches(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name, reason string + edit codersdk.UpdateMCPServerConfigRequest + }{ + {name: "disable", reason: "DisabledExternally", edit: codersdk.UpdateMCPServerConfigRequest{Enabled: ptrTo(false)}}, + {name: "credential clear", reason: "CredentialClearedExternally", edit: codersdk.UpdateMCPServerConfigRequest{APIKeyValue: ptrTo("")}}, + {name: "auth type change", reason: "CredentialClearedExternally", edit: codersdk.UpdateMCPServerConfigRequest{AuthType: ptrTo("none")}}, + } { + 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) + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, tc.edit) + require.NoError(t, err) + mark := len(env.writeRoutes()) + for range 2 { + var result ctrl.Result + got, result = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, tc.reason) + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no Kubernetes event follows a dashboard edit") + } + require.Empty(t, env.writesSince(mark), "a kill switch holds every write") + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "re-armed" + 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.Equal(t, got.Generation-1, got.Status.AppliedGeneration) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + stored := env.stored(t, got) + require.True(t, stored.view.Enabled) + require.Equal(t, "api_key", stored.view.AuthType) + require.Equal(t, testMCPKey, stored.apiKeyValue) + require.Equal(t, "re-armed", stored.view.Description) + }) + } +} + +// Plan 4.7 item 4 and S19: a dashboard switch to user_oidc is always set +// back, also when it cleared the credentials. The one PATCH carries only +// auth fields, so a config that is also disabled stays disabled. +func TestCoderMCPServerSyncUserOIDC(t *testing.T) { + t.Parallel() + authFields := []string{ + "auth_type", "api_key_header", "api_key_value", "custom_headers", "oauth2_client_id", "oauth2_client_secret", + "oauth2_auth_url", "oauth2_token_url", "oauth2_revocation_url", "oauth2_scopes", + } + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + secrets func(context.Context, *testing.T, mcpServerEnv) + disabled bool + }{ + {name: "none", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} }}, + {name: "api key"}, + {name: "api key, also disabled", disabled: true}, + {name: "custom headers", secrets: headerMCPSecret, spec: func(s *coderv1alpha1.CoderMCPServerSpec) { useCustomHeaders(s, "X-Api-Key") }}, + {name: "oauth2", secrets: oauth2MCPSecret, spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + useOAuth2(s, func(o *coderv1alpha1.CoderMCPServerOAuth2Auth) { o.Scopes = []string{"read"} }) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + if tc.secrets == nil { + tc.secrets = func(ctx context.Context, t *testing.T, env mcpServerEnv) { env.keySecret(ctx, t, testMCPKey) } + } + tc.secrets(ctx, t, env) + got, id := env.applied(ctx, t, tc.spec) + want := env.stored(t, got) + + edit := codersdk.UpdateMCPServerConfigRequest{AuthType: ptrTo("user_oidc")} + if tc.disabled { + edit.Enabled, want.view.Enabled = ptrTo(false), false + } + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, edit) + require.NoError(t, err) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServer, writes[0].route) + var body map[string]json.RawMessage + require.NoError(t, json.Unmarshal(writes[0].body, &body)) + for field := range body { + require.Contains(t, authFields, field, "the PATCH carries only auth fields") + } + require.JSONEq(t, `"`+got.Spec.Auth.Type+`"`, string(body["auth_type"])) + require.Equal(t, want, env.stored(t, got)) + if tc.disabled { + require.Contains(t, requireMCPServerStep(t, got, true, "DisabledExternally"), "set the auth type back from user_oidc") + } else { + requireMCPServerApplied(t, got) + } + + mark = len(env.writeRoutes()) + env.reconcile(ctx, t, got) + require.Empty(t, env.writesSince(mark)) + }) + } +} + +// Plan 4.6 item 4 and S13: a metadata-only Secret change moves the change +// token. Status records the new token and nothing is sent. A restart empties +// the in-memory hashes, so the next such change resends the value once. +func TestCoderMCPServerSyncSecretMetadataChange(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) + touch := func(note string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Annotations["example.com/note"] = note + require.NoError(t, k8sClient.Update(ctx, secret)) + } + + touch("one") + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Empty(t, env.writesSince(mark)) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) + + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + touch("two") + mark = len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"api_key_value":"`+testMCPKey+`"}`, string(writes[0].body)) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) +} From f693edeb858c476b9b6656c9915bd6bf993d8672 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:19:11 +0000 Subject: [PATCH 3/7] =?UTF-8?q?=F0=9F=A4=96=20feat:=20disable=20the=20Code?= =?UTF-8?q?rMCPServer=20config=20when=20a=20credential=20Secret=20withdraw?= =?UTF-8?q?s=20consent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a credential read fails for a config that this object created, the controller checks whether every credential that Coder holds still comes from a recorded Secret that allows the URLs Coder holds. If not, it disables the config in Coder with one PATCH that holds only enabled: false. A spec edit is needed to enable it again. Fixes #311 Refs #155 Change-Id: I37c5933cb4d3abd50351308e454f8d6c09499fc9 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 4 + .../codermcpserver_controller_test.go | 15 +- internal/controller/codermcpserver_disable.go | 130 ++++++++++++++ .../controller/codermcpserver_disable_test.go | 160 ++++++++++++++++++ 4 files changed, 307 insertions(+), 2 deletions(-) create mode 100644 internal/controller/codermcpserver_disable.go create mode 100644 internal/controller/codermcpserver_disable_test.go diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index cb4a4c942..9d7e25f4e 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -218,6 +218,10 @@ func (r *CoderMCPServerReconciler) check( 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) + } if step != nil || err != nil { return step, err } diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index ea5b18262..099d6bf52 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -586,6 +586,17 @@ 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. + rearm := func(description string) { + t.Helper() + requireReason("DisabledExternally") + got := &coderv1alpha1.CoderMCPServer{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), got)) + got.Spec.Description = description + require.NoError(t, k8sClient.Update(ctx, got)) + requireReason("Applied") + } require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(tenant), tenant)) tenant.Labels = nil @@ -593,7 +604,7 @@ func TestCoderMCPServerWatches(t *testing.T) { requireReason(credentialsecret.ReasonNotOptedIn) tenant.Labels = map[string]string{credentialsecret.LabelKey: credentialsecret.LabelValue} require.NoError(t, k8sClient.Update(ctx, tenant)) - requireReason("Applied") + rearm("one") env.setOptIn(ctx, t, nil) requireReason("MCPServersNotAllowed") @@ -605,7 +616,7 @@ func TestCoderMCPServerWatches(t *testing.T) { other := createControlPlaneUsing(ctx, t, env.namespace, "license", tenant.Name) requireReason(credentialsecret.ReasonRefused) require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: other, Namespace: env.namespace}})) - requireReason("Applied") + rearm("two") require.NoError(t, k8sClient.Delete(ctx, key)) requireReason(credentialsecret.ReasonSecretNotFound) diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go new file mode 100644 index 000000000..eb63522bd --- /dev/null +++ b/internal/controller/codermcpserver_disable.go @@ -0,0 +1,130 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// disableMCPServerWithoutConsent handles a config that this object created +// when a credential read fails (#311, plan G8). Coder keeps the stored +// credentials and sends them to the URLs that Coder holds. Unless every +// credential that Coder holds comes from a recorded Secret that still passes +// the contract for those URLs, the controller disables the config in Coder +// with one PATCH that holds no credential and no destination field. +// +// It fails closed on every stalled credential failure, as CoderAIProvider +// does (#290): a deleted Secret, a removed opt-in label or origin, and also +// a missing or malformed value. The Secret then no longer proves consent for +// the credential that Coder uses. +// +// The disable is a kill switch like a dashboard disable: appliedGeneration +// stays, so a later sync reports DisabledExternally and only a spec edit +// enables the config again. +func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( + 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 credential failure to be disabled", server.Namespace, server.Name) + } + // Status messages are cut at 256 characters, so the new fact goes first. + 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 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 isCoderNotFound(err): + 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 + } + ok, err := r.mcpServerConsentHolds(ctx, server, found) + if err != nil { + return nil, err + } + if ok { + return stalled("Coder sends each credential of MCP server config %s only to origins that its recorded Secret allows.", id), nil + } + + 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 + } + if updated.ID != id || 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 + } + // 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 +} + +// mcpServerConsentHolds reports whether every credential that found holds +// comes from a Secret in status.credentials that still passes the contract, +// with the same UID, for each URL of found that receives it (plan 5.2). +// Coder holds the value of that Secret, so the spec's current refs and URLs +// do not matter here. +func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig) (bool, error) { + toURL := []string{found.URL} + toOAuth2 := []string{found.URL, found.OAuth2AuthURL, found.OAuth2TokenURL} + if found.OAuth2RevocationURL != "" { + toOAuth2 = append(toOAuth2, found.OAuth2RevocationURL) + } + held := []struct { + held bool + match func(field string) bool + destinations []string + }{ + {found.HasAPIKey, func(f string) bool { return f == "apiKey" }, toURL}, + {found.HasCustomHeaders, func(f string) bool { return strings.HasPrefix(f, "header:") }, toURL}, + {found.HasOAuth2Secret, func(f string) bool { return f == "oauth2ClientSecret" }, toOAuth2}, + } + for _, h := range held { + if !h.held { + continue + } + recorded := 0 + for _, c := range server.Status.Credentials { + if !h.match(c.Field) { + continue + } + recorded++ + for _, raw := range h.destinations { + destination, err := aiProviderBaseURL(raw) + if err != nil { + return false, nil //nolint:nilerr // An origin that does not parse has no proof of consent. + } + 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) || err == nil && string(value.Token.SecretUID) != c.SecretUID { + return false, nil + } else if err != nil { + return false, err + } + } + } + if recorded == 0 { + // A credential without a recorded Secret has no proof of consent. + return false, nil + } + } + return true, nil +} diff --git a/internal/controller/codermcpserver_disable_test.go b/internal/controller/codermcpserver_disable_test.go new file mode 100644 index 000000000..7a1d09107 --- /dev/null +++ b/internal/controller/codermcpserver_disable_test.go @@ -0,0 +1,160 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// Plan G8 and S12 (#311): when a credential Secret withdraws consent, the +// controller disables the config in Coder with one PATCH that holds no +// credential and no destination field. The disable is a kill switch: the +// restored consent does not undo it, and a spec edit enables the config. +func TestCoderMCPServerDisablesWithoutConsent(t *testing.T) { + t.Parallel() + oauth2 := func(s *coderv1alpha1.CoderMCPServerSpec) { useOAuth2(s, nil) } + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + secret func(context.Context, *testing.T, mcpServerEnv) *corev1.Secret + edit func(*corev1.Secret) // nil deletes the Secret. + reason string + }{ + {name: "Secret deleted", reason: credentialsecret.ReasonSecretNotFound}, + {name: "label removed", reason: credentialsecret.ReasonNotOptedIn, edit: func(s *corev1.Secret) { delete(s.Labels, credentialsecret.LabelKey) }}, + {name: "origin removed", reason: credentialsecret.ReasonDestinationNotAllowed, edit: func(s *corev1.Secret) { + s.Annotations[credentialsecret.DestinationsAnnotation] = "https://other.example.com" + }}, + { + name: "OAuth2 token origin removed", reason: credentialsecret.ReasonDestinationNotAllowed, spec: oauth2, + secret: func(ctx context.Context, t *testing.T, env mcpServerEnv) *corev1.Secret { + return env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin) + }, + edit: func(s *corev1.Secret) { s.Annotations[credentialsecret.DestinationsAnnotation] = testMCPOrigin }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + if tc.secret == nil { + tc.secret = func(ctx context.Context, t *testing.T, env mcpServerEnv) *corev1.Secret { + return env.keySecret(ctx, t, testMCPKey) + } + } + secret := tc.secret(ctx, t, env) + consented := secret.DeepCopy() + got, _ := env.applied(ctx, t, tc.spec) + before := env.stored(t, got) + if tc.edit == nil { + require.NoError(t, k8sClient.Delete(ctx, secret)) + } else { + editSecret(ctx, t, secret, tc.edit) + } + + // A failed disable is retried, and the message says that Coder + // can still send the credentials. + 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 send the credentials") + 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()) + 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)) + + // Restored consent does not enable the config again. + if tc.edit == nil { + consented.ResourceVersion, consented.UID = "", "" + createSecret(ctx, t, consented) + } else { + editSecret(ctx, t, secret, func(s *corev1.Secret) { s.Labels, s.Annotations = consented.Labels, consented.Annotations }) + } + 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) + }) + } +} + +// Plan S12: a spec change to an origin that the Secret does not list sends +// nothing. The consent check uses what Coder holds: Coder keeps the old URL, +// which the recorded Secret still allows, so the config stays enabled. A +// credential that Coder holds without a recorded Secret, or a recorded +// Secret replaced by another one with the same name, has no proof of consent, +// so the config is disabled. +func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + prepare func(context.Context, *testing.T, mcpServerEnv, *corev1.Secret, uuid.UUID) + disabled bool + }{ + {name: "Coder keeps an allowed URL"}, + {name: "Coder holds an unrecorded credential", disabled: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, _ *corev1.Secret, id uuid.UUID) { + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{CustomHeaders: &map[string]string{"X-Extra": "v"}}) + require.NoError(t, err) + }}, + {name: "Secret replaced", disabled: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, secret *corev1.Secret, _ uuid.UUID) { + require.NoError(t, k8sClient.Delete(ctx, secret)) + env.keySecret(ctx, t, testMCPKey) + }}, + } { + t.Run(tc.name, func(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) + if tc.prepare != nil { + tc.prepare(ctx, t, env, secret, id) + } + before := env.stored(t, got) + + 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) + message := requireMCPServerStep(t, got, true, credentialsecret.ReasonDestinationNotAllowed) + if tc.disabled { + require.Contains(t, message, "The controller disabled MCP server config") + require.Len(t, env.writesSince(mark), 1) + before.view.Enabled = false + } else { + require.Contains(t, message, "only to origins that its recorded Secret allows") + require.Empty(t, env.writesSince(mark)) + } + require.Equal(t, before, env.stored(t, got)) + }) + } +} From c2de88cf74371cc598e7fbd04f3e89d2df9c86b4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:46:37 +0000 Subject: [PATCH 4/7] =?UTF-8?q?=F0=9F=A4=96=20fix:=20drop=20the=20hashes?= =?UTF-8?q?=20of=20cleared=20CoderMCPServer=20credentials?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PATCH that clears a credential left its hash in memory. After an auth type round trip, the hash claimed that Coder held the Secret's value while Coder held a key set in the dashboard, so the key was not sent. Refs #155 Change-Id: Id76a171929671334c958fc990bce5eec696ce132 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 11 +++++-- .../controller/codermcpserver_sync_test.go | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index e9eb44c3e..c54bfa326 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -5,7 +5,6 @@ import ( "context" "crypto/sha256" "fmt" - "maps" "net/http" "slices" "strings" @@ -305,11 +304,17 @@ func mcpServerMismatch(auth coderv1alpha1.CoderMCPServerAuth, want codersdk.Upda // rememberSentCredentials keeps a SHA-256 of each credential value that Coder // now holds, in process memory only (plan 4.6 item 4), so a metadata-only // Secret change sends nothing. After a restart the map is empty, so one -// resend can happen. +// resend can happen. It drops the hashes of fields that the spec's auth type +// does not use: the PATCH cleared those credentials, so a later round trip +// of the auth type must send them again. func (r *CoderMCPServerReconciler) rememberSentCredentials(server *coderv1alpha1.CoderMCPServer, sent []string, values map[string]credentialsecret.Value) { sums := map[string][sha256.Size]byte{} if old, ok := r.sentCredentials.Load(server.UID); ok { - maps.Copy(sums, old.(map[string][sha256.Size]byte)) + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + if sum, ok := old.(map[string][sha256.Size]byte)[c.field]; ok { + sums[c.field] = sum + } + } } for _, field := range sent { sums[field] = sha256.Sum256([]byte(values[field].Reveal())) diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index 8d3db7d56..ee2613a56 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -332,3 +332,33 @@ func TestCoderMCPServerSyncSecretMetadataChange(t *testing.T) { require.JSONEq(t, `{"api_key_value":"`+testMCPKey+`"}`, string(writes[0].body)) require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) } + +// A PATCH that clears a credential also drops its hash. Otherwise, after an +// auth type round trip, the hash would claim that Coder holds the Secret's +// value while Coder holds a key set in the dashboard. +func TestCoderMCPServerSyncAuthRoundTripResends(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) + setAuth := func(auth coderv1alpha1.CoderMCPServerAuth) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Auth = auth + require.NoError(t, k8sClient.Update(ctx, got)) + } + original := got.Spec.Auth + setAuth(coderv1alpha1.CoderMCPServerAuth{Type: "none"}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{ + AuthType: ptrTo("api_key"), APIKeyHeader: ptrTo("Authorization"), APIKeyValue: ptrTo("dashboard-key"), + }) + require.NoError(t, err) + setAuth(original) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, testMCPKey, env.stored(t, got).apiKeyValue) +} From 4e84a445ea57315b19cabd4378fdb9e6d8db3e54 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:47:55 +0000 Subject: [PATCH 5/7] =?UTF-8?q?=F0=9F=A4=96=20fix:=20disable=20a=20CoderMC?= =?UTF-8?q?PServer=20config=20that=20Coder=20switched=20to=20user=5Foidc?= =?UTF-8?q?=20when=20the=20credential=20read=20fails?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without a readable credential the controller cannot set user_oidc back, and user_oidc sends each chat user's login token to the server URL. The consent check now disables such a config. Refs #155 Change-Id: I66b394e3d13ece60d23b604333803f9537d5a4a6 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_disable.go | 14 +++++++++++--- internal/controller/codermcpserver_disable_test.go | 7 ++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index eb63522bd..d44421b96 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -20,6 +20,9 @@ import ( // the contract for those URLs, the controller disables the config in Coder // with one PATCH that holds no credential and no destination field. // +// An observed auth type user_oidc is disabled too: the controller cannot set +// it back without the credentials (plan 4.7 item 4). +// // It fails closed on every stalled credential failure, as CoderAIProvider // does (#290): a deleted Secret, a removed opt-in label or origin, and also // a missing or malformed value. The Secret then no longer proves consent for @@ -56,9 +59,14 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( if !found.Enabled { return stalled("MCP server config %s is disabled in Coder.", id), nil } - ok, err := r.mcpServerConsentHolds(ctx, server, found) - if err != nil { - return nil, err + // 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 + // it disables the config. + ok := false + if found.AuthType != "user_oidc" { + if ok, err = r.mcpServerConsentHolds(ctx, server, found); err != nil { + return nil, err + } } if ok { return stalled("Coder sends each credential of MCP server config %s only to origins that its recorded Secret allows.", id), nil diff --git a/internal/controller/codermcpserver_disable_test.go b/internal/controller/codermcpserver_disable_test.go index 7a1d09107..a7cd278ff 100644 --- a/internal/controller/codermcpserver_disable_test.go +++ b/internal/controller/codermcpserver_disable_test.go @@ -111,7 +111,8 @@ func TestCoderMCPServerDisablesWithoutConsent(t *testing.T) { // which the recorded Secret still allows, so the config stays enabled. A // credential that Coder holds without a recorded Secret, or a recorded // Secret replaced by another one with the same name, has no proof of consent, -// so the config is disabled. +// so the config is disabled. So is a config whose auth type was switched to +// user_oidc, which the controller cannot set back without the credential. func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -124,6 +125,10 @@ func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{CustomHeaders: &map[string]string{"X-Extra": "v"}}) require.NoError(t, err) }}, + {name: "Coder uses user_oidc", disabled: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, _ *corev1.Secret, id uuid.UUID) { + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{AuthType: ptrTo("user_oidc")}) + require.NoError(t, err) + }}, {name: "Secret replaced", disabled: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, secret *corev1.Secret, _ uuid.UUID) { require.NoError(t, k8sClient.Delete(ctx, secret)) env.keySecret(ctx, t, testMCPKey) From 203f73b02ed1dfa78e0829bbc88926a782808301 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:52:18 +0000 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=A4=96=20fix:=20check=20the=20CoderMC?= =?UTF-8?q?PServer=20credential=20value=20again=20in=20the=20consent=20che?= =?UTF-8?q?ck?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Secret with the same UID whose value became invalid UTF-8 or gained a CR, LF or NUL failed the credential read, but the consent check accepted it. The check now applies the same value rules as the read. Refs #155 Change-Id: I04342bc648f04d7cdcb96d0a8d673ccd0fa82353 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 24 ++++++++++++------- internal/controller/codermcpserver_disable.go | 17 +++++++++---- .../controller/codermcpserver_disable_test.go | 5 +++- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 9d7e25f4e..e4f5bf24e 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -415,22 +415,28 @@ func readMCPServerCredentials( "Secret %s changed during the reconcile. The controller reads it again.", c.ref.Name), nil } seen[c.ref.Name] = token - // Coder receives every credential inside a JSON string, and - // encoding/json replaces invalid UTF-8 with U+FFFD, so Coder would - // store another value. - if !utf8.ValidString(value.Reveal()) { + if problem := mcpServerValueProblem(value, c.headerValue); problem != "" { return nil, mcpServerStalled(0, credentialsecret.ReasonMalformed, - "%s: the value of key %s in Secret %s is not valid UTF-8.", c.field, c.ref.Key, c.ref.Name), nil - } - if c.headerValue && !isHTTPFieldValue(value.Reveal()) { - return nil, mcpServerStalled(0, credentialsecret.ReasonMalformed, - "%s: the value of key %s in Secret %s is not a valid HTTP header value.", c.field, c.ref.Key, c.ref.Name), nil + "%s: the value of key %s in Secret %s %s.", c.field, c.ref.Key, c.ref.Name, problem), nil } } } return values, nil, nil } +// mcpServerValueProblem says why Coder cannot store value, or returns "". +// Coder receives every credential inside a JSON string, and encoding/json +// replaces invalid UTF-8 with U+FFFD, so Coder would store another value. +func mcpServerValueProblem(value credentialsecret.Value, headerValue bool) string { + switch { + case !utf8.ValidString(value.Reveal()): + return "is not valid UTF-8" + case headerValue && !isHTTPFieldValue(value.Reveal()): + return "is not a valid HTTP header value" + } + return "" +} + // isHTTPFieldValue reports whether every byte of s may appear in an RFC 9110 // field value: HTAB, SP, visible ASCII, or obs-text. CR, LF and NUL may not. func isHTTPFieldValue(s string) bool { diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index d44421b96..dbd765d79 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -88,7 +88,8 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( // mcpServerConsentHolds reports whether every credential that found holds // comes from a Secret in status.credentials that still passes the contract, -// with the same UID, for each URL of found that receives it (plan 5.2). +// with the same UID and a value that Coder can store, for each URL of found +// that receives it (plan 5.2). // Coder holds the value of that Secret, so the spec's current refs and URLs // do not matter here. func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig) (bool, error) { @@ -101,10 +102,11 @@ func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, se held bool match func(field string) bool destinations []string + headerValue bool }{ - {found.HasAPIKey, func(f string) bool { return f == "apiKey" }, toURL}, - {found.HasCustomHeaders, func(f string) bool { return strings.HasPrefix(f, "header:") }, toURL}, - {found.HasOAuth2Secret, func(f string) bool { return f == "oauth2ClientSecret" }, toOAuth2}, + {found.HasAPIKey, func(f string) bool { return f == "apiKey" }, toURL, true}, + {found.HasCustomHeaders, func(f string) bool { return strings.HasPrefix(f, "header:") }, toURL, true}, + {found.HasOAuth2Secret, func(f string) bool { return f == "oauth2ClientSecret" }, toOAuth2, false}, } for _, h := range held { if !h.held { @@ -122,11 +124,16 @@ func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, se return false, nil //nolint:nilerr // An origin that does not parse has no proof of consent. } 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) || err == nil && string(value.Token.SecretUID) != c.SecretUID { + if credErr := (*credentialsecret.Error)(nil); errors.As(err, &credErr) { return false, nil } else if err != nil { return false, err } + // A malformed value also withdraws consent, as in the read of + // readMCPServerCredentials. + if string(value.Token.SecretUID) != c.SecretUID || mcpServerValueProblem(value, h.headerValue) != "" { + return false, nil + } } } if recorded == 0 { diff --git a/internal/controller/codermcpserver_disable_test.go b/internal/controller/codermcpserver_disable_test.go index a7cd278ff..19fdcd5bd 100644 --- a/internal/controller/codermcpserver_disable_test.go +++ b/internal/controller/codermcpserver_disable_test.go @@ -35,6 +35,7 @@ func TestCoderMCPServerDisablesWithoutConsent(t *testing.T) { {name: "origin removed", reason: credentialsecret.ReasonDestinationNotAllowed, edit: func(s *corev1.Secret) { s.Annotations[credentialsecret.DestinationsAnnotation] = "https://other.example.com" }}, + {name: "value malformed", reason: credentialsecret.ReasonMalformed, edit: func(s *corev1.Secret) { s.Data["token"] = []byte(testMCPKey + "\nx") }}, { name: "OAuth2 token origin removed", reason: credentialsecret.ReasonDestinationNotAllowed, spec: oauth2, secret: func(ctx context.Context, t *testing.T, env mcpServerEnv) *corev1.Secret { @@ -90,7 +91,9 @@ func TestCoderMCPServerDisablesWithoutConsent(t *testing.T) { consented.ResourceVersion, consented.UID = "", "" createSecret(ctx, t, consented) } else { - editSecret(ctx, t, secret, func(s *corev1.Secret) { s.Labels, s.Annotations = consented.Labels, consented.Annotations }) + editSecret(ctx, t, secret, func(s *corev1.Secret) { + s.Labels, s.Annotations, s.Data = consented.Labels, consented.Annotations, consented.Data + }) } got, _ = env.reconcile(ctx, t, got) requireMCPServerStep(t, got, true, "DisabledExternally") From 8f0a048d14a72b4fc19f1c8690665447f2f4c9f6 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 06:07:33 +0000 Subject: [PATCH 7/7] =?UTF-8?q?=F0=9F=A4=96=20fix:=20tighten=20the=20Coder?= =?UTF-8?q?MCPServer=20consent=20check=20and=20hold=20a=20pending=20genera?= =?UTF-8?q?tion=20after=20the=20disable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The consent check requires the spec to name the recorded Secrets, so a credential write whose answer was lost cannot pass on the old Secret. - All reads of one Secret in the check must see one resourceVersion. - The disable answer must be for the same organization and slug. - The disable sets appliedGeneration, so a generation that was not applied yet does not enable the config after consent comes back. Refs #155 Change-Id: I4b0ee409911f9edb18cf43e1552f761ed413b2bf Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_disable.go | 52 +++++++-- .../controller/codermcpserver_disable_test.go | 110 +++++++++++++++++- 2 files changed, 149 insertions(+), 13 deletions(-) diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index dbd765d79..5da84c7f2 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -28,9 +28,9 @@ import ( // a missing or malformed value. The Secret then no longer proves consent for // the credential that Coder uses. // -// The disable is a kill switch like a dashboard disable: appliedGeneration -// stays, so a later sync reports DisabledExternally and only a spec edit -// enables the config again. +// The disable is a kill switch like a dashboard disable: it sets +// 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, ) (*mcpServerStep, error) { @@ -64,7 +64,10 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( // it disables the config. ok := false if found.AuthType != "user_oidc" { - if ok, err = r.mcpServerConsentHolds(ctx, server, found); err != nil { + ok, err = r.mcpServerConsentHolds(ctx, server, found) + if errors.Is(err, errMCPServerSecretChanged) { + return unchecked("a Secret changed during the consent check, so the controller reads it again"), nil + } else if err != nil { return nil, err } } @@ -77,22 +80,42 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( if err != nil { return unchecked("the disable failed: %s", coderErrorSummary(err)), nil } - if updated.ID != id || updated.Enabled { + 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 } + // 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 } +// errMCPServerSecretChanged reports that a Secret changed between two reads +// of one consent check. +var errMCPServerSecretChanged = errors.New("a Secret changed during the consent check") + // mcpServerConsentHolds reports whether every credential that found holds // comes from a Secret in status.credentials that still passes the contract, // with the same UID and a value that Coder can store, for each URL of found -// that receives it (plan 5.2). -// Coder holds the value of that Secret, so the spec's current refs and URLs -// do not matter here. +// that receives it (plan 5.2). Coder holds the value of that Secret, so the +// spec's current URLs do not matter here. +// +// The spec must still name the recorded Secrets: a PATCH whose answer was +// lost after the commit can have sent a Secret of the spec that status does +// not record yet. CoderAIProvider reads the spec's ref for the same reason. func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig) (bool, error) { + recorded := map[string]coderv1alpha1.CoderMCPServerCredentialStatus{} + for _, c := range server.Status.Credentials { + recorded[c.Field] = c + } + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + if got, ok := recorded[c.field]; !ok || got.SecretName != c.ref.Name || got.SecretKey != c.ref.Key { + return false, nil + } + } toURL := []string{found.URL} toOAuth2 := []string{found.URL, found.OAuth2AuthURL, found.OAuth2TokenURL} if found.OAuth2RevocationURL != "" { @@ -108,16 +131,17 @@ func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, se {found.HasCustomHeaders, func(f string) bool { return strings.HasPrefix(f, "header:") }, toURL, true}, {found.HasOAuth2Secret, func(f string) bool { return f == "oauth2ClientSecret" }, toOAuth2, false}, } + seen := map[string]credentialsecret.ChangeToken{} for _, h := range held { if !h.held { continue } - recorded := 0 + matched := 0 for _, c := range server.Status.Credentials { if !h.match(c.Field) { continue } - recorded++ + matched++ for _, raw := range h.destinations { destination, err := aiProviderBaseURL(raw) if err != nil { @@ -134,9 +158,15 @@ func (r *CoderMCPServerReconciler) mcpServerConsentHolds(ctx context.Context, se if string(value.Token.SecretUID) != c.SecretUID || mcpServerValueProblem(value, h.headerValue) != "" { return false, nil } + // All reads of one Secret must see one Secret state, as in + // readMCPServerCredentials. + if first, ok := seen[c.SecretName]; ok && first.ResourceVersion != value.Token.ResourceVersion { + return false, errMCPServerSecretChanged + } + seen[c.SecretName] = value.Token } } - if recorded == 0 { + if matched == 0 { // A credential without a recorded Secret has no proof of consent. return false, nil } diff --git a/internal/controller/codermcpserver_disable_test.go b/internal/controller/codermcpserver_disable_test.go index 19fdcd5bd..3644dba52 100644 --- a/internal/controller/codermcpserver_disable_test.go +++ b/internal/controller/codermcpserver_disable_test.go @@ -3,6 +3,7 @@ package controller_test import ( "context" "net/http" + "sync/atomic" "testing" "time" @@ -11,6 +12,7 @@ import ( "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" "github.com/coder/coder-k8s/internal/credentialsecret" @@ -121,9 +123,16 @@ func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { for _, tc := range []struct { name string prepare func(context.Context, *testing.T, mcpServerEnv, *corev1.Secret, uuid.UUID) + edit func(*coderv1alpha1.CoderMCPServerSpec) // nil moves the URL to an unlisted origin. + reason string // "" is CredentialDestinationNotAllowed. disabled bool }{ {name: "Coder keeps an allowed URL"}, + { + // A PATCH whose answer was lost can have sent the new Secret. + name: "spec names another Secret", disabled: true, reason: credentialsecret.ReasonSecretNotFound, + edit: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Auth.APIKey.SecretRef.Name = "other-mcp" }, + }, {name: "Coder holds an unrecorded credential", disabled: true, prepare: func(ctx context.Context, t *testing.T, env mcpServerEnv, _ *corev1.Secret, id uuid.UUID) { _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{CustomHeaders: &map[string]string{"X-Extra": "v"}}) require.NoError(t, err) @@ -148,12 +157,18 @@ func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { } before := env.stored(t, got) + if tc.edit == nil { + tc.edit = func(s *coderv1alpha1.CoderMCPServerSpec) { s.URL = "https://other.example.com/mcp" } + } + if tc.reason == "" { + tc.reason = credentialsecret.ReasonDestinationNotAllowed + } require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) - got.Spec.URL = "https://other.example.com/mcp" + tc.edit(&got.Spec) require.NoError(t, k8sClient.Update(ctx, got)) mark := len(env.writeRoutes()) got, _ = env.reconcile(ctx, t, got) - message := requireMCPServerStep(t, got, true, credentialsecret.ReasonDestinationNotAllowed) + message := requireMCPServerStep(t, got, true, tc.reason) if tc.disabled { require.Contains(t, message, "The controller disabled MCP server config") require.Len(t, env.writesSince(mark), 1) @@ -166,3 +181,94 @@ func TestCoderMCPServerConsentCheckUsesCoderState(t *testing.T) { }) } } + +// A disable while a newer generation is not applied yet also holds that +// generation: restored consent does not apply it, and only a spec edit after +// the disable enables the config. +func TestCoderMCPServerDisableHoldsPendingGeneration(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + consented := secret.DeepCopy() + got, _ := env.applied(ctx, t, nil) + edit := func(description string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = description + require.NoError(t, k8sClient.Update(ctx, got)) + } + edit("pending") + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + + require.NoError(t, k8sClient.Delete(ctx, secret)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, credentialsecret.ReasonSecretNotFound) + require.False(t, env.stored(t, got).view.Enabled) + + consented.ResourceVersion, consented.UID = "", "" + createSecret(ctx, t, consented) + 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") + + edit("after the disable") + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.stored(t, got).view.Enabled) +} + +// The disable answer must be for the same config, as the GET before it. +func TestCoderMCPServerDisableChecksTheAnswer(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)) + env.fake.failNext(routeUpdateMCPServer, rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Slug = "other" })) + got, result := env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderAnswerMismatch") + require.Equal(t, 30*time.Second, result.RequeueAfter) +} + +// All reads of one Secret in the consent check must see one Secret state, as +// in the credential read. A change between them gives a retry and no write. +func TestCoderMCPServerConsentCheckNeedsOneSecretState(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + oauth2MCPSecret(ctx, t, env) + got, _ := env.applied(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { useOAuth2(s, nil) }) + 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)) + + // Read 1 is the failed credential read for the new URL. Reads 2 and 3 are + // the consent check for Coder's URL and authorize URL. Read 3 sees another + // resourceVersion, so the check stops there. + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + var gets 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 err := c.Get(ctx, key, obj, opts...); err != nil { + return err + } + if s, ok := obj.(*corev1.Secret); ok && key.Name == "tracker-oauth" && gets.Add(1) == 3 { + s.ResourceVersion += "0" + } + return nil + }, + }) + mark := len(env.writeRoutes()) + got, result := env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, credentialsecret.ReasonDestinationNotAllowed), "a Secret changed during the consent check") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Equal(t, int32(3), gets.Load()) + require.Empty(t, env.writesSince(mark)) +}