mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd): cut DB fan-out on agent instance-identity auth (#24973)
## Summary Restores `v2.33.0-rc.2`-equivalent query cost for agent instance-identity auth on `v2.33.0-rc.3`, which currently saturates the pgx pool when multiple agents share an instance ID. Customer report against rc.3 traced 233× `Internal error fetching provisioner job resource. fetch related workspace build: context canceled` 500s during a 50-minute incident window to this path. Backport to `release/2.33` will follow as a separate PR after this merges. ## Root cause [#24325](https://github.com/coder/coder/pull/24325) ("support multiple agents with shared instance-identity auth") rewrote `coderd/workspaceresourceauth.go::handleAuthInstanceID` to use the new `:many` agent lookup followed by a per-candidate filter loop. Each iteration synchronously calls `GetWorkspaceResourceByID` and `GetProvisionerJobByID`. Both go through `dbauthz`, and both fan out into the same `provisioner_job → workspace_build → workspace` cascade because `authorizeProvisionerJob` always re-authorizes the workspace via `GetWorkspaceBuildByJobID → GetWorkspaceByID`. The handler then re-fetches resource and job again for the surviving agent. Net effect on the agent-auth happy path: | | SQL | RBAC | |---|---|---| | rc.2 baseline | 13 | 5 | | rc.3 today, 1 agent | 19 | 7 | | rc.3 today, 2 agents | 26 | 9 | | **After this PR, 1 agent** | **6** | **3** | | **After this PR, 2 agents** | **7** | **3** | Under load, the rc.3 chain blocks on pool acquire and the request blows past the 30s HTTP write timeout. ## Changes ### 1. System fast-path on `authorizeProvisionerJob` (`coderd/database/dbauthz/dbauthz.go`) Add an `AsSystemRestricted` early-return at the top of `authorizeProvisionerJob`. Instance-identity auth has already proven cloud identity before reaching the DB layer, so re-authorizing the workspace on every provisioner-job lookup is pure overhead. Existing `GetWorkspaceAgentsByInstanceID` already uses the same fast-path pattern. ```go if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err == nil { return nil } ``` ### 2. Drop survivor re-fetch in `handleAuthInstanceID` (`coderd/workspaceresourceauth.go`) Capture the provisioner job alongside each candidate during the filter loop so the survivor lookup does not re-fetch resource and job after selection. The previous code fired the resource→job→build→workspace cascade twice for the surviving agent. ## Tests Adds `TestAuthorizeProvisionerJob_SystemFastPath` in `coderd/database/dbauthz/dbauthz_test.go` with two sub-tests: - `AsSystemRestricted/SkipsCascade` — strict mock fails the test if `GetWorkspaceBuildByJobID` or `GetWorkspaceByID` is called. - `NonSystemActor/StillCascades` — auditor (no `ResourceSystem`) still pays the cascade and produces a `NotAuthorized` error, proving the fast-path is gated correctly. Updates 12 existing dbauthz suite cases to expect the new `ResourceSystem.Read` check ahead of the workspace/template-version check, with `FailSystemObjectChecks()` to force the slow path. Existing integration coverage in `TestPostWorkspaceAuthAWSInstanceIdentity/Ambiguous/{SingleAgent, MultipleAgentsWithSelector, MultipleAgentsNoSelector, SubAgentExcluded, ...}` exercises Part 2 end-to-end and continues to pass. ## Footprint - 3 files changed, +166/-48 - No SQL changes - No `make gen` - No migrations - No audit-table updates ## Validation - [x] `go test ./coderd/database/dbauthz/` — full suite, ~6s - [x] `go test -run TestPostWorkspaceAuth ./coderd/` — instance-identity handler tests - [x] `go test -run TestProvisionerJob ./coderd/` - [x] `go test -run TestWorkspaceAgent ./coderd/` - [x] `go test ./coderd/provisionerdserver/` - [x] `gofmt -l` clean ## Alternatives considered - **SQL-side filter:** rewrite `GetWorkspaceAgentsByInstanceID` to join `workspace_resources`/`provisioner_jobs` and filter `job.type = 'workspace_build'` server-side, eliminating the filter loop entirely. Cleaner long-term, but changes generated SQL and is too much surface for a release-branch hotfix. Worth doing as a follow-up. - **Full revert of #24325:** removes the multi-agent feature outright; conflicts with downstream commits ([#24441](https://github.com/coder/coder/pull/24441), [#24438](https://github.com/coder/coder/pull/24438), [#24313](https://github.com/coder/coder/pull/24313)). Reserved as fallback if the surgical fix doesn't hold under load testing.
This commit is contained in:
@@ -1503,6 +1503,28 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole,
|
||||
}
|
||||
|
||||
func (q *querier) authorizeProvisionerJob(ctx context.Context, job database.ProvisionerJob) error {
|
||||
// System-restricted callers (e.g. instance-identity agent auth via
|
||||
// AsSystemRestricted) have already passed an outer authz check before
|
||||
// reaching the provisioner job. Skip the per-job RBAC fan-out through
|
||||
// GetWorkspaceBuildByJobID -> GetWorkspaceByID, which serializes 2
|
||||
// extra DB queries + 1 RBAC eval per call. Under saturated pgx pools
|
||||
// this cascade can block agent auth past the HTTP write timeout (see
|
||||
// incident report against v2.33.0-rc.3 with multi-agent
|
||||
// instance-identity templates).
|
||||
//
|
||||
// We check the subject type directly rather than calling
|
||||
// authorizeContext(ResourceSystem) so we do not record a site-scoped
|
||||
// authz call on every provisioner-job lookup; tests like
|
||||
// TestCreateUserWorkspace/AuthzStory assert that workspace creation
|
||||
// only emits org-scoped authz calls. The same actor.Type check is
|
||||
// already used elsewhere in this file (see GetChatDiffStatusesByChatIDs).
|
||||
//
|
||||
// If a future system actor needs the same fast-path, add its
|
||||
// SubjectType here explicitly rather than broadening to a permission
|
||||
// check.
|
||||
if actor, ok := ActorFromContext(ctx); ok && actor.Type == rbac.SubjectTypeSystemRestricted {
|
||||
return nil
|
||||
}
|
||||
switch job.Type {
|
||||
case database.ProvisionerJobTypeWorkspaceBuild:
|
||||
// Authorized call to get workspace build. If we can read the build, we can
|
||||
|
||||
@@ -6259,6 +6259,114 @@ func TestGetWorkspaceAgentByID_FastPath(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
// TestAuthorizeProvisionerJob_SystemFastPath verifies that
|
||||
// authorizeProvisionerJob short-circuits for system-restricted callers
|
||||
// instead of fanning out into GetWorkspaceBuildByJobID -> GetWorkspaceByID.
|
||||
// That cascade adds 2 SQL queries + 1 RBAC eval per provisioner-job lookup
|
||||
// and saturates the pgx pool when called repeatedly from agent
|
||||
// instance-identity auth (see incident report against v2.33.0-rc.3).
|
||||
func TestAuthorizeProvisionerJob_SystemFastPath(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
jobID := uuid.New()
|
||||
job := database.ProvisionerJob{
|
||||
ID: jobID,
|
||||
Type: database.ProvisionerJobTypeWorkspaceBuild,
|
||||
}
|
||||
|
||||
authorizer := rbac.NewAuthorizer(prometheus.NewRegistry())
|
||||
|
||||
t.Run("AsSystemRestricted/SkipsCascade", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
mockDB := dbmock.NewMockStore(ctrl)
|
||||
|
||||
mockDB.EXPECT().Wrappers().Return([]string{})
|
||||
// The fast-path must short-circuit before GetWorkspaceBuildByJobID
|
||||
// or GetWorkspaceByID can be called. The strict mock will fail
|
||||
// the test if either is invoked.
|
||||
mockDB.EXPECT().GetProvisionerJobByID(gomock.Any(), jobID).Return(job, nil)
|
||||
|
||||
q := dbauthz.New(mockDB, authorizer, slogtest.Make(t, nil), coderdtest.AccessControlStorePointer())
|
||||
ctx := dbauthz.AsSystemRestricted(context.Background())
|
||||
|
||||
got, err := q.GetProvisionerJobByID(ctx, jobID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, job, got)
|
||||
})
|
||||
|
||||
t.Run("AsSystemRestricted/TemplateVersion/SkipsCascade", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// The fast-path is type-agnostic: it must short-circuit the
|
||||
// template-version cascade as well, so neither
|
||||
// GetTemplateVersionByJobID nor GetTemplateByID is invoked.
|
||||
tvJobID := uuid.New()
|
||||
tvJob := database.ProvisionerJob{
|
||||
ID: tvJobID,
|
||||
Type: database.ProvisionerJobTypeTemplateVersionImport,
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
mockDB := dbmock.NewMockStore(ctrl)
|
||||
|
||||
mockDB.EXPECT().Wrappers().Return([]string{})
|
||||
mockDB.EXPECT().GetProvisionerJobByID(gomock.Any(), tvJobID).Return(tvJob, nil)
|
||||
|
||||
q := dbauthz.New(mockDB, authorizer, slogtest.Make(t, nil), coderdtest.AccessControlStorePointer())
|
||||
ctx := dbauthz.AsSystemRestricted(context.Background())
|
||||
|
||||
got, err := q.GetProvisionerJobByID(ctx, tvJobID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, tvJob, got)
|
||||
})
|
||||
|
||||
t.Run("NonSystemActor/StillCascades", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// An auditor has no ResourceSystem permission, so the fast-path
|
||||
// must fall through to the workspace-build cascade. That cascade
|
||||
// then fails authz on the workspace because auditors cannot read
|
||||
// arbitrary workspaces. The error type is what we assert: it
|
||||
// proves the cascade ran rather than the fast-path short-circuiting.
|
||||
orgID := uuid.New()
|
||||
wsID := uuid.New()
|
||||
workspace := database.Workspace{
|
||||
ID: wsID,
|
||||
OwnerID: uuid.New(),
|
||||
OrganizationID: orgID,
|
||||
}
|
||||
build := database.WorkspaceBuild{
|
||||
ID: uuid.New(),
|
||||
WorkspaceID: wsID,
|
||||
JobID: jobID,
|
||||
}
|
||||
auditor := rbac.Subject{
|
||||
ID: uuid.NewString(),
|
||||
Roles: rbac.RoleIdentifiers{rbac.RoleAuditor()},
|
||||
Groups: []string{orgID.String()},
|
||||
Scope: rbac.ScopeAll,
|
||||
}
|
||||
|
||||
ctrl := gomock.NewController(t)
|
||||
mockDB := dbmock.NewMockStore(ctrl)
|
||||
|
||||
mockDB.EXPECT().Wrappers().Return([]string{})
|
||||
mockDB.EXPECT().GetProvisionerJobByID(gomock.Any(), jobID).Return(job, nil)
|
||||
mockDB.EXPECT().GetWorkspaceBuildByJobID(gomock.Any(), jobID).Return(build, nil)
|
||||
mockDB.EXPECT().GetWorkspaceByID(gomock.Any(), wsID).Return(workspace, nil)
|
||||
|
||||
q := dbauthz.New(mockDB, authorizer, slogtest.Make(t, nil), coderdtest.AccessControlStorePointer())
|
||||
ctx := dbauthz.As(context.Background(), auditor)
|
||||
|
||||
_, err := q.GetProvisionerJobByID(ctx, jobID)
|
||||
require.Error(t, err)
|
||||
require.True(t, dbauthz.IsNotAuthorizedError(err),
|
||||
"cascade must run and produce a NotAuthorized error for auditor: got %v", err)
|
||||
})
|
||||
}
|
||||
|
||||
func TestAsAutostart(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user