Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 49 additions & 29 deletions internal/controller/codermcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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
}

Expand Down Expand Up @@ -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":
Expand All @@ -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 {
Expand All @@ -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",
Expand Down Expand Up @@ -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
Expand Down
16 changes: 6 additions & 10 deletions internal/controller/codermcpserver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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",
Expand Down
Loading
Loading