fix(gateway): register the shared ACME account under the rotation lock - #1138
Open
kvinwang wants to merge 1 commit into
Open
fix(gateway): register the shared ACME account under the rotation lock#1138kvinwang wants to merge 1 commit into
kvinwang wants to merge 1 commit into
Conversation
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.
Problem
A cluster registers its shared ACME account lazily, on whichever renewal first
finds the credentials record empty:
Nothing serializes that. The only lock held here is
try_acquire_cert_lock,which is per domain, so it does not order two domains against each other at
all:
RenewZtDomainCertrun on separate tasks and take separate locksAnd the empty-record state is exactly what a fresh cluster starts in: every
node runs
init_allat boot, so the first issuance of each domain races theothers.
Both registrations succeed at the CA.
save_acme_credentialsis a WaveKV write,last-writer-wins with no compare-and-swap (
try_acquire_rotation_lock's owndocs say so), so the record keeps one account and the other is lost. The cost:
10 new accounts per IP per 3 hours.
generate_and_save_acme_attestationwrites under its own key and racesseparately, so the surviving attestation can be the other account's. The
cluster then issues with an account it cannot prove it holds, which is
visible to anyone verifying the attestation and not visible locally at all.
SetCaarun in between pins CAA to the loser, and issuance stays brokenuntil
SetCaais rerun.Fix
Registration takes the lock that already exists for this key, the one
RotateAcmeCredentialsuses, and re-reads the record under it:The re-read is the point: a waiter let through after the holder finishes adopts
the account that appeared rather than registering a second one. Lock order
matches rotation's (
caa_lockthen the KV lock), so the two cannot deadlock,and the lock is released whether registration succeeded or failed.
The DNS provider client is now built after the lock is granted rather than in
the shared prologue. Constructing it resolves the zone through an authenticated
provider API call, and a run that is about to be refused should not spend one.
The fast path — credentials already present — is unchanged, and still builds it.
This narrows the window to WaveKV's replication latency; it does not close it.
The KV lock is last-writer-wins without compare-and-swap, so two nodes can still
both acquire during a replication gap. That is the same guarantee rotation has
had, and closing it properly means compare-and-swap in WaveKV, which is a
different change.
A node refused by the lock returns an error from that renewal.
try_renew_alllogs it and continues, and the periodic task retries on its next pass, by which
time the account exists and is adopted.
Verification
cargo test -p dstack-gateway— 290 pass, including a new hermetic regressiontest: with the rotation lock held, a first-use registration fails with
registering or rotatingrather than proceeding. The test configures a DNScredential pointed at an unreachable provider (
http://127.0.0.1:1), soreaching the lock error at all is the assertion — before this change the same
run failed while building the DNS client, which happened first.
cargo fmtandcargo clippy -p dstack-gateway --all-targets -D warningsareclean.
Scope
Pre-existing on
next; found while reviewing thedns-persist-01work in#1132, where the consequence is sharper — a
_validation-persistrecord namesone account, so a silently replaced account fails every order until an operator
republishes. #1132 touches the same function and will rebase onto this.