Repository navigation
🤖 feat: disable the CoderMCPServer config when the control plane withdraws its opt-in - #360
Conversation
…draws its opt-in When spec.mcpServers is removed, no longer lists the organization, or no longer allows force_on for a force_on server, the controller now disables a config that the object created, with one PATCH that holds only enabled: false. It reads no credential. The disable is a kill switch, so a restored opt-in needs a spec edit to enable the config. The read and the PATCH are shared with the consent disable. Fixes #336 Refs #155 Change-Id: Iebea23e388d9c43540daebf8380f256c59f282cf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…lookup and resync MCPServersNotAllowed The organization lookup treated any 404 as a missing organization. A proxy page or "Route not found." for a listed name then skipped that name, so the scope check stalled with MCPServersNotAllowed, which had no timed requeue. The lookup now uses isCoderResourceNotFound, so another 404 waits with CoderUnavailable, and MCPServersNotAllowed resyncs every 10 minutes. The fake answers an unknown organization like Coder. Fixes #358 Refs #155 Change-Id: I90fa60c56e9f867d4c7c53bce35c1be49be366a8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de723169c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…it the opt-in disable to opt-in stalls, and document it - A read that finds the config already disabled now records the generation, so a lost disable answer and a restored opt-in do not apply a pending generation that enables the config. - Only MCPServersNotAllowed and ForceOnNotAllowed run the opt-in disable. Another stall of the scope check, such as a mismatched Coder answer, keeps the config. - The spec.mcpServers field docs describe the fail-closed disable. Refs #155 Change-Id: If9ea006bcd30e178dc579172f03ddce31b0b3923 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5dd49ac2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Change-Id: Ib67e349731c2986ff930b25276e6ba3ed6671b2c Signed-off-by: Thomas Kosiewski <tk@coder.com>
…isable through the pinned organization - The opt-in and consent disables write appliedGeneration to status before the disable PATCH, and a failed status write sends no PATCH. A lost disable answer followed by a restored opt-in then gives DisabledExternally instead of applying a pending generation. - When the organization name no longer resolves (OrganizationNotFound) or resolves to another ID (OrganizationChanged), a withdrawn opt-in still disables the config through the pinned organization ID. Refs #155 Change-Id: Ie54facd7656489a5436d639734912e3c5c26050b Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c187d5cca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… and document appliedGeneration - A fail-closed disable that finds the config gone (Coder's own 404) now records the generation, so a restored opt-in or consent gives DeletedExternally instead of creating an enabled config before a spec edit. - The status.appliedGeneration doc now says that a kill switch also sets it, before the disable is sent. The dormant CRD is regenerated. Refs #155 Change-Id: I781f44dfea54d7834faf712e75d4fe764f65323f Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dacbde9c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… consent check, and test the consent disable marker (#363) ## Summary The dormant `CoderMCPServer` consent check now requires https for the OAuth2 revocation URL that Coder holds, as the spec does. This PR also adds the consent-path tests for the disable marker that #360 introduced. The kind is not registered, so users see no change. **Base branch:** `main`. #360 landed as `69c18dd5`. I merged `main` into this branch with a merge commit, and the diff against `main` is the reviewed M6f change only. Fixes #311 Fixes #334 ## Background - #311 (plan gate G8): #330 disables the config when a credential Secret withdraws consent. #311 stayed open for one case: with a pending generation, a disable whose answer or status write is lost, followed by restored consent, applied the pending generation and enabled the config without a spec edit. #360 fixes this for both fail-closed paths: the controller writes `appliedGeneration` to status before the disable PATCH, and a read that finds the config disabled or gone also records the generation. This PR adds the tests for the consent path. - #334: Coder accepts an `http` revocation URL for loopback hosts, while the CRD and the controller require `https` for `spec.auth.oauth2.revocationURL`. The consent check of the disable path checked Coder's revocation URL only against the Secret's origins, so a dashboard-set `http` loopback revocation URL that the Secret listed passed the check. ## Implementation - `mcpServerConsentHolds` returns false when Coder's revocation URL is not `https` (4 lines), so the config is disabled. - New tests in `internal/controller/codermcpserver_marker_test.go`. ## Validation - `TestCoderMCPServerLostDisableAnswerHolds` (#311): with a pending generation, the Secret is deleted, and the disable PATCH commits but answers 502. Status already holds the current `appliedGeneration`. The Secret comes back before any other reconcile, and the sync gives `DisabledExternally` with no write and the pending description not applied. A spec edit after the disable enables the config. - `TestCoderMCPServerDisableNeedsTheMarker` (#311): when the status write before the disable PATCH fails, the reconcile returns the error and sends no PATCH. - `TestCoderMCPServerFoundDisabledHolds` (#311): with a pending generation and a deleted Secret, Coder already holds the disable (a lost answer). The disable path records the generation, and the restored Secret gives `DisabledExternally` with no write. - `TestCoderMCPServerConsentCheckNeedsHTTPSRevocation` (#334): Coder holds `http://127.0.0.1:8080/revoke`, and the Secret lists that origin. A spec URL edit that fails the credential read gives exactly one `{"enabled":false}` PATCH. - Mutation check: without the https check, the #334 test fails. The marker mutants are in #360. - Local gates on this head: `make verify-vendor`, `make build`, `make lint`, `GOFLAGS=-mod=vendor go tool govulncheck ./...`, `make test`, `make test-integration`, `go test -race ./internal/controller/`, and `make docs-check` pass. `make codegen`, `make manifests`, `make build-installer` and `make docs-reference` leave no diff. ## Size 158 lines added, all hand-written: 4 in `codermcpserver_disable.go` and 154 in the new tests. Refs #155 --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$171.92`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=171.92 --> --------- Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 participant
Summary
When the control plane withdraws its
CoderMCPServeropt-in, the dormant reconciler now disables the config that the object created in Coder, with one PATCH that holds onlyenabled: false. It reads no credential. The organization lookup also trusts only Coder's own 404 now, andMCPServersNotAllowedresyncs on a timer. The kind is not registered, so users see no change.Fixes #336
Fixes #358
Background
CoderAIProviderdoes.MCPServersNotAllowed, and that stall had no timed requeue.How the opt-in disable works
When the scope gate stalls with
MCPServersNotAllowedorForceOnNotAllowedandstatus.configIDis set, the reconciler runsdisableMCPServerWithoutOptIn. Any other stall of the scope gate, such as a mismatched Coder answer for a listed name, keeps the config as it is. The opt-in stalls come from three cases:spec.mcpServersis removed (MCPServersNotAllowed)MCPServersNotAllowed)allowForceOnfor aforce_onserver (ForceOnNotAllowed)The disable path:
CoderAnswerMismatch. A failed read waits 30 s with "Coder can still use MCP server config".PATCH {"enabled": false}, with no credential and no destination field. A failed PATCH, or an answer for another or an enabled config, waits 30 s and retries.appliedGenerationto status. 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 the opt-in comes back. A failed status write sends no PATCH. The consent disable of 🤖 feat: disable the CoderMCPServer config when a credential Secret withdraws consent #330 uses the same step.DisabledExternallyand writes nothing until a spec edit.The scope gate runs before the credential read, so this path never reads a Secret.
When the organization name no longer resolves (
OrganizationNotFound) or now resolves to another ID (OrganizationChanged), the config stays in the pinned organization. The reconciler then runs the same opt-in check with the pinned ID fromstatus.organizationID, and a withdrawn opt-in still disables the config there. A name that is still listed changes nothing, and the original step is reported. The read and the PATCH are now shared helpers (readMCPServerToDisable,sendMCPServerDisable) used by both the consent disable and the opt-in disable. An object without a config ID still sends no MCP request (plan S17).API docs
The field docs of
spec.mcpServers,organizationsandallowForceOnnow describe the fail-closed disable: a removed organization, a removedspec.mcpServersor a withdrawnallowForceOndisables the configs thatCoderMCPServerobjects created, with one request that holds onlyenabled: false, and a spec edit enables a server again. The CRD,dist/install.yamlanddocs/reference/api/codercontrolplane.mdare regenerated.The
status.appliedGenerationfield doc now says that a kill switch also sets it, before the disable is sent, and thatReadytells whether Coder holds the spec. The dormantCoderMCPServerCRD is regenerated.#358
resolveMCPServerOrganizationusesisCoderResourceNotFound. Any other 404 waits 30 s withCoderUnavailable, both forspec.organizationand for a listed name.MCPServersNotAllowedstalls resync every 10 minutes. A listed name can resolve later, when it is created or renamed in Coder, and no Kubernetes event follows.coderd/httpmw/organizationparam.go:94).Review record
Codex round 1 (normal review of
de72316) found three items, all fixed ina5dd49ac: record the generation when the config is found disabled, disable only on opt-in stalls, and update the API field docs. The security review ofde72316found no issues.Codex round 2 (normal review of
a5dd49a) found two items, both fixed in7c187d5c: write the disable marker before the PATCH (moved here from the planned M6f), and disable through the pinned organization when its name no longer resolves. The security review ofa5dd49afound no issues. Before that fix, I mergedmaininto this branch (merge commit531c4712) because #346 had changed the regenerated files.Codex round 3 (normal review of
7c187d5) found two items, both fixed in1dacbde9: record the generation when the disable read finds the config gone, and document the kill-switch meaning ofstatus.appliedGeneration. The security review of7c187d5found no issues.CI's first
testrun onde72316failed while downloading Go modules (proxy.golang.orgstream error), before any test ran.Validation
TestCoderMCPServerDisablesWithoutOptIn, for each of the three withdrawals:{"enabled":false}, and stalls with the scope reason. Coder holds the same state with onlyenabledchanged.DisabledExternallywith no write. A spec edit applies and enables the config.TestCoderMCPServerOptInWithdrawnWithoutConfig: with no config ID, no MCP request, and a 10-minute requeue.TestCoderMCPServerOrganizationNotCoder404: an HTML 404 andRoute not found., on the lookup of the object's organization and of a listed name, each give aCoderUnavailablewait (30 s) and no MCP request. The next reconcile givesMCPServersNotAllowedwith a 10-minute requeue.TestCoderMCPServerOptInFoundDisabledHolds: with a pending generation, the opt-in disable PATCH commits but answers 502. The next reconcile finds the config disabled and records the generation. The restored opt-in givesDisabledExternallywith no write, and the pending description is not applied.TestCoderMCPServerOptInDisableMarkerFirst: with a pending generation, the disable PATCH commits but answers 502, and status already holds the currentappliedGeneration. The opt-in comes back before any other reconcile, and the sync givesDisabledExternallywith no write and the pending description not applied. When the status write before the PATCH fails, the reconcile returns the error and sends no PATCH.TestCoderMCPServerOptInUsesPinnedOrganization: Coder's own 404, or an answer with another ID, for the object's organization, with the opt-in removed, gives exactly one{"enabled":false}PATCH through the pinned ID. With the name still listed, the reconcile reportsOrganizationNotFoundorOrganizationChangedand writes nothing.TestCoderMCPServerOptInGoneConfigHolds: with a pending generation, the config is deleted in Coder and the opt-in is withdrawn. The disable path stalls with "does not exist in Coder" and records the generation. The restored opt-in givesDeletedExternallywith no create. A spec edit then creates the config again.TestCoderMCPServerScopeMismatchKeepsConfig: a mismatched Coder answer for a listed name givesCoderAnswerMismatch, no MCP request, and the config stays enabled.TestCoderMCPServerControlPlaneAndOptInandTestCoderMCPServerWatchesnow expect the disable and the re-arm by spec edit.isCoderNotFoundon the organization lookup, no timed requeue for an unlisted organization, no generation recorded for a config found disabled, a disable on any stall of the scope gate, no status write before the disable PATCH, an ignored error of that write, no pinned-organization fallback, and no generation recorded for a gone config.make verify-vendor,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./...,make test,make test-integration,go test -race ./internal/controller/, andmake docs-checkpass.make codegen,make manifests,make build-installerandmake docs-referenceleave no diff.Size
575 lines added and 60 removed. 44 changed lines are regenerated (CRDs,
dist/install.yaml, API reference). The rest are hand-written: 126 changed lines incodermcpserver_disable.go(most of them move the read and the PATCH into shared helpers), 52 in the reconciler, 6 in the fake Coder, 369 in the new tests, 17 in existing tests, and 21 in the API field docs. The three review rounds added 307 of these lines, most of them tests. Without generated code and tests it is 205 changed lines.Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$171.92