Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

chore: optimize auth check recordings#20300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
dannykopping wants to merge3 commits intomainfromdk/authzchecks
Closed

Conversation

@dannykopping
Copy link
Contributor

@dannykoppingdannykopping commentedOct 15, 2025•
edited
Loading

Disclaimer: mostly implemented with Claude Code.

recordAuthzCheck is currently very inefficient. Aggravating this is the fact that the checks are recorded inAuthzCheckRecorder'schecks slice which grows inexorably for long-running requests like WebSocket conns whose contexts can remain active for hours or days.

image

(source)

I've implemented a ring-buffer (cap 20) to help with this.

Fortunately it's only in use when running the dev version, so this won't affect production instances.

Benchmarks:

$ gotest -run=^$ -bench'^BenchmarkRecorder$' -benchmem ./coderd/rbac/... -benchtime=10000x -count=10> $ /tmp/old.txtgotest -run=^$ -bench'^BenchmarkRecorder$' -benchmem ./coderd/rbac/... -benchtime=10000x -count=10> /tmp/new.txt$ benchstat /tmp/{old,new}.txtgoos: linuxgoarch: amd64pkg: github.com/coder/coder/v2/coderd/rbaccpu: AMD Ryzen 9 7900 12-Core Processor                                    │ /tmp/old.txt  │             /tmp/new.txt              │                       │    sec/op     │    sec/op      vs base                │Recorder/Minimal-24      160.45n ± 37%   78.01n ±  21%  -51.38% (p=0.000 n=10)Recorder/WithID-24        225.1n ± 82%   111.6n ±  44%  -50.40% (p=0.000 n=10)Recorder/WithOwner-24     268.4n ± 43%   146.1n ± 114%  -45.58% (p=0.011 n=10)Recorder/WithOrg-24       553.8n ± 42%   159.3n ± 131%  -71.24% (p=0.001 n=10)Recorder/AllFields-24     494.4n ± 37%   365.0n ±  57%~ (p=0.052 n=10)Recorder/Contention-24    295.8n ± 46%   301.4n ±  23%~ (p=0.971 n=10)geomean                   303.7n         167.8n         -44.77%                       │ /tmp/old.txt │            /tmp/new.txt            │                       │     B/op     │    B/op     vs base                │Recorder/Minimal-24       178.00 ± 0%   40.00 ± 0%  -77.53% (p=0.000 n=10)Recorder/WithID-24        322.00 ± 0%   88.00 ± 0%  -72.67% (p=0.000 n=10)Recorder/WithOwner-24      530.0 ± 0%   392.0 ± 0%  -26.04% (p=0.000 n=10)Recorder/WithOrg-24        658.0 ± 0%   440.0 ± 0%  -33.13% (p=0.000 n=10)Recorder/AllFields-24      658.0 ± 0%   440.0 ± 0%  -33.13% (p=0.000 n=10)Recorder/Contention-24     659.0 ± 0%   441.0 ± 0%  -33.08% (p=0.000 n=10)geomean                    453.2        221.4       -51.15%                       │ /tmp/old.txt │            /tmp/new.txt            │                       │  allocs/op   │ allocs/op   vs base                │Recorder/Minimal-24        4.000 ± 0%   2.000 ± 0%  -50.00% (p=0.000 n=10)Recorder/WithID-24         6.000 ± 0%   2.000 ± 0%  -66.67% (p=0.000 n=10)Recorder/WithOwner-24      8.000 ± 0%   3.000 ± 0%  -62.50% (p=0.000 n=10)Recorder/WithOrg-24        9.000 ± 0%   3.000 ± 0%  -66.67% (p=0.000 n=10)Recorder/AllFields-24      9.000 ± 0%   3.000 ± 0%  -66.67% (p=0.000 n=10)Recorder/Contention-24     9.000 ± 0%   3.000 ± 0%  -66.67% (p=0.000 n=10)geomean                    7.206        2.621       -63.63%

Signed-off-by: Danny Kopping <danny@coder.com>
…ntextsSigned-off-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykopping changed the titlechore: optimizerecordAuthzCheckchore: optimize auth check recordingsOct 15, 2025
Signed-off-by: Danny Kopping <danny@coder.com>
const (
// MaxRecordedChecks is the maximum number of authorization checks to keep
// in the ring buffer. This prevents unbounded memory growth for long-running contexts.
MaxRecordedChecks=20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do we have any data on how many authz checks are performed, on average, per request? Is this just a guesstimate?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just a guesstimate, yeah.

Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why not just disable authz recording on dev builds unless it's a testing build? We get no value out of it on dogfood, because AFAIK there's no API to actually look at the recorded authz checks.

@johnstcn
Copy link
Member

johnstcn commentedOct 15, 2025•
edited
Loading

Why not just disable authz recording on dev builds unless it's a testing build? We get no value out of it on dogfood, because AFAIK there's no API to actually look at the recorded authz checks.

We set a HTTP headerx-authz-checks. See

funcSetAuthzCheckRecorderHeader(ctx context.Context,rw http.ResponseWriter) {
ifrec,ok:=rbac.GetAuthzCheckRecorder(ctx);ok {
// If you're here because you saw this header in a response, and you're
// trying to investigate the code, here are a couple of notable things
// for you to know:
// - If any of the checks are `false`, they might not represent the whole
// picture. There could be additional checks that weren't performed,
// because processing stopped after the failure.
// - The checks are recorded by the `authzRecorder` type, which is
// configured on server startup for development and testing builds.
// - If this header is missing from a response, make sure the response is
// being written by calling `httpapi.Write`!
checks:=rec.String()
iflen(checks)>maxHeaderLength {
checks=checks[:maxHeaderLength]
checks+="<truncated>"
}
rw.Header().Set("x-authz-checks",checks)
}
}

@deansheather
Copy link
Member

This seems so niche that it could just be something that someone can enable with a const or a build flag. Do people even use this on dogfood?

@Emyrk
Copy link
Member

Ideally the requester could add some header to request the response header be present. Then the cli and ui could have some hidden switch to staple the header on these requests

Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The ring buffer would omit some authz checks, which returns an incomplete story.

I would rather us find a way to opt in on a per request basis

@dannykopping
Copy link
ContributorAuthor

We're rather going to take the approach of enabling the auth recordings on a per-request basis.

@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 15, 2025
@Emyrk
Copy link
Member

We're rather going to take the approach of enabling the auth recordings on a per-request basis.

#20310

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@deansheatherdeansheatherdeansheather left review comments

@EmyrkEmyrkEmyrk requested changes

@johnstcnjohnstcnjohnstcn approved these changes

@spikecurtisspikecurtisAwaiting requested review from spikecurtis

Assignees

@dannykoppingdannykopping

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dannykopping@johnstcn@deansheather@Emyrk
[8]ページ先頭
©2009-2025 Movatter.jp