Repository navigation
perf(policy): add fast paths to AdminAction.IsValid and HasResource - #11
Merged
Merged
Conversation
Co-authored-by: Klaus Post <klauspost@gmail.com> Signed-off-by: Denis Babchuk <babchukdenis@icloud.com>
Member
|
Thanks, @botdeniska, for the focused optimization and careful regression coverage, and @klauspost for the original work. We independently reproduced the performance improvement: about 82% less evaluation time in the many-bucket benchmark, with no extra allocations. Full race tests, fuzzing, and a second comparison of 50,000 policies found no behavior differences. Integration checks with the published mcli also matched the v3.14.2 baseline. CI is green on Go 1.26 and 1.27, including LDAP and vulnerability checks. Merging this for silo-pkg v3.14.3. Thanks for the contribution! |
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.
Contribution Licensing (no CLA, inbound=outbound, DCO required)
This pull request contributes to PGSTY SILO Shared Packages (
pgsty/silo-pkg).Code contributions are accepted under
GNU AGPL v3.0 or later
(
AGPL-3.0-or-later), the same license as this package.By submitting this pull request I represent that I have the right to contribute
the changes under that license and retain copyright in my original work.
No CLA, copyright assignment, or separate Apache-2.0 license grant is required.
Existing copyright and license notices remain intact; separately licensed
material keeps its applicable terms. Every commit must carry a DCO
Signed-off-bytrailer (git commit -s); seeCONTRIBUTING.md.
Description and motivation
For every statement whose actions match the request,
Statement.IsAllowedPtrcalls
isAdmin()andhasAdminResource(). These runAdminAction.IsValid()and
HasResource()on each action, and both scanned the whole admin action mapwith
wildcard.Match. That is about 100 matches per action just to learn thats3:GetObjectis not an admin action. The scan has been on the per-requestpath since 8c45d07 (wildcard admin actions, inherited from minio/pkg).
Take a user with 1000
s3:GetObject/s3:PutObjectstatements on differentbuckets, with the matching grant last.
Policy.IsAllowedtakes 1.7 ms for thatuser, and 71% of the CPU goes to
AdminAction.IsValid.This ports the fast path from
minio/pkg#261 by @klauspost. #4
already took the wildcard part of that PR. The fast path:
*or?;IsValid, also stops when the text before the first wildcard cannot agreewith
admin:. The check works both ways, soadm*anda*d*m*i*n*stayvalid.
Patterns that can still match go through the old scan, so no result changes.
TestAdminActionNamespacePrefixpins theadmin:invariant that the fastpath relies on.
I left out the rest of the performance part of minio#261 to keep this PR
small. That covers the cached statement classification, the action index fix
and building the resource string once per request. I can send it as a
follow-up.
Test changes:
policy/admin-action_test.gois adapted from Fix policy validation & improve speed minio/pkg#261. It has traptables, an equivalence check against the old scan over 921 patterns, a fuzz
target and the prefix invariant.
BenchmarkIsAllowedgets aManyBucketGrantsscenario. In it, everystatement matches the action but not the bucket, which no existing scenario
covers.
Validation
make testpasses: lint reports 0 issues, andgo test -race -tags kqueue ./...is green.
go mod tidy -diffandgit diff --checkare clean.FuzzAdminActionIsValidran for 60s: 10.7M executions, no divergence.requests. The policies mix Allow/Deny, NotAction, NotResource, conditions,
and s3/admin/sts/kms actions with wildcards. Both versions accept and reject
the same policies in
ParseConfig, and ~6MIsAlloweddecisions match,including on merged policies. This check is not committed.
HasPrefix, both thenew tests and the local comparison fail.
Benchmarks were run on an Apple M4 Pro, alternating the old and new binaries
for 10 rounds. The change adds no allocations.
BDP/1x1000/...is anout-of-tree benchmark of the 1000-statement case above.
Full benchstat output
Downstream impact
None. There is no API or behaviour change; the server, Console and mcli pick
this up with the next release.
Checklist
git commit -s) per the DCO