ateapi: authorize MintCert against the store, not the worker cache - #965
Open
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
Conversation
A worker's store key included its pool, which a mint request cannot carry, so authorization resolved it through the watch-fed worker cache and denied assignments the store had already committed. Keying workers by the Pod backing them lets the gate read the authoritative record. Removing the pool from the key also removed the ownership check it implicitly provided, so the syncer now carries the Pod identity in its queue key and conditions worker deletes and actor releases on the exact incarnation it inspected.
Collaborator
|
Hmmm, I'm not sure we want to remove caching on reads here. In the analogous situation in Kubernetes, we return an error code indicating that the denial could be due to watch/cache lag, and the caller must retry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #964
Resume can deny
MintCertafter the worker assignment is committed because authorization reads an asynchronously updated worker cache. The issue has representative CI failures and a reproduction.Design decisions
Authorize from the store, not the worker cache. Credential minting must use committed assignment state. Cache retries only reduce the stale window; a version barrier would add coordination with the replica's watch. A direct store read avoids both. Authorization already reads the actor from the store, so this adds one worker point read rather than a new service dependency. Store failures return
Internal; an authoritative mismatch returnsPermissionDenied.Identify a worker by
(namespace, pod). Kubernetes makes a Pod name unique within its namespace. Pool identifies ownership and configuration, not the Pod itself. Keeping pool in the key would require carrying that redundant attribute intoMintCert; using(namespace, pod)lets the request locate the worker and then validates the authenticated atelet node and requested Pod UID against the stored record. Both storage backends now use the same logical identity.Use Pod UID as an incarnation check, not as part of the key.
(namespace, pod)names the current logical worker, while Pod UID distinguishes delete-and-recreate incarnations. Keeping UID out of the key prevents multiple incarnations from coexisting; comparing it during authorization and mutation rejects stale requests and snapshots.Keep
worker_poolas an immutable attribute. Pool still drives scheduling, metrics, WorkerPool lookup, sandbox configuration, and CLI output. It remains on the worker record, and both PostgreSQL and Redis reject changing it in place.Preserve the existing relabel-as-replacement behavior. A pool relabel runs the existing worker cleanup path, deletes the old record, and recreates it under the current pool. This PR preserves that behavior while changing the key; it does not decide whether relabel should become nondisruptive.
Key the workqueue by
(namespace, pod). Including pool or UID would give old and new events different keys, allowing them to run concurrently on the two syncer workers. One key serializes and coalesces events for the same Pod name, and reconcile reads the latest Pod state from the informer.Make deletion conditional on the record inspected. Both ateapi replicas run the syncer, so a worker can change between read and delete.
DeleteWorkeratomically checks namespace, pool, pod, Pod UID, and version. Version alone cannot distinguish an ABA delete-and-recreate because a new record starts again at version 1.Release actors only from the worker placement inspected. Worker deletion and actor update are separate operations. Before changing actor state, the syncer compares actor UID plus namespace, pool, pod, and Pod UID. A stale snapshot therefore cannot affect an actor that has moved or a new Pod incarnation that reused the name.
The worker cache remains in scheduling and worker-count paths, where eventual consistency is acceptable; only credential authorization stops depending on it.
Evidence
Before, in run 31628573958, attempt 1:
The denied read occurred 11.7ms after the authoritative write.
The regression suite covers these counterexamples:
After, local
go test ./cmd/ateapi/... -race -vpasses the corresponding cases on this branch:Boundaries and rollout
The project has other known flaky tests. This PR fixes only the stale-cache
MintCertfailure described in #964; the rest are not addressed here.This PR contains no schema or data migration and no compatibility layer for the old worker identity:
An existing PostgreSQL table keeps its
(namespace, pool, pod)primary key. Before deploying the new code, migration must remove duplicate(namespace, pod)rows and replace that primary key.Existing Redis worker keys include pool and are not read by the new code. Their records must be rewritten. Resetting them is safe only after all actors have no live worker placement; the syncer can rebuild worker metadata from Pods, but it cannot reconstruct actor assignments.
Old and new ateapi versions must not write the same store concurrently. Existing PostgreSQL worker page tokens also become invalid after migration.
Tests pass
Appropriate changes to documentation are included in the PR