From 48ea538db6d1576d3b3a2bdabe183934eb7f7b50 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 01:48:56 +0000 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=A4=96=20feat:=20create=20the=20Coder?= =?UTF-8?q?=20MCP=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()) +} From 6d83d2f30b9214eb6ad24eee9a4699d1a0a47c59 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 01:58:28 +0000 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=A4=96=20feat:=20apply=20the=20CoderM?= =?UTF-8?q?CPServer=20spec=20and=20access=20list=20to=20the=20created=20co?= =?UTF-8?q?nfig?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I51f32054c2be80a04ccb71cd66155c257d695749 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/codermcpserver_types.go | 9 +- .../dormant/coder.com_codermcpservers.yaml | 9 +- internal/controller/codermcpserver_apply.go | 107 ++++++++ .../controller/codermcpserver_apply_test.go | 255 ++++++++++++++++++ .../controller/codermcpserver_controller.go | 18 +- .../codermcpserver_controller_test.go | 38 +-- .../controller/codermcpserver_create_test.go | 27 +- 7 files changed, 421 insertions(+), 42 deletions(-) create mode 100644 internal/controller/codermcpserver_apply.go create mode 100644 internal/controller/codermcpserver_apply_test.go diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index c76871b36..7ccbd6cdd 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -133,9 +133,12 @@ type CoderMCPServerSpec struct { // Auth is how Coder authenticates to the server. Auth CoderMCPServerAuth `json:"auth"` // Access, when set, makes the controller own the server's access list in - // Coder: exactly the listed groups can read the server. When omitted, the - // controller leaves the access list alone, and Coder's default lets every - // organization member read the server. + // Coder: exactly the listed groups can read the server. Every sync + // therefore also removes each user entry, for example a user that an admin + // added in the Coder dashboard. Coder does not show the entry of a deleted + // group, so the sync cannot remove it, but it grants nothing. + // When omitted, the controller leaves the access list alone, and Coder's + // default lets every organization member read the server. // +optional Access *CoderMCPServerAccess `json:"access,omitempty"` } diff --git a/config/crd/dormant/coder.com_codermcpservers.yaml b/config/crd/dormant/coder.com_codermcpservers.yaml index 03333c966..334fcfd38 100644 --- a/config/crd/dormant/coder.com_codermcpservers.yaml +++ b/config/crd/dormant/coder.com_codermcpservers.yaml @@ -65,9 +65,12 @@ spec: access: description: |- Access, when set, makes the controller own the server's access list in - Coder: exactly the listed groups can read the server. When omitted, the - controller leaves the access list alone, and Coder's default lets every - organization member read the server. + Coder: exactly the listed groups can read the server. Every sync + therefore also removes each user entry, for example a user that an admin + added in the Coder dashboard. Coder does not show the entry of a deleted + group, so the sync cannot remove it, but it grants nothing. + When omitted, the controller leaves the access list alone, and Coder's + default lets every organization member read the server. properties: groups: description: |- diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go new file mode 100644 index 000000000..e66e0ade6 --- /dev/null +++ b/internal/controller/codermcpserver_apply.go @@ -0,0 +1,107 @@ +package controller + +import ( + "context" + "fmt" + "net/http" + "strings" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// applyMCPServer sets the spec on the config that this object created (plan +// 4.5 create steps 5 and 6). It reads the config, makes the ACL match +// spec.access, and then sends one PATCH with every spec field and every +// credential of the auth type. The ACL comes first, so the config is never +// enabled before its ACL matches (plan S20). 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( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, +) (*mcpServerStep, 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) + } + current, err := sdk.MCPServerConfigByID(ctx, orgID, id) + switch { + 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 + case err != nil: + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), nil + } + if current.ID != id || current.OrganizationID != orgID || current.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 + } + if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, err + } + req, err := mcpServerUpdateRequest(server, values) + if err != nil { + return nil, err + } + updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) + switch status := coderStatus(err); { + case isCoderNotFound(err): + return mcpServerStalled(0, "DeletedExternally", "MCP server config %s no longer exists in Coder. This controller version does not create it again.", id), nil + 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. + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update MCP server config %s: %s", id, coderErrorSummary(err)), nil + } + if updated.ID != id || updated.DisplayName != *req.DisplayName || updated.Enabled != *req.Enabled || updated.URL != *req.URL { + return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with other values", id), nil + } + server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) + server.Status.AppliedGeneration = server.Generation + return mcpServerReady(server), nil +} + +// mcpServerReady reports a config that matched the spec when this generation +// was applied. This version reads Coder only to apply a new generation. +func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { + return &mcpServerStep{ + ready: true, reason: "Applied", requeue: mcpServerResync, + message: fmt.Sprintf("MCP server config %s matches generation %d of the spec.", server.Status.ConfigID, server.Status.AppliedGeneration), + } +} + +// mcpServerUpdateRequest sets every spec field and the credentials of the +// auth type. Fields of the other auth types stay unset: a changed auth type +// makes Coder clear them (plan F9). The tool lists stay unset until the spec +// has them. Credentials are revealed only here, into the request body. +func mcpServerUpdateRequest(server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value) (codersdk.UpdateMCPServerConfigRequest, error) { + create, err := mcpServerCreateRequest(server, values) + if err != nil { + return codersdk.UpdateMCPServerConfigRequest{}, err + } + spec := server.Spec + enabled := spec.Enabled == nil || *spec.Enabled + req := codersdk.UpdateMCPServerConfigRequest{ + DisplayName: &spec.DisplayName, Description: &create.Description, IconURL: &create.IconURL, + Transport: &create.Transport, URL: &create.URL, AuthType: &create.AuthType, Availability: &create.Availability, + Enabled: &enabled, ModelIntent: &create.ModelIntent, AllowInPlanMode: &create.AllowInPlanMode, ForwardCoderHeaders: &create.ForwardCoderHeaders, + } + switch spec.Auth.Type { + case "api_key": + req.APIKeyHeader, req.APIKeyValue = &create.APIKeyHeader, &create.APIKeyValue + case "custom_headers": + req.CustomHeaders = &create.CustomHeaders + case "oauth2": + req.OAuth2ClientID, req.OAuth2ClientSecret = &create.OAuth2ClientID, &create.OAuth2ClientSecret + req.OAuth2AuthURL, req.OAuth2TokenURL = &create.OAuth2AuthURL, &create.OAuth2TokenURL + // "" clears a revocation URL that an earlier generation set. + req.OAuth2RevocationURL, req.OAuth2Scopes = &create.OAuth2RevocationURL, &create.OAuth2Scopes + } + if strings.TrimSpace(*req.DisplayName) == "" { + return req, fmt.Errorf("assertion failed: MCP server %s/%s has an empty display name after admission", server.Namespace, server.Name) + } + return req, nil +} diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go new file mode 100644 index 000000000..361da71ee --- /dev/null +++ b/internal/controller/codermcpserver_apply_test.go @@ -0,0 +1,255 @@ +package controller_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "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" +) + +// writeRoutes returns the routes of the fake's recorded write requests, in +// request order. +func (env mcpServerEnv) writeRoutes() []string { + env.fake.mu.Lock() + defer env.fake.mu.Unlock() + var out []string + for _, b := range env.fake.mcp.bodies { + out = append(out, b.route) + } + return out +} + +func (env mcpServerEnv) lastUpdate(t *testing.T) codersdk.UpdateMCPServerConfigRequest { + t.Helper() + bodies := env.fake.mcpRequestBodies(routeUpdateMCPServer) + require.NotEmpty(t, bodies) + var req codersdk.UpdateMCPServerConfigRequest + require.NoError(t, json.Unmarshal(bodies[len(bodies)-1], &req)) + return req +} + +// Plan S20 and 4.5 steps 5 and 6: the config is created disabled, its ACL +// matches spec.access, and only then one PATCH enables it with the spec +// values. That PATCH sets url, so it carries the credential too. +func TestCoderMCPServerApplyOrder(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", "") + got, result := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"Everyone", "data"}} + })) + requireMCPServerApplied(t, got) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, []string{routeCreateMCPServer, routeUpdateMCPServerACL, routeUpdateMCPServer}, env.writeRoutes(), "create, then the ACL, then enable") + stored := env.requireCreated(t, got) + require.True(t, stored.view.Enabled) + require.Equal(t, "GitHub", stored.view.DisplayName) + require.Equal(t, testMCPKey, stored.apiKeyValue) + groups, users := env.fake.mcpACL(stored.view.ID) + require.ElementsMatch(t, []uuid.UUID{env.orgID, 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.Len(t, got.Status.Credentials, 1) + + before := env.mcpRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, env.mcpRequests(), "an applied generation sends nothing") + require.Equal(t, got.ResourceVersion, again.ResourceVersion) +} + +// Plan S15: access with no groups and enabled false reach Coder as an empty +// ACL on a disabled config. Without access, the controller never touches the +// ACL, so Coder's default Everyone entry stays. +func TestCoderMCPServerApplyAccess(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + access *coderv1alpha1.CoderMCPServerAccess + enabled bool + acl bool // The ACL routes are called. + }{ + {name: "no groups, disabled", access: &coderv1alpha1.CoderMCPServerAccess{Groups: []string{}}, acl: true}, + {name: "access omitted", enabled: 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, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access, s.Enabled = tc.access, ptrTo(tc.enabled) + })) + requireMCPServerApplied(t, got) + stored := env.requireCreated(t, got) + require.Equal(t, tc.enabled, stored.view.Enabled) + groups, _ := env.fake.mcpACL(stored.view.ID) + aclCalls := env.fake.requestCount(routeMCPServerACL) + env.fake.requestCount(routeUpdateMCPServerACL) + if tc.acl { + require.Empty(t, groups, "no Everyone entry") + require.NotZero(t, aclCalls) + } else { + require.Equal(t, []uuid.UUID{env.orgID}, groups) + require.Zero(t, aclCalls) + } + }) + } +} + +// A group that does not resolve keeps the new config disabled with the nonce, +// and sends no PATCH. Once the group exists, the next reconcile applies. +func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data"}} + })) + requireMCPServerStep(t, got, true, "GroupNotFound") + stored := env.requireCreated(t, got) + require.False(t, stored.view.Enabled) + require.Equal(t, "coder-k8s:"+string(got.UID), stored.view.DisplayName) + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)+env.fake.requestCount(routeUpdateMCPServerACL)) + require.Zero(t, got.Status.AppliedGeneration) + + env.fake.addGroup(env.orgID, "data", "") + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.requireCreated(t, got).view.Enabled) +} + +// Each failure of the apply leaves the generation unapplied, sends no other +// write, and never puts a credential into status or logs. +func TestCoderMCPServerApplyFailures(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + route string + fault fakeFault + reason string + 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) + c.Slug = "other" + return c + }}}, + {name: "update fails", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusInternalServerError, Detail: "upstream " + testMCPKey}, reason: "CoderUnavailable", retry: true}, + {name: "update rejected", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusBadRequest, Detail: "rejected " + testMCPKey}, reason: "CoderRejected", stalled: true}, + {name: "update gone", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "DeletedExternally", stalled: true}, + {name: "update answers other values", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: fakeFault{AfterCommit: true, Rewrite: func(answer any) any { + c := answer.(codersdk.MCPServerConfig) + c.Enabled = false + return c + }}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(tc.route, tc.fault) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, tc.stalled, tc.reason) + require.Zero(t, got.Status.AppliedGeneration) + require.NotEmpty(t, got.Status.ConfigID, "the ID is stored before the apply") + require.Equal(t, 1, env.posts()) + if tc.retry { + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.requireCreated(t, got).view.Enabled) + } + }) + } +} + +// A spec change after the apply sends one PATCH with the new values. It sets +// url, so it carries the current credential (plan 4.6 item 5), and status +// records that credential's change token. +func TestCoderMCPServerApplySpecChange(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + rotated := testMCPKey + "-rotated" + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(rotated) + require.NoError(t, k8sClient.Update(ctx, secret)) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.URL, got.Spec.Description = "https://mcp.example.com/v2/mcp", "Issues" + require.NoError(t, k8sClient.Update(ctx, got)) + before := env.fake.requestCount(routeUpdateMCPServer) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)-before) + require.Equal(t, 1, env.posts()) + stored := env.requireCreated(t, got) + require.Equal(t, "https://mcp.example.com/v2/mcp", stored.view.URL) + require.Equal(t, "Issues", stored.view.Description) + require.Equal(t, rotated, *env.lastUpdate(t).APIKeyValue) + require.Equal(t, rotated, stored.apiKeyValue) + require.Len(t, got.Status.Credentials, 1) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) +} + +// appliedStatusFailClient fails the status write that records an applied +// generation, like a crash after the PATCH. +type appliedStatusFailClient struct{ client.Client } + +func (c appliedStatusFailClient) Status() client.SubResourceWriter { + return appliedStatusFailWriter{c.Client.Status()} +} + +type appliedStatusFailWriter struct{ client.SubResourceWriter } + +func (w appliedStatusFailWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if s, ok := obj.(*coderv1alpha1.CoderMCPServer); ok && s.Status.AppliedGeneration != 0 { + return errors.New("lost status write") + } + return w.SubResourceWriter.Update(ctx, obj, opts...) +} + +// The PATCH replaces the nonce, so the config ID is stored before it. When +// the status write after the PATCH is lost, the next reconcile still knows +// the config and applies again, instead of reporting its own config as a +// slug conflict. +func TestCoderMCPServerApplyLostStatusWrite(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: appliedStatusFailClient{k8sClient}, Scheme: scheme} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) + require.ErrorContains(t, err, "lost status write") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + require.NotEmpty(t, server.Status.ConfigID) + require.Zero(t, server.Status.AppliedGeneration) + require.Equal(t, "GitHub", env.requireCreated(t, server).view.DisplayName, "the PATCH replaced the nonce") + + got, _ := env.reconcile(ctx, t, server) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.posts()) + require.Equal(t, 2, env.fake.requestCount(routeUpdateMCPServer)) +} diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 72375da88..06d7b5fac 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -184,9 +184,10 @@ 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 then creates the config -// or records the one that this object created. Every Coder call before the -// slug lookup is an organization lookup. +// control plane's opt-in, reads the credentials, creates the config or +// records the one that this object created, and applies a new generation of +// the spec. 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) { @@ -228,9 +229,14 @@ func (r *CoderMCPServerReconciler) check( *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) + switch { + case step != nil: + case server.Status.AppliedGeneration == server.Generation: + step = mcpServerReady(server) + default: + if step, err = applyMCPServer(ctx, sdk, server, org.ID, values); err != nil { + return nil, err + } } // Coder messages can echo the request, so scrub the credentials from them. all := make([]credentialsecret.Value, 0, len(values)) diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 134c08170..ea5b18262 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -122,12 +122,17 @@ func (env mcpServerEnv) mcpRequests() int { return env.fake.totalRequests() - env.fake.requestCount(routeOrganization) } -// requireMCPServerStep checks the kstatus conditions of a wait or a stall and -// returns the message. +// requireMCPServerStep checks the kstatus conditions of a wait, a stall, or +// the ready reason Applied, and returns the message. func requireMCPServerStep(t *testing.T, server *coderv1alpha1.CoderMCPServer, stalled bool, reason string) string { t.Helper() want := map[string]metav1.ConditionStatus{"Ready": metav1.ConditionFalse, "Reconciling": metav1.ConditionTrue, "Stalled": metav1.ConditionFalse} - if stalled { + switch { + case reason == "Applied": + require.False(t, stalled) + want["Ready"], want["Reconciling"] = metav1.ConditionTrue, metav1.ConditionFalse + require.Equal(t, server.Generation, server.Status.AppliedGeneration) + case stalled: want["Reconciling"], want["Stalled"] = metav1.ConditionFalse, metav1.ConditionTrue } for conditionType, status := range want { @@ -251,10 +256,11 @@ 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"), "exists in Coder, disabled.") + requireMCPServerStep(t, got, false, "Applied") require.Equal(t, 1, env.fake.requestCount(routeMCPServers), "usable credentials reach the slug lookup") require.Equal(t, 1, env.fake.requestCount(routeCreateMCPServer)) - require.Equal(t, 2, env.mcpRequests()) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)) + require.Equal(t, 4, env.mcpRequests(), "list, create, get and update") require.Equal(t, 10*time.Minute, result.RequeueAfter) return } @@ -322,15 +328,15 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { }{ {optIn: nil, reason: "MCPServersNotAllowed", lookups: 1}, {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "missing"}}, reason: "MCPServersNotAllowed", lookups: 3}, - {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"CODER"}}, reason: "Pending", lookups: 1}, - {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "coder"}}, reason: "Pending", lookups: 2}, + {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"CODER"}}, reason: "Applied", lookups: 1}, + {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "coder"}}, reason: "Applied", lookups: 2}, } { env.setOptIn(ctx, t, tc.optIn) orgLookups, mcp := env.fake.requestCount(routeOrganization), env.mcpRequests() got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, tc.reason != "Pending", tc.reason) + requireMCPServerStep(t, got, tc.reason != "Applied", tc.reason) require.Equal(t, tc.lookups, env.fake.requestCount(routeOrganization)-orgLookups, tc.reason) - if tc.reason != "Pending" { + if tc.reason != "Applied" { require.Equal(t, mcp, env.mcpRequests(), "only organization lookups happen") } } @@ -352,7 +358,7 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { require.Equal(t, mcp, env.mcpRequests()) env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}) got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") } // The organization ID is pinned at the first resolution. A later resolution @@ -371,7 +377,7 @@ func TestCoderMCPServerOrganizationPinning(t *testing.T) { first := env.fake.addOrganization("data") got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") require.Equal(t, first.String(), got.Status.OrganizationID) // The organization is deleted and another one takes its name. @@ -504,7 +510,7 @@ func TestCoderMCPServerSecretChangesDuringReconcile(t *testing.T) { require.Zero(t, env.mcpRequests()) got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") } // Plan S15: access with no groups and enabled false survive the finalizer @@ -579,7 +585,7 @@ func TestCoderMCPServerWatches(t *testing.T) { return condition != nil && condition.Reason == reason }, 30*time.Second, 100*time.Millisecond, reason) } - requireReason("Pending") + requireReason("Applied") require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(tenant), tenant)) tenant.Labels = nil @@ -587,19 +593,19 @@ func TestCoderMCPServerWatches(t *testing.T) { requireReason(credentialsecret.ReasonNotOptedIn) tenant.Labels = map[string]string{credentialsecret.LabelKey: credentialsecret.LabelValue} require.NoError(t, k8sClient.Update(ctx, tenant)) - requireReason("Pending") + requireReason("Applied") env.setOptIn(ctx, t, nil) requireReason("MCPServersNotAllowed") env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) - requireReason("Pending") + requireReason("Applied") // Another control plane that uses a credential Secret for its own access // makes it unusable. Deleting that control plane makes it usable again. other := createControlPlaneUsing(ctx, t, env.namespace, "license", tenant.Name) requireReason(credentialsecret.ReasonRefused) require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: other, Namespace: env.namespace}})) - requireReason("Pending") + requireReason("Applied") require.NoError(t, k8sClient.Delete(ctx, key)) requireReason(credentialsecret.ReasonSecretNotFound) diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go index 0d5b50b2c..0832f63ab 100644 --- a/internal/controller/codermcpserver_create_test.go +++ b/internal/controller/codermcpserver_create_test.go @@ -48,11 +48,10 @@ func (env mcpServerEnv) requireCreated(t *testing.T, server *coderv1alpha1.Coder 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) { +// requireMCPServerApplied checks that the spec was applied after the create. +func requireMCPServerApplied(t *testing.T, server *coderv1alpha1.CoderMCPServer) { t.Helper() - require.Contains(t, requireMCPServerStep(t, server, false, "Pending"), "exists in Coder, disabled.") + require.Contains(t, requireMCPServerStep(t, server, false, "Applied"), "matches generation") } // Plan 4.5 create steps 1 to 3 for every auth type: one create, disabled with @@ -125,7 +124,7 @@ func TestCoderMCPServerCreate(t *testing.T) { 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) + requireMCPServerApplied(t, got) require.Equal(t, 1, env.posts()-base) require.Equal(t, 10*time.Minute, result.RequeueAfter) require.Equal(t, "github", stored.view.Slug) @@ -145,7 +144,7 @@ func TestCoderMCPServerCreate(t *testing.T) { 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, before, env.mcpRequests(), "an applied generation needs no MCP request in this version") require.Equal(t, got.ResourceVersion, again.ResourceVersion) }) } @@ -175,7 +174,7 @@ func TestCoderMCPServerCreateAnswers(t *testing.T) { }}, 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}, + {name: "400 after commit", fault: fakeFault{Status: http.StatusBadRequest, AfterCommit: true}, reason: "Applied", requeue: 10 * time.Minute, posts: 1}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -189,12 +188,12 @@ func TestCoderMCPServerCreateAnswers(t *testing.T) { 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" { + if tc.reason != "Applied" { require.Empty(t, got.Status.ConfigID) got, _ = env.reconcile(ctx, t, got) } env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) require.Equal(t, tc.posts, env.posts()) }) } @@ -233,7 +232,7 @@ func TestCoderMCPServerCreateRejected(t *testing.T) { require.NoError(t, k8sClient.Update(ctx, got)) got, _ = env.reconcile(ctx, t, got) env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) require.Equal(t, 2, env.posts()) } @@ -269,8 +268,8 @@ func TestCoderMCPServerCreateRecovery(t *testing.T) { reason string posts int }{ - {name: "create committed", existing: true, reason: "Pending"}, - {name: "nothing committed", reason: "Pending", posts: 1}, + {name: "create committed", existing: true, reason: "Applied"}, + {name: "nothing committed", reason: "Applied", posts: 1}, {name: "nothing visible to a non-owner", notOwner: true, reason: "ConfirmingCreate"}, } { t.Run(tc.name, func(t *testing.T) { @@ -296,9 +295,9 @@ func TestCoderMCPServerCreateRecovery(t *testing.T) { 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" { + if tc.reason == "Applied" { env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) } else { requireMCPServerStep(t, got, false, tc.reason) require.NotNil(t, got.Status.CreateAttemptTime) From 0dc8a590f73ddda636645d5467bd500eae65b8ef Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 02:37:52 +0000 Subject: [PATCH 3/4] fix: never broaden the ACL before the config change, confirm a 404, check every answered field Change-Id: I33c42b0d71af811d67f331d775c0a0ecb171b35d Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_acl.go | 23 ++++- internal/controller/codermcpserver_apply.go | 82 +++++++++++++++--- .../controller/codermcpserver_apply_test.go | 83 ++++++++++++++++--- 3 files changed, 164 insertions(+), 24 deletions(-) diff --git a/internal/controller/codermcpserver_acl.go b/internal/controller/codermcpserver_acl.go index 37f317d5f..01c9e1be5 100644 --- a/internal/controller/codermcpserver_acl.go +++ b/internal/controller/codermcpserver_acl.go @@ -34,6 +34,23 @@ var mcpServerGroupNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]+(?:-[a-zA-Z0-9] // It returns nil when the ACL matches. func syncMCPServerACL( ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, +) (*mcpServerStep, error) { + return syncMCPServerACLPass(ctx, sdk, orgID, configID, access, false) +} + +// narrowMCPServerACL removes every ACL entry that access does not list, and +// adds nothing. A caller that changes the config runs it before the change +// and syncMCPServerACL after it. Then no principal outside both the old and +// the new access list can reach the config at any point, also when the +// change fails. It returns nil when nothing is left to remove. +func narrowMCPServerACL( + ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, +) (*mcpServerStep, error) { + return syncMCPServerACLPass(ctx, sdk, orgID, configID, access, true) +} + +func syncMCPServerACLPass( + ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, removeOnly bool, ) (*mcpServerStep, error) { if sdk == nil || orgID == uuid.Nil || configID == uuid.Nil { return nil, fmt.Errorf("assertion failed: MCP server ACL sync needs a Coder client, an organization ID, and a config ID") @@ -62,8 +79,10 @@ func syncMCPServerACL( } delete(want, g.ID) } - for id := range want { - patch.GroupRoles[id.String()] = codersdk.MCPServerConfigRoleRead + if !removeOnly { + for id := range want { + patch.GroupRoles[id.String()] = codersdk.MCPServerConfigRoleRead + } } // access lists groups only, so every user entry goes. for _, u := range acl.Users { diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index e66e0ade6..0e561d764 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -14,10 +14,12 @@ import ( ) // applyMCPServer sets the spec on the config that this object created (plan -// 4.5 create steps 5 and 6). It reads the config, makes the ACL match -// spec.access, and then sends one PATCH with every spec field and every -// credential of the auth type. The ACL comes first, so the config is never -// enabled before its ACL matches (plan S20). Every PATCH sets url, so it +// 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( @@ -39,7 +41,7 @@ func applyMCPServer( 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 } - if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { return step, err } req, err := mcpServerUpdateRequest(server, values) @@ -48,22 +50,82 @@ func applyMCPServer( } updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) switch status := coderStatus(err); { - case isCoderNotFound(err): - return mcpServerStalled(0, "DeletedExternally", "MCP server config %s no longer exists in Coder. This controller version does not create it again.", id), nil 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. + // 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 } - if updated.ID != id || updated.DisplayName != *req.DisplayName || updated.Enabled != *req.Enabled || updated.URL != *req.URL { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with other values", id), nil + if field := mcpServerUpdateMismatch(id, 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 step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, err } server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) server.Status.AppliedGeneration = server.Generation return mcpServerReady(server), nil } +// 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. +func mcpServerUpdateMismatch(id uuid.UUID, 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 { + 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 { + return f.name + } + } + 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. func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index 361da71ee..8340af98a 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -39,9 +39,12 @@ func (env mcpServerEnv) lastUpdate(t *testing.T) codersdk.UpdateMCPServerConfigR return req } -// Plan S20 and 4.5 steps 5 and 6: the config is created disabled, its ACL -// matches spec.access, and only then one PATCH enables it with the spec -// values. That PATCH sets url, so it carries the credential too. +// Plan S20 and 4.5 steps 5 and 6: the config is created disabled with +// Coder's default Everyone entry. The entries that spec.access does not list +// go first, then one PATCH enables the config with the spec values, and only +// then are the missing groups added. So the enabled config never has a group +// that the spec does not list. The PATCH sets url, so it carries the +// credential too. func TestCoderMCPServerApplyOrder(t *testing.T) { t.Parallel() ctx := context.Background() @@ -49,17 +52,21 @@ func TestCoderMCPServerApplyOrder(t *testing.T) { env.keySecret(ctx, t, testMCPKey) data := env.fake.addGroup(env.orgID, "data", "") got, result := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { - s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"Everyone", "data"}} + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data"}} })) requireMCPServerApplied(t, got) require.Equal(t, 10*time.Minute, result.RequeueAfter) - require.Equal(t, []string{routeCreateMCPServer, routeUpdateMCPServerACL, routeUpdateMCPServer}, env.writeRoutes(), "create, then the ACL, then enable") + require.Equal(t, []string{routeCreateMCPServer, routeUpdateMCPServerACL, routeUpdateMCPServer, routeUpdateMCPServerACL}, env.writeRoutes(), + "create, remove Everyone, enable, then add data") + acls := env.fake.mcpRequestBodies(routeUpdateMCPServerACL) + require.JSONEq(t, `{"group_roles":{"`+env.orgID.String()+`":""}}`, string(acls[0]), "the first ACL write only removes") + require.JSONEq(t, `{"group_roles":{"`+data.String()+`":"read"}}`, string(acls[1])) stored := env.requireCreated(t, got) require.True(t, stored.view.Enabled) require.Equal(t, "GitHub", stored.view.DisplayName) require.Equal(t, testMCPKey, stored.apiKeyValue) groups, users := env.fake.mcpACL(stored.view.ID) - require.ElementsMatch(t, []uuid.UUID{env.orgID, data}, groups) + 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") @@ -72,6 +79,48 @@ func TestCoderMCPServerApplyOrder(t *testing.T) { require.Equal(t, got.ResourceVersion, again.ResourceVersion) } +// A new generation that moves an enabled config to other groups and also +// disables or changes it: when the PATCH fails, the ACL holds only groups +// that both generations allow, so no newly listed group can use the old +// configuration. +func TestCoderMCPServerApplyNeverBroadensBeforeTheChange(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + edit func(*coderv1alpha1.CoderMCPServerSpec) + fault fakeFault + }{ + {name: "disable rejected", edit: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Enabled = ptrTo(false) }, fault: fakeFault{Status: http.StatusBadRequest}}, + {name: "URL change fails", edit: func(s *coderv1alpha1.CoderMCPServerSpec) { s.URL = "https://mcp.example.com/v2/mcp" }, fault: fakeFault{Status: http.StatusBadGateway}}, + } { + t.Run(tc.name, func(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", "") + env.fake.addGroup(env.orgID, "ops", "") + got, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"Everyone", "data"}} + })) + requireMCPServerApplied(t, got) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Access.Groups = []string{"data", "ops"} + tc.edit(&got.Spec) + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, tc.fault) + got, _ = env.reconcile(ctx, t, got) + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + groups, _ := env.fake.mcpACL(env.requireCreated(t, got).view.ID) + require.Equal(t, []uuid.UUID{data}, groups, "Everyone is gone, ops is not added yet") + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + }) + } +} + // Plan S15: access with no groups and enabled false reach Coder as an empty // ACL on a disabled config. Without access, the controller never touches the // ACL, so Coder's default Everyone entry stays. @@ -133,6 +182,15 @@ func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { require.True(t, env.requireCreated(t, got).view.Enabled) } +// rewriteUpdate changes the update answer after Coder applied the update. +func rewriteUpdate(edit func(*codersdk.MCPServerConfig)) fakeFault { + return fakeFault{AfterCommit: true, Rewrite: func(answer any) any { + c := answer.(codersdk.MCPServerConfig) + edit(&c) + return c + }} +} + // Each failure of the apply leaves the generation unapplied, sends no other // write, and never puts a credential into status or logs. func TestCoderMCPServerApplyFailures(t *testing.T) { @@ -154,12 +212,13 @@ func TestCoderMCPServerApplyFailures(t *testing.T) { }}}, {name: "update fails", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusInternalServerError, Detail: "upstream " + testMCPKey}, reason: "CoderUnavailable", retry: true}, {name: "update rejected", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusBadRequest, Detail: "rejected " + testMCPKey}, reason: "CoderRejected", stalled: true}, - {name: "update gone", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "DeletedExternally", stalled: true}, - {name: "update answers other values", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: fakeFault{AfterCommit: true, Rewrite: func(answer any) any { - c := answer.(codersdk.MCPServerConfig) - c.Enabled = false - return c - }}}, + {name: "update answers 404", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "CoderUnavailable", retry: true}, + {name: "update answers another enabled", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Enabled = false })}, + {name: "update answers another description", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Description = "x" })}, + {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 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) { t.Parallel() From db5b3e0d9eaab2a261e3685049866e6cd2d73e5a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 03:12:40 +0000 Subject: [PATCH 4/4] fix: send a rotated credential, and check that an auth type change cleared the old credential Change-Id: Ia71b65e0fb3a49afd17495c7d8bd91edf329a119 Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 18 ++++- .../controller/codermcpserver_apply_test.go | 68 +++++++++++++++++++ .../controller/codermcpserver_controller.go | 5 +- 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index 0e561d764..d023c0db7 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -58,7 +58,7 @@ func applyMCPServer( // 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 } - if field := mcpServerUpdateMismatch(id, req, updated); field != "" { + 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 step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { @@ -71,8 +71,10 @@ func applyMCPServer( // 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. -func mcpServerUpdateMismatch(id uuid.UUID, req codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) string { +// 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" } @@ -123,6 +125,16 @@ func mcpServerUpdateMismatch(id uuid.UUID, req codersdk.UpdateMCPServerConfigReq case req.OAuth2ClientSecret != nil && !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 "" } diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index 8340af98a..16b35f3e7 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -312,3 +312,71 @@ func TestCoderMCPServerApplyLostStatusWrite(t *testing.T) { require.Equal(t, 1, env.posts()) require.Equal(t, 2, env.fake.requestCount(routeUpdateMCPServer)) } + +// A Secret change moves its change token but not the generation. The next +// reconcile sends the new value in one PATCH and records the new token, and +// the one after sends nothing. +func TestCoderMCPServerApplySecretRotation(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + rotated := testMCPKey + "-rotated" + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(rotated) + require.NoError(t, k8sClient.Update(ctx, secret)) + before := env.fake.requestCount(routeUpdateMCPServer) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)-before) + require.Equal(t, rotated, env.requireCreated(t, got).apiKeyValue) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) + + before = env.mcpRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, env.mcpRequests(), "a recorded token sends nothing") + require.Equal(t, got.ResourceVersion, again.ResourceVersion) +} + +// A spec change of the auth type relies on Coder clearing the old type's +// credential. An answer that still holds it is not accepted. +func TestCoderMCPServerApplyAuthTypeChange(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fault *fakeFault + }{ + {name: "old key cleared"}, + {name: "old key kept", fault: ptrTo(rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.HasAPIKey = 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, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} + require.NoError(t, k8sClient.Update(ctx, got)) + if tc.fault != nil { + env.fake.failNext(routeUpdateMCPServer, *tc.fault) + } + got, _ = env.reconcile(ctx, t, got) + stored := env.requireCreated(t, got) + require.Equal(t, "none", stored.view.AuthType) + if tc.fault == nil { + requireMCPServerApplied(t, got) + require.Empty(t, stored.apiKeyValue, "Coder cleared the key") + require.Empty(t, got.Status.Credentials) + return + } + require.Contains(t, requireMCPServerStep(t, got, true, "CoderAnswerMismatch"), "has_api_key") + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + }) + } +} diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 06d7b5fac..74cfdea3a 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/url" + "slices" "strings" "time" "unicode/utf8" @@ -231,7 +232,9 @@ func (r *CoderMCPServerReconciler) check( } switch { case step != nil: - case server.Status.AppliedGeneration == server.Generation: + 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 {