From 48ea538db6d1576d3b3a2bdabe183934eb7f7b50 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 01:48:56 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20feat:=20create=20the=20Coder=20M?= =?UTF-8?q?CP=20server=20config=20for=20a=20CoderMCPServer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I6a1b09dd1e5811eaa57977b70199a075cc4b2b35 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 78 ++-- .../codermcpserver_controller_test.go | 16 +- internal/controller/codermcpserver_create.go | 246 +++++++++++++ .../controller/codermcpserver_create_test.go | 339 ++++++++++++++++++ 4 files changed, 640 insertions(+), 39 deletions(-) create mode 100644 internal/controller/codermcpserver_create.go create mode 100644 internal/controller/codermcpserver_create_test.go diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 01e88fc90..72375da88 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -172,7 +172,7 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque if err := r.patchFinalizer(ctx, server, true); err != nil { return ctrl.Result{}, err } - step, err := r.check(ctx, server) + step, err := r.check(ctx, server, before) if err != nil { return ctrl.Result{}, err } @@ -184,9 +184,12 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque } // check gets Coder access, resolves and pins the organization, applies the -// control plane's opt-in, reads the credentials, and looks up the slug. Every -// Coder call before the slug lookup is an organization lookup. -func (r *CoderMCPServerReconciler) check(ctx context.Context, server *coderv1alpha1.CoderMCPServer) (*mcpServerStep, error) { +// control plane's opt-in, reads the credentials, and then creates the config +// or records the one that this object created. Every Coder call before the +// slug lookup is an organization lookup. +func (r *CoderMCPServerReconciler) check( + ctx context.Context, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, +) (*mcpServerStep, error) { sdk, controlPlane, wait, err := controlPlaneCoderClient(ctx, r.Client, server.Namespace, server.Spec.ControlPlaneRef.Name) if err != nil { return nil, err @@ -212,9 +215,29 @@ func (r *CoderMCPServerReconciler) check(ctx context.Context, server *coderv1alp if step != nil || err != nil { return step, err } - step = lookUpMCPServerSlug(ctx, sdk, server, org.ID) + if server.Status.ConfigID == "" { + if step, err = r.createOrRecord(ctx, sdk, server, org.ID, values, before); err != nil { + return nil, err + } + if step == nil { + // 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 + } + *before = *server.Status.DeepCopy() + } + } + if step == nil { + step = mcpServerWait(mcpServerResync, "Pending", + "MCP server config %s exists in Coder, disabled. This controller version does not apply the spec yet.", server.Status.ConfigID) + } // Coder messages can echo the request, so scrub the credentials from them. - step.message = credentialsecret.Redact(step.message, values...) + all := make([]credentialsecret.Value, 0, len(values)) + for _, v := range values { + all = append(all, v) + } + step.message = credentialsecret.Redact(step.message, all...) return step, nil } @@ -339,10 +362,12 @@ func mcpServerURLs(spec coderv1alpha1.CoderMCPServerSpec) (map[string]*url.URL, } // readMCPServerCredentials reads every credential of the spec's auth type -// through the credential Secret contract, once per destination. It returns -// the values for redaction. All reads of one Secret must see the same Secret -// state, or the Secret changed during the reconcile (plan 5.2 rule 2). -func readMCPServerCredentials(ctx context.Context, reader client.Reader, server *coderv1alpha1.CoderMCPServer) ([]credentialsecret.Value, *mcpServerStep, error) { +// through the credential Secret contract, once per destination, and returns +// them by status.credentials field. All reads of one Secret must see the same +// Secret state, or the Secret changed during the reconcile (plan 5.2 rule 2). +func readMCPServerCredentials( + ctx context.Context, reader client.Reader, server *coderv1alpha1.CoderMCPServer, +) (map[string]credentialsecret.Value, *mcpServerStep, error) { auth := server.Spec.Auth switch auth.Type { case "none", "api_key", "custom_headers", "oauth2": @@ -356,7 +381,7 @@ func readMCPServerCredentials(ctx context.Context, reader client.Reader, server if step != nil { return nil, step, nil } - var values []credentialsecret.Value + values := map[string]credentialsecret.Value{} seen := map[string]credentialsecret.ChangeToken{} for _, c := range mcpServerCredentialRefs(auth) { for _, destination := range c.destinations { @@ -366,7 +391,7 @@ func readMCPServerCredentials(ctx context.Context, reader client.Reader, server } else if err != nil { return nil, nil, err } - values = append(values, value) + values[c.field] = value token := value.Token if first, ok := seen[c.ref.Name]; ok && (first.SecretUID != token.SecretUID || first.ResourceVersion != token.ResourceVersion) { return nil, mcpServerWait(mcpServerSecretChangedRetry, "CredentialSecretChanged", @@ -400,34 +425,29 @@ func isHTTPFieldValue(s string) bool { return true } -// lookUpMCPServerSlug lists the organization's configs, because Coder has no -// lookup by slug. A config with the slug and a display name other than this -// object's nonce belongs to someone else, and the controller never changes it -// (no adoption). -func lookUpMCPServerSlug(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID) *mcpServerStep { +// findMCPServerSlug lists the organization's configs, because Coder has no +// lookup by slug, and returns the one with the spec's slug, or nil. Slugs are +// unique per organization, with case. +func findMCPServerSlug( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, +) (*codersdk.MCPServerConfig, *mcpServerStep) { configs, err := sdk.MCPServerConfigs(ctx, orgID) if err != nil { - return mcpServerWait(mcpServerRetry, "CoderUnavailable", "list MCP server configs: %s", coderErrorSummary(err)) + return nil, mcpServerWait(mcpServerRetry, "CoderUnavailable", "list MCP server configs: %s", coderErrorSummary(err)) } - for _, config := range configs { + for i, config := range configs { if err := coderAnswerFor("MCP server config organization", orgID.String(), config.OrganizationID.String()); err != nil { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "%s", err.Error()) + return nil, mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "%s", err.Error()) } if config.Slug != server.Spec.Slug { continue } if config.ID == uuid.Nil { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the MCP server config with slug %s without an ID", config.Slug) + return nil, mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the MCP server config with slug %s without an ID", config.Slug) } - if config.DisplayName == mcpServerNoncePrefix+string(server.UID) { - return mcpServerWait(mcpServerResync, "Pending", - "MCP server config %s carries this object's nonce. This controller version does not record it.", config.ID) - } - return mcpServerStalled(mcpServerResync, "SlugConflict", - "Coder organization %s already has an MCP server config with slug %s (ID %s). The controller does not change a config that it did not create.", - server.Spec.Organization, server.Spec.Slug, config.ID) + return &configs[i], nil } - return mcpServerWait(mcpServerResync, "Pending", "Coder has no MCP server config with slug %s. This controller version does not create it.", server.Spec.Slug) + return nil, nil } // patchFinalizer adds or removes the cleanup finalizer with a merge patch. The diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 05a56be5d..134c08170 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -251,9 +251,10 @@ func TestCoderMCPServerCredentialChecks(t *testing.T) { } got, result := env.reconcile(ctx, t, env.create(ctx, t, tc.spec)) if tc.reason == "" { - require.Contains(t, requireMCPServerStep(t, got, false, "Pending"), "Coder has no MCP server config with slug github.") + require.Contains(t, requireMCPServerStep(t, got, false, "Pending"), "exists in Coder, disabled.") require.Equal(t, 1, env.fake.requestCount(routeMCPServers), "usable credentials reach the slug lookup") - require.Equal(t, 1, env.mcpRequests()) + require.Equal(t, 1, env.fake.requestCount(routeCreateMCPServer)) + require.Equal(t, 2, env.mcpRequests()) require.Equal(t, 10*time.Minute, result.RequeueAfter) return } @@ -386,9 +387,9 @@ func TestCoderMCPServerOrganizationPinning(t *testing.T) { require.Equal(t, mcp, env.mcpRequests()) } -// The slug lookup only reads. A config that this object did not create is a -// conflict (plan S4, read-only part), and Coder errors never put Detail or a -// credential into status or logs. +// A config that this object did not create is a conflict (plan S4): the +// lookup sends no write. Coder errors never put Detail or a credential into +// status or logs. func TestCoderMCPServerSlugLookup(t *testing.T) { t.Parallel() create := func(slug, displayName string) func(context.Context, *testing.T, mcpServerEnv, *coderv1alpha1.CoderMCPServer) { @@ -414,11 +415,6 @@ func TestCoderMCPServerSlugLookup(t *testing.T) { name: "slug taken", prepare: create("github", "GitHub"), stalled: true, reason: "SlugConflict", requeue: 10 * time.Minute, message: "Coder organization coder already has an MCP server config with slug github (ID ", }, - { - name: "slug in other case", prepare: create("GITHUB", "GitHub"), reason: "Pending", requeue: 10 * time.Minute, - message: "Coder has no MCP server config with slug github.", - }, - {name: "own nonce", prepare: create("github", ""), reason: "Pending", requeue: 10 * time.Minute, message: "carries this object's nonce"}, { name: "Coder error with a credential in its detail", reason: "CoderUnavailable", requeue: 30 * time.Second, message: "list MCP server configs: Coder answered 500: fake fault 500", diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go new file mode 100644 index 000000000..de5fd6bc8 --- /dev/null +++ b/internal/controller/codermcpserver_create.go @@ -0,0 +1,246 @@ +package controller + +import ( + "context" + "fmt" + "net/http" + "slices" + "strings" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +const ( + // mcpServerConfirmPoll is the poll while a create result is uncertain. + mcpServerConfirmPoll = 5 * time.Second + // mcpServerConfirmingRejectedCreate is the wait reason after a create that + // Coder rejected, while the read that confirms absence fails. + mcpServerConfirmingRejectedCreate = "ConfirmingRejectedCreate" +) + +// mcpServerNonce is the display name of a config that this object created, +// until the spec's display name replaces it. Coder stores no owner, so the +// nonce is the proof (plan 5.3). +func mcpServerNonce(server *coderv1alpha1.CoderMCPServer) string { + return mcpServerNoncePrefix + string(server.UID) +} + +// createOrRecord handles an object without status.configID, following the +// CoderAIProvider create (#253). It reads Coder first, so a create that +// committed earlier is recorded instead of sent again. Coder's unique slug +// index (plan F3) makes a second create of this slug answer 409, so a late +// commit never leaves two configs. It returns nil when status.configID is set. +func (r *CoderMCPServerReconciler) createOrRecord( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, + values map[string]credentialsecret.Value, before *coderv1alpha1.CoderMCPServerStatus, +) (*mcpServerStep, error) { + stalled := meta.FindStatusCondition(server.Status.Conditions, coderv1alpha1.CoderMCPServerConditionStalled) + if server.Status.CreateAttemptTime == nil && stalled != nil && stalled.Status == metav1.ConditionTrue && + stalled.Reason == "CoderRejected" && stalled.ObservedGeneration == server.Generation { + // Coder rejected this spec. Only a spec change retries. + return mcpServerStalled(0, stalled.Reason, "%s", stalled.Message), nil + } + ready := meta.FindStatusCondition(server.Status.Conditions, coderv1alpha1.CoderMCPServerConditionReady) + rejected := server.Status.CreateAttemptTime != nil && ready != nil && + ready.Reason == mcpServerConfirmingRejectedCreate && ready.ObservedGeneration == server.Generation + found, step := findMCPServerSlug(ctx, sdk, server, orgID) + if step != nil { + return keepRejectedMCPServerCreate(step, rejected), nil + } + if found != nil { + return recordMCPServer(server, *found), nil + } + if server.Status.CreateAttemptTime != nil { + // A create of this object may have been sent. A missing config proves + // that nothing committed only when the operator can see every config. + if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { + return keepRejectedMCPServerCreate(step, rejected), nil + } + if rejected { + server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil + return mcpServerStalled(0, "CoderRejected", "Coder rejected the create request of MCP server config %s, and a read confirmed that nothing was created.", server.Spec.Slug), nil + } + } + return r.createMCPServer(ctx, sdk, server, orgID, values, before) +} + +// keepRejectedMCPServerCreate keeps the memory of a rejected create in the +// reason of a wait, so the create is not sent again once the wait ends. +func keepRejectedMCPServerCreate(step *mcpServerStep, rejected bool) *mcpServerStep { + if rejected { + step.reason = mcpServerConfirmingRejectedCreate + } + return step +} + +// mcpServerRequireSiteOwner returns a wait unless the operator user is a site +// owner, who sees every config in the list. +func mcpServerRequireSiteOwner(ctx context.Context, sdk *codersdk.Client) *mcpServerStep { + operator, err := sdk.User(ctx, codersdk.Me) + if err != nil { + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get operator user: %s", coderErrorSummary(err)) + } + if !slices.ContainsFunc(operator.Roles, func(role codersdk.SlimRole) bool { return role.Name == codersdk.RoleOwner && role.OrganizationID == "" }) { + return mcpServerWait(mcpServerResync, "ConfirmingCreate", "The operator user is not a site owner, so a missing MCP server config proves nothing.") + } + return nil +} + +// recordMCPServer decides whether found, the config with the spec's slug, is +// the config that this object created. Only the nonce proves it. Anything +// else is a config that the controller never touches (no adoption). It +// returns nil when it recorded the ID. +func recordMCPServer(server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig) *mcpServerStep { + if found.DisplayName != mcpServerNonce(server) { + // No create of this object can commit while another config holds the + // slug, so nothing of this object can exist in Coder. + server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil + return mcpServerStalled(mcpServerResync, "SlugConflict", + "Coder organization %s already has an MCP server config with slug %s (ID %s). The controller does not change a config that it did not create.", + server.Spec.Organization, server.Spec.Slug, found.ID) + } + server.Status.ConfigID, server.Status.CreateAttemptTime = found.ID.String(), nil + return nil +} + +// createMCPServer writes the create marker and the change tokens, then sends +// the only create request of this attempt. A conflict on the marker write +// ends the reconcile before any request. The config starts disabled with the +// nonce as its display name, so members never see it and chats never use it +// (plan F6) before the apply step. +func (r *CoderMCPServerReconciler) createMCPServer( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, + values map[string]credentialsecret.Value, before *coderv1alpha1.CoderMCPServerStatus, +) (*mcpServerStep, error) { + server.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) + applyMCPServerStep(server, mcpServerWait(0, "CreatingConfig", "Creating MCP server config %s in Coder.", server.Spec.Slug)) + if err := r.writeStatus(ctx, server, before); err != nil { + return nil, err + } + *before = *server.Status.DeepCopy() + + req, err := mcpServerCreateRequest(server, values) + if err != nil { + return nil, err + } + created, err := sdk.CreateMCPServerConfig(ctx, orgID, req) + status := coderStatus(err) + switch { + case err == nil: + if created.ID == uuid.Nil || created.OrganizationID != orgID || created.Slug != server.Spec.Slug || created.DisplayName != req.DisplayName { + // A wrong answer leaves the marker, so a read decides. + return mcpServerWait(mcpServerConfirmPoll, "ConfirmingCreate", + "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 + return nil, nil + case status == 0 && createHadNoEffect(err): + // A failed dial or TLS handshake never sent the request. + server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil + return mcpServerWait(mcpServerRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(err)), nil + case status >= 400 && status < 500: + return confirmRejectedMCPServerCreate(ctx, sdk, server, orgID, status, err), nil + default: + return mcpServerWait(mcpServerConfirmPoll, "ConfirmingCreate", + "The create result is uncertain (%s). The controller reads Coder instead of sending it again.", coderErrorSummary(err)), nil + } +} + +// confirmRejectedMCPServerCreate reads Coder after a 4xx answer to a create. +// Coder sends its 4xx answers before the insert commits, but a proxy can put a +// 4xx in place of a committed answer, so only a read proves that nothing of +// this object exists. Until then the marker stays (#253 review round 3). +func confirmRejectedMCPServerCreate( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, status int, createErr error, +) *mcpServerStep { + retryable := mcpServerCreateRetryable(status) + found, step := findMCPServerSlug(ctx, sdk, server, orgID) + switch { + case step != nil: + return keepRejectedMCPServerCreate(step, !retryable) + case found != nil: + // Ours (the create committed) or someone else's (SlugConflict). + return recordMCPServer(server, *found) + } + if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { + return keepRejectedMCPServerCreate(step, !retryable) + } + server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil + if retryable { + return mcpServerWait(mcpServerRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(createErr)) + } + return mcpServerStalled(0, "CoderRejected", "Coder rejected the create request: %s", coderErrorSummary(createErr)) +} + +// mcpServerCreateRetryable reports 4xx create answers that a retry can fix +// without a spec change: an operator token or role problem, a slug that was +// freed again, or a rate limit. +func mcpServerCreateRetryable(status int) bool { + switch status { + case http.StatusUnauthorized, http.StatusForbidden, http.StatusConflict, http.StatusTooManyRequests: + return true + } + return false +} + +// mcpServerCredentialStatus returns the change token of every credential that +// the create request carries. +func mcpServerCredentialStatus(auth coderv1alpha1.CoderMCPServerAuth, values map[string]credentialsecret.Value) []coderv1alpha1.CoderMCPServerCredentialStatus { + var out []coderv1alpha1.CoderMCPServerCredentialStatus + for _, c := range mcpServerCredentialRefs(auth) { + token := values[c.field].Token + out = append(out, coderv1alpha1.CoderMCPServerCredentialStatus{ + Field: c.field, SecretName: token.SecretName, SecretKey: token.SecretKey, SecretUID: string(token.SecretUID), SecretResourceVersion: token.ResourceVersion, + }) + } + return out +} + +// mcpServerCreateRequest builds the create request: every spec field, the +// credentials of the auth type, enabled false, and the nonce as the display +// name. Credentials are revealed only here, into the request body. +func mcpServerCreateRequest(server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value) (codersdk.CreateMCPServerConfigRequest, error) { + spec := server.Spec + if spec.Transport == "" || spec.Availability == "" { + return codersdk.CreateMCPServerConfigRequest{}, fmt.Errorf("assertion failed: MCP server %s/%s has no transport or availability after admission defaults", server.Namespace, server.Name) + } + req := codersdk.CreateMCPServerConfigRequest{ + DisplayName: mcpServerNonce(server), Slug: spec.Slug, Description: spec.Description, IconURL: spec.IconURL, + Transport: spec.Transport, URL: spec.URL, AuthType: spec.Auth.Type, Availability: spec.Availability, + Enabled: false, ModelIntent: spec.ModelIntent, AllowInPlanMode: spec.AllowInPlanMode, ForwardCoderHeaders: spec.ForwardCoderHeaders, + } + reveal := func(field string) (string, error) { + v, ok := values[field] + if !ok { + return "", fmt.Errorf("assertion failed: MCP server %s/%s has no value for credential %s", server.Namespace, server.Name, field) + } + return v.Reveal(), nil + } + var err error + switch auth := spec.Auth; auth.Type { + case "api_key": + req.APIKeyHeader = auth.APIKey.Header + req.APIKeyValue, err = reveal("apiKey") + case "custom_headers": + req.CustomHeaders = make(map[string]string, len(auth.CustomHeaders)) + for _, h := range auth.CustomHeaders { + if req.CustomHeaders[h.Name], err = reveal("header:" + h.Name); err != nil { + break + } + } + case "oauth2": + o := auth.OAuth2 + req.OAuth2ClientID, req.OAuth2AuthURL, req.OAuth2TokenURL, req.OAuth2RevocationURL = o.ClientID, o.AuthURL, o.TokenURL, o.RevocationURL + req.OAuth2Scopes = strings.Join(o.Scopes, " ") + req.OAuth2ClientSecret, err = reveal("oauth2ClientSecret") + } + return req, err +} diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go new file mode 100644 index 000000000..0d5b50b2c --- /dev/null +++ b/internal/controller/codermcpserver_create_test.go @@ -0,0 +1,339 @@ +package controller_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + 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" +) + +func (env mcpServerEnv) posts() int { return env.fake.requestCount(routeCreateMCPServer) } + +// requireCreated checks that Coder holds exactly one config with the spec's +// slug, that status records it, that no create marker is left, and that every +// create request carried enabled false and the nonce. It returns the config. +func (env mcpServerEnv) requireCreated(t *testing.T, server *coderv1alpha1.CoderMCPServer) fakeMCPServer { + t.Helper() + var own []fakeMCPServer + for _, s := range env.fake.mcpServers() { + if s.view.Slug == server.Spec.Slug { + own = append(own, s) + } + } + require.Len(t, own, 1, "exactly one config with the slug") + require.Equal(t, own[0].view.ID.String(), server.Status.ConfigID) + require.Nil(t, server.Status.CreateAttemptTime) + for _, body := range env.fake.mcpRequestBodies(routeCreateMCPServer) { + var req codersdk.CreateMCPServerConfigRequest + require.NoError(t, json.Unmarshal(body, &req)) + if req.Slug == server.Spec.Slug { + require.False(t, req.Enabled, "created disabled") + require.Equal(t, "coder-k8s:"+string(server.UID), req.DisplayName) + } + } + return own[0] +} + +// requireMCPServerCreatedStep checks the reason after a create in this +// version, which does not apply the spec yet. +func requireMCPServerCreatedStep(t *testing.T, server *coderv1alpha1.CoderMCPServer) { + t.Helper() + require.Contains(t, requireMCPServerStep(t, server, false, "Pending"), "exists in Coder, disabled.") +} + +// Plan 4.5 create steps 1 to 3 for every auth type: one create, disabled with +// the nonce, every spec field and the credentials, and status records the ID +// and the change tokens. The next reconcile sends nothing. +func TestCoderMCPServerCreate(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + secrets func(context.Context, *testing.T, mcpServerEnv) + check func(*testing.T, fakeMCPServer) + fields []string + }{ + { + name: "api key", secrets: func(ctx context.Context, t *testing.T, env mcpServerEnv) { env.keySecret(ctx, t, testMCPKey) }, + fields: []string{"apiKey"}, check: func(t *testing.T, s fakeMCPServer) { + require.Equal(t, "Authorization", s.view.APIKeyHeader) + require.Equal(t, testMCPKey, s.apiKeyValue) + }, + }, + {name: "slug taken in another case", secrets: func(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.keySecret(ctx, t, testMCPKey) + _, err := env.fake.client(t, 2*time.Second).CreateMCPServerConfig(ctx, env.orgID, apiKeyMCPRequest("GITHUB")) // Slugs are case-sensitive (plan F3). + require.NoError(t, err) + }, fields: []string{"apiKey"}, check: func(t *testing.T, s fakeMCPServer) { require.Equal(t, testMCPKey, s.apiKeyValue) }}, + {name: "custom headers", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + useCustomHeaders(s, "X-Api-Key", "X-Tenant") + s.Auth.CustomHeaders[1].SecretRef.Name = "tenant-mcp" + }, secrets: func(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.secret(ctx, t, "tracker-mcp", map[string]string{"x-api-key": testMCPKey}, testMCPOrigin) + env.secret(ctx, t, "tenant-mcp", map[string]string{"x-tenant": testMCPTenant}, testMCPOrigin) + }, fields: []string{"header:X-Api-Key", "header:X-Tenant"}, check: func(t *testing.T, s fakeMCPServer) { + require.Equal(t, map[string]string{"X-Api-Key": testMCPKey, "X-Tenant": testMCPTenant}, s.customHeaders) + }}, + {name: "oauth2", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + useOAuth2(s, func(o *coderv1alpha1.CoderMCPServerOAuth2Auth) { + o.RevocationURL, o.Scopes = "https://auth.example.com/oauth/revoke", []string{"read", "write"} + }) + }, secrets: func(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin) + }, fields: []string{"oauth2ClientSecret"}, check: func(t *testing.T, s fakeMCPServer) { + require.Equal(t, testMCPClientSecret, s.oauth2ClientSecret) + require.Equal(t, "coder-agents", s.view.OAuth2ClientID) + require.Equal(t, "https://auth.example.com/oauth/authorize", s.view.OAuth2AuthURL) + require.Equal(t, "https://auth.example.com/oauth/token", s.view.OAuth2TokenURL) + require.Equal(t, "https://auth.example.com/oauth/revoke", s.view.OAuth2RevocationURL) + require.Equal(t, "read write", s.view.OAuth2Scopes) + }}, + {name: "no auth", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} + s.Description, s.IconURL, s.Availability, s.Transport = "Issues", "/icon/github.svg", "default_on", "sse" + s.ModelIntent, s.AllowInPlanMode, s.ForwardCoderHeaders = true, true, true + }, check: func(t *testing.T, s fakeMCPServer) { + require.Equal(t, "Issues", s.view.Description) + require.Equal(t, "/icon/github.svg", s.view.IconURL) + require.Equal(t, "sse", s.view.Transport) + require.Equal(t, "default_on", s.view.Availability) + require.True(t, s.view.ModelIntent && s.view.AllowInPlanMode && s.view.ForwardCoderHeaders) + require.False(t, s.view.HasAPIKey || s.view.HasCustomHeaders || s.view.HasOAuth2Secret) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + if tc.secrets != nil { + tc.secrets(ctx, t, env) + } + base := env.posts() // Creates of the test setup. + got, result := env.reconcile(ctx, t, env.create(ctx, t, tc.spec)) + stored := env.requireCreated(t, got) + requireMCPServerCreatedStep(t, got) + require.Equal(t, 1, env.posts()-base) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, "github", stored.view.Slug) + require.Equal(t, "https://mcp.example.com/github/mcp", stored.view.URL) + require.Equal(t, got.Spec.Auth.Type, stored.view.AuthType) + tc.check(t, stored) + + require.Len(t, got.Status.Credentials, len(tc.fields)) + for i, field := range tc.fields { + c := got.Status.Credentials[i] + require.Equal(t, field, c.Field) + s := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: c.SecretName}, s)) + require.Equal(t, string(s.UID), c.SecretUID) + require.Equal(t, s.ResourceVersion, c.SecretResourceVersion) + } + + before := env.mcpRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, env.mcpRequests(), "a recorded config needs no MCP request in this version") + require.Equal(t, got.ResourceVersion, again.ResourceVersion) + }) + } +} + +// Each create answer class of plan 4.5 step 4, and the reconcile after it. +// Every case ends with exactly one config of this object, and a create is +// sent again only when a read proved that the first one did not commit. +func TestCoderMCPServerCreateAnswers(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fault fakeFault + reason string + requeue time.Duration + marker bool // The first reconcile keeps the create marker. + posts int // Creates sent until the config is recorded. + }{ + {name: "reset before commit", fault: fakeFault{Reset: true}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 2}, + {name: "reset after commit", fault: fakeFault{Reset: true, AfterCommit: true}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 1}, + {name: "504 after commit", fault: fakeFault{Status: http.StatusGatewayTimeout, AfterCommit: true}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 1}, + {name: "proxy page after commit", fault: fakeFault{Status: http.StatusOK, HTML: "sign in", AfterCommit: true}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 1}, + {name: "answer for another slug", fault: fakeFault{Rewrite: func(answer any) any { + c := answer.(codersdk.MCPServerConfig) + c.Slug = "other" + return c + }}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 1}, + {name: "429", fault: fakeFault{Status: http.StatusTooManyRequests}, reason: "CreateRetrying", requeue: 30 * time.Second, posts: 2}, + {name: "401", fault: fakeFault{Status: http.StatusUnauthorized}, reason: "CreateRetrying", requeue: 30 * time.Second, posts: 2}, + {name: "400 after commit", fault: fakeFault{Status: http.StatusBadRequest, AfterCommit: true}, reason: "Pending", requeue: 10 * time.Minute, posts: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + env.fake.failNext(routeCreateMCPServer, tc.fault) + got, result := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, false, tc.reason) + require.Equal(t, tc.requeue, result.RequeueAfter) + require.Equal(t, 1, env.posts()) + require.Equal(t, tc.marker, got.Status.CreateAttemptTime != nil, "create marker") + if tc.reason != "Pending" { + require.Empty(t, got.Status.ConfigID) + got, _ = env.reconcile(ctx, t, got) + } + env.requireCreated(t, got) + requireMCPServerCreatedStep(t, got) + require.Equal(t, tc.posts, env.posts()) + }) + } +} + +// A 400 that a read confirms clears the marker and stalls. The same spec is +// never sent again, also when the confirming read failed first. A spec change +// creates the config. +func TestCoderMCPServerCreateRejected(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + env.fake.failNext(routeCreateMCPServer, fakeFault{Status: http.StatusBadRequest, Detail: "rejected " + testMCPKey}) + env.fake.failNext(routeMCPServers, fakeFault{Rewrite: func(answer any) any { return answer }}) // The lookup before the create. + env.fake.failNext(routeMCPServers, fakeFault{Status: http.StatusInternalServerError}) // The confirming read. + got, result := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, false, "ConfirmingRejectedCreate") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.NotNil(t, got.Status.CreateAttemptTime) + + got, result = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "CoderRejected"), "a read confirmed that nothing was created") + require.Zero(t, result.RequeueAfter) + require.Nil(t, got.Status.CreateAttemptTime) + require.Equal(t, 1, env.posts()) + + before := env.mcpRequests() + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "CoderRejected") + require.Equal(t, before, env.mcpRequests(), "a rejected spec sends nothing") + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.DisplayName = "GitHub tools" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + env.requireCreated(t, got) + requireMCPServerCreatedStep(t, got) + require.Equal(t, 2, env.posts()) +} + +// Plan S4: a config that another client created between the lookup and the +// create answers 409. The confirming read finds it, and the controller never +// changes it. +func TestCoderMCPServerCreateSlugRace(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + _, err := env.fake.client(t, 2*time.Second).CreateMCPServerConfig(ctx, env.orgID, apiKeyMCPRequest("github")) + require.NoError(t, err) + env.fake.failNext(routeMCPServers, fakeFault{Rewrite: func(any) any { return []codersdk.MCPServerConfig{} }}) + got, _ := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Nil(t, got.Status.CreateAttemptTime) + require.Empty(t, got.Status.ConfigID) + require.Equal(t, 2, env.posts(), "the setup create and the controller's create") + require.Len(t, env.fake.mcpServers(), 1) + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)+env.fake.requestCount(routeDeleteMCPServer)) +} + +// A marker left by an earlier reconcile: the controller reads Coder before it +// sends anything. A reconcile after a restart uses a new reconciler. +func TestCoderMCPServerCreateRecovery(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + existing bool // Coder holds a config with this object's nonce. + notOwner bool // The operator user is not a site owner. + reason string + posts int + }{ + {name: "create committed", existing: true, reason: "Pending"}, + {name: "nothing committed", reason: "Pending", posts: 1}, + {name: "nothing visible to a non-owner", notOwner: true, reason: "ConfirmingCreate"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + server.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + require.NoError(t, k8sClient.Status().Update(ctx, server)) + if tc.existing { + req := apiKeyMCPRequest("github") + req.DisplayName = "coder-k8s:" + string(server.UID) + _, err := env.fake.client(t, 2*time.Second).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) + } + if tc.notOwner { + // An organization role named owner is not the site owner role. + env.fake.updateUser(env.fake.operatorID, func(u *codersdk.User) { + u.Roles = []codersdk.SlimRole{{Name: codersdk.RoleOwner, OrganizationID: env.orgID.String()}, {Name: codersdk.RoleMember}} + }) + } + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + base := env.posts() // Creates of the test setup. + got, _ := env.reconcile(ctx, t, server) + if tc.reason == "Pending" { + env.requireCreated(t, got) + requireMCPServerCreatedStep(t, got) + } else { + requireMCPServerStep(t, got, false, tc.reason) + require.NotNil(t, got.Status.CreateAttemptTime) + } + require.Equal(t, tc.posts, env.posts()-base) + }) + } +} + +// markerConflictMCPClient fails the status write that sets the create marker, +// like a write from a stale object after a leader handover. +type markerConflictMCPClient struct{ client.Client } + +func (c markerConflictMCPClient) Status() client.SubResourceWriter { + return markerConflictMCPWriter{c.Client.Status()} +} + +type markerConflictMCPWriter struct{ client.SubResourceWriter } + +func (w markerConflictMCPWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if s, ok := obj.(*coderv1alpha1.CoderMCPServer); ok && s.Status.CreateAttemptTime != nil { + return apierrors.NewConflict(schema.GroupResource{Group: "coder.com", Resource: "codermcpservers"}, s.Name, errors.New("stale object")) + } + return w.SubResourceWriter.Update(ctx, obj, opts...) +} + +// A conflict on the marker write ends the reconcile before the create. +func TestCoderMCPServerMarkerConflictSendsNoCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + r := &controller.CoderMCPServerReconciler{Client: markerConflictMCPClient{k8sClient}, Scheme: scheme} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) + require.True(t, apierrors.IsConflict(err), "%v", err) + require.Zero(t, env.posts()) +}