From 0507afef8949f5cb53f447b8d0e1bee44aabca6f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:17:26 +0000 Subject: [PATCH 1/5] =?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/5] =?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 c2de88cf74371cc598e7fbd04f3e89d2df9c86b4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:46:37 +0000 Subject: [PATCH 3/5] =?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 bbd8cbb23993e567852e520d2977eeed62723bbd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:12:10 +0000 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A4=96=20fix:=20forget=20the=20CoderM?= =?UTF-8?q?CPServer=20credential=20hashes=20before=20a=20PATCH=20that=20se?= =?UTF-8?q?nds=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A credential PATCH can commit in Coder while its answer is lost. The hashes were updated only after a good answer, so after a Secret change from A to B (lost answer) and back to A, the retry matched the old hash for A and sent nothing while Coder held B. The controller now drops the hashes of the fields a PATCH sends before the PATCH. Refs #155 Change-Id: Id641bb5a2efa10a5000cf4108339a2e4bdaa96ea Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 19 ++++++++++ .../controller/codermcpserver_sync_test.go | 35 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index c54bfa326..965441512 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -5,6 +5,7 @@ import ( "context" "crypto/sha256" "fmt" + "maps" "net/http" "slices" "strings" @@ -93,6 +94,10 @@ func (r *CoderMCPServerReconciler) syncMCPServer( return step, false, err } } + // Coder can commit the PATCH and the answer still get lost. Until a + // good answer, the controller does not know which value Coder holds, + // so a retry must send these credentials again. + r.forgetSentCredentials(server, sent) updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) switch status := coderStatus(err); { case status == http.StatusBadRequest: @@ -322,6 +327,20 @@ func (r *CoderMCPServerReconciler) rememberSentCredentials(server *coderv1alpha1 r.sentCredentials.Store(server.UID, sums) } +// forgetSentCredentials drops the hashes of fields before a write that sends +// them. +func (r *CoderMCPServerReconciler) forgetSentCredentials(server *coderv1alpha1.CoderMCPServer, fields []string) { + old, ok := r.sentCredentials.Load(server.UID) + if !ok || len(fields) == 0 { + return + } + sums := maps.Clone(old.(map[string][sha256.Size]byte)) + for _, field := range fields { + delete(sums, field) + } + 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 { diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index ee2613a56..50d08cb0c 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -362,3 +362,38 @@ func TestCoderMCPServerSyncAuthRoundTripResends(t *testing.T) { requireMCPServerApplied(t, got) require.Equal(t, testMCPKey, env.stored(t, got).apiKeyValue) } + +// A credential PATCH can commit in Coder while its answer is lost. The +// controller then does not know which value Coder holds, so the retry sends +// the credential even when its value matches an earlier hash: here the +// Secret goes from A to B, the PATCH with B commits without an answer, and +// the Secret goes back to A. +func TestCoderMCPServerSyncLostAnswerResends(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) + setKey := func(value string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(value) + require.NoError(t, k8sClient.Update(ctx, secret)) + } + + rotated := testMCPKey + "-rotated" + setKey(rotated) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, rotated, env.stored(t, got).apiKeyValue, "the PATCH committed") + + setKey(testMCPKey) + 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, testMCPKey, env.stored(t, got).apiKeyValue) +} From 632a76863f1f15ae1eae621007d81046ba09fa86 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:38:20 +0000 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A4=96=20fix:=20send=20the=20CoderMCP?= =?UTF-8?q?Server=20url=20with=20the=20user=5Foidc=20correction=20and=20dr?= =?UTF-8?q?op=20stale=20hashes=20on=20every=20sync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The user_oidc correction while a kill switch holds sent the credentials without url. A dashboard edit of url, user_oidc and a disable then made the controller store the Secret's credential next to a url the Secret does not allow. The auth-only PATCH now carries url too. - A change of the auth type whose answer was lost kept the old field's hash, because the retry found nothing to send. Every sync now drops the hashes of fields that the spec's auth type does not use. Refs #155 Change-Id: Ie46ffe4d585ddc522a213094ca190964d0e67a3b Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 28 ++++++- .../controller/codermcpserver_sync_test.go | 74 ++++++++++++------- 2 files changed, 73 insertions(+), 29 deletions(-) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index 965441512..e4bcb1980 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -32,7 +32,7 @@ import ( // 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. +// is always set back, with only the auth fields and url, 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. @@ -84,6 +84,9 @@ func (r *CoderMCPServerReconciler) syncMCPServer( if err != nil { return nil, false, err } + // A hash is only for a field of the spec's auth type: a change of the + // auth type cleared the others in Coder, also when its answer was lost. + r.pruneSentCredentials(server) req, sent := r.mcpServerSparseRequest(server, full, found, values) if kill != nil { req = mcpServerAuthOnly(req) @@ -215,10 +218,13 @@ func (r *CoderMCPServerReconciler) mcpServerSparseRequest( return req, sent } -// mcpServerAuthOnly keeps only the auth fields of req. +// mcpServerAuthOnly keeps only the auth fields of req and url. The auth +// fields can carry credentials, so url goes with them: Coder must never +// store a credential next to a url that the Secret does not allow (plan 4.6 +// item 5). func mcpServerAuthOnly(req codersdk.UpdateMCPServerConfigRequest) codersdk.UpdateMCPServerConfigRequest { return codersdk.UpdateMCPServerConfigRequest{ - AuthType: req.AuthType, APIKeyHeader: req.APIKeyHeader, APIKeyValue: req.APIKeyValue, CustomHeaders: req.CustomHeaders, + URL: req.URL, 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, } @@ -327,6 +333,22 @@ func (r *CoderMCPServerReconciler) rememberSentCredentials(server *coderv1alpha1 r.sentCredentials.Store(server.UID, sums) } +// pruneSentCredentials drops the hashes of fields that the spec's auth type +// does not use. +func (r *CoderMCPServerReconciler) pruneSentCredentials(server *coderv1alpha1.CoderMCPServer) { + old, ok := r.sentCredentials.Load(server.UID) + if !ok { + return + } + sums := 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 + } + } + r.sentCredentials.Store(server.UID, sums) +} + // forgetSentCredentials drops the hashes of fields before a write that sends // them. func (r *CoderMCPServerReconciler) forgetSentCredentials(server *coderv1alpha1.CoderMCPServer, fields []string) { diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index 50d08cb0c..e260da3dc 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -240,7 +240,7 @@ func TestCoderMCPServerSyncKillSwitches(t *testing.T) { 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", + "url", "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 { @@ -248,10 +248,12 @@ func TestCoderMCPServerSyncUserOIDC(t *testing.T) { spec func(*coderv1alpha1.CoderMCPServerSpec) secrets func(context.Context, *testing.T, mcpServerEnv) disabled bool + movedURL bool // The dashboard also moves url to another origin. }{ {name: "none", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} }}, {name: "api key"}, {name: "api key, also disabled", disabled: true}, + {name: "api key, also disabled and moved", disabled: true, movedURL: 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"} }) @@ -272,6 +274,9 @@ func TestCoderMCPServerSyncUserOIDC(t *testing.T) { if tc.disabled { edit.Enabled, want.view.Enabled = ptrTo(false), false } + if tc.movedURL { + edit.URL = ptrTo("https://evil.example.net/mcp") + } _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, edit) require.NoError(t, err) mark := len(env.writeRoutes()) @@ -285,6 +290,9 @@ func TestCoderMCPServerSyncUserOIDC(t *testing.T) { require.Contains(t, authFields, field, "the PATCH carries only auth fields") } require.JSONEq(t, `"`+got.Spec.Auth.Type+`"`, string(body["auth_type"])) + if tc.movedURL { + require.JSONEq(t, `"`+got.Spec.URL+`"`, string(body["url"]), "the credential goes only with the allowed url") + } 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") @@ -333,34 +341,48 @@ func TestCoderMCPServerSyncSecretMetadataChange(t *testing.T) { 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. +// A change of the auth type clears the old credential in Coder, so its +// hash goes too, also when the answer of that PATCH was lost. 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) + for _, tc := range []struct { + name string + lost bool // The PATCH to none commits, but its answer is lost. + }{{name: "answer received"}, {name: "answer lost", lost: true}} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + 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"}) + if tc.lost { + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + } + 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) + _, 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) + }) + } } // A credential PATCH can commit in Coder while its answer is lost. The