From 0507afef8949f5cb53f447b8d0e1bee44aabca6f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 05:17:26 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20feat:=20sync=20the=20CoderMCPSer?= =?UTF-8?q?ver=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) +}