Repository navigation
Commit 06ce2b9
authored
🤖 feat: add the CoderMCPServer kill switches and the user_oidc correction (#329)
## Summary
After a full apply, the dormant `CoderMCPServer` reconciler now treats a
dashboard disable, credential clear or auth type change as a kill
switch: it reports `Stalled=True` and writes nothing until the spec
changes. An observed `user_oidc` auth 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 as `45f27ee560`. I merged `main`
into this branch with a merge commit, and the diff against `main` is 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`:
| Coder shows | Reason | Writes |
|---|---|---|
| `enabled: false` while the spec says enabled | `DisabledExternally` |
none |
| a `has_*` flag false for a credential of the spec's auth type, and the
auth type is not `user_oidc` | `CredentialClearedExternally` | none |
A 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_oidc` sends each chat user's login token to `url` (plan F14), and
the CRD never allows it. An observed `user_oidc` is 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) plus `url`,
so the config stays disabled, and the stall message says that the auth
type was set back. `url` goes 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.
- Fixed in `c2de88cf`: stale hashes after a PATCH that clears a
credential.
- Fixed in #330 (`4e84a445`): `user_oidc` while the credential read
fails. A read failure is the #330 layer.
- Not fixed, by design: the removal of one custom header of several.
Coder shows one `has_custom_headers` flag and no header names, so this
is plan risk R2, like a dashboard replacement of a value.
The security review of `0ea11aa` found no issues.
Round 2 (`c2de88cf`): normal review "Didn't find any major issues",
security review no issues.
Round 3 (refresh on `main` as `01e015a1`): the security review found no
issues. The normal review found one item, fixed in `bbd8cbb2`: 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's
`has_*` 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 in `632a7686`, and I requested no
further review:
- (security) 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.
- a change of the auth type whose answer was lost kept the old field's
hash, because the retry found nothing to send. The round-3 check missed
the cleared fields.
## Validation
New tests in `internal/controller/codermcpserver_sync_test.go`:
- `TestCoderMCPServerSyncKillSwitches` (S14): a dashboard disable,
credential clear and auth type change to `none` each give `Stalled=True`
with their reason and a 10-minute requeue on two reconciles, with zero
writes. A spec edit whose PATCH fails gives `CoderUnavailable` and keeps
the old `appliedGeneration`. The next reconcile applies: enabled,
`api_key`, the key, the new description.
- `TestCoderMCPServerSyncUserOIDC` (S19): for `none`, `api_key`,
`custom_headers` and `oauth2` configs, for an `api_key` config that is
also disabled, and for one that is also disabled with its `url` moved to
another origin, a dashboard switch to `user_oidc` gives exactly one
PATCH. Every key of its body is an auth field or `url`, its `auth_type`
is the spec's, and a moved `url` is set back in the same PATCH. Coder
then holds the applied state again (the disabled config stays disabled
with `DisabledExternally`). 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_key` to `none`,
then a dashboard key, then the spec back to `api_key` sends the Secret's
key again. A second case loses the answer of the PATCH to `none`.
- `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.
- Mutation checks (run on the stack head): each of these 10 changes
makes a test fail:
- no `DisabledExternally` kill switch
- no `CredentialClearedExternally` kill switch
- `user_oidc` held by a kill switch instead of set back
- the full sparse PATCH instead of the auth fields while a kill switch
holds
- no hash check (a metadata-only change resends)
- no hash recorded at create
- no hash dropped before the PATCH (two variants)
- no hash pruning on every sync
- no `url` in the auth-only PATCH
One 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_oidc` switch on a disabled config.
- 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
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`](https://github.com/coder/xum) • Model:
`anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$127.81`_
<!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high
costs=127.81 -->
---------
Signed-off-by: Thomas Kosiewski <tk@coder.com>1 parent 1f336cf commit 06ce2b9
4 files changed
Lines changed: 402 additions & 19 deletions
File tree
- internal/controller
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| 6 | + | |
6 | 7 | | |
| 8 | + | |
7 | 9 | | |
8 | 10 | | |
9 | 11 | | |
| |||
27 | 29 | | |
28 | 30 | | |
29 | 31 | | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
36 | 40 | | |
37 | 41 | | |
38 | 42 | | |
| |||
47 | 51 | | |
48 | 52 | | |
49 | 53 | | |
| 54 | + | |
50 | 55 | | |
51 | 56 | | |
52 | 57 | | |
| |||
56 | 61 | | |
57 | 62 | | |
58 | 63 | | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
59 | 83 | | |
60 | 84 | | |
61 | 85 | | |
62 | 86 | | |
63 | | - | |
64 | | - | |
65 | | - | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
66 | 99 | | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
67 | 104 | | |
68 | 105 | | |
69 | 106 | | |
| |||
74 | 111 | | |
75 | 112 | | |
76 | 113 | | |
77 | | - | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
78 | 119 | | |
79 | 120 | | |
| 121 | + | |
80 | 122 | | |
81 | 123 | | |
82 | 124 | | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
83 | 129 | | |
84 | 130 | | |
85 | 131 | | |
| |||
97 | 143 | | |
98 | 144 | | |
99 | 145 | | |
100 | | - | |
101 | | - | |
102 | | - | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
103 | 150 | | |
104 | | - | |
| 151 | + | |
105 | 152 | | |
106 | 153 | | |
107 | 154 | | |
| |||
125 | 172 | | |
126 | 173 | | |
127 | 174 | | |
128 | | - | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
129 | 180 | | |
130 | 181 | | |
131 | 182 | | |
| |||
138 | 189 | | |
139 | 190 | | |
140 | 191 | | |
141 | | - | |
142 | | - | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
143 | 199 | | |
144 | 200 | | |
145 | 201 | | |
| |||
159 | 215 | | |
160 | 216 | | |
161 | 217 | | |
162 | | - | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
163 | 231 | | |
164 | 232 | | |
165 | 233 | | |
| |||
244 | 312 | | |
245 | 313 | | |
246 | 314 | | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
247 | 375 | | |
248 | 376 | | |
249 | 377 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
9 | 10 | | |
10 | 11 | | |
11 | 12 | | |
| |||
52 | 53 | | |
53 | 54 | | |
54 | 55 | | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
55 | 60 | | |
56 | 61 | | |
57 | 62 | | |
| |||
230 | 235 | | |
231 | 236 | | |
232 | 237 | | |
233 | | - | |
| 238 | + | |
234 | 239 | | |
235 | 240 | | |
236 | 241 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
141 | 141 | | |
142 | 142 | | |
143 | 143 | | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
144 | 149 | | |
145 | 150 | | |
146 | 151 | | |
| |||
0 commit comments