- Notifications
You must be signed in to change notification settings - Fork926
chore: improve rbac and add benchmark tooling#18584
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
base:main
Are you sure you want to change the base?
Conversation
ca7deed toc1fe8e3Comparec1fe8e3 to29222a1Compare| @@ -229,7 +229,7 @@ func BenchmarkRBACAuthorizeGroups(b *testing.B) { | |||
| // BenchmarkRBACFilter benchmarks the rbac.Filter method. | |||
| // | |||
| //go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out | |||
| //go test -bench'^BenchmarkRBACFilter$' -benchmem -memprofile memprofile.out -cpuprofile profile.out | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it makes sense to separate thePrepareOnly benchmarks into a dedicated test (e.g.,BenchmarkRBACPrepare). TheFilter method already callsPrepare, so it will always have a longer execution time than justPrepare. Mixing both leads to benchstat averages that can be misleading. See for example the benchstat results from the policy update PR:
| geomean 212.7µ 1.136m +434.09% |
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That sounds reasonable to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is fine with me 👍
Ideally theFilter benchmark would omit the time used duringPrepare? Then the total time is the combination of the benchmarks.
Regardless, feel free to make that refactor 👍
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added this script in therbac folder, but wondering if it might make more sense to move it under the main scripts directory instead, maybe something likescripts/rbac to keep things consistent. Wdyt? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you can throw it under./scripts, you might also want to look into some of our other scripts in there, especially the helpers inlib.sh.
I also note that you have to be in the `coderd/rbac folder to run this right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, you can throw it under/scripts 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is awesome btw ❤️
| @@ -0,0 +1,93 @@ | |||
| package main | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I used this script to test the policy update changes by generating a replica of the template admin role. It was helpful for local testing, but let me know what you think. It could use some refactoring to support different roles, actions, and object types.
Also, similar tobenchmark_authz.sh, maybe it makes sense to move it to the mainscripts directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is cool! I think we could probably usecoderdtest /dbgen to spin up a set of subjects and objects for testing.
There is of course the matter of combinatorial explosion so we'd want to be careful about how many we add here, otherwise running this benchmark will take forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a note: this script isn’t for benchmarking. It’s meant to generate aninput.json file to use with theopa eval command. This helps verify that the policy returns the correct output, we already have a default file:https://github.com/coder/coder/blob/main/coderd/rbac/input.json
This was especially useful for testing the template admin that has many roles, since updating the JSON file manually was a bit slow 😅
I would also like to add (optional) arguments like--action=[one from the available actions],--subject=[built-in roles], and--object=[one from the supported resources] to make it more flexible.
I’ll also add a small comment at the beginning of the file to explain this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ssncferreira I've had similar looking programs to do exactly the same. I'd create a./scripts/rbactest or something and toss the bash script and this in there.
Throw this in, and we can iterate as we revisit this stuff. There are other go programs that I iterate on each time I use them. This is great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wow! Thanks for all of the work you put into this!
Out of curiousity, I wonder if your changes to the policy had any measurable benchmark difference? I would assume that it remains basically the same, but it could be interesting to see the benchmark results now that we have the tools 😁
| @@ -229,7 +229,7 @@ func BenchmarkRBACAuthorizeGroups(b *testing.B) { | |||
| // BenchmarkRBACFilter benchmarks the rbac.Filter method. | |||
| // | |||
| //go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out | |||
| //go test -bench'^BenchmarkRBACFilter$' -benchmem -memprofile memprofile.out -cpuprofile profile.out | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That sounds reasonable to me!
| done | ||
| else | ||
| echo"Usage:" | ||
| echo"$0 --single# run benchmarks on current branch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: indentation
Also, we could probably infer the--single argument if omitted.
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you can throw it under./scripts, you might also want to look into some of our other scripts in there, especially the helpers inlib.sh.
I also note that you have to be in the `coderd/rbac folder to run this right now.
| @@ -0,0 +1,93 @@ | |||
| package main | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is cool! I think we could probably usecoderdtest /dbgen to spin up a set of subjects and objects for testing.
There is of course the matter of combinatorial explosion so we'd want to be careful about how many we add here, otherwise running this benchmark will take forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work commenting and refactoring!
Thank you! 🫶 I had some of these things stashed and thought they would be a good addition for future reference.
I was a bit worried about breaking something 😅 so I ran the tests yesterday, it was slightly better, but nothing significantly different:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is all excellent!
| bool_flip(b):=flipped if{ | ||
| bool_flip(b):=false if{ | ||
| b | ||
| flipped=false | ||
| } | ||
| bool_flip(b):=flipped if{ | ||
| bool_flip(b):=true if{ | ||
| notb | ||
| flipped=true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Very nice 👍
| number(set):= c if{ | ||
| # Return -1 if the set contains any 'false' value (i.e., an explicit deny) | ||
| number(set):=-1 if{ | ||
| false inset | ||
| c:=-1 | ||
| } | ||
| number(set):= c if{ | ||
| # Return 0 if the set is empty (no matching permissions) | ||
| number(set):=0 if{ | ||
| count(set)==0 | ||
| } | ||
| # Return 1 if the set is non-empty and contains no 'false' values (i.e., only allows) | ||
| number(set):=1 if{ | ||
| notfalse inset | ||
| set[_] | ||
| c:=1 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, you can throw it under/scripts 👍
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is awesome btw ❤️
| @@ -0,0 +1,93 @@ | |||
| package main | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ssncferreira I've had similar looking programs to do exactly the same. I'd create a./scripts/rbactest or something and toss the bash script and this in there.
Throw this in, and we can iterate as we revisit this stuff. There are other go programs that I iterate on each time I use them. This is great 👍
| // TODO: support arguments for subject, action and object | ||
| funcmain() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
totally fine to start with this 👍
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR improves the RBAC package by refactoring the policy, enhancing documentation, and adding utility scripts.
Changes
policy.regofor clarity and readabilitybenchmark_authz.shscript for authz performance testing and comparisongen_input.goto generate input foropa evaltesting