feat: fill an empty placeholder serving-CA Secret instead of creating it - #146
Conversation
An identity that may only get and update the coder-k8s-apiserver-tls Secret cannot create it. Namespace-wide create on Secrets would let such an identity mint tokens for other ServiceAccounts in the namespace, so the server now also accepts an existing placeholder: exactly the managed type and zero data keys. It fills the placeholder with a new CA through a resourceVersion-guarded update and adopts another replica's fill on conflict. Every other state stays corrupt and is never overwritten. When create is forbidden, the error explains how to provide the placeholder. The create path for a missing Secret is unchanged. Refs #143 Change-Id: I9c2cd26f46f0b6f694fcf42fc41bd0b8ea60fefd Signed-off-by: Thomas Kosiewski <tk@coder.com>
Refs #143 Change-Id: Ia833dd8819aa71f59555ce68b3b90ce19efc33cb 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: 138786258e
ℹ️ 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".
…rors Codex review on #146: a filled placeholder now carries the managed labels, like a created Secret, and troubleshooting names the permission behind each get, create, fill and update error. Refs #143 Change-Id: I4deb2b7e8f8dfeb7f37409d646c95d50139aa6a3 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: 905a80e6ba
ℹ️ 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. |
Codex review on #146: Kubernetes forbids data changes on an immutable Secret, so an immutable empty placeholder can never be filled. It now fails startup with a message saying to recreate it without immutable. Refs #143 Change-Id: Ie7977f7613e066af889bca836232928db847e3eb 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. |
1 participant
Refs #143. Step 2 of 3.
The aggregated API server can now fill an empty placeholder
coder-k8s-apiserver-tlsSecret instead of creating one. That lets a future standalone ServiceAccount work with onlygetandupdateon this one Secret.createon Secrets cannot be limited by name, and a namespace-widecreatewould let that identity mint tokens for other ServiceAccounts in the namespace. Existing installs are unaffected: when the Secret is missing, the server still creates it as before.What changed
servingcert.IsPlaceholder: true only when the type iscoder.com/aggregated-apiserver-serving-caand there are zerodatakeys. Every other Secret is still parsed, and invalid material stays corrupt: startup fails and nothing is overwritten.Manager.Ensure:resourceVersion.resourceVersionfails with anassertion failed:error instead of risking an unconditional overwrite.Tests
New tests cover:
data, with the update carrying the fetchedresourceVersion;resourceVersion;[get, create]).9 of 9 mutations are caught: wrong-type placeholder accepted, filling a Secret that has data (2 variants), update without
resourceVersion, assertion removed, conflict treated as an error, create hint dropped, fill via create, and fill with no data.The fake clientset does not enforce
resourceVersionconflicts. Step 3 adds envtest proofs against a real API server, along with the standalone ServiceAccount, its namespaced Role and the manifests.Validation
On the exact tree
cb181d102fd63260f50e48029fc99b46e5c8f024, these pass:make verify-vendor,make build,make test,make lint,make test-integrationandmkdocs build --strict.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:medium