Repository navigation
🤖 feat: add the CoderMCPServer kill switches and the user_oidc correction - #329
Conversation
The reconciler now reads the config on every reconcile and sets back any field that differs with one sparse PATCH. A PATCH that sets a URL or the auth type carries every credential of the auth type. Credentials of other auth types are cleared and checked on every apply (#314). The ACL sync also runs every reconcile, so a user grant added in the dashboard goes. A config deleted before its generation is applied is created again. Refs #155 Change-Id: Iecb93e07d2fe2d13630dd026827c948afa0c6a73 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tion After a full apply, a dashboard disable, credential clear or auth type change now stops every write until the spec changes. An observed user_oidc auth type is always set back with a PATCH that holds only auth fields, so a disable stays. An in-memory SHA-256 of each sent credential skips the resend after a metadata-only Secret change. Refs #155 Change-Id: I0ded9edf8413c7dff9c7cb3aaf1186ccf2681188 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.
Security findingsAdvisory findings (1)ℹ️ 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: 0ea11aa97b
ℹ️ 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".
A PATCH that clears a credential left its hash in memory. After an auth type round trip, the hash claimed that Coder held the Secret's value while Coder held a key set in the dashboard, so the key was not sent. Refs #155 Change-Id: Id76a171929671334c958fc990bce5eec696ce132 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: If9a6a706575752c9342fd9fcd88cf1cc9a752545 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: 01e015a198
ℹ️ 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".
…t sends them A credential PATCH can commit in Coder while its answer is lost. The hashes were updated only after a good answer, so after a Secret change from A to B (lost answer) and back to A, the retry matched the old hash for A and sent nothing while Coder held B. The controller now drops the hashes of the fields a PATCH sends before the PATCH. Refs #155 Change-Id: Id641bb5a2efa10a5000cf4108339a2e4bdaa96ea 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: bbd8cbb239
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: bbd8cbb239
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…drop stale hashes on every sync - The user_oidc correction while a kill switch holds sent the credentials without url. A dashboard edit of url, user_oidc and a disable then made the controller store the Secret's credential next to a url the Secret does not allow. The auth-only PATCH now carries url too. - A change of the auth type whose answer was lost kept the old field's hash, because the retry found nothing to send. Every sync now drops the hashes of fields that the spec's auth type does not use. Refs #155 Change-Id: Ie46ffe4d585ddc522a213094ca190964d0e67a3b Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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. |
1 participant
Summary
After a full apply, the dormant
CoderMCPServerreconciler now treats a dashboard disable, credential clear or auth type change as a kill switch: it reportsStalled=Trueand writes nothing until the spec changes. An observeduser_oidcauth type is always set back, with a PATCH that holds only auth fields. An in-memory hash skips the credential resend after a metadata-only Secret change. The kind is not registered, so users see no change.Base branch:
main. #328 landed as45f27ee560. I mergedmaininto this branch with a merge commit, and the diff againstmainis the reviewed M6b change only.Background
#328 reads the config on every reconcile and sets back every difference. That also undoes an emergency disable or credential clear in the dashboard. Plan 4.7 item 2 (D9) says an admin's disable, delete or credential clear wins until someone edits the spec. #328 already holds the delete. This PR adds the other three.
Kill switches (plan 4.7)
Only when
status.appliedGeneration == metadata.generation:enabled: falsewhile the spec says enabledDisabledExternallyhas_*flag false for a credential of the spec's auth type, and the auth type is notuser_oidcCredentialClearedExternallyA dashboard change to another auth type clears the old credentials (plan F9), so it lands in the second row. Each stall resyncs every 10 minutes, because no Kubernetes event follows a dashboard edit. A spec edit makes the generation unapplied, so the next sync sends the full state again: enable, credentials, auth type. An apply that fails keeps the old
appliedGeneration.user_oidc(plan 4.7 item 4, S19)user_oidcsends each chat user's login token tourl(plan F14), and the CRD never allows it. An observeduser_oidcis always drift, never a kill switch, also when the switch cleared a credential. When a disable kill switch also holds, the controller sends only the auth fields of the sparse PATCH (auth type, header, credentials, OAuth2 fields) plusurl, so the config stays disabled, and the stall message says that the auth type was set back.urlgoes with them because the auth fields can carry credentials, and Coder must never store a credential next to a URL that the Secret does not allow (review round 3).Metadata-only Secret changes (plan 4.6 item 4)
A label or annotation change moves the Secret's resourceVersion, so the change token differs. The reconciler keeps a SHA-256 of each value that Coder holds, in process memory only, keyed by object UID and credential field. When the token differs but the hash matches, it records the new token and sends nothing. The create and every PATCH that sends a credential update the map. After a restart the map is empty, so one resend can happen. Every sync first drops the hashes of fields that the spec's auth type does not use, because a change of the auth type cleared them in Coder, also when its answer was lost. Before a PATCH that sends a credential, the reconciler also drops that field's hash: Coder can commit the PATCH while the answer is lost, so a retry must send the value again (review round 3). A PATCH also drops the hashes of fields that the spec's auth type does not use, because Coder cleared those credentials. Otherwise an auth type round trip could skip a needed resend (review round 1).
Review record
Codex round 1 (normal review of
0ea11aa): three findings.c2de88cf: stale hashes after a PATCH that clears a credential.4e84a445):user_oidcwhile the credential read fails. A read failure is the 🤖 feat: disable the CoderMCPServer config when a credential Secret withdraws consent #330 layer.has_custom_headersflag and no header names, so this is plan risk R2, like a dashboard replacement of a value.The security review of
0ea11aafound no issues.Round 2 (
c2de88cf): normal review "Didn't find any major issues", security review no issues.Round 3 (refresh on
mainas01e015a1): the security review found no issues. The normal review found one item, fixed inbbd8cbb2: hashes were updated only after a good answer, so a lost answer followed by a Secret revert skipped a needed resend. I checked the other parts of the sparse PATCH for the same pattern: every other field is compared with Coder's GET on each reconcile, the change tokens in status only move forward (a Secret's resourceVersion grows and a recreated Secret has a new UID), credentials of other auth types are cleared from Coder'shas_*flags, and the create records hashes only after a matching 201.The review of
bbd8cbb2(round 3, normal and security) found two more items. I fixed both as blockers in632a7686, and I requested no further review:user_oidccorrection while a kill switch holds sent the credentials withouturl. A dashboard edit ofurl,user_oidcand a disable then made the controller store the Secret's credential next to a URL the Secret does not allow.Validation
New tests in
internal/controller/codermcpserver_sync_test.go:TestCoderMCPServerSyncKillSwitches(S14): a dashboard disable, credential clear and auth type change tononeeach giveStalled=Truewith their reason and a 10-minute requeue on two reconciles, with zero writes. A spec edit whose PATCH fails givesCoderUnavailableand keeps the oldappliedGeneration. The next reconcile applies: enabled,api_key, the key, the new description.TestCoderMCPServerSyncUserOIDC(S19): fornone,api_key,custom_headersandoauth2configs, for anapi_keyconfig that is also disabled, and for one that is also disabled with itsurlmoved to another origin, a dashboard switch touser_oidcgives exactly one PATCH. Every key of its body is an auth field orurl, itsauth_typeis the spec's, and a movedurlis set back in the same PATCH. Coder then holds the applied state again (the disabled config stays disabled withDisabledExternally). The next reconcile sends nothing.TestCoderMCPServerSyncSecretMetadataChange(S13): an annotation change on the Secret gives no write and the new token in status. After a restart (a new reconciler), the next annotation change sends{"api_key_value":...}once.TestCoderMCPServerSyncAuthRoundTripResends:api_keytonone, then a dashboard key, then the spec back toapi_keysends the Secret's key again. A second case loses the answer of the PATCH tonone.TestCoderMCPServerSyncLostAnswerResends: the Secret goes from A to B, the PATCH with B commits in Coder but answers 502, and the Secret goes back to A. The next reconcile sends exactly{"api_key_value":A}, and Coder holds A.DisabledExternallykill switchCredentialClearedExternallykill switchuser_oidcheld by a kill switch instead of set backurlin the auth-only PATCHOne change survives: narrowing the ACL also while a kill switch holds. It only removes access, and no test combines an ACL change with a
user_oidcswitch on a disabled 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
402 lines added and 19 removed, all hand-written: 164 changed lines in
codermcpserver_apply.go, 12 in the reconciler and the create step, and 245 in the new tests.Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$127.81