mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat: invalidate provisioner daemon sessions on key deletion (#26532)
## Summary Closes PLAT-305. When a provisioner key is deleted, the associated daemon kept operating on its existing WebSocket connection, because authentication was only checked at connection establishment and deletion was a bare `DELETE` with no session invalidation. This adds four layers of defense so a deleted key promptly stops doing work: 1. **Publish on delete.** `deleteProvisionerKey` publishes to a new per-key pubsub channel (`coderd/pubsub.ProvisionerKeyDeletedChannel`) after a successful delete. Publish errors are logged but still return `204`, since layer 3 is the durable backstop. 2. **Subscribe and tear down.** The daemon serve handler subscribes to its key's channel and terminates the DRPC session on a deletion event. Termination is deferred while a job claimed by the session is active: the daemon may finish and report the in-flight job (`UpdateJob`/`CompleteJob` have no key check), and the last active job's completion performs the cancellation. Because Postgres `LISTEN`/`NOTIFY` does not buffer for non-listeners, the handler also performs a synchronous key-existence re-check immediately after subscribing to close the race between auth and subscription. The subscription uses `SubscribeWithErr` so that an `ErrDroppedMessages` signal (emitted when the pubsub listener reconnects) triggers the same key re-check, closing the listener-outage window in which a deletion notification could be missed. 3. **Backstop on acquire.** `AcquireJob` and `AcquireJobWithCancel` verify the key still exists before waiting for a job, and the `Acquirer` claims jobs in a transaction that first locks the worker's deletable key (`LockProvisionerKeyByIDForShare`, a `FOR KEY SHARE` row lock held until commit) before running the `AcquireProvisionerJob` claim, so a claim cannot commit after the key's deletion. This guards against a missed pubsub message. A missing key row surfaces as its own result rather than overloading the claim query's no-rows response: the acquire terminates with `ErrProvisionerKeyDeleted` (terminating the session, with the same active-job deferral) and hands the consumed wakeup to another waiting daemon in the same domain, rather than silently re-parking and starving peers of job postings. 4. **Heartbeat watchdog.** The per-session heartbeat loop (1m interval) also re-checks the key, so even a session whose deletion notification was silently lost terminates within one heartbeat interval instead of living until the connection breaks (same active-job deferral as layer 2). Reserved keys skip the check. A job that is claimed but never delivered (the session or connection dies between the database claim and the stream send) is marked failed immediately on a fresh context, instead of staying assigned to the worker until the job reaper. Reserved keys (built-in, user-auth, PSK) are exempt throughout, since they are not deletable rows. The acquire-time lookup runs as `dbauthz.AsSystemReadProvisionerDaemons`, because the provisionerd role cannot read provisioner keys and a provisioner key's RBAC object is a provisioner daemon. A single key can back many daemons (and span HA replicas), so the per-key channel fans out to invalidate all of them at once. Per-key channels keep the `LISTEN` count proportional to distinct keys rather than waking every daemon on unrelated deletions. ### Known limitations - **`UpdateJob`/`CompleteJob` intentionally have no key check.** By the time those RPCs arrive the work has already run; rejecting completion would strand a build in "running" (until the job reaper fails it) with real infrastructure left unreconciled. Session termination is deferred while a job is active so the completion can be reported; the daemon may not receive the final RPC response when the deferred termination fires, but the job's outcome is already persisted. - **After termination, the daemon process redials and receives 401s until restarted.** The dial-time exit logic only triggers on 403, and the auth middleware returns 401 for an invalid key; this dial behavior predates this PR and is tracked as a follow-up in [PLAT-452](https://linear.app/codercom/issue/PLAT-452) (return 403 for invalid provisioner keys). ## Tests - `coderd/provisionerdserver`: `TestAcquireJob_ProvisionerKeyDeleted` (both RPC variants), `TestAcquireJob_ReservedProvisionerKey`, `TestHeartbeat_ProvisionerKeyDeleted` (heartbeat watchdog cancels the session after key deletion), `TestAcquirer_ProvisionerKeyDeleted` (a dead-key acquiree exits terminally and its clearance is promoted to a peer in the same domain), and `TestTerminateSession_Deferral` (termination is immediate when idle and deferred until the last active job finishes). - `coderd/database`: `TestAcquireProvisionerJob/ProvisionerKeyLock` covers the lock query against real Postgres: it returns the key ID while the row exists and no rows once it is deleted. The lock-then-claim composition is pinned by `TestAcquirer_ProvisionerKeyDeleted`. - `enterprise/coderd`: `TestProvisionerDaemonServe/KeyDeletionClosesSession` asserts an active session closes after its key is deleted. `KeyDeletedDuringSetupClosesSession` covers the post-subscribe re-check when a key is deleted between auth and subscription, and `DroppedMessageClosesSession` covers the `ErrDroppedMessages` re-check when a deletion is missed during a listener outage. ## Validation - `make` pre-commit (gen/fmt/lint/build) passed via git hooks. - Targeted tests pass; existing acquire tests pass with no regression. - Manual: brought up a dev deployment (coder-in-coder) with a Premium license, created a deletable provisioner key, and started an external daemon with `coder provisionerd start`. Confirmed it authenticated via the key and connected, appearing as `idle` in both `coder provisioner list` (with the key name) and the organization Provisioners UI. - Manual, idle teardown: deleted the key while the daemon was idle. The server logged `provisioner key deleted, terminating session`, the daemon's session closed immediately, and it dropped from `coder provisioner list` (then entered the known 401 redial loop, PLAT-452). - Manual, deferred termination: ran a workspace build (tagged template, `sleep 45` in `local-exec`) pinned to the external daemon and deleted the key mid-build. The server logged `deferring session cancellation until active jobs finish`; the heartbeat watchdog re-checked mid-build and re-deferred rather than force-killing. The build ran to completion (`Apply complete`, workspace `Started`) and only then did `canceling session after job completion` fire. The documented caveat reproduced: the daemon lost the final `CompleteJob` ack, and the build outcome was still persisted correctly. <details> <summary>Implementation plan and design decisions</summary> ### Design - **Per-key vs global channel:** chose per-key (`provisioner_key_deleted:<keyID>`) so daemons do not wake on unrelated deletions. The cost is one `LISTEN` per distinct key per replica on the shared listener connection, which is negligible against Coder's existing channels. - **Missing-key behavior on acquire:** returns an error that tears down the acquire rather than silently returning an empty job. - **Subscribe-startup race:** ordering is `authorize -> UpsertProvisionerDaemon -> Subscribe -> GetProvisionerKeyByID`. The post-subscribe re-check handles a deletion that committed before the `LISTEN` registered (Postgres does not buffer notifications for non-listeners; the in-process buffer only smooths bursts and drops on overflow). - **`NewServer` change:** `KeyID` was added to `provisionerdserver.Options` to avoid a positional signature change across call sites. The in-memory (built-in) daemon leaves it unset and is therefore exempt. ### Files - `coderd/pubsub/provisionerkeydeleted.go` (new) — channel helper. - `enterprise/coderd/provisionerkeys.go` — publish on delete. - `enterprise/coderd/provisionerdaemons.go` — subscribe, re-check, cancel session; pass `KeyID`. - `coderd/provisionerdserver/provisionerdserver.go` — `KeyID` option and acquire-time existence check. </details> --- This pull request was created by Coder Agents on behalf of @jscottmiller.
This commit is contained in:
@@ -21,10 +21,12 @@ import (
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtime"
|
||||
dbpubsub "github.com/coder/coder/v2/coderd/database/pubsub"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw"
|
||||
"github.com/coder/coder/v2/coderd/httpmw/loggermw"
|
||||
"github.com/coder/coder/v2/coderd/provisionerdserver"
|
||||
"github.com/coder/coder/v2/coderd/pubsub"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
"github.com/coder/coder/v2/coderd/rbac/policy"
|
||||
"github.com/coder/coder/v2/coderd/telemetry"
|
||||
@@ -356,6 +358,8 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request)
|
||||
OIDCConfig: api.OIDCConfig,
|
||||
AISeatTracker: api.AGPL.AISeatTracker,
|
||||
Clock: api.Clock,
|
||||
KeyID: authRes.keyID,
|
||||
SessionCancel: srvCancel,
|
||||
},
|
||||
api.NotificationsEnqueuer,
|
||||
&api.AGPL.PrebuildsReconciler,
|
||||
@@ -389,8 +393,58 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request)
|
||||
rl.WriteLog(ctx, http.StatusAccepted)
|
||||
}
|
||||
|
||||
err = server.Serve(ctx, session)
|
||||
srvCancel()
|
||||
if codersdk.IsDeletableProvisionerKey(authRes.keyID) {
|
||||
keyDeleted := func(ctx context.Context) (deleted bool, err error) {
|
||||
_, err = api.Database.GetProvisionerKeyByID(ctx, authRes.keyID)
|
||||
if xerrors.Is(err, sql.ErrNoRows) {
|
||||
return true, nil
|
||||
}
|
||||
return false, err
|
||||
}
|
||||
|
||||
closeSubscribe, err := api.Pubsub.SubscribeWithErr(
|
||||
pubsub.ProvisionerKeyDeletedChannel(authRes.keyID),
|
||||
func(_ context.Context, _ []byte, subErr error) {
|
||||
// ErrDroppedMessages means the Postgres listener reconnected; a
|
||||
// deletion published during the outage may not have been
|
||||
// delivered, so query the key directly instead of relying on the
|
||||
// notification.
|
||||
if xerrors.Is(subErr, dbpubsub.ErrDroppedMessages) {
|
||||
deleted, err := keyDeleted(authCtx)
|
||||
if err != nil {
|
||||
logger.Warn(ctx, "failed to re-check provisioner key after dropped messages",
|
||||
slog.F("provisioner_key_id", authRes.keyID), slog.Error(err))
|
||||
return
|
||||
}
|
||||
if !deleted {
|
||||
return
|
||||
}
|
||||
}
|
||||
logger.Info(ctx, "provisioner key deleted, terminating session",
|
||||
slog.F("provisioner_key_id", authRes.keyID))
|
||||
srv.TerminateSession()
|
||||
},
|
||||
)
|
||||
if err != nil {
|
||||
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("subscribe to provisioner key deletion: %s", err))
|
||||
return
|
||||
}
|
||||
defer closeSubscribe()
|
||||
|
||||
// Postgres LISTEN/NOTIFY does not deliver notifications published before
|
||||
// registration, so re-check after subscribing.
|
||||
if deleted, err := keyDeleted(authCtx); err != nil {
|
||||
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("check provisioner key: %s", err))
|
||||
return
|
||||
} else if deleted {
|
||||
logger.Info(ctx, "provisioner key no longer exists, closing connection",
|
||||
slog.F("provisioner_key_id", authRes.keyID))
|
||||
_ = conn.Close(websocket.StatusGoingAway, "provisioner key deleted")
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
err = server.Serve(srvCtx, session)
|
||||
logger.Info(ctx, "provisioner daemon disconnected", slog.Error(err))
|
||||
if err != nil && !xerrors.Is(err, io.EOF) {
|
||||
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("serve: %s", err))
|
||||
|
||||
@@ -7,12 +7,15 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3/sloggers/slogtest"
|
||||
"github.com/coder/coder/v2/apiversion"
|
||||
@@ -21,7 +24,10 @@ import (
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
||||
dbpubsub "github.com/coder/coder/v2/coderd/database/pubsub"
|
||||
"github.com/coder/coder/v2/coderd/provisionerkey"
|
||||
"github.com/coder/coder/v2/coderd/pubsub"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
"github.com/coder/coder/v2/coderd/util/ptr"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
@@ -171,6 +177,360 @@ func TestProvisionerDaemonServe(t *testing.T) {
|
||||
require.Contains(t, string(b), fmt.Sprintf("server is at version %s, behind requested major version %s", proto.CurrentVersion.String(), v.String()))
|
||||
})
|
||||
|
||||
t.Run("KeyDeletionClosesSession", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
client, _ := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureExternalProvisionerDaemons: 1,
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
}})
|
||||
org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgAdmin(org.ID))
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
res, err := orgAdmin.CreateProvisionerKey(ctx, org.ID, codersdk.CreateProvisionerKeyRequest{
|
||||
Name: "my-key",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
srv, err := orgAdmin.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{
|
||||
Name: testutil.MustRandString(t, 63),
|
||||
Organization: org.ID,
|
||||
Provisioners: []codersdk.ProvisionerType{
|
||||
codersdk.ProvisionerTypeEcho,
|
||||
},
|
||||
Tags: map[string]string{},
|
||||
ProvisionerKey: res.Key,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
defer srv.DRPCConn().Close()
|
||||
|
||||
// The session is established and open.
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
t.Fatal("connection closed before key deletion")
|
||||
default:
|
||||
}
|
||||
|
||||
// Deleting the key must tear down the active daemon session.
|
||||
err = orgAdmin.DeleteProvisionerKey(ctx, org.ID, "my-key")
|
||||
require.NoError(t, err)
|
||||
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for provisioner daemon session to close")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("KeyDeletedDuringSetupClosesSession", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Provisioner key auth fetches the key by name, so the only
|
||||
// GetProvisionerKeyByID reads in the serve path are the post-subscribe
|
||||
// re-check and the heartbeat watchdog, whose first beat fires
|
||||
// immediately at session start. Deleting the key on the first such
|
||||
// read reproduces a key deleted between authentication and
|
||||
// subscription; either reader must close the session, so this test
|
||||
// pins the setup-window behavior rather than the re-check in
|
||||
// isolation. No pubsub notification is delivered.
|
||||
db, ps := dbtestutil.NewDB(t)
|
||||
store := &deleteKeyOnReadStore{Store: db}
|
||||
client, _ := coderdenttest.New(t, &coderdenttest.Options{
|
||||
Options: &coderdtest.Options{
|
||||
Database: store,
|
||||
Pubsub: ps,
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureExternalProvisionerDaemons: 1,
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgAdmin(org.ID))
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
res, err := orgAdmin.CreateProvisionerKey(ctx, org.ID, codersdk.CreateProvisionerKeyRequest{
|
||||
Name: "my-key",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
keys, err := orgAdmin.ListProvisionerKeys(ctx, org.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, keys, 1)
|
||||
keyID := keys[0].ID
|
||||
store.keyID.Store(&keyID)
|
||||
|
||||
srv, err := orgAdmin.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{
|
||||
Name: testutil.MustRandString(t, 63),
|
||||
Organization: org.ID,
|
||||
Provisioners: []codersdk.ProvisionerType{
|
||||
codersdk.ProvisionerTypeEcho,
|
||||
},
|
||||
Tags: map[string]string{},
|
||||
ProvisionerKey: res.Key,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
defer srv.DRPCConn().Close()
|
||||
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for re-check to close the session")
|
||||
}
|
||||
// Confirm the close was driven by a key lookup, not another path (no
|
||||
// pubsub notification is published in this test).
|
||||
require.True(t, store.deleted.Load())
|
||||
})
|
||||
|
||||
t.Run("DroppedMessageClosesSession", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// A dropped-messages signal is delivered when the Postgres listener
|
||||
// reconnects. If the key was deleted while the listener was down, the
|
||||
// deletion notification is never delivered, so the serve handler must
|
||||
// re-check the key on the dropped-messages signal. This test deletes
|
||||
// the key directly in the database (no notification published) and then
|
||||
// drives the captured listener with ErrDroppedMessages.
|
||||
//
|
||||
// The heartbeat watchdog also detects a deleted key, but its first
|
||||
// beat fires at session start while the key still exists and later
|
||||
// beats (1m default) exceed the test deadline (testutil.WaitLong), so
|
||||
// a close after the signal is attributable to the dropped-messages
|
||||
// re-check. If either timing changes, the heartbeat could close the
|
||||
// session instead and mask removal of the re-check.
|
||||
db, ps := dbtestutil.NewDB(t)
|
||||
capturePS := newCaptureKeyDeletePubsub(ps)
|
||||
client, _ := coderdenttest.New(t, &coderdenttest.Options{
|
||||
Options: &coderdtest.Options{
|
||||
Database: db,
|
||||
Pubsub: capturePS,
|
||||
// The wrapper is not a *PGPubsub, so provide the real one for
|
||||
// replica sync.
|
||||
ReplicaSyncPubsub: ps.(*dbpubsub.PGPubsub),
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureExternalProvisionerDaemons: 1,
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgAdmin(org.ID))
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
res, err := orgAdmin.CreateProvisionerKey(ctx, org.ID, codersdk.CreateProvisionerKeyRequest{
|
||||
Name: "my-key",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
keys, err := orgAdmin.ListProvisionerKeys(ctx, org.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, keys, 1)
|
||||
keyID := keys[0].ID
|
||||
capturePS.target.Store(ptr.Ref(pubsub.ProvisionerKeyDeletedChannel(keyID)))
|
||||
|
||||
srv, err := orgAdmin.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{
|
||||
Name: testutil.MustRandString(t, 63),
|
||||
Organization: org.ID,
|
||||
Provisioners: []codersdk.ProvisionerType{
|
||||
codersdk.ProvisionerTypeEcho,
|
||||
},
|
||||
Tags: map[string]string{},
|
||||
ProvisionerKey: res.Key,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
defer srv.DRPCConn().Close()
|
||||
|
||||
// Capture the listener the serve handler registered for this key.
|
||||
listener := capturePS.waitListener(ctx, t)
|
||||
|
||||
// The session is established and open.
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
t.Fatal("connection closed before key deletion")
|
||||
default:
|
||||
}
|
||||
|
||||
// Delete the key without publishing, simulating a deletion missed while
|
||||
// the listener was down.
|
||||
//nolint:gocritic // The test deletes the key outside the request actor.
|
||||
err = db.DeleteProvisionerKey(dbauthz.AsSystemRestricted(ctx), keyID)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Deliver the dropped-messages signal the reconnect would have produced.
|
||||
listener(ctx, nil, dbpubsub.ErrDroppedMessages)
|
||||
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for dropped-message re-check to close the session")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("DroppedMessageKeyExistsKeepsSession", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// A dropped-messages signal with the key still present must leave the
|
||||
// session running; only a confirmed missing key may terminate it.
|
||||
db, ps := dbtestutil.NewDB(t)
|
||||
capturePS := newCaptureKeyDeletePubsub(ps)
|
||||
client, _ := coderdenttest.New(t, &coderdenttest.Options{
|
||||
Options: &coderdtest.Options{
|
||||
Database: db,
|
||||
Pubsub: capturePS,
|
||||
// The wrapper is not a *PGPubsub, so provide the real one for
|
||||
// replica sync.
|
||||
ReplicaSyncPubsub: ps.(*dbpubsub.PGPubsub),
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureExternalProvisionerDaemons: 1,
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgAdmin(org.ID))
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
res, err := orgAdmin.CreateProvisionerKey(ctx, org.ID, codersdk.CreateProvisionerKeyRequest{
|
||||
Name: "my-key",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
keys, err := orgAdmin.ListProvisionerKeys(ctx, org.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, keys, 1)
|
||||
keyID := keys[0].ID
|
||||
capturePS.target.Store(ptr.Ref(pubsub.ProvisionerKeyDeletedChannel(keyID)))
|
||||
|
||||
srv, err := orgAdmin.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{
|
||||
Name: testutil.MustRandString(t, 63),
|
||||
Organization: org.ID,
|
||||
Provisioners: []codersdk.ProvisionerType{
|
||||
codersdk.ProvisionerTypeEcho,
|
||||
},
|
||||
Tags: map[string]string{},
|
||||
ProvisionerKey: res.Key,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
defer srv.DRPCConn().Close()
|
||||
|
||||
listener := capturePS.waitListener(ctx, t)
|
||||
|
||||
// Deliver the dropped-messages signal without deleting the key. The
|
||||
// re-check finds the key and the session must stay open.
|
||||
listener(ctx, nil, dbpubsub.ErrDroppedMessages)
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
t.Fatal("session closed although provisioner key still exists")
|
||||
case <-time.After(testutil.IntervalMedium):
|
||||
}
|
||||
|
||||
// The same signal after the key is gone closes the session, confirming
|
||||
// the session was still fully functional above.
|
||||
//nolint:gocritic // The test deletes the key outside the request actor.
|
||||
err = db.DeleteProvisionerKey(dbauthz.AsSystemRestricted(ctx), keyID)
|
||||
require.NoError(t, err)
|
||||
listener(ctx, nil, dbpubsub.ErrDroppedMessages)
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for dropped-message re-check to close the session")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("DroppedMessageKeyCheckErrorKeepsSession", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// A dropped-messages signal whose key re-check fails must leave the
|
||||
// session running; a transient database error is not evidence of
|
||||
// deletion.
|
||||
db, ps := dbtestutil.NewDB(t)
|
||||
store := &failKeyReadStore{Store: db}
|
||||
capturePS := newCaptureKeyDeletePubsub(ps)
|
||||
client, _ := coderdenttest.New(t, &coderdenttest.Options{
|
||||
Options: &coderdtest.Options{
|
||||
Database: store,
|
||||
Pubsub: capturePS,
|
||||
// The wrapper is not a *PGPubsub, so provide the real one for
|
||||
// replica sync.
|
||||
ReplicaSyncPubsub: ps.(*dbpubsub.PGPubsub),
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureExternalProvisionerDaemons: 1,
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgAdmin(org.ID))
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
res, err := orgAdmin.CreateProvisionerKey(ctx, org.ID, codersdk.CreateProvisionerKeyRequest{
|
||||
Name: "my-key",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
keys, err := orgAdmin.ListProvisionerKeys(ctx, org.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, keys, 1)
|
||||
keyID := keys[0].ID
|
||||
store.keyID.Store(&keyID)
|
||||
capturePS.target.Store(ptr.Ref(pubsub.ProvisionerKeyDeletedChannel(keyID)))
|
||||
|
||||
srv, err := orgAdmin.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{
|
||||
Name: testutil.MustRandString(t, 63),
|
||||
Organization: org.ID,
|
||||
Provisioners: []codersdk.ProvisionerType{
|
||||
codersdk.ProvisionerTypeEcho,
|
||||
},
|
||||
Tags: map[string]string{},
|
||||
ProvisionerKey: res.Key,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
defer srv.DRPCConn().Close()
|
||||
|
||||
listener := capturePS.waitListener(ctx, t)
|
||||
|
||||
// A completed RPC proves the handler finished connection setup, which
|
||||
// includes the post-subscribe key re-check. That re-check closes the
|
||||
// connection both when the key is gone and when the read errors, so it
|
||||
// has to run before the key is touched below. The error must come from
|
||||
// the job lookup: a transport error would mean the session is already
|
||||
// gone, which proves nothing about setup.
|
||||
_, err = srv.UpdateJob(ctx, &proto.UpdateJobRequest{JobId: uuid.NewString()})
|
||||
require.ErrorContains(t, err, "get job")
|
||||
|
||||
// Make the key re-check fail before deleting the key, so no reader
|
||||
// (including the heartbeat key check) can observe the bare deletion;
|
||||
// then delete the key so a successful re-check would terminate. The
|
||||
// error path must leave the session running. The write gate holds off
|
||||
// key reads for both steps, so none is in flight across the deletion.
|
||||
func() {
|
||||
store.gate.Lock()
|
||||
defer store.gate.Unlock()
|
||||
store.fail.Store(true)
|
||||
//nolint:gocritic // The test deletes the key outside the request actor.
|
||||
err = db.DeleteProvisionerKey(dbauthz.AsSystemRestricted(ctx), keyID)
|
||||
}()
|
||||
require.NoError(t, err)
|
||||
listener(ctx, nil, dbpubsub.ErrDroppedMessages)
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
t.Fatal("session closed although the key re-check failed")
|
||||
case <-time.After(testutil.IntervalMedium):
|
||||
}
|
||||
|
||||
// Once the re-check succeeds again it observes the deletion and closes
|
||||
// the session.
|
||||
store.fail.Store(false)
|
||||
listener(ctx, nil, dbpubsub.ErrDroppedMessages)
|
||||
select {
|
||||
case <-srv.DRPCConn().Closed():
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for dropped-message re-check to close the session")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("NoLicense", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
client, user := coderdenttest.New(t, &coderdenttest.Options{DontAddLicense: true})
|
||||
@@ -986,3 +1346,96 @@ func TestGetProvisionerDaemons(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// failKeyReadStore returns an error from GetProvisionerKeyByID for the key
|
||||
// identified by keyID while fail is set. All other reads pass through. keyID
|
||||
// is set after the key is created so earlier lookups are unaffected.
|
||||
//
|
||||
// Reads of that key hold gate for reading while they check fail and query the
|
||||
// database. A caller that takes gate for writing therefore runs with no such
|
||||
// read in flight, and any read that arrives afterwards observes the writer's
|
||||
// changes to fail and to the key itself. gate is not reentrant: a writer must
|
||||
// not read through this store while holding it.
|
||||
type failKeyReadStore struct {
|
||||
database.Store
|
||||
keyID atomic.Pointer[uuid.UUID]
|
||||
fail atomic.Bool
|
||||
gate sync.RWMutex
|
||||
}
|
||||
|
||||
func (s *failKeyReadStore) GetProvisionerKeyByID(ctx context.Context, id uuid.UUID) (database.ProvisionerKey, error) {
|
||||
if target := s.keyID.Load(); target != nil && *target == id {
|
||||
s.gate.RLock()
|
||||
defer s.gate.RUnlock()
|
||||
if s.fail.Load() {
|
||||
return database.ProvisionerKey{}, xerrors.New("transient database error")
|
||||
}
|
||||
}
|
||||
return s.Store.GetProvisionerKeyByID(ctx, id)
|
||||
}
|
||||
|
||||
// deleteKeyOnReadStore deletes the provisioner key identified by keyID the
|
||||
// first time it is fetched by ID, simulating a key deleted during connection
|
||||
// setup. keyID is set after the key is created so earlier lookups are
|
||||
// unaffected. deleted records that the interception fired.
|
||||
type deleteKeyOnReadStore struct {
|
||||
database.Store
|
||||
keyID atomic.Pointer[uuid.UUID]
|
||||
once sync.Once
|
||||
deleted atomic.Bool
|
||||
}
|
||||
|
||||
func (s *deleteKeyOnReadStore) GetProvisionerKeyByID(ctx context.Context, id uuid.UUID) (database.ProvisionerKey, error) {
|
||||
if target := s.keyID.Load(); target != nil && *target == id {
|
||||
s.once.Do(func() {
|
||||
//nolint:gocritic // The test deletes the key outside the request actor.
|
||||
_ = s.Store.DeleteProvisionerKey(dbauthz.AsSystemRestricted(context.Background()), id)
|
||||
s.deleted.Store(true)
|
||||
})
|
||||
}
|
||||
return s.Store.GetProvisionerKeyByID(ctx, id)
|
||||
}
|
||||
|
||||
// captureKeyDeletePubsub records the ListenerWithErr registered for the channel
|
||||
// named by target so a test can invoke it directly, e.g. with
|
||||
// ErrDroppedMessages. All other pubsub operations pass through to the embedded
|
||||
// Pubsub unchanged. target is set before the subscription is expected so the
|
||||
// capturing subscribe observes it.
|
||||
type captureKeyDeletePubsub struct {
|
||||
dbpubsub.Pubsub
|
||||
target atomic.Pointer[string]
|
||||
mu sync.Mutex
|
||||
listener dbpubsub.ListenerWithErr
|
||||
once sync.Once
|
||||
got chan struct{}
|
||||
}
|
||||
|
||||
func newCaptureKeyDeletePubsub(ps dbpubsub.Pubsub) *captureKeyDeletePubsub {
|
||||
return &captureKeyDeletePubsub{Pubsub: ps, got: make(chan struct{})}
|
||||
}
|
||||
|
||||
func (p *captureKeyDeletePubsub) SubscribeWithErr(event string, listener dbpubsub.ListenerWithErr) (func(), error) {
|
||||
cancel, err := p.Pubsub.SubscribeWithErr(event, listener)
|
||||
if err != nil {
|
||||
return cancel, err
|
||||
}
|
||||
if target := p.target.Load(); target != nil && *target == event {
|
||||
p.mu.Lock()
|
||||
p.listener = listener
|
||||
p.mu.Unlock()
|
||||
p.once.Do(func() { close(p.got) })
|
||||
}
|
||||
return cancel, nil
|
||||
}
|
||||
|
||||
func (p *captureKeyDeletePubsub) waitListener(ctx context.Context, t *testing.T) dbpubsub.ListenerWithErr {
|
||||
t.Helper()
|
||||
select {
|
||||
case <-p.got:
|
||||
case <-ctx.Done():
|
||||
t.Fatal("timed out waiting for provisioner key deletion subscription")
|
||||
}
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
return p.listener
|
||||
}
|
||||
|
||||
@@ -7,12 +7,14 @@ import (
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw"
|
||||
"github.com/coder/coder/v2/coderd/provisionerdserver"
|
||||
"github.com/coder/coder/v2/coderd/provisionerkey"
|
||||
"github.com/coder/coder/v2/coderd/pubsub"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
)
|
||||
|
||||
@@ -196,9 +198,7 @@ func (api *API) deleteProvisionerKey(rw http.ResponseWriter, r *http.Request) {
|
||||
ctx := r.Context()
|
||||
provisionerKey := httpmw.ProvisionerKeyParam(r)
|
||||
|
||||
if provisionerKey.ID.String() == codersdk.ProvisionerKeyIDBuiltIn ||
|
||||
provisionerKey.ID.String() == codersdk.ProvisionerKeyIDUserAuth ||
|
||||
provisionerKey.ID.String() == codersdk.ProvisionerKeyIDPSK {
|
||||
if codersdk.IsReservedProvisionerKey(provisionerKey.ID) {
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: fmt.Sprintf("Cannot delete reserved '%s' provisioner key", provisionerKey.Name),
|
||||
})
|
||||
@@ -211,6 +211,13 @@ func (api *API) deleteProvisionerKey(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
// Notify subscribers that this key was deleted so active sessions tear down.
|
||||
// Publishing is best effort; a failure does not leave the key usable.
|
||||
if err := api.Pubsub.Publish(pubsub.ProvisionerKeyDeletedChannel(provisionerKey.ID), nil); err != nil {
|
||||
api.Logger.Warn(ctx, "failed to publish provisioner key deletion",
|
||||
slog.F("provisioner_key_id", provisionerKey.ID), slog.Error(err))
|
||||
}
|
||||
|
||||
httpapi.Write(ctx, rw, http.StatusNoContent, nil)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user