diff --git a/api/v1alpha1/codercontrolplane_types.go b/api/v1alpha1/codercontrolplane_types.go index 13c2cfae..590be414 100644 --- a/api/v1alpha1/codercontrolplane_types.go +++ b/api/v1alpha1/codercontrolplane_types.go @@ -126,8 +126,9 @@ type CoderControlPlaneSpec struct { TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"` // MCPServers configures CoderMCPServer objects that use this control - // plane. It has no effect yet: the CoderMCPServer controller is not - // shipped. + // plane. Removing it disables in Coder each server config that a + // CoderMCPServer created through this control plane. It has no effect + // yet: the CoderMCPServer controller is not shipped. // +optional MCPServers *MCPServersSpec `json:"mcpServers,omitempty"` diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index 7ccbd6cd..8d4478c0 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -28,8 +28,11 @@ type MCPServersSpec struct { // control plane's operator token. This delegates Coder organization-admin // power over MCP servers in these organizations to everyone who can // create CoderMCPServer objects here. A CoderMCPServer for an - // organization outside this list sends no MCP request. Removing an - // organization stops further changes to its servers. Deleting a + // organization outside this list creates and changes nothing. Removing + // an organization disables in Coder each server config that a + // CoderMCPServer created there, with one request that holds only + // enabled: false, and then changes nothing else. After the organization + // is listed again, a spec edit enables the server. Deleting a // CoderMCPServer still deletes the config it created. // // This field has no effect yet: the CoderMCPServer controller is not @@ -45,7 +48,8 @@ type MCPServersSpec struct { // AllowForceOn lets CoderMCPServer objects use availability: force_on, // which adds a server to every new chat of each user who can read it, // without that user's choice. When false (the default), such objects - // send no MCP request. + // create and change nothing, and a server config that one of them + // created is disabled in Coder, as for a removed organization. // // This field has no effect yet: the CoderMCPServer controller is not // shipped. @@ -293,7 +297,11 @@ type CoderMCPServerStatus struct { // ObservedGeneration is the generation the controller last processed. // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // AppliedGeneration is the last generation whose spec Coder fully holds. + // AppliedGeneration is the last generation whose spec Coder fully holds, + // or that a kill switch holds: when the controller disables the config + // because a credential Secret or the control plane withdrew consent, it + // sets this field before it sends the disable, so only a later spec edit + // enables the config again. Ready tells whether Coder holds the spec. // +optional AppliedGeneration int64 `json:"appliedGeneration,omitempty"` // OrganizationID is the Coder organization ID that spec.organization diff --git a/config/crd/bases/coder.com_codercontrolplanes.yaml b/config/crd/bases/coder.com_codercontrolplanes.yaml index a0953b7c..3aa6f301 100644 --- a/config/crd/bases/coder.com_codercontrolplanes.yaml +++ b/config/crd/bases/coder.com_codercontrolplanes.yaml @@ -1399,15 +1399,17 @@ spec: mcpServers: description: |- MCPServers configures CoderMCPServer objects that use this control - plane. It has no effect yet: the CoderMCPServer controller is not - shipped. + plane. Removing it disables in Coder each server config that a + CoderMCPServer created through this control plane. It has no effect + yet: the CoderMCPServer controller is not shipped. properties: allowForceOn: description: |- AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects - send no MCP request. + create and change nothing, and a server config that one of them + created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. @@ -1419,8 +1421,11 @@ spec: control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an - organization outside this list sends no MCP request. Removing an - organization stops further changes to its servers. Deleting a + organization outside this list creates and changes nothing. Removing + an organization disables in Coder each server config that a + CoderMCPServer created there, with one request that holds only + enabled: false, and then changes nothing else. After the organization + is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not diff --git a/config/crd/dormant/coder.com_codermcpservers.yaml b/config/crd/dormant/coder.com_codermcpservers.yaml index 334fcfd3..47f1654d 100644 --- a/config/crd/dormant/coder.com_codermcpservers.yaml +++ b/config/crd/dormant/coder.com_codermcpservers.yaml @@ -421,8 +421,12 @@ spec: description: CoderMCPServerStatus defines the observed state of a CoderMCPServer. properties: appliedGeneration: - description: AppliedGeneration is the last generation whose spec Coder - fully holds. + description: |- + AppliedGeneration is the last generation whose spec Coder fully holds, + or that a kill switch holds: when the controller disables the config + because a credential Secret or the control plane withdrew consent, it + sets this field before it sends the disable, so only a later spec edit + enables the config again. Ready tells whether Coder holds the spec. format: int64 type: integer conditions: diff --git a/dist/install.yaml b/dist/install.yaml index 8138d499..23b1e3b7 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -2061,15 +2061,17 @@ spec: mcpServers: description: |- MCPServers configures CoderMCPServer objects that use this control - plane. It has no effect yet: the CoderMCPServer controller is not - shipped. + plane. Removing it disables in Coder each server config that a + CoderMCPServer created through this control plane. It has no effect + yet: the CoderMCPServer controller is not shipped. properties: allowForceOn: description: |- AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects - send no MCP request. + create and change nothing, and a server config that one of them + created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. @@ -2081,8 +2083,11 @@ spec: control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an - organization outside this list sends no MCP request. Removing an - organization stops further changes to its servers. Deleting a + organization outside this list creates and changes nothing. Removing + an organization disables in Coder each server config that a + CoderMCPServer created there, with one request that holds only + enabled: false, and then changes nothing else. After the organization + is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not diff --git a/docs/reference/api/codercontrolplane.md b/docs/reference/api/codercontrolplane.md index 1a5da6f3..38356227 100644 --- a/docs/reference/api/codercontrolplane.md +++ b/docs/reference/api/codercontrolplane.md @@ -40,7 +40,7 @@ | `tolerations` | [Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#toleration-v1-core) array | Tolerations are applied to the control plane pod. | | `affinity` | [Affinity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#affinity-v1-core) | Affinity configures pod affinity/anti-affinity rules. | | `topologySpreadConstraints` | [TopologySpreadConstraint](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#topologyspreadconstraint-v1-core) array | TopologySpreadConstraints control pod topology spread. | -| `mcpServers` | [MCPServersSpec](#mcpserversspec) | MCPServers configures CoderMCPServer objects that use this control plane. It has no effect yet: the CoderMCPServer controller is not shipped. | +| `mcpServers` | [MCPServersSpec](#mcpserversspec) | MCPServers configures CoderMCPServer objects that use this control plane. Removing it disables in Coder each server config that a CoderMCPServer created through this control plane. It has no effect yet: the CoderMCPServer controller is not shipped. | | `aiGateway` | [AIGatewaySpec](#aigatewayspec) | AIGateway configures CoderAIGatewayKey objects that use this control plane. | | `workspacePools` | [WorkspacePoolsSpec](#workspacepoolsspec) | WorkspacePools configures CoderWorkspacePool objects that use this control plane. It has no effect yet: the CoderWorkspacePool controller is not shipped. | | `templateTests` | [TemplateTestsSpec](#templatetestsspec) | TemplateTests configures CoderTemplateTest runs against this control plane. | @@ -157,8 +157,8 @@ CoderControlPlane (CoderControlPlane.spec.mcpServers). | Field | Type | Description | | --- | --- | --- | -| `organizations` | string array | Organizations are the names of the Coder organizations whose MCP server configs CoderMCPServer objects can create, change and delete with this control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an organization outside this list sends no MCP request. Removing an organization stops further changes to its servers. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not shipped. | -| `allowForceOn` | boolean | AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects send no MCP request. This field has no effect yet: the CoderMCPServer controller is not shipped. | +| `organizations` | string array | Organizations are the names of the Coder organizations whose MCP server configs CoderMCPServer objects can create, change and delete with this control plane's operator token. This delegates Coder organization-admin power over MCP servers in these organizations to everyone who can create CoderMCPServer objects here. A CoderMCPServer for an organization outside this list creates and changes nothing. Removing an organization disables in Coder each server config that a CoderMCPServer created there, with one request that holds only enabled: false, and then changes nothing else. After the organization is listed again, a spec edit enables the server. Deleting a CoderMCPServer still deletes the config it created. This field has no effect yet: the CoderMCPServer controller is not shipped. | +| `allowForceOn` | boolean | AllowForceOn lets CoderMCPServer objects use availability: force_on, which adds a server to every new chat of each user who can read it, without that user's choice. When false (the default), such objects create and change nothing, and a server config that one of them created is disabled in Coder, as for a removed organization. This field has no effect yet: the CoderMCPServer controller is not shipped. | ### OperatorAccessSpec diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index e4f5bf24..6a5c4a18 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -206,21 +206,32 @@ func (r *CoderMCPServerReconciler) check( return mcpServerWait(mcpServerRetry, wait.reason, "%s", wait.message), nil } org, step, err := resolveMCPServerOrganization(ctx, sdk, server.Spec.Organization) - if step != nil || err != nil { - return step, err + if err != nil { + return nil, err + } + pinned := server.Status.OrganizationID + if step == nil && pinned != "" && pinned != org.ID.String() { + step = mcpServerStalled(mcpServerResync, "OrganizationChanged", + "Coder organization %s now has ID %s, not the pinned ID %s. Re-create this object to use the new organization.", server.Spec.Organization, org.ID, pinned) } - if pinned := server.Status.OrganizationID; pinned != "" && pinned != org.ID.String() { - return mcpServerStalled(mcpServerResync, "OrganizationChanged", - "Coder organization %s now has ID %s, not the pinned ID %s. Re-create this object to use the new organization.", server.Spec.Organization, org.ID, pinned), nil + if step != nil { + // The config of a renamed, deleted or replaced organization stays in + // the pinned organization, and a withdrawn opt-in still disables it. + if pinnedID, parseErr := uuid.Parse(pinned); (step.reason == "OrganizationNotFound" || step.reason == "OrganizationChanged") && parseErr == nil && server.Status.ConfigID != "" { + if optInStep, err := r.optInGate(ctx, sdk, controlPlane, server, before, pinnedID); optInStep != nil || err != nil { + return optInStep, err + } + } + return step, nil } server.Status.OrganizationID = org.ID.String() - if step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, org.ID); step != nil || err != nil { + if step, err := r.optInGate(ctx, sdk, controlPlane, server, before, org.ID); step != nil || err != nil { return step, err } values, step, err := readMCPServerCredentials(ctx, r.Client, server) if step != nil && step.stalled && server.Status.ConfigID != "" { // Fail closed when consent goes (plan G8). - step, err = r.disableMCPServerWithoutConsent(ctx, sdk, server, org.ID, step) + step, err = r.disableMCPServerWithoutConsent(ctx, sdk, server, before, org.ID, step) } if step != nil || err != nil { return step, err @@ -258,12 +269,29 @@ func (r *CoderMCPServerReconciler) check( return step, nil } +// optInGate applies the control plane's opt-in for the organization orgID. +// When the opt-in is withdrawn for a config that this object created, it +// disables the config (#336). Any other stall of the scope check, such as a +// mismatched Coder answer, keeps the config as it is. +func (r *CoderMCPServerReconciler) optInGate( + ctx context.Context, sdk *codersdk.Client, controlPlane *coderv1alpha1.CoderControlPlane, + server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, +) (*mcpServerStep, error) { + step, err := mcpServerScopeGate(ctx, sdk, controlPlane, server.Spec, orgID) + if step != nil && (step.reason == "MCPServersNotAllowed" || step.reason == "ForceOnNotAllowed") && server.Status.ConfigID != "" { + return r.disableMCPServerWithoutOptIn(ctx, sdk, server, before, orgID, step) + } + return step, err +} + // resolveMCPServerOrganization looks up a Coder organization by name. Coder -// compares names without case. +// compares names without case. Only Coder's own 404 proves that the +// organization does not exist: another 404 (a proxy page, "Route not +// found.") waits like any failed read (#358). func resolveMCPServerOrganization(ctx context.Context, sdk *codersdk.Client, name string) (codersdk.Organization, *mcpServerStep, error) { org, err := sdk.OrganizationByName(ctx, name) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): return org, mcpServerWait(mcpServerResync, "OrganizationNotFound", "Coder organization %s does not exist.", name), nil case err != nil: return org, mcpServerWait(mcpServerRetry, "CoderUnavailable", "get organization %s: %s", name, coderErrorSummary(err)), nil @@ -286,7 +314,7 @@ func mcpServerScopeGate( ) (*mcpServerStep, error) { optIn := controlPlane.Spec.MCPServers if optIn == nil { - return mcpServerStalled(0, "MCPServersNotAllowed", "CoderControlPlane %s does not set spec.mcpServers.", controlPlane.Name), nil + return mcpServerStalled(mcpServerResync, "MCPServersNotAllowed", "CoderControlPlane %s does not set spec.mcpServers.", controlPlane.Name), nil } allowed := false for _, name := range optIn.Organizations { @@ -310,7 +338,9 @@ func mcpServerScopeGate( } } if !allowed { - return mcpServerStalled(0, "MCPServersNotAllowed", + // A listed name can resolve later, when it is created or renamed in + // Coder, and no Kubernetes event follows (#358). + return mcpServerStalled(mcpServerResync, "MCPServersNotAllowed", "CoderControlPlane %s does not list Coder organization %s in spec.mcpServers.organizations.", controlPlane.Name, spec.Organization), nil } if spec.Availability == "force_on" && !optIn.AllowForceOn { diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 099d6bf5..e07666d6 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -354,10 +354,17 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { require.NoError(t, k8sClient.Update(ctx, server)) mcp := env.mcpRequests() got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, true, "ForceOnNotAllowed") - require.Equal(t, mcp, env.mcpRequests()) + // The config exists, so the withdrawn opt-in disables it (#336): one read + // and one PATCH. + require.Contains(t, requireMCPServerStep(t, got, true, "ForceOnNotAllowed"), "The controller disabled") + require.Equal(t, mcp+2, env.mcpRequests()) env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}) got, _ = env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + server.Spec.Description = "re-armed" + require.NoError(t, k8sClient.Update(ctx, server)) + got, _ = env.reconcile(ctx, t, server) requireMCPServerStep(t, got, false, "Applied") } @@ -586,8 +593,8 @@ func TestCoderMCPServerWatches(t *testing.T) { }, 30*time.Second, 100*time.Millisecond, reason) } requireReason("Applied") - // A withdrawn consent disables the config in Coder (plan G8), so only a - // spec edit applies it again. + // A withdrawn consent (plan G8) or opt-in (#336) disables the config in + // Coder, so only a spec edit applies it again. rearm := func(description string) { t.Helper() requireReason("DisabledExternally") @@ -609,7 +616,7 @@ func TestCoderMCPServerWatches(t *testing.T) { env.setOptIn(ctx, t, nil) requireReason("MCPServersNotAllowed") env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) - requireReason("Applied") + rearm("opt-in back") // Another control plane that uses a credential Secret for its own access // makes it unusable. Deleting that control plane makes it usable again. diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index e2fe0096..3a4b9e62 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/internal/controller/codermcpserver_disable.go @@ -32,7 +32,7 @@ import ( // appliedGeneration, so a later sync reports DisabledExternally and only a // spec edit after the disable enables the config again. func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( - ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, failed *mcpServerStep, + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, failed *mcpServerStep, ) (*mcpServerStep, error) { id, err := uuid.Parse(server.Status.ConfigID) if err != nil || id == uuid.Nil || failed == nil || !failed.stalled { @@ -45,20 +45,9 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( unchecked := func(format string, args ...any) *mcpServerStep { return mcpServerWait(mcpServerRetry, failed.reason, "Coder can still send the credentials of MCP server config %s (%s). %s", id, fmt.Sprintf(format, args...), failed.message) } - found, err := sdk.MCPServerConfigByID(ctx, orgID, id) - switch { - case isCoderResourceNotFound(err): - // Only Coder's own 404 proves that the config is gone (#337). - return stalled("MCP server config %s does not exist in Coder.", id), nil - case err != nil: - return unchecked("get MCP server config: %s", coderErrorSummary(err)), nil - } - if found.ID != id || found.OrganizationID != orgID || found.Slug != server.Spec.Slug { - return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", - "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id), nil - } - if !found.Enabled { - return stalled("MCP server config %s is disabled in Coder.", id), nil + found, step := readMCPServerToDisable(ctx, sdk, server, orgID, id, stalled, unchecked) + if step != nil { + return step, nil } // user_oidc sends each chat user's login token to url (plan F14). Without // a readable credential the controller cannot set the auth type back, so @@ -76,6 +65,75 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( return stalled("Coder sends each credential of MCP server config %s only to origins that its recorded Secret allows.", id), nil } + if step, err := r.sendMCPServerDisable(ctx, sdk, server, before, orgID, id, failed.reason, unchecked); step != nil || err != nil { + return step, err + } + // The reason stays the credential reason, so it does not flap between + // this reconcile and the next one, which finds the config disabled. + return stalled("The controller disabled MCP server config %s in Coder: a credential Secret no longer proves consent for a credential that Coder uses. After the Secret is fixed, a spec edit enables the config.", id), nil +} + +// readMCPServerToDisable reads the config of a fail-closed path. It returns +// a step when there is nothing to disable or the read failed: a config that +// is gone (only Coder's own 404 proves it, #337) or already disabled, an +// answer for another config, or an error. +func readMCPServerToDisable( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID, id uuid.UUID, + stalled, unchecked func(format string, args ...any) *mcpServerStep, +) (codersdk.MCPServerConfig, *mcpServerStep) { + found, err := sdk.MCPServerConfigByID(ctx, orgID, id) + switch { + case isCoderResourceNotFound(err): + // The kill switch holds a gone config too: a pending generation must + // not create an enabled config again once consent or the opt-in comes + // back, before a spec edit. + server.Status.AppliedGeneration = server.Generation + return found, stalled("MCP server config %s does not exist in Coder.", id) + case err != nil: + return found, unchecked("get MCP server config: %s", coderErrorSummary(err)) + } + if found.ID != id || found.OrganizationID != orgID || found.Slug != server.Spec.Slug { + return found, mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", + "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id) + } + if !found.Enabled { + // The disable answer can be lost after Coder committed it. Whoever + // disabled the config, the kill switch now holds this generation too, + // so an opt-in or consent that comes back does not apply a pending + // generation that enables the config. + server.Status.AppliedGeneration = server.Generation + return found, stalled("MCP server config %s is disabled in Coder.", id) + } + return found, nil +} + +// sendMCPServerDisable sends the one PATCH of a fail-closed path. It holds +// only enabled false: no credential and no destination field. It returns a +// wait step when the PATCH failed or Coder answered for another or an +// enabled config. +// +// The disable counts as an external disable of this generation, also of one +// that is not applied yet, so the DisabledExternally kill switch holds until +// the next spec edit. The controller writes appliedGeneration to status +// before the PATCH: Coder can commit the PATCH while its answer or the next +// status write is lost, and a pending generation must not enable the config +// once consent or the opt-in comes back. A failed status write sends no +// PATCH. +func (r *CoderMCPServerReconciler) sendMCPServerDisable( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, + orgID, id uuid.UUID, reason string, unchecked func(format string, args ...any) *mcpServerStep, +) (*mcpServerStep, error) { + if before == nil { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs the status before the reconcile to be disabled", server.Namespace, server.Name) + } + if server.Status.AppliedGeneration != server.Generation { + server.Status.AppliedGeneration = server.Generation + applyMCPServerStep(server, mcpServerWait(0, reason, "Disabling MCP server config %s in Coder.", id)) + if err := r.writeStatus(ctx, server, before); err != nil { + return nil, err + } + *before = *server.Status.DeepCopy() + } disabled := false updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, codersdk.UpdateMCPServerConfigRequest{Enabled: &disabled}) if err != nil { @@ -85,13 +143,37 @@ func (r *CoderMCPServerReconciler) disableMCPServerWithoutConsent( return mcpServerWait(mcpServerRetry, "CoderAnswerMismatch", "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id), nil } - // The disable counts as an external disable of this generation, also of - // one that is not applied yet: the kill switch then holds until the next - // spec edit. - server.Status.AppliedGeneration = server.Generation - // The reason stays the credential reason, so it does not flap between - // this reconcile and the next one, which finds the config disabled. - return stalled("The controller disabled MCP server config %s in Coder: a credential Secret no longer proves consent for a credential that Coder uses. After the Secret is fixed, a spec edit enables the config.", id), nil + return nil, nil +} + +// disableMCPServerWithoutOptIn handles a config that this object created +// when the control plane withdraws its opt-in (#336): spec.mcpServers is +// removed, it no longer lists the organization, or it no longer allows +// force_on for a force_on server. The operator may no longer manage MCP +// servers there, so the controller disables the config with the one PATCH +// of sendMCPServerDisable. It reads no credential. The disable is a kill +// switch, as in disableMCPServerWithoutConsent: a restored opt-in needs a +// spec edit to enable the config again. +func (r *CoderMCPServerReconciler) disableMCPServerWithoutOptIn( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, orgID uuid.UUID, failed *mcpServerStep, +) (*mcpServerStep, error) { + id, err := uuid.Parse(server.Status.ConfigID) + if err != nil || id == uuid.Nil || failed == nil || !failed.stalled { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs a config ID and a stalled opt-in failure to be disabled", server.Namespace, server.Name) + } + stalled := func(format string, args ...any) *mcpServerStep { + return mcpServerStalled(mcpServerResync, failed.reason, "%s %s", fmt.Sprintf(format, args...), failed.message) + } + unchecked := func(format string, args ...any) *mcpServerStep { + return mcpServerWait(mcpServerRetry, failed.reason, "Coder can still use MCP server config %s (%s). %s", id, fmt.Sprintf(format, args...), failed.message) + } + if _, step := readMCPServerToDisable(ctx, sdk, server, orgID, id, stalled, unchecked); step != nil { + return step, nil + } + if step, err := r.sendMCPServerDisable(ctx, sdk, server, before, orgID, id, failed.reason, unchecked); step != nil || err != nil { + return step, err + } + return stalled("The controller disabled MCP server config %s in Coder: the control plane no longer allows it. After the opt-in is back, a spec edit enables the config.", id), nil } // errMCPServerSecretChanged reports that a Secret changed between two reads diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go new file mode 100644 index 00000000..1ca2172b --- /dev/null +++ b/internal/controller/codermcpserver_optin_test.go @@ -0,0 +1,369 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "sync/atomic" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// #336: when the control plane withdraws its opt-in, the controller disables +// a config that this object created with one PATCH that holds only enabled +// false, and it reads no credential Secret. No other write follows while the +// opt-in stays withdrawn. The disable is a kill switch: the restored opt-in +// gives DisabledExternally, and a spec edit enables the config again. +func TestCoderMCPServerDisablesWithoutOptIn(t *testing.T) { + t.Parallel() + forceOn := func(s *coderv1alpha1.CoderMCPServerSpec) { s.Availability = "force_on" } + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + optIn *coderv1alpha1.MCPServersSpec // The opt-in before the withdrawal. + withdraw *coderv1alpha1.MCPServersSpec + reason string + }{ + {name: "opt-in removed", reason: "MCPServersNotAllowed"}, + {name: "organization unlisted", withdraw: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other"}}, reason: "MCPServersNotAllowed"}, + { + name: "force_on no longer allowed", spec: forceOn, reason: "ForceOnNotAllowed", + optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}, + withdraw: &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + optIn := tc.optIn + if optIn == nil { + optIn = &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}} + } + env.setOptIn(ctx, t, optIn) + got, _ := env.applied(ctx, t, tc.spec) + before := env.stored(t, got) + + // Count the reads of the credential Secret from here on. + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + var secretReads atomic.Int32 + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok && key.Name == "github-mcp" { + secretReads.Add(1) + } + return c.Get(ctx, key, obj, opts...) + }, + }) + env.setOptIn(ctx, t, tc.withdraw) + + // A failed disable is retried, and the message says that Coder + // can still use the config. + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, result := env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, tc.reason), "Coder can still use MCP server config") + require.Equal(t, 30*time.Second, result.RequeueAfter) + + mark := len(env.writeRoutes()) + got, result = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, tc.reason), "The controller disabled MCP server config") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServer, writes[0].route) + require.JSONEq(t, `{"enabled":false}`, string(writes[0].body)) + before.view.Enabled = false + require.Equal(t, before, env.stored(t, got), "only enabled changed") + + mark = len(env.writeRoutes()) + for range 2 { + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, tc.reason), "is disabled in Coder") + } + require.Empty(t, env.writesSince(mark), "no write while the opt-in stays withdrawn") + require.Zero(t, secretReads.Load(), "no credential read") + + env.setOptIn(ctx, t, optIn) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "re-armed" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.stored(t, got).view.Enabled) + }) + } +} + +// An object without a config ID has nothing in Coder to disable, so a +// withdrawn opt-in still sends no MCP request (plan S17). +func TestCoderMCPServerOptInWithdrawnWithoutConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.setOptIn(ctx, t, nil) + got, result := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.Equal(t, 10*time.Minute, result.RequeueAfter, "a timed resync (#358)") + require.Zero(t, env.mcpRequests()) +} + +// #358: only Coder's own 404 proves that an organization does not exist. A +// proxy page or "Route not found." on the lookup of spec.organization, or of +// a listed name, waits with CoderUnavailable instead of OrganizationNotFound +// or MCPServersNotAllowed. An unlisted organization resyncs on a timer, +// because a listed name can resolve later without a Kubernetes event. +func TestCoderMCPServerOrganizationNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + for _, lookup := range []string{"own", "listed"} { + t.Run(name+"/"+lookup, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.addOrganization("alias") + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"alias"}}) + server := env.create(ctx, t, nil) + if lookup == "listed" { + // The object's own organization resolves first. + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { return answer }}) + } + env.fake.failNext(routeOrganization, fault) + got, result := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Zero(t, env.mcpRequests()) + + got, result = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + }) + } + } +} + +// A disable answer can be lost after Coder committed it. The next reconcile +// then finds the config disabled and records the generation, so the +// restored opt-in does not apply a pending generation that enables it. +func TestCoderMCPServerOptInFoundDisabledHolds(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + + env.setOptIn(ctx, t, nil) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, "MCPServersNotAllowed"), "the disable failed") + require.False(t, env.stored(t, got).view.Enabled, "the PATCH committed") + + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "is disabled in Coder") + require.Equal(t, got.Generation, got.Status.AppliedGeneration) + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + stored := env.stored(t, got) + require.False(t, stored.view.Enabled) + require.Empty(t, stored.view.Description, "the pending generation is not applied") + require.Equal(t, id, stored.view.ID) +} + +// Only a withdrawn opt-in disables the config. Another stall of the scope +// check, such as a mismatched Coder answer for a listed name, keeps the +// config as it is and sends no MCP request. +func TestCoderMCPServerScopeMismatchKeepsConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.addOrganization("alias") + got, _ := env.applied(ctx, t, nil) + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"alias"}}) + // The object's own organization resolves, then the listed name gets an + // answer for another organization. + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { return answer }}) + env.fake.failNext(routeOrganization, fakeFault{Rewrite: func(answer any) any { + org := answer.(codersdk.Organization) + org.Name = "other" + return org + }}) + mcp := env.mcpRequests() + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "CoderAnswerMismatch") + require.Equal(t, mcp, env.mcpRequests()) + require.True(t, env.stored(t, got).view.Enabled) +} + +// A disable PATCH can commit while its answer is lost, and the opt-in can +// come back before the next reconcile. The controller wrote +// appliedGeneration before the PATCH, so the sync then finds +// DisabledExternally and does not apply a pending generation. When that +// status write fails, the controller sends no PATCH. +func TestCoderMCPServerOptInDisableMarkerFirst(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + lostStatus bool // The status write before the PATCH fails. + }{{name: "answer lost, opt-in back"}, {name: "marker write fails", lostStatus: 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.applied(ctx, t, nil) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + env.setOptIn(ctx, t, nil) + + if tc.lostStatus { + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + r := &controller.CoderMCPServerReconciler{Client: interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { + return errors.New("lost status write") + }, + }), Scheme: scheme} + mark := len(env.writeRoutes()) + _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.ErrorContains(t, err, "lost status write") + require.Empty(t, env.writesSince(mark), "no PATCH without the marker") + require.True(t, env.stored(t, got).view.Enabled) + return + } + + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, false, "MCPServersNotAllowed"), "the disable failed") + require.False(t, env.stored(t, got).view.Enabled, "the PATCH committed") + require.Equal(t, got.Generation, got.Status.AppliedGeneration, "written before the PATCH") + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DisabledExternally") + require.Empty(t, env.writesSince(mark)) + require.Empty(t, env.stored(t, got).view.Description, "the pending generation is not applied") + }) + } +} + +// A renamed, deleted or replaced organization keeps the config in the +// pinned organization. A withdrawn opt-in still disables it there, while a +// name that is still listed changes nothing. +func TestCoderMCPServerOptInUsesPinnedOrganization(t *testing.T) { + t.Parallel() + gone := fakeFault{Status: http.StatusNotFound, Message: coderResourceNotFound} + replaced := fakeFault{Rewrite: func(answer any) any { + org := answer.(codersdk.Organization) + org.ID = uuid.New() + return org + }} + for _, tc := range []struct { + name string + fault fakeFault + reason string + withdraw bool + }{ + {name: "not found, opt-in removed", fault: gone, reason: "OrganizationNotFound", withdraw: true}, + {name: "replaced, opt-in removed", fault: replaced, reason: "OrganizationChanged", withdraw: true}, + {name: "not found, still listed", fault: gone, reason: "OrganizationNotFound"}, + {name: "replaced, still listed", fault: replaced, reason: "OrganizationChanged"}, + } { + 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.applied(ctx, t, nil) + if tc.withdraw { + env.setOptIn(ctx, t, nil) + } + env.fake.failNext(routeOrganization, tc.fault) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + if !tc.withdraw { + message := requireMCPServerStep(t, got, tc.reason == "OrganizationChanged", tc.reason) + require.NotContains(t, message, "disabled") + require.Empty(t, env.writesSince(mark)) + require.True(t, env.stored(t, got).view.Enabled) + return + } + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "The controller disabled MCP server config") + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"enabled":false}`, string(writes[0].body)) + require.False(t, env.stored(t, got).view.Enabled) + }) + } +} + +// A config that is gone while a generation is pending and the opt-in is +// withdrawn: the kill switch holds it too. The restored opt-in gives +// DeletedExternally and no create, and only a spec edit creates the config +// again. +func TestCoderMCPServerOptInGoneConfigHolds(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, id)) + env.setOptIn(ctx, t, nil) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "does not exist in Coder") + require.Equal(t, got.Generation, got.Status.AppliedGeneration) + + env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "DeletedExternally") + require.Empty(t, env.writesSince(mark), "no create before a spec edit") + require.Equal(t, 1, env.posts()) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "after the disable" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 2, env.posts()) +} diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 91468374..3c53d444 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -115,7 +115,11 @@ func newFakeCoder(t *testing.T) *fakeCoder { return found(f.userByIdent(r.PathValue("user"))) }) f.route(mux, "GET /api/v2/organizations/{org}", routeOrganization, func(r *http.Request) (int, any) { - return found(f.orgByIdent(r.PathValue("org"))) + org, ok := f.orgByIdent(r.PathValue("org")) + if !ok { // coderd/httpmw/organizationparam.go:94 answers httpapi.ResourceNotFound. + return http.StatusNotFound, fakeResourceNotFound + } + return http.StatusOK, org }) f.route(mux, "GET /api/v2/organizations/{org}/members/{user}", routeOrgMember, f.getOrgMember) f.route(mux, "GET /api/v2/organizations/{org}/templates/{name}", routeTemplateByName, func(r *http.Request) (int, any) {